Repository navigation
refactor(scripts): one constant-test loop, not two - #326
Conversation
#324 and #325 landed equivalent loops within an hour of each other, built in parallel against the same brief. main now carries constant_test_loop.py and scoped_smoke_loop.py, which assert overlapping invariants against the same endpoints and document themselves separately. Two monitors that drift apart is worse than either alone, because nobody can say which one going red is the one that matters. constant_test_loop.py survives — it shipped first, has docs, enforces HTTPS off loopback, and self-provisions on loopback. scoped_smoke_loop.py and its tests are removed, with its distinct parts folded in: Tool selection no longer dies when neither partner.echo nor partner.notes.write is registered. --tool (or $CI_SMOKE_TOOL) names one instead, and the failure message lists what the deployment actually exposes. There is deliberately no "just use the first one" fallback: schema-derived arguments satisfy a tool's declared shape but not its semantics — verified locally, awi_passkey_challenge accepts the payload and then refuses the action — and that failure reads as a broken trust plane when it is really a bad choice of tool. Arguments come from the selected tool's inputSchema rather than a hardcoded {"text": ...}, so the loop runs against a registry it was not written for. The replay still reuses the identical body, so it stays a replay. The permit cap is derived from the tool's advertised creditsPerCall with headroom rather than fixed at 10. Tools here are priced 1 to 200 credits, so a fixed cap fails permit_budget_exceeded on a healthy deployment as soon as a pricier tool is selected, which reads as a product fault. Off loopback, --tool-args is required. Derivation fills required fields from types, defaults, and the first enum member, which for a consequential tool could be "delete", sent again every run. Schema validity is not evidence of safety. --tool-args '{}' is a fine answer; it just has to be an answer. The companion tool for the out-of-scope check is selected through select_companion_tool, which requires it to be registered and to differ from the permitted tool, so the check cannot pass by proving tool-not-found. Also drops OUT_OF_SCOPE_TOOL, a constant referenced nowhere — the denial check already picked a real tool from the registry. Verified against a local instance exposing neither preferred tool: the selection error names the available tools, and with --tool semantic-search the full loop reports ALL INVARIANTS HELD at exit 0, charging 15 credits where the old fixed cap of 10 would have refused the call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 11 minutes Limit details: You’ve used the included review currently available. Your 91 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 319b209. Configure here.
Tool-selection problems raised SmokeTestFailure, which exits 1 — the same signal a genuinely broken trust plane gives. A typo in $CI_SMOKE_TOOL, or a registry that happens to carry neither preferred tool, would page whoever is on call as though the product had failed. They are ConfigurationError now, exiting 2, matching how the off-loopback flag guards already behave. Writing the test for that surfaced a second gap: --other-tool was resolved inside the denial block, which only ran when the deployment exposed more than one tool. On a single-tool deployment an invalid pin was silently ignored, so an operator who misspelled it got a clean exit 0 and a SKIP line rather than being told. Selection now happens beside the governed tool, so the pin is validated regardless of how many tools exist, and the denial block simply asks whether a companion was found. That also removes the separate tool-count condition, which was expressing the same thing less directly. Also corrects the select_governed_tool docstring, which still described the "any registered tool" fallback removed earlier in this branch. A docstring promising a fallback the code deliberately refuses to implement is worse than no docstring on a decision this one turns on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b9ae02e2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| It reports SKIP rather than a false PASS for the out-of-scope denial when the | ||
| deployment registers only one tool, since invoking an unregistered name proves | ||
| "tool not found" rather than "the permit refused it". | ||
| `constant_test_loop.py` is the one built to run continuously: it needs no |
There was a problem hiding this comment.
Preserve continuous execution in the surviving loop
The surviving script executes run_constant_test exactly once and exits, while this newly updated proof matrix calls it the loop “built to run continuously.” The deleted scoped_smoke_loop.py was the only implementation of --interval, --max-iterations, and continuing after transient failures, so deployments replacing make scoped-smoke-loop with this documented command silently lose continuous monitoring. Either retain equivalent scheduling behavior in the consolidated script or describe the required external scheduler.
Useful? React with 👍 / 👎.
| |---|---|---| | ||
| | `make trust-conformance-live` | Golden path; sequential replay; 15 identical concurrent requests exposing one receipt identity or explicit `idempotency_in_progress`, followed by a completed replay and one charge; a changed payload under a reused key conflicting rather than replaying; budget denial; expired and forged permit rejection; receipt and audit-chain verification; tenant isolation against a directly-supplied foreign wallet and permit id | `AGENT_MIDDLEWARE_API_KEY`; set `AGENT_MIDDLEWARE_API_URL` or it defaults to production | | ||
| | `make scoped-smoke-loop` | Authority readable for the caller's own wallet; scoped permit issued and carrying a nonce; governed invoke; the debit matching the receipt's own stated cost; replay returning the same receipt and charging nothing further; a changed payload under a reused key refused; an out-of-scope but genuinely registered tool denied and not charged; the portable receipt carrying a signature, key id, and named canonicalization | `AGENT_MIDDLEWARE_API_KEY` holding a **wallet-scoped** key (the loop refuses a bootstrap admin key); `--tool` required off loopback | | ||
| | `python scripts/constant_test_loop.py` | Scoped permit sized from the tool's advertised `creditsPerCall`; governed invoke; signed success receipt with a ledger entry; replay returning the same `receipt_id` with no second debit; an out-of-scope but genuinely registered tool refused with `permit_tool_not_allowed` and charged nothing | `CI_SMOKE_AGENT_KEY` holding a **wallet-scoped** key (self-provisions on loopback when absent); `--tool` and `--tool-args` required off loopback | |
There was a problem hiding this comment.
Restore portable receipt verification in the consolidated loop
The deleted smoke loop fetched /v1/receipts/{receipt_id}/portable and checked its signature, key ID, and canonicalization, but run_constant_test now stops after checking receipt outcome, ledger linkage, and charge. Consequently, a deployment with a broken portable receipt endpoint or missing verification metadata still prints ALL INVARIANTS HELD; the matrix's “signed success receipt” wording overstates what this replacement actually proves. Restore those checks before treating this as the sole continuous receipt proof.
AGENTS.md reference: AGENTS.md:L68-L80
Useful? React with 👍 / 👎.
| if args.tool_args is None: | ||
| raise ConfigurationError( | ||
| f"refusing to send derived arguments to {api_url}. Pass " | ||
| "--tool-args with a payload you have approved for repeated " | ||
| "invocation, or --tool-args '{}' if the tool needs none." |
There was a problem hiding this comment.
Update all production commands for the required flags
For every non-loopback URL, this new guard rejects a run without --tool-args, but both the module docstring's production command and docs/constant-test-loop.md's final production command still invoke the script with only API_URL. Following either documented path now exits 2 before authentication or testing, so update those examples to pass both the pinned tool and approved payload (or the corresponding environment configuration).
Useful? React with 👍 / 👎.
| "--tool-args", | ||
| type=json.loads, | ||
| default=None, |
There was a problem hiding this comment.
Reject non-object tool argument JSON as configuration
json.loads accepts arrays, strings, numbers, and booleans even though MCP tool arguments and this option's help require an object. For example, --tool-args '[]' passes the production safety guard and is sent to /mcp/messages, where the resulting schema/protocol rejection is reported as an invariant failure rather than an operator configuration error. Validate that the decoded value is a dictionary and add malformed-input coverage.
AGENTS.md reference: AGENTS.md:L95-L101
Useful? React with 👍 / 👎.
--tool and --other-tool already fall back to $CI_SMOKE_TOOL and $CI_SMOKE_OTHER_TOOL, but --tool-args had no equivalent. A CI job could therefore satisfy every part of the off-loopback guard except the payload, and had to reach for argv to supply the one remaining piece — which for a secret-adjacent value is the wrong place, and which makes the guard read as an obstacle rather than a check. --tool-args now falls back to $CI_SMOKE_TOOL_ARGS. This is not cosmetic: PR 327 wires the loop into CI and its production step invokes it with neither flag, so the guard added in #326 exits 2 on every push to main. Configuring that step through secrets is only possible once the payload has an environment path. An explicitly empty payload set through the environment counts as a decision, exactly as --tool-args '{}' does on the command line, so it satisfies the guard rather than falling through to "unset". Malformed values are ConfigurationError (exit 2), never SmokeTestFailure: invalid JSON, or valid JSON that is not an object, means the loop was configured wrong rather than that the trust plane broke. The message names which source was bad, since --tool-args and $CI_SMOKE_TOOL_ARGS fail identically otherwise. A list or scalar is rejected up front rather than forwarded as the tool's arguments and refused downstream, where it would surface as a tool error and read as the product failing. Dropping argparse's type=json.loads is part of that: it exits 2 for bad JSON too, but with usage text that never says what was wrong with the payload. Verified against production with a placeholder credential: with only CI_SMOKE_TOOL and CI_SMOKE_TOOL_ARGS set, the run reaches real authentication instead of either guard, and a malformed value reports "$CI_SMOKE_TOOL_ARGS is not valid JSON" at exit 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
Rewriting tests/test_constant_test_loop.py dropped the module-scoped test_server fixture, but two tests still take it: test_constant_test_loop_never_logs_api_key (from #324) and test_unusable_tool_pin_exits_as_configuration_not_invariant_failure (from #326). Neither was deleted, so both became collection errors rather than removed coverage — "15 passed, 1 skipped, 2 errors", which is why CI is red and the PR is blocked. They are not pre-existing: this branch removed the fixture they need. Restoring it brings the file to 17 passed, 1 skipped, 0 errors. The fixture is worth having back on its own merits. It boots a real server with a static dev key, which is what makes the surrounding assertions mean anything: that a bootstrap key never reaches stdout or stderr, and that a minted key actually authenticates, are claims about runtime behaviour. Inspecting source for the absence of a print statement cannot establish either. Also drops a redundant local `from pathlib import Path` in test_partner_api_key_bootstrap_json_output, which shadowed the module-level import and failed ruff F401 — the other half of the red CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
The step ran on every push to main and would have failed on all of them, two ways over. With CI_SMOKE_AGENT_KEY unset — the state today — the loop does not skip. It falls through to self-provisioning, and /v1/dev-keys/self-provision is disabled on production by design, so the run fails. The PR described this path as skipping gracefully; there was no skip to do it. A secret cannot be tested in a step-level `if`, so the check has to live in the script. With the key set it would still have failed: #326 made --tool and --tool-args mandatory off loopback, and the step passed neither, so the loop exits 2 before opening a connection. Which tool a monitor invokes on every push, and with what payload, is an operator's decision rather than one discovery makes for them. The step now skips unless both the credential and CI_SMOKE_TOOL are configured, and passes --tool and --tool-args explicitly when they are. Enabling the production smoke is deliberate in two parts, which is what the description already promised. Passing the flags rather than relying on environment fallbacks keeps this independent of #328: it works against main as it stands today. Verified by running the step's own shell: unset skips at exit 0, key-without- tool skips at exit 0, and fully configured reaches the loop rather than either guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
Runs the local trust-plane loop on every PR and push to main, with the production smoke against api.thisisatest.tech genuinely opt-in. ENABLE_DOGFOOD_SECOND_TOOL registers partner.notes.count, a read-only counter, so the out-of-scope denial path is exercised rather than skipped: the permit allows the first tool only, and the second is refused with permit_tool_not_allowed. The production step skips unless both CI_SMOKE_AGENT_KEY and CI_SMOKE_TOOL are configured. Without the key the loop would fall through to self-provisioning, which production disables, and fail rather than skip; a secret cannot be tested in a step-level if, so the check lives in the script. When enabled it passes --tool and --tool-args explicitly, which #326 made mandatory off loopback. Restores the module-scoped test_server fixture that a rewrite of tests/test_constant_test_loop.py dropped while leaving two tests that take it, and re-enables the two tests that had been marked skip for that reason.
--tool and --other-tool already fall back to $CI_SMOKE_TOOL and $CI_SMOKE_OTHER_TOOL, but --tool-args had no equivalent. A CI job could therefore satisfy every part of the off-loopback guard except the payload, and had to reach for argv to supply the one remaining piece — which for a secret-adjacent value is the wrong place, and which makes the guard read as an obstacle rather than a check. --tool-args now falls back to $CI_SMOKE_TOOL_ARGS. This is not cosmetic: PR 327 wires the loop into CI and its production step invokes it with neither flag, so the guard added in #326 exits 2 on every push to main. Configuring that step through secrets is only possible once the payload has an environment path. An explicitly empty payload set through the environment counts as a decision, exactly as --tool-args '{}' does on the command line, so it satisfies the guard rather than falling through to "unset". Malformed values are ConfigurationError (exit 2), never SmokeTestFailure: invalid JSON, or valid JSON that is not an object, means the loop was configured wrong rather than that the trust plane broke. The message names which source was bad, since --tool-args and $CI_SMOKE_TOOL_ARGS fail identically otherwise. A list or scalar is rejected up front rather than forwarded as the tool's arguments and refused downstream, where it would surface as a tool error and read as the product failing. Dropping argparse's type=json.loads is part of that: it exits 2 for bad JSON too, but with usage text that never says what was wrong with the payload. Verified against production with a placeholder credential: with only CI_SMOKE_TOOL and CI_SMOKE_TOOL_ARGS set, the run reaches real authentication instead of either guard, and a malformed value reports "$CI_SMOKE_TOOL_ARGS is not valid JSON" at exit 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
* Wire second dogfood tool into CI constant test loop for out-of-scope denial Problem: ENABLE_DOGFOOD_SECOND_TOOL=true registers partner.notes.count, but the CI constant_test_loop.py invocation doesn't pass --other-tool, so the out-of-scope denial check silently skips rather than proving permit scoping. Solution: Pass --other-tool partner.notes.count to the CI invocation. The constant test loop already has the logic (added in #326) — this wires it. Changes: - .github/workflows/ci.yml: add --other-tool partner.notes.count to local test - tests/test_ci_second_tool_denial.py: 4 tests verifying CI wiring + tool IDs Wedge alignment: This is a reliability fix in the existing one-tool loop — it hardens the CI smoke test to prove permit scoping on every run rather than only when a deployment happens to register a companion tool the auto-select finds. No new capability, no customer blocker, no schema change. Tests: - test_ci_registers_second_tool_and_passes_it_to_constant_test: CI must wire the registered second tool to the denial check - test_constant_test_loop_accepts_other_tool_flag: script accepts --other-tool - test_second_tool_is_distinct_from_first: partner.notes.count != .write - test_ci_workflow_syntax_is_valid: YAML is valid All 4 new tests pass. Co-authored-by: Christopher <PetrefiedThunder@users.noreply.github.com> * fix(lint): remove unused subprocess import Ruff flagged subprocess as imported but unused in test_ci_second_tool_denial.py. No subprocess calls in the test file; removed the import. Co-authored-by: Christopher <PetrefiedThunder@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Christopher <PetrefiedThunder@users.noreply.github.com>

Summary
#324 and #325 merged within an hour of each other, built in parallel against the same brief.
mainnow carries two constant-test loops —constant_test_loop.pyandscoped_smoke_loop.py— asserting overlapping invariants against the same endpoints, each with its own documentation. That's worse than either alone: when one goes red, nobody can say whether it's the one that matters.constant_test_loop.pysurvives. It shipped first, hasdocs/constant-test-loop.md, enforces HTTPS off loopback, and self-provisions on loopback.scoped_smoke_loop.pyand its tests are deleted, with the parts worth keeping folded in.I want to be straight about how this arose: I built #325 without checking whether the work was already in flight, and #324 was merged before I opened mine. The duplication is mine to clean up.
What folded in
Tool selection no longer dies when neither preferred tool is registered.
--tool(or$CI_SMOKE_TOOL) names one, and the failure message lists what the deployment actually exposes rather than just the two names it wanted.There is deliberately no "just use the first one" fallback, and that's a correction to my own #325 design. Verified locally: auto-selecting
awi_passkey_challengeproduced a schema-valid payload that the tool then refused on semantic grounds — a failure that reads as a broken trust plane when it's really a bad choice of tool. Asking beats guessing.Arguments are derived from the selected tool's
inputSchemarather than a hardcoded{"text": ...}, so the loop runs against a registry it wasn't written for. The replay still reuses the identical body, so it remains a replay rather than becoming a conflict.The permit cap comes from the tool's advertised
annotations.creditsPerCallwith headroom, replacing the fixedPERMIT_MAX_CREDITS = 10. Tools here are priced 1–200 credits, so a fixed cap failspermit_budget_exceededon a perfectly healthy deployment the moment a pricier tool is selected — confirmed:semantic-searchat 15 credits would have been refused outright.Off loopback,
--tool-argsis required. Derivation fills required fields from types, defaults, and the first enum member, which for a consequential tool could bedelete, sent again every run. Schema validity is not evidence of safety.--tool-args '{}'is a fine answer; it just has to be an answer.The companion tool for the out-of-scope check goes through
select_companion_tool, which requires it to be registered and to differ from the permitted tool, so the check can't pass by proving tool-not-found.Also drops
OUT_OF_SCOPE_TOOL = "some.other.tool", a constant referenced nowhere — the denial check already selected a real tool from the registry. I initially misread this as a live false-pass bug in #324 and checked before saying so; it was only dead code.Type of Change
Validation
pytest -q— 14 tests intests/test_constant_test_loop.py(6 pre-existing, still passing unchanged, plus 8 ported) and the 46 site tests;ruff check .cleanpython scripts/check_doc_references.py— 309 references across 246 files resolvepython scripts/export_openapi.py --check— not applicable, no API surface changedpython scripts/generate_sim_inventory.py --check— not applicable--tool semantic-search --tool-args '{}'the full loop reportsALL INVARIANTS HELDat exit 0, charging 15 credits where the old cap of 10 would have refused the call; off-loopback guards exit 2 before any network call, leaking no credentialdocs/constant-test-loop.mdgains a Production Use section explaining why the pin and payload are mandatory;docs/PROOF_MATRIX.mdnow describes one loop; thescoped-smoke-loopMakefile target is removedSECURITY_LIMITATIONS.md— coverage limits are stated rather than implied away, and the loop skips rather than false-passes when a deployment registers only one toolThe six tests #324 shipped pass unchanged against the consolidated script — the fixture sets
ENABLE_DOGFOOD_TOOL=true, so the preferred-tool path still resolves without a pin. The ported tests cover tool selection (preferred order, pin validation, no silent fallback), companion selection (registered, distinct, single-tool skip), argument derivation (types, defaults, enums, schemaless), price parsing, both off-loopback guards, and--key-only.Risk
No trust-plane code is touched. One behavioral change to be aware of: a deployment registering neither
partner.echonorpartner.notes.writenow gets an actionable error naming its tools instead of a bare "not discoverable", and off-loopback runs require two flags they previously did not. Both are refusals to guess, and both surface as exit code 2 with a message naming the fix. Rollback is a revert.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
Generated by Claude Code
Note
Cursor Bugbot is generating a summary for commit 319b209. Configure here.