refactor(queue): put the six PR-command handlers behind one shared prologue (#9541) - #9580
Merged
Merged
Conversation
…ologue (#9541) Deliverable 1 of #9541. Behaviour-preserving: the full suite passes 23,919 tests with NO test modified, which is the property requirement 1 asks for. Six handlers — resolve, review, pause, resume, explain, generate-tests — opened with a byte-identical eleven-step sequence: parse, name guard, classify, skip-if-unclassifiable, target key, redelivery guard, load PR + settings, skip-if-no-PR, authorize, record-and-stop-if-denied. Copy-pasted six times, 30 to 300 lines apart inside a 16,000-line file. Confirmed identical by extracting each one's step order first and diffing them, not by eye. That distance is the whole defect mechanism, and it has fired twice in a week: - #9312 added the redelivery guard to five of the six and missed `resolve`, which then wrote a SECOND permanent review-memory suppression row per finding on every queue retry until #9561 caught it. - #9562 found the two PR-panel twins missing the same guard, for a paid model call. src/queue/pr-command-prologue.ts now owns the sequence once, with the IO injected so the seam is directly testable without a webhook or a database. WHAT IT DELIBERATELY DOES NOT OWN The response to each step. Every handler still supplies its own audit event names and skip/denied recorders, because those strings are its public contract — operators query `github_app.finding_resolved_skipped` and tests assert on it. Centralising them would be a behaviour change wearing a refactor's clothes. Two orderings are load-bearing and preserved exactly: the redelivery guard runs BEFORE the loads (a replay costs no database reads), and targetKey is derived from the classified request, since the unclassifiable path reports against req.targetKey, which may legitimately be null. ONE REAL DIVERGENCE, made explicit rather than smoothed over generate-tests carried an extra `pr.state !== "open"` step. That is a named policy, not an accident — a command that spends AI generation and attempts a branch commit must not run on a closed PR, and both PR-panel twins carry the identical guard (their comments say so). It becomes `requireOpenPr`, opt-in, so read-only commands (pause/resume/explain) keep working on a closed PR exactly as before. It runs before authorization, so a closed PR costs no miner lookup. `notMine` and `handled` are separate outcomes on purpose: a handler returns false on the first (keep dispatching to siblings) and true on the second (this was ours, it is done). Collapsing them into one falsy result is how a command silently stops reaching its siblings. scripts/check-command-redelivery-guards.ts now accepts delegation to the prologue as satisfying the guard, so the cheapest way to pass the check is also the structurally correct one. 14 direct tests on the seam: 100% statement and branch coverage, including both distinct-outcome arms, the guard-before-loads ordering, the all-fields-absent classifier result, and requireOpenPr's opt-in and pre-authorization placement.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 12:06:11 UTC
Review summary Nits — 4 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
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.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9580 +/- ##
==========================================
- Coverage 89.62% 89.62% -0.01%
==========================================
Files 868 869 +1
Lines 110876 110835 -41
Branches 26362 26349 -13
==========================================
- Hits 99374 99333 -41
Misses 10237 10237
Partials 1265 1265
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
Rebase onto main after #9579 and #9580 landed. The newly-enabled noUnusedLocals immediately flagged twelve dead symbols in code merged since this PR opened -- including two I left in #9580 myself: - src/queue/processors.ts: PrCommandPrologueOutcome imported but unused (the adapter's annotated return type was dropped in favour of inference), plus eight unused bindings in the prologue destructures -- handlers that do not need `pr`, `settings`, `authorization` or `command` were still pulling them out. - src/queue/pr-command-prologue.ts: LoopOverMentionCommandName, superseded by LoopOverActionCommandName once the spec narrowed to action verbs. - src/mcp/dispatch-telemetry-sink.ts: an unused McpToolCallTelemetry import from #9579. Which is the point of the PR: the flags catch dead code at the commit that introduces it rather than at the next audit. Zero behaviour change -- every removal is a binding or an import TypeScript proved unreferenced, and the suite passes 24,014 tests. The rebase conflict itself was in processors.ts's import block: main added the pr-command-prologue import on the same lines this PR removed the unused runRetentionPrune one. Both intents kept.
9 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…#9553, #9570, #9571, #9572) (#9573) * build(typescript): enable noUnusedLocals/noUnusedParameters repo-wide (#9553) Dead code is the substrate every drift bug in the 2026-07-27 audit grew on: a stale import or an orphaned constant reads exactly like a live wire, so the next person greps, finds it, and reasons about a code path that no longer runs. Enabling the flags made the compiler enumerate all 515 instances. Every one in src/** and packages/** was traced to its replacement before deletion -- all 82 were genuine supersession leftovers, no behaviour bug hiding among them -- but the triage turned up three real problems that were invisible under the noise: 1. src/queue/processors.ts -- sweepRepoBacklogConvergence accepted `requestedBy` ("schedule" | "api" | "test") and dropped it. Every sibling sweep stamps it into recordAuditEvent's metadata; both agent.sweep.backlog_convergence events here omitted it, so those records could not be attributed to a schedule vs a manual API trigger. Now wired into both. (Note the mechanical fix would have been to rename it `_requestedBy`, which cements the gap instead of closing it.) 2. test/unit/openapi.test.ts -- the #9302 REST<->MCP parity guard asserted against src/mcp/server.ts's gatePrecisionOutputSchema and maintainerMeasurementReportOutputSchema, which the tools stopped registering when #9518 moved their outputs to @loopover/contract. The shapes are still identical, so nothing had drifted YET -- but the guard was watching objects no runtime reads and would not have caught a future contract change. Re-anchored onto GetGatePrecisionOutput.shape / GetOutcomeCalibrationOutput.shape, which is what the tools actually register, and what that file's own header comment already claimed it did. 3. src/github/resolve-command.ts was reachable only from its own test, because src/review/review-memory-wire.ts carried a SECOND byte-identical copy of normalizeResolveFindingRef (regex included) and that copy was the one production used. Two independent implementations of the same public-safety validation, free to drift. Deduped onto the original via re-export; dead-source-files:check now passes on a file it was about to start failing on. Also corrects packages/loopover-engine/src/scoring/preview.ts's header, which claimed a ReDoS guarantee via a hasUnsafeWildcardCount import that had been dead since 625e236 deduped its label matching onto label-match.ts. The guarantee is real and unchanged; it now arrives through labelMatchesPattern, and the comment says so. Pre-existing import-specifier violations fixed in the same pass, since the tree has to be green for the flags to mean anything: - scripts/actionlint.ts imported a `.ts` specifier (TS5097) - test/unit/contract-registry.test.ts had three `.js` specifiers in a Bundler zone - check-dead-source-files-script.test.ts tripped check-import-specifiers on its own string FIXTURES, the same self-referential false positive that checker's ALLOWED_FILENAMES already documents for its own test Mechanics: unused parameters are renamed with a leading underscore, never deleted -- they are positional, so removing one silently re-binds every later argument. Everything else was removed by its real TypeScript AST node span (a regex pass was tried first and mis-bounded declarations badly enough to produce unparseable files). Full suite green: 23,894 passed, 0 failed. tsc clean with the flags on. * chore(engine): bump to 3.15.4 for the dead type-import removal in gate-advisory.ts check-engine-parity holds the two hand-duplicated gate-decision twins (packages/loopover-engine/src/advisory/gate-advisory.ts and src/rules/advisory.ts) in lockstep: touching one without the other requires an engine version bump. That is the mechanism, and it is doing its job here. the engine twin. They are genuinely dead there and NOT in the host: the engine copy is a deliberately slimmed re-implementation (#4881) that omits buildIssueAdvisory / addIssueFindings / collisionClustersForPull, which are what use those types on the host side. So there is no matching host edit to make -- the asymmetry is correct, and the version bump is the sanctioned way to record it. No behaviour change: type-only imports are erased at compile time. The bump exists so the parity contract stays enforceable, not because the gate decides anything differently. packages/loopover-miner/expected-engine.version moves in lockstep, as its own check requires. * fix(scripts,mcp): restore actionlint's `.ts` specifier and drop a dead shape #9565 added Two rebase follow-ups after #9565 and #9574 landed. 1. scripts/actionlint.ts gets its `.ts` extension back. This PR had removed it to satisfy check-import-specifiers, which broke the script outright -- it runs under `node --experimental-strip-types`, whose ESM resolver does no extension resolution, so the process dies at startup with ERR_MODULE_NOT_FOUND. #9565 independently reached the same conclusion and added TYPE_STRIPPED_ENTRYPOINTS to the checker for exactly this file, so the extension is now permitted where it is required. Verified by running `npm run actionlint`, which fails before this change and passes after. #9565's version of the checker is taken wholesale over this PR's: it solves the same two problems (that entrypoint set, plus allowlisting check-dead-source-files-script.test.ts for its string fixtures), and re-litigating a file main just rewrote buys nothing. 2. src/mcp/server.ts's `loginRepoPullShape` is removed -- dead on arrival in #9565, and the first thing the newly-enabled noUnusedLocals caught on main. Which is the point of this PR: dead code now surfaces at the commit that introduces it rather than at the next audit. The engine bump lands at 3.16.1 (main released 3.16.0 while this was open). It is required by check-engine-parity: this PR removes two dead TYPE-only imports from packages/loopover-engine/src/advisory/gate-advisory.ts, and the parity contract holds that file in lockstep with its host twin src/rules/advisory.ts. There is no matching host edit to make -- the engine copy is a deliberately slimmed re-implementation (#4881) omitting the functions that use those types -- so the version bump is the sanctioned way to record a one-sided change. No behaviour change: type-only imports are erased at compile time. packages/loopover-miner/expected-engine.version moves with it, as its own check requires. * chore(release): sync .release-please-manifest.json to the 3.16.1 engine bump The manifest is a generated artifact that must move with any package.json version, and release-manifest:sync:check fails CI when it drifts. Regenerated with the repo's own `npm run release-manifest:sync` rather than hand-edited. * chore: prune the dead symbols the new flags caught in newly-merged code Rebase onto main after #9579 and #9580 landed. The newly-enabled noUnusedLocals immediately flagged twelve dead symbols in code merged since this PR opened -- including two I left in #9580 myself: - src/queue/processors.ts: PrCommandPrologueOutcome imported but unused (the adapter's annotated return type was dropped in favour of inference), plus eight unused bindings in the prologue destructures -- handlers that do not need `pr`, `settings`, `authorization` or `command` were still pulling them out. - src/queue/pr-command-prologue.ts: LoopOverMentionCommandName, superseded by LoopOverActionCommandName once the spec narrowed to action verbs. - src/mcp/dispatch-telemetry-sink.ts: an unused McpToolCallTelemetry import from #9579. Which is the point of the PR: the flags catch dead code at the commit that introduces it rather than at the next audit. Zero behaviour change -- every removal is a binding or an import TypeScript proved unreferenced, and the suite passes 24,014 tests. The rebase conflict itself was in processors.ts's import block: main added the pr-command-prologue import on the same lines this PR removed the unused runRetentionPrune one. Both intents kept.
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
1 participant
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.
Partially addresses #9541 — deliverable 1 of 3.
Summary
Six handlers —
resolve,review,pause,resume,explain,generate-tests— opened with a byte-identical eleven-step sequence: parse → name guard → classify → skip-if-unclassifiable → target key → redelivery guard → load PR + settings → skip-if-no-PR → authorize → record-and-stop-if-denied. Copy-pasted six times, 30 to 300 lines apart inside a 16,000-line file.I confirmed they were identical by extracting each one's step order and diffing, not by eye:
That distance is the whole defect mechanism, and it has fired twice in the last week:
resolve, which then wrote a second permanent review-memory suppression row per finding on every queue retry, until queue: @loopover resolve and gate-override are missing #9312's webhook-redelivery guard #9561 caught it.src/queue/pr-command-prologue.tsnow owns the sequence once, with its IO injected so the seam is directly testable without a webhook or a database.Behaviour-preserving, as requirement 1 demands
The full suite passes 23,919 tests with no test modified. That is the property the requirement asks for, and it is the main evidence here — a refactor that needed its tests edited would not be one.
Two orderings are load-bearing and preserved exactly:
targetKeyis derived from the classified request, because the unclassifiable path reports againstreq.targetKey, which may legitimately be nullWhat it deliberately does not own
The response to each step. Every handler still supplies its own audit event names and skip/denied recorders, because those strings are its public contract — operators query
github_app.finding_resolved_skipped, and tests assert on it. Centralising them would be a behaviour change wearing a refactor's clothes.One real divergence, made explicit rather than smoothed over
generate-testscarried an extrapr.state !== "open"step. That is a named policy, not an accident: a command that spends AI generation and attempts a branch commit must not run on a closed PR, and both PR-panel twins carry the identical guard (their own comments say so).It becomes
requireOpenPr, opt-in — sopause/resume/explainkeep working on a closed PR exactly as before — and it runs before authorization, so a closed PR costs no miner lookup.notMinevshandledKept as distinct outcomes on purpose. A handler returns
falseon the first (keep dispatching to siblings) andtrueon the second (this was ours, it is finished). Collapsing them into one falsy result is exactly how a command silently stops reaching its siblings — so the type makes that impossible to write by accident.The checker now steers toward the seam
scripts/check-command-redelivery-guards.ts(added in #9567) accepts delegation to the prologue as satisfying the guard. A handler that delegates cannot skip it, because the sequence is no longer the handler's to get wrong — so the cheapest way to pass the check is also the structurally correct one.Testing
14 direct tests on the seam, 100% statement and branch coverage, including:
notMinevshandled)missing_repo_pr_installation_or_actoris precisely that case) still recording a well-formed skip with explicit nullsrequireOpenPr's opt-in behaviour and its pre-authorization placementneedsMinerDetectionthreaded verbatim — flipping it silently denies confirmed miners, since no other role could match themRemaining in #9541
Deliverable 2 shipped as #9557. Deliverable 3 — the plan-and-execute pass into its own module with a required, typed decision-pass context — is not in this PR; it is a separate extraction with its own risk profile, and requirement 2 is explicit that structural changes land apart from anything behavioural.