Cover the agent-callable tools, the tunnel supervisor and the operator commands - #5
Draft
garethx wants to merge 2 commits into
Draft
Cover the agent-callable tools, the tunnel supervisor and the operator commands#5garethx wants to merge 2 commits into
garethx wants to merge 2 commits into
Conversation
…r commands Coverage was 67% overall, but the distribution was the problem: the modules an LLM can drive unsupervised against a live project were the least tested, and tools.py had no test file at all. tools.py 29% -> 100% tunnel.py 41% -> 91% cli.py 34% -> 85% dashboard/… 64% -> 83% overall 67% -> 86% The emphasis is on what decides *which* resource gets mutated, rather than on return-value shape: * the pause ceiling, every way a model can get a number wrong * the pause -> auto-resume contract driven end to end, tool half and adapter half together, because covered separately they can disagree about the ledger path and both still pass * connection resolution by name, including the name that matches nothing * the dashboard's ownership check on its refusal path — the endpoint is reachable whether or not the tab renders a button for it * the tunnel's backoff reset rule, which is easy to get backwards * doctor's diagnoses, including a retry rule too narrow for what the adapter emits Each of those was mutation-checked: inverting the backoff reset, removing the pause ceiling, and disabling the ownership check all fail the suite. Also ignores .coverage, which is a build artifact rather than source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of these were coupled to wording and env-var names that #3 changes, so they would have failed on merge for reasons that are not regressions: * the missing-API-key assertion pinned `HOOKDECK_API_KEY` by name, and #3 renames it to `HOOKDECK_EG_API_KEY`. It now accepts either, because the regression worth catching is doctor staying quiet about a missing key. * the push-mode test asserted "Hookdeck CLI" was absent from the output to prove the cli-mode checks had not run. #3's API-key message mentions the Hookdeck CLI, so that assertion broke without anything being wrong. It now records calls to `shutil.which`, which is what it meant all along. * the fixture cleared only the unprefixed env vars, so a shell with the namespaced ones set could make a doctor test pass for the wrong reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2.
Coverage was 67% overall, but as #2 argued, the distribution was the problem: the surface an LLM drives unsupervised against a live project was the least tested part of the codebase, and
tools.pyhad no test file at all.tools.pycli.pytunnel.pydashboard/plugin_api.py88 new tests. No production code changes — the only non-test edit is adding
.coverageto.gitignore.What is actually asserted
The emphasis is on what decides which resource gets mutated, rather than on return-value shape.
MAX_PAUSE_MINUTESis the only thing between a confused agent and an indefinitely paused connection. Every way a model can produce a bad number is covered:"10",-30,[],None, absent, over-cap. Plus that the minutes the tool reports are the minutes it actually scheduled, since the model plans against that number.adapter._resume_due_connectionshonours it. Covered separately, both halves pass while disagreeing about the ledger path or the row shape — and a deadline written where the adapter never looks is indistinguishable from the feature silently not existing._require_own_connectionis what stops this tab pausing somebody else's production traffic, and the endpoint is reachable whether or not the tab renders a button for it. Also that every configured route is consulted, not just the first.hookdeck ciopt-in.doctor, including the check for a retry rule narrower than what the adapter emits — the silent-data-loss case.Verification
The three safety-critical assertions were mutation-checked rather than trusted to the coverage number. Each of these fails the suite:
One test initially passed for the wrong reason and is worth calling out:
RunLedger.__init__callsmkdir(parents=True), so an "unwritable path" was being silently created. It now patchesRunLedgerto raise, and asserts both that the connection is still paused and that the failure is logged — a pause recorded nowhere is recoverable, a pause that fails silently is not.Merge order
This should land after #3. It is based on
mainand adds no new merge conflicts — test-merging this branch against #3 produces exactly the same four conflicts (README.md,adapter.py,cli.py,test_adapter.py) that #3 already has againstmain, because #3 is four commits behind.tests/test_cli.pyandtests/test_dashboard_api.pyauto-merge cleanly.The second commit here removes the couplings to #3 that would otherwise have caused spurious failures: the doctor tests no longer pin
HOOKDECK_API_KEYby name (either spelling satisfies them), no longer assert on wording that #3's API-key message changes, and clear both env-var spellings so a developer's shell cannot make a check pass for the wrong reason.Left alone, per #2's own ordering:
adapter.py(72%) anddashboard/dist/index.js.