fix(safety): kill switches become ratchets; stop a dry-run GitHub write; breaker covers content-inspection closes#9115
Conversation
…rite, subject content-inspection closes to the breaker #9049 -- VERIFIED LIVE. The global default private-config layer carries `agentDryRun: false`, and resolveEffectiveSettings merges as `{...dbSettings, ...manifestSettings}`, so the manifest wins. Setting `agent_dry_run=1` in the DB -- via the dashboard, PUT /settings, `loopover-mcp maintain`, or the loopover_set_agent_dry_run MCP tool -- was therefore SILENTLY DISCARDED on every read, for every repo. The tool writes the raw row back and never re-resolves effective settings, so the operator got a success message while ORB kept making real GitHub writes. A kill switch that reports success and does nothing is worse than one that does not exist. Both agentPaused and agentDryRun are now RATCHETS: `true` from EITHER layer wins. Config-as-code can still pause or dry-run a repo (tightening -- the whole point of the feature); it can no longer un-pause or un-dry-run what an operator set. Every other setting keeps manifest-wins semantics unchanged. #9067 -- the structural invariant is that every installation write routes through makeInstallationOctokit, whose request hook suppresses mutations when mode !== "live". dispatchVisualCaptureFallback escapes it with a raw timeoutFetch POST to .../dispatches -- a real write that starts an Actions run and burns CI minutes in the target repo. runVisualCapture short-circuits only on `paused`, so under `dry_run` it fired for real, contradicting the documented dry-run contract. The mode is now threaded onto CaptureTarget and the writer itself refuses anything that is not "live", with an absent mode treated as non-live -- so the guarantee is structural rather than dependent on each caller remembering a check. (This compounded with #9049: dry-run was defeated fleet-wide, which masked the misbehavior rather than preventing it.) #9086 (breaker half) -- downgradeCloseToHold keyed on `closeKind === "heuristic"` alone, so all six non-heuristic close paths were returned untouched by both the project breaker and the per-rule precision check. The "deterministic => zero-hallucination" justification holds for the IDENTITY-based closes (blacklist, contributor_cap read facts about an account) but not for the CONTENT-INSPECTION ones, whose determinism is a regex heuristic over free-text markdown -- and screenshot_table is our top close reason all-time. A deterministic rule can still be systematically wrong, which is what the breaker exists to catch. screenshot_table / review_nag / copycat are now breaker-eligible; identity-based closes keep their exemption. POLICY REVERSAL, called out explicitly: this inverts an existing test asserting screenshot_table is breaker-exempt. A companion test pins the identity-based exemptions so the narrowing is visible in both directions. Closes #9049 Closes #9067 Closes #9086 Tests: 5 ratchet cases through the REAL parseFocusManifest path (manifest false cannot clear either DB flag; manifest true still tightens; both-off stays off; absent key leaves the DB value intact in both directions), 2 dry-run refusal tests (dry_run and absent-mode both refuse to dispatch), and 2 breaker tests (screenshot_table now downgrades; blacklist/contributor_cap still exempt). 100% line+branch coverage on all 55 added lines; 1806/1806 across 8 suites; all drift gates green.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9115 +/- ##
==========================================
+ Coverage 90.56% 93.47% +2.91%
==========================================
Files 96 717 +621
Lines 22490 58253 +35763
Branches 3884 20624 +16740
==========================================
+ Hits 20367 54452 +34085
- Misses 1945 2899 +954
- Partials 178 902 +724
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-26 21:22:27 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
…ontracts
CI caught what my local runs missed: I had only been running test/unit, so the
test/integration suites that exercised the OLD fail-open ingest default were
never exercised locally. Fixed properly rather than by weakening the change:
- test/integration/{orb,ams}-ingest.test.ts: every request-shape test
(200/400/413, and the dropped-connection 413 regression) now presents a real
credential, because those assertions are about BODY handling and would
otherwise stop at the new 401. The two "optional collector token" tests are
rewritten to assert the actual new contract -- unset ⇒ 401, not 200.
- test/integration/orb-oauth.test.ts: the path-specificity test proves a request
routes PAST the callback checks into the ingest handler's body handling, so it
needs a credential to still reach that point.
- test/unit/actions-fallback.test.ts: the two direct dispatcher tests pass
mode: "live", plus a new test pinning the refusal for dry_run, paused, and an
absent mode.
Also verified the full local suite (unit + integration, 22,146 passing) after
building every artifact test:ci builds first -- the 327 failures I initially saw
were entirely missing MCP/miner/discovery-index/ui-kit bundles in a fresh
worktree, not this diff.
1 participant
Summary
Three ways a safety control was silently bypassed. The first is live right now.
#9049 — the dry-run kill switch is defeated fleet-wide. The global default private-config layer carries
agentDryRun: false, andresolveEffectiveSettingsmerges as{...dbSettings, ...manifestSettings}— so the manifest wins. Settingagent_dry_run=1in the DB (dashboard,PUT /settings,loopover-mcp maintain, or theloopover_set_agent_dry_runMCP tool) was silently discarded on every read, for every repo. The tool writes the raw row back and never re-resolves effective settings, so you got a success message while ORB kept making real GitHub writes.Both
agentPausedandagentDryRunare now ratchets:truefrom either layer wins. Config-as-code can still pause or dry-run a repo — that's the point of the feature — it just can't un-pause what an operator set. Every other setting keeps manifest-wins semantics.#9067 — a real GitHub write escaped the dry-run suppression. The structural invariant is that every installation write routes through
makeInstallationOctokit, whose request hook suppresses mutations when mode ≠live.dispatchVisualCaptureFallbackescapes it with a rawtimeoutFetchPOST to.../dispatches— which starts an Actions run and burns CI minutes in the target repo.runVisualCaptureshort-circuits only onpaused, so underdry_runit fired for real.The mode is now threaded onto
CaptureTargetand the writer itself refuses anything notlive, with an absent mode treated as non-live — so the guarantee is structural rather than dependent on each caller remembering a check. (This compounded with #9049: dry-run being defeated fleet-wide masked this rather than preventing it.)#9086 (breaker half) — six close paths skipped the precision breaker.
downgradeCloseToHoldkeyed oncloseKind === "heuristic"alone. The "deterministic ⇒ zero-hallucination" justification holds for identity-based closes (blacklist,contributor_capread facts about an account) but not for content-inspection ones, whose determinism is a regex heuristic over free-text markdown — andscreenshot_tableis our top close reason all-time. A deterministic rule can still be systematically wrong; that's what the breaker is for.screenshot_table/review_nag/copycatare now breaker-eligible; identity-based closes keep their exemption.Closes #9049
Closes #9067
Closes #9086
screenshot_tableis breaker-exempt. A companion test pins the identity-based exemptions, so the narrowing is visible in both directions rather than looking like a blanket removal..loopover.ymlcan no longer turn off a DB-set pause/dry-run. If any repo currently relies on that to opt out, it will now stay paused — worth a scan before merging, though the live config only setsagentDryRun: false, which is exactly the value this is meant to stop honoring.Scope note
#9086 also asks to move the five anti-abuse short-circuits below the CI-pending settle, and lists three latent false-positive classes in the screenshot gate. Those are behavior changes to the gate itself rather than to the breaker, so I've left #9086's remaining items for a follow-up rather than bundling ordering changes into a safety PR.
Test plan
parseFocusManifestpath: manifestfalsecannot clear either DB flag; manifesttruestill tightens; both-off stays off (no accidental fleet-wide pause); an absent key leaves the DB value intact in both directionsdry_runand absent-mode both refuse to dispatchscreenshot_tablenow downgrades to a hold;blacklist/contributor_capstill exemptmode: "live"— they were failing precisely because the new refusal works