Repository navigation
feat(scripts): let CI configure the smoke loop entirely through env - #328
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Limit details: You’ve used the included review currently available. Your 92 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 (3)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ 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 20dc3eb. Configure here.
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
--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
…MOKE test_environment_alone_satisfies_the_off_loopback_guard pointed at api.thisisatest.tech, so every pytest run — including pull-request CI — opened a real HTTPS session to production, and a slow reject would surface as TimeoutExpired rather than a configuration assertion. That is precisely the unattended production traffic the guard under test exists to make deliberate. It now targets a reserved .invalid host (RFC 6761): still non-loopback, so both guards apply, but it never resolves. test_tool_args_resolve_from_flag_environment_or_neither asserted that _resolve_tool_args(None) returns None without clearing CI_SMOKE_TOOL_ARGS. The resolver reads the real environment, so an ambient value made the assertion fail even though the flag path was correct. Running the suite under a deliberately hostile environment to check that second fix surfaced a third problem, which the new fallback made reachable: test_constant_test_loop_never_logs_api_key passes **os.environ through to the subprocess, so an exported CI_SMOKE_TOOL_ARGS is forwarded as the tool's arguments and the run dies with "_write_note() got an unexpected keyword argument 'ambient'" — a message that points at the tool rather than at the stray variable. The fragility predates this branch (an ambient CI_SMOKE_WALLET_ID would mislead the same way) but only became reachable once the payload gained an environment path. That test now strips CI_SMOKE_* before setting the three values it means to set, matching what the other subprocess tests here already do. Verified with CI_SMOKE_TOOL_ARGS, CI_SMOKE_TOOL and CI_SMOKE_WALLET_ID all exported to junk: 19 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
1b64b50 to
31064f0
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31064f07e2
ℹ️ 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".
| try: | ||
| parsed = json.loads(raw) | ||
| except json.JSONDecodeError as exc: | ||
| raise ConfigurationError(f"{source} is not valid JSON: {exc}") from exc |
There was a problem hiding this comment.
When CI_SMOKE_TOOL_ARGS contains NaN, Infinity, or -Infinity, Python's json.loads accepts these non-standard constants, so the value passes this validation as a dictionary. Once the configured tool is invoked, HTTPX refuses to serialize the payload with an uncaught ValueError, producing a traceback and exit 1 instead of the documented configuration error and exit 2. Configure json.loads with a rejecting parse_constant callback and cover these malformed-input cases.
AGENTS.md reference: AGENTS.md:L83-L101
Useful? React with 👍 / 👎.
This branch was successfully deployed
2 participants
Summary
--tooland--other-toolalready fall back to$CI_SMOKE_TOOLand$CI_SMOKE_OTHER_TOOL.--tool-argshad no equivalent, so a CI job could satisfy every part of the off-loopback guard except the payload, and had to reach for argv for the one remaining piece — the wrong place for a secret-adjacent value, and enough to make the guard read as an obstacle rather than a check.This is not cosmetic. #327 wires the loop into CI, and its production step (
ci.yml) invokes it with neither flag. The guard I added in #326 exits 2 before any network call, so that step fails on every push tomain:Configuring that step through secrets is only possible once the payload has an environment path. That break is mine — #326 tightened the interface after #327 was written against the old one.
Behavior
--tool-argsfalls back to$CI_SMOKE_TOOL_ARGS; an explicit flag still wins.--tool-args '{}'does, so it satisfies the guard rather than falling through to "unset".ConfigurationError(exit 2), neverSmokeTestFailure(exit 1) — invalid JSON, or valid JSON that isn't an object, means the loop was configured wrong, not that the trust plane broke. The message names which source was bad, since the flag and the variable fail identically otherwise.Dropping argparse's
type=json.loadsis part of that: it exits 2 for bad JSON too, but with usage text that never says what was wrong with the payload.Type of Change
Validation
pytest -q— 19 loop tests (15 pre-existing, unchanged) plus 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 applicableCI_SMOKE_TOOLandCI_SMOKE_TOOL_ARGSset, the run reaches real authentication (403 on the placeholder) rather than either guard; a malformed value reports$CI_SMOKE_TOOL_ARGS is not valid JSONat exit 2docs/constant-test-loop.mdgains a flag/variable table and states the exit-code contract for a malformed payloadSECURITY_LIMITATIONS.md— no trust-plane behavior changes; this is argument resolution in a client scriptNew tests cover flag-beats-env precedence, the env fallback, an explicitly empty payload counting as a decision, malformed JSON and non-object JSON as configuration errors with the source named, and an end-to-end run configured purely through environment getting past both guards.
Risk
Additive: a new fallback where there was none, so existing invocations behave identically. The one behavior change is that malformed
--tool-argsnow reports what was wrong instead of argparse usage text — same exit code, better message. Rollback is a revert.Follow-up for #327
With this merged, #327's production step can be configured entirely through repository secrets:
Worth confirming
partner.echois actually registered on production before relying on it — I've only ever exercised this loop against a local instance.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ri576RW3ocgqpx1xYN9vBm
Generated by Claude Code
Note
Cursor Bugbot is generating a summary for commit 20dc3eb. Configure here.