feat(miner-governor): fail-closed chokepoint composing every write calculator (#2340) - #5018
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | 8d09504 | Commit Preview URL Branch Preview URL |
Jul 11 2026, 09:17 AM |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/miner-governor-dry-run-default-2342 #5018 +/- ##
============================================================================
- Coverage 94.36% 94.29% -0.07%
============================================================================
Files 459 460 +1
Lines 39256 39312 +56
Branches 14317 14339 +22
============================================================================
+ Hits 37042 37069 +27
- Misses 1558 1582 +24
- Partials 656 661 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Caution 🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥 🛑 Gittensory review result - fixes requiredReview updated: 2026-07-11 09:12:51 UTC
🛑 Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Why this is blocked
CI checks failing
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
85f1e72 to
84b7f83
Compare
…lculator (#2340) The single decision point every miner write action (open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, generate_tests) must pass through. Composes the kill-switch (#2341) and dry-run-default (#2342) primitives with the previously-shipped pure calculators -- rate-limit (#2344), budget/turn/termination caps, non-convergence detection, self-reputation throttle, and self-plagiarism -- into one precedence ladder: global kill-switch > per-repo pause > dry-run > rate-limit > budget cap > non-convergence > reputation throttle > self-plagiarism > allow. Reputation-throttle and self-plagiarism extend beyond the issue's three explicitly-named calculators, per those two modules' own doc comments forward-referencing this exact chokepoint; both reuse their own already-reviewed boolean gate semantic (throttled/allowed) rather than inventing new policy. Both apply only to actionClass "open_pr" (their own ledger builders are submission-scoped). Any calculator that throws denies immediately with stage "internal_error", never falls through to allow. Pure engine module (no IO); the miner-lib wrapper owns persisting the ledger event and advancing rate-limit bucket state only when the rate-limit stage actually ran. Stacked on #2341 + #2342 (imports from both).
d5af343 to
8d09504
Compare
8138bc9
into
feat/miner-governor-dry-run-default-2342
…lculator (#2340) (#5018) The single decision point every miner write action (open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, generate_tests) must pass through. Composes the kill-switch (#2341) and dry-run-default (#2342) primitives with the previously-shipped pure calculators -- rate-limit (#2344), budget/turn/termination caps, non-convergence detection, self-reputation throttle, and self-plagiarism -- into one precedence ladder: global kill-switch > per-repo pause > dry-run > rate-limit > budget cap > non-convergence > reputation throttle > self-plagiarism > allow. Reputation-throttle and self-plagiarism extend beyond the issue's three explicitly-named calculators, per those two modules' own doc comments forward-referencing this exact chokepoint; both reuse their own already-reviewed boolean gate semantic (throttled/allowed) rather than inventing new policy. Both apply only to actionClass "open_pr" (their own ledger builders are submission-scoped). Any calculator that throws denies immediately with stage "internal_error", never falls through to allow. Pure engine module (no IO); the miner-lib wrapper owns persisting the ledger event and advancing rate-limit bucket state only when the rate-limit stage actually ran. Stacked on #2341 + #2342 (imports from both).
…lculator (#2340) (#5018) The single decision point every miner write action (open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, generate_tests) must pass through. Composes the kill-switch (#2341) and dry-run-default (#2342) primitives with the previously-shipped pure calculators -- rate-limit (#2344), budget/turn/termination caps, non-convergence detection, self-reputation throttle, and self-plagiarism -- into one precedence ladder: global kill-switch > per-repo pause > dry-run > rate-limit > budget cap > non-convergence > reputation throttle > self-plagiarism > allow. Reputation-throttle and self-plagiarism extend beyond the issue's three explicitly-named calculators, per those two modules' own doc comments forward-referencing this exact chokepoint; both reuse their own already-reviewed boolean gate semantic (throttled/allowed) rather than inventing new policy. Both apply only to actionClass "open_pr" (their own ledger builders are submission-scoped). Any calculator that throws denies immediately with stage "internal_error", never falls through to allow. Pure engine module (no IO); the miner-lib wrapper owns persisting the ledger event and advancing rate-limit bucket state only when the rate-limit stage actually ran. Stacked on #2341 + #2342 (imports from both).
…lculator (#2340) (#5018) The single decision point every miner write action (open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, generate_tests) must pass through. Composes the kill-switch (#2341) and dry-run-default (#2342) primitives with the previously-shipped pure calculators -- rate-limit (#2344), budget/turn/termination caps, non-convergence detection, self-reputation throttle, and self-plagiarism -- into one precedence ladder: global kill-switch > per-repo pause > dry-run > rate-limit > budget cap > non-convergence > reputation throttle > self-plagiarism > allow. Reputation-throttle and self-plagiarism extend beyond the issue's three explicitly-named calculators, per those two modules' own doc comments forward-referencing this exact chokepoint; both reuse their own already-reviewed boolean gate semantic (throttled/allowed) rather than inventing new policy. Both apply only to actionClass "open_pr" (their own ledger builders are submission-scoped). Any calculator that throws denies immediately with stage "internal_error", never falls through to allow. Pure engine module (no IO); the miner-lib wrapper owns persisting the ledger event and advancing rate-limit bucket state only when the rate-limit stage actually ran. Stacked on #2341 + #2342 (imports from both).
…okepoint (#2342, #2340) (#5014) * feat(miner-governor): dry-run-by-default enforcement (#2342) Resolves the miner's overall action mode with "safest wins" precedence mirroring resolveAgentActionMode (src/settings/agent-execution.ts): paused > dry_run > live. A freshly-configured miner (no opt-in anywhere) always defaults to dry_run, never live. Live mode requires an explicit, hard-to-fat-finger opt-in -- the exact string literal "live", never a boolean -- from either the operator's own global env var (GITTENSORY_MINER_LIVE_MODE) or the target repo's own .gittensory-miner.yml (MinerGoalSpec.execution.liveModeOptIn). The #2341 kill-switch always wins over any live-mode opt-in. Dry-run mode's would-be action is recorded to the governor ledger with a distinct dry_run decision, without ever executing. Stacked on #2341 (imports isMinerKillSwitchActive from kill-switch.ts). * fix(miner-governor): close codecov/patch coverage gaps flagged on #2342 Two branches were untested: MinerGoalSpec's execution.liveModeOptIn normalizer's "mapping present, key absent" path (distinct from the field being omitted entirely), and buildMinerDryRunGovernorLedgerEvent's repoFullName ?? null arm. Also narrows MinerExecutionPolicy.liveModeOptIn from `string | null` to the literal `typeof MINER_LIVE_MODE_OPT_IN | null`, so a typo in code constructing a MinerGoalSpec is caught at compile time rather than relying solely on the runtime exact-match check. * feat(miner-governor): fail-closed chokepoint composing every write calculator (#2340) (#5018) The single decision point every miner write action (open_pr, file_issue, apply_labels, post_eligibility_comment, create_branch, delete_branch, generate_tests) must pass through. Composes the kill-switch (#2341) and dry-run-default (#2342) primitives with the previously-shipped pure calculators -- rate-limit (#2344), budget/turn/termination caps, non-convergence detection, self-reputation throttle, and self-plagiarism -- into one precedence ladder: global kill-switch > per-repo pause > dry-run > rate-limit > budget cap > non-convergence > reputation throttle > self-plagiarism > allow. Reputation-throttle and self-plagiarism extend beyond the issue's three explicitly-named calculators, per those two modules' own doc comments forward-referencing this exact chokepoint; both reuse their own already-reviewed boolean gate semantic (throttled/allowed) rather than inventing new policy. Both apply only to actionClass "open_pr" (their own ledger builders are submission-scoped). Any calculator that throws denies immediately with stage "internal_error", never falls through to allow. Pure engine module (no IO); the miner-lib wrapper owns persisting the ledger event and advancing rate-limit bucket state only when the rate-limit stage actually ran. Stacked on #2341 + #2342 (imports from both). * test(miner-governor): close remaining branch-coverage gaps in chokepoint tests Measured coverage showed chokepoint.js at 84.44% branch despite every stage's fail-closed catch block already being exercised -- every existing error test threw a genuine TypeError (a real Error instance) via a null-cast input, so error instanceof Error was always true and the String(error) fallback arm never ran. Add a throwingProxy helper that throws a plain string instead, exercising that arm for all five calculator stages. Also covers the rateLimitRandomFn conditional-spread branch and the self-plagiarism similarity ?? null fallback (via a whitespace-only fingerprint, which denies with no computed similarity). chokepoint.js now measures 100/100/100 (lines/branch/funcs), up from 84.44% branch. No production code changes -- test-only. * test(miner-governor): close chokepoint.ts patch-coverage gap (#2340) test/unit/miner-governor-chokepoint.test.ts is the root-vitest file Codecov actually measures (packages/gittensory-engine's own node:test suite is invisible to it), and it only covered allow/kill_switch/dry_run/rate_limit before this change -- budget_cap, non_convergence, reputation_throttle, and self_plagiarism denies, their omitted-stage skips, the non-open_pr actionClass skip, and every stage's fail-closed internal_error catch (including the error-instanceof-Error vs String(error) fallback arms) were all untested. Add 21 tests covering: each deny path, each stage running-but-not-tripping, both single-stage-omitted skips plus the combined non-open_pr skip, the rateLimitRandomFn conditional-spread, the self-plagiarism similarity ?? null fallback (whitespace-only fingerprint), and both throw shapes (null-deref TypeError and a non-Error thrown value via a throwing Proxy) for all five calculator call sites. chokepoint.ts now measures 100/100/100/100 (statements/branches/functions/ lines), up from 44.64% patch coverage.
Summary
feat/miner-governor-dry-run-default-2342. The diff below is additive-only once the chain merges tomainin order.packages/gittensory-engine/src/governor/chokepoint.tsexportsevaluateGovernorChokepoint, the single decision point every miner write action must pass through before executing aLocalWriteActionSpec(src/mcp/local-write-tools.ts:open_pr,file_issue,apply_labels,post_eligibility_comment,create_branch,delete_branch,generate_tests).resolveAgentActionMode):global kill-switch (#2341) > per-repo pause (#2341) > dry-run (#2342) > rate-limit (#2344) > budget/turn/termination cap > non-convergence > self-reputation throttle > self-plagiarism > allow.self-plagiarism.ts: "the Governor open_pr chokepoint (maintainer: wire the fail-closed Governor chokepoint before every write action #2340) composes this verdict with rate-limit, budget caps, and non-convergence";reputation-throttle.ts: "the chokepoint can record WHY a submission cadence was scaled"), and both already ship a*LedgerEventbuilder keyed on their own booleanthrottled/allowedfield — reusing an existing, already-reviewed gate semantic rather than me inventing new policy inside a safety-critical module. Both stages apply only whenactionClass === "open_pr"(their own ledger builders are submission-scoped; a label-apply or branch-delete has no diff fingerprint or "submission cadence" to throttle) and are entirely skippable — omitting their inputs (reputationHistory/selfPlagiarismCandidateleftundefined) cleanly skips the stage rather than erroring.stage: "internal_error"and never falls through toallow. Caught and fixed a real bug during testing (see Notes).packages/gittensory-miner/lib/governor-chokepoint.jsis the stateful wrapper: it persists the returned ledger event and advances/backs-off the rate-limit bucket state, but ONLY when the rate-limit stage actually ran (kill-switch and dry-run short-circuit before rate-limit is ever evaluated, so bucket state is provably untouched in those cases — tested explicitly).Scope
CONTRIBUTING.md.Validation
git diff --checknpm run typecheck— clean.npm run build+npm run testinpackages/gittensory-engine— 399/399 passing. Newchokepoint.test.ts(21 tests) covers: full allow path; kill-switch (both scopes) short-circuiting before any later stage runs; dry-run short-circuiting before rate-limit; each resource stage (rate-limit, budget-cap incl. the termination/kill_switcheventType, non-convergence) denying and halting the ladder; reputation-throttle denying (and its own fail-open-on-insufficient-history behavior reaching allow); self-plagiarism denying a losing near-duplicate claim; both submission-scoped stages being skipped entirely for a non-open_praction even with inputs that would otherwise deny; and one fail-closed test per calculator (5 total) proving a thrown error denies withstage: "internal_error".npm run buildinpackages/gittensory-miner(node --check, including the newgovernor-chokepoint.js) — clean.vitest run test/unit/miner-governor-chokepoint.test.ts— 4/4 passing (ledger persistence + bucket-mutation-only-when-rate-limit-ran, verified for the allow path, a kill-switch denial, a dry-run shadow, and a rate-limit denial).npm run test:coverage,test:workers,build:mcp,test:mcp-pack,ui:*,npm audit— not re-run locally; this PR touches onlypackages/gittensory-engine/**,packages/gittensory-miner/**, andtest/unit/**, and perCONTRIBUTING.mdonlysrc/**is Codecov-measured. CI runs them authoritatively.Safety
nullcast, the same technique a genuinely malformed upstream caller could produce) into each of the 5 composable calculators and confirm the chokepoint denies withstage: "internal_error"rather than silently skipping to the next stage or allowing.LocalWriteActionSpeccall site — that live actuation wiring is separate, later, maintainer-owned work (maintainer: wire the submission-gate trigger into the harness driving loop #2337).Notes
if (input.reputationHistory)), which treats a caller-suppliednullidentically to an intentionally-omittedundefined— silently SKIPPING the stage instead of failing closed on a bad value, exactly the wrong direction for a safety-critical gate. Two of my own fail-closed tests caught this (true !== falseon theallowedassertion) before I'd even finished the PR. Fixed by switching both guards to!== undefined(presence, not truthiness) so any caller-supplied value other than a genuinely-omitted key reaches the calculator and, if unusable, hits the fail-closed catch. Leaving this in the PR description since it's exactly the kind of subtle correctness bug this issue exists to prevent.