fix(hex): handle empty helper output and fix public key response tuple order#14211
fix(hex): handle empty helper output and fix public key response tuple order#14211thavaahariharangit wants to merge 5 commits intomainfrom
Conversation
…e order
- Add empty output check in run.exs to return a clear error instead of
crashing on binary_to_term with empty data
- Fix fetch_public_key tuple pattern to match {status, headers, body}
instead of {status, body, headers}
- Add error handling in lockfile_updater for public key fingerprint
mismatch, public key download failure, empty output, and JSON parse
errors
- Add corresponding test coverage for new error paths in both
lockfile_updater and update_checker
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #14168 where Hex builds fail when fetching the public key from a private registry. The root cause was a tuple destructuring bug in the Elixir helper script where Hex.Repo.get_public_key returns {status, headers, body} but the code expected {status, body, headers}, causing the public key to never be extracted correctly. Additionally, when helpers returned empty output, binary_to_term("") would crash instead of providing a clear error message.
Changes:
- Fixed tuple pattern matching in
fetch_public_keyfrom{200, key, _}to{200, _headers, key}to correctly extract the public key from HTTP responses - Added empty output guard that returns a descriptive error instead of crashing on
binary_to_term("") - Added comprehensive test coverage for public key errors, download failures, and empty helper output scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hex/helpers/lib/run.exs | Fixed tuple order bug in fetch_public_key (line 127) and added empty output check (lines 9-15) to prevent binary_to_term crashes |
| hex/spec/dependabot/hex/file_updater/lockfile_updater_spec.rb | Added test coverage for public key fingerprint mismatch, download failure, and empty output errors |
| hex/spec/dependabot/hex/update_checker_spec.rb | Added test coverage for registry verification crashes and public key download HTTP errors |
| before do | ||
| allow(Dependabot::SharedHelpers).to receive(:run_helper_subprocess) | ||
| .with(hash_including(function: "get_latest_resolvable_version")) | ||
| .and_raise( | ||
| Dependabot::SharedHelpers::HelperSubprocessFailed.new( | ||
| message: "No output returned from helper script", | ||
| error_context: {} | ||
| ) | ||
| ) | ||
| end | ||
|
|
||
| it "raises a helpful error" do | ||
| expect { latest_resolvable_version } | ||
| .to raise_error(Dependabot::DependencyFileNotResolvable) do |error| | ||
| expect(error.message).to include("No output returned from helper script") | ||
| end | ||
| end |
There was a problem hiding this comment.
I'm struggling to see the value these tests actually provide. The way they're set up, you could pass any message to HelperSubprocessFailed and just assert that same message comes back out. For example you could set the message to Hello, world! and then check that error.message includes Hello, world! and the test would pass. There are too many things being mocked here so we end up only testing the mock itself rather than any real behaviour
There was a problem hiding this comment.
Thanks, you're right that these tests were only verifying the mock itself rather than any real behavior. I've removed both test contexts ("when registry verification crashes with no output" and "when public key download returns HTTP error code") from update_checker_spec.rb in the latest push. The error-handling regex logic is already properly covered by the equivalent tests in lockfile_updater_spec.rb.
| defp fetch_public_key(repo, repo_url, auth_key, fingerprint) do | ||
| case Hex.Repo.get_public_key(%{trusted: true, url: repo_url, auth_key: auth_key}) do | ||
| {:ok, {200, key, _}} -> | ||
| {:ok, {200, _headers, key}} -> |
There was a problem hiding this comment.
I agree than this change needs to be made based on the hex docs. But I believe that this will only fix this affect self hosted runners and its possible that you will have to update Dependabot proxy as well
There was a problem hiding this comment.
Thanks for confirming the tuple order fix is correct per the hex docs. Good call on the Dependabot proxy — I'll take a look at the proxy repo to check whether it also needs a corresponding update for this to work end-to-end on hosted runners.
The tests for 'registry verification crashes with no output' and 'public key download returns HTTP error code' only tested the mocks themselves rather than real behavior. The error-handling regex logic is already covered by the equivalent tests in lockfile_updater_spec.rb.
What are you trying to accomplish?
Fixing #14168 - Hex builds fail when fetching the public key from a private registry.
Root cause: Two bugs in the Elixir helper script (run.exs):
fetch_public_keydestructured theHex.Repo.get_public_keyresponse tuple as{status, body, headers}when the actual shape is{status, headers, body}, so the public key was never extracted correctly.binary_to_term("")crashed instead of producing a clear error.Changes:
run.exs: Fixfetch_public_keytuple pattern from{200, key, _}to{200, _headers, key}. Add an empty-output guard that returns a descriptive error instead of crashing onbinary_to_term.lockfile_updater_spec.rb: Add test coverage for public key fingerprint mismatch, public key download failure, and empty helper output error paths.update_checker_spec.rb: Add test coverage for registry verification crash (empty output) and public key download HTTP error scenarios.Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
lockfile_updater_spec.rbandupdate_checker_spec.rb.PrivateSourceAuthenticationFailurewith the repo name instead of crashing with an opaquebinary_to_termerror.Checklist