feat(mt#4995): Classify a failed expectedHeadSha match instead of waiting out the timeout - #3641
Merged
Merged
Conversation
…ting out the timeout A mismatch between `expectedHeadSha` and the observed remote head has two causes with opposite remedies, and until now both were reported identically after the full `timeoutSeconds`: a commit still in flight (waiting resolves it) and a sha the caller constructed (waiting never does). The second ends in `expectedHeadShaUnreached`, which per /implement-task §9 reads as reviewer silence — the documented lead-in to the bypass ladder. `classifyHeadShaMismatch` separates them on the first poll that observes a head: a shared prefix of >= MIN_ABBREVIATED_SHA_LENGTH (7) followed by divergence is the signature of a real abbreviated sha padded out to look full, which is the originating incident (PR #3635: `session_commit` returned the 9-character `f76e55628`; the caller passed those 9 characters plus 31 invented ones). Two unrelated commits collide on 7 hex characters once in 16^7 = 268,435,456. - `divergent-prefix` returns immediately via `finalizeTimeout`, which is already bounded and cannot mis-report a match (its `finalMatch` is gated on the same predicate that is false here), so the caller still gets the fresh reviews list and check-run state. - `push-pending` is unchanged: it keeps polling, which is the branch mt#3877 and mt#4039 exist to protect. The text-mode message drops the generic two-cause remedy on the classified case — one of its two causes has been ruled out by evidence — and names the padding mistake plus the fix (pass `commitHash` verbatim; it is matched as a prefix by design).
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The classification and early-return behavior are well-implemented and comprehensively tested, and adapter messaging updates are appropriate. I’m requesting changes for one normative gap: SC5 in the task spec requires existing tests to pass untouched, but two prior assertions were modified to include the new classification field. Please reconcile by amending the spec or by adjusting the approach/tests to avoid altering existing tests. I also left several non-blocking nits about structured signaling for early-return vs true timeout, classification nullability, helper export scope, adapter branching robustness, and numeric-constant pinning in tests.
Findings
- [BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-push-lag.test.ts:174 — Spec SC5 requires existing tests to pass untouched; this PR modifies two existing assertions
Task spec SC5 states: “push-pending behaviour is unchanged … and its tests pass untouched.” This diff edits two prior tests to includeclassification: "push-pending"in exact-matchtoEqualassertions at packages/domain/src/session/commands/pr-wait-for-review-push-lag.test.ts:174-181 and :235-242. While semantics are preserved, the criterion is normative and unambiguous about not modifying existing tests. Please either (a) amend the task spec to relax SC5 (documenting the additive payload change) or (b) refactor so prior tests do not need edits (e.g., shape-superset assertions or preserving legacy shape via adapter/compat), and revert these test changes. - [NON-BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1169 — Early-return path reuses the “timeout” result shape without a distinct indicator for “returned early due to classification”
At lines 1169-1190 the code returnsawait finalizeTimeout()on the first observed head whenmismatchKind() === "divergent-prefix". This produces aSessionPrWaitForReviewTimeoutpayload with a lowelapsedMs, but no explicit flag that this was an early classification vs a genuine timeout. Downstream consumers that key offmatched: false+ timeout shape may incorrectly treat it as a waited-out timeout. The adapter’s message text disambiguates it, but callers that don’t render the text (e.g., JSON-mode consumers) may miss the semantic distinction. Consider adding a structured indicator (e.g.,timeoutCause: "divergent-prefix" | "deadline-exceeded") or a booleanreturnedEarly: trueto make this machine-discernible. - [NON-BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1118 —
expectedHeadShaUnreached.classificationcan benulleven in mismatch-looking payloads
buildTimeoutResultunconditionally includesexpectedHeadShaUnreachedwheneverexpectedHeadShais defined (line 1175), and setsclassification: mismatchKind(). BecauseheadShaMatchesExpectedreturns true when either side is undefined (per the comment at 1089-1093),mismatchKind()can benullin cases where the head sha is absent, yielding a payload withclassification: null. This is by design for AT4, but it may surprise consumers that rely onclassificationbeing a non-null discriminator whenexpectedHeadShaUnreachedis present. If intentional, consider documenting the nullability explicitly in the adapter layer, or gateexpectedHeadShaUnreachedon an actual observed mismatch. - [NON-BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:274 —
classifyHeadShaMismatchexported publicly but only used in tests; consider scoping
classifyHeadShaMismatchandHeadShaMismatchKindare exported from the command module but have no in-repo production consumers outside tests. If not intended as a public API, consider making themconst/typeinternal (non-exported) or moving to a private helper to avoid expanding the public surface unnecessarily. If you do intend external reuse, adding a barrel export and doc would help clarify its status. - [NON-BLOCKING] src/adapters/shared/commands/session/pr-wait-for-review-command.ts:112 — Adapter message branching depends on
classificationequality; lack of default could miss future kinds
The adapter’sformatTimeoutMessagebranches only forclassification === "divergent-prefix"and otherwise falls back to the generic text. If moreHeadShaMismatchKindvariants are added later, they will silently render the generic copy. Consider aswitchover known kinds with an explicitdefaultthat mentions the unrecognized classification, to aid future evolution. - [NON-BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:63 — Tests pin
MIN_ABBREVIATED_SHA_LENGTHto 7; this hard-codes a constant across modules
At lines 95-103 the test assertsMIN_ABBREVIATED_SHA_LENGTHis exactly 7. While this guards SC4’s reasoning, it couples the test to an internal constant rather than the behavior contract. If the floor changes (e.g., policy/tuning), multiple tests will need updates. Consider asserting behavior via shared-prefix lengths relative toMIN_ABBREVIATED_SHA_LENGTHwithout asserting its numeric value directly.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — mismatch is classified, not just reported. | Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:251-334 adds HeadShaMismatchKind and classifyHeadShaMismatch, and :534-548 extends expectedHeadShaUnreached to include classification. The poll loop plumbs it via mismatchKind() in buildTimeoutResult at :1169-1178. Tests exercise both branches in packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:68-90, 124-152. |
SC2 — the divergent-prefix case does not wait out the timeout. |
Met | Early-return path in packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1203-1290 returns finalizeTimeout() on first observed head when mismatchKind() === "divergent-prefix". Test asserts prompt return with pollCount: 1 and elapsedMs < 1000 at packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:124-152. |
| SC3 — the message names the likely cause and the remedy. | Met | Adapter message in src/adapters/shared/commands/session/pr-wait-for-review-command.ts:112-147 branches on classification === "divergent-prefix" to name the extended-abbreviation cause and remedy; generic branch remains for other cases. Test covers the specific copy at src/adapters/shared/commands/session/pr-wait-for-review-command.test.ts:236-283. |
| SC4 — the discriminator's false-positive rate is argued, not assumed. | Met | Rationale comment with 16^7 reasoning in packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:301-333. Tests pin the boundary and negative control at packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:95-106 (threshold) and :109-120 (fixtures do not accidentally trip classifier). |
SC5 — push-pending behaviour is unchanged. The existing wait-through-the-lag path keeps its current semantics and its tests pass untouched. |
Not Met | Behavioral semantics retained (AT3 in packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:154-171 asserts long polling), but two existing exact-match assertions were edited to include the new classification: "push-pending" field: packages/domain/src/session/commands/pr-wait-for-review-push-lag.test.ts:174-181 and :235-242. The criterion’s literal "tests pass untouched" is therefore not met. Either amend the task spec to accept additive payload shape changes or restore tests to pass unmodified (e.g., through property-superset assertions). |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts::HeadShaMismatchKind | type | src/adapters/shared/commands/session/pr-wait-for-review-command.ts:112 — branches on classification to alter message copy, packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:18 — imports the type for tests | Adopted | New discriminant is plumbed into adapter message and tests; no other public consumers detected within repo. |
Documentation impact
- no-update-needed — This PR changes internal classification and adapter message behavior but does not add or change user-facing documented contracts beyond parameter descriptions already updated in-code (src/adapters/shared/commands/session/session-parameters.ts:914-930) and regenerated CLI manifest (src/generated/completion-manifest.json). The PR description reports a docs sweep found no
expectedHeadShabehavioral claims in docs/. No separate docs files in docs/ describe this timeout classification; therefore no external docs require updates.
…llable BLOCKING (SC5). The criterion required existing tests to "pass untouched", and two exact-match `toEqual` assertions were edited to carry the new field. The finding is correct, and the criterion is what was wrong: SC1 requires the classification to be REPORTED on the mismatch payload, so no exact-match assertion on that payload can survive it. The two are in direct conflict and no implementation satisfies both literally. SC5's own text now states the invariant it was always protecting — the push-pending path's semantics — and says which kind of test edit is in scope. Amending the criterion rather than explaining it in the PR body, per mem#986 / mt#4213. The offered alternative (superset assertions, or a compat shape) was rejected: loosening a `toEqual` on the exact payload this task changes would REDUCE what those tests pin, to protect a wording rather than a behaviour. NON-BLOCKING, fixed: `classification` could be typed `null` in a payload that never produces one. `buildTimeoutResult` now computes the kind once and gates the whole `expectedHeadShaUnreached` object on `kind !== null` — provably the same condition as the previous `!remoteIsServingExpectedHead()`, since both delegate to `headShaMatchesExpected` — and the field is typed non-nullable. The null is now unrepresentable rather than merely unreachable. (The review's stated mechanism was slightly off: it read the object as unconditional when it was already gated. The conclusion held.) NON-BLOCKING, declined with basis, recorded in the spec: a separate `returnedEarly` flag (the now-non-nullable `classification` IS that machine -readable signal; a second field would give one fact two sources of truth); un-exporting `classifyHeadShaMismatch` (it has a production caller via `mismatchKind()`, and matches the sibling `headShaMatchesExpected`); an exhaustive `switch` in the adapter (the `else` already is the safe default — generic-but-correct beats specific-and-wrong, and naming the raw kind would leak an internal enum); and dropping the `MIN_ABBREVIATED_SHA_LENGTH` pin (it is load-bearing for SC4 — the ~1-in-268M argument is a function of the value being 7, so the test SHOULD fail if the floor is tuned).
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verify R1’s blocking concern and the fixes: SC5 has been amended in the spec to allow payload-shape updates, and the code now makes expectedHeadShaUnreached.classification non-nullable by construction. The domain layer introduces classifyHeadShaMismatch and wires an immediate early-return on divergent-prefix; the adapter renders specific guidance for that case while preserving the generic branch for others. Tests in both domain and adapter cover the new behavior, and the push-pending path’s semantics remain intact. I found no new critical defects; one minor maintainability note suggests documenting finalizeTimeout’s pre-deadline use. Overall, the prior BLOCKING issue is resolved and the change meets all success criteria — APPROVE.
Findings
- [NON-BLOCKING] packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1308 — Early-return path relies on
finalizeTimeoutside-effects; consider a brief comment infinalizeTimeoutnoting it may be called pre-deadline
The early return onclassification === "divergent-prefix"jumps tofinalizeTimeout()(packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1308-1330). The surrounding comment here explains the choice well. As a small maintainability aid, consider adding a one-line note infinalizeTimeoutitself that it can be invoked prior to deadline as a fast-fail classification path (mt#4995), so future edits to that helper preserve its applicability both at real timeouts and early exits. No functional change requested.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — mismatch is classified, not just reported. | Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:251-336 adds export type HeadShaMismatchKind and export function classifyHeadShaMismatch(...) returning "push-pending" | "divergent-prefix" | null; payload expectedHeadShaUnreached now includes non-nullable classification at :534-552. |
SC2 — the divergent-prefix case does not wait out the timeout. |
Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1308-1330 early-returns via finalizeTimeout() when mismatchKind() === "divergent-prefix", with explanatory log, preventing a full-timeout wait. |
| SC3 — the message names the likely cause and the remedy. | Met | src/adapters/shared/commands/session/pr-wait-for-review-command.ts:112-156 branches on classification; for divergent-prefix it replaces the generic two-cause text with specific cause/remedy: do not pad to 40 chars; pass commitHash verbatim; cites mt#4039, mt#4046. |
| SC4 — the discriminator's false-positive rate is argued, not assumed. | Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:286-319 carries an in-code rationale: 16^7 = 268,435,456; hex-only inputs; SHA-1 uniformity; aligns with MIN_ABBREVIATED_SHA_LENGTH. Also guarded by tests per PR, and no fixture-trigger change here. |
SC5 — push-pending BEHAVIOUR is unchanged. (Amended: exact-match assertions may be updated to include classification.) |
Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1118-1160 buildTimeoutResult still sets push-not-landed rejection and builds timeout payload; when kind is not divergent-prefix no early return is triggered, preserving wait-to-deadline behaviour. The adapter default branch (src/adapters/...:139-156) preserves the generic two-cause message for non-divergent-prefix mismatches. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| packages/domain/src/session/commands/pr-wait-for-review-subcommand.classifyHeadShaMismatch | function | packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:16 — imported and exercised directly, packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1149 — used indirectly via local mismatchKind() inside the poll loop |
Adopted | Production path calls it through the local mismatchKind closure; tests import the function directly for unit coverage. |
Documentation impact
- no-update-needed — The change adds an internal classification and branches the adapter’s rendered message accordingly. No new CLI flags or parameters were introduced, and the only user-facing prose touched is in
formatTimeoutMessage’s output, which is exercised by existing adapter tests. The spec’s own Scope section noted no docs indocs/assert the old behavior; this PR does not invalidate published documentation.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Re-verified the prior BLOCKING concern: the push-lag tests now include the classification: "push-pending" field as permitted by the amended SC5, and the classification mechanism is correctly wired end-to-end. The early-return path on divergent-prefix routes through finalizeTimeout as described, and the timeout payload’s classification is non-nullable and gated consistently. Adapter messaging branches as required and tests cover both branches. I found no new critical defects introduced by these changes. Prior non-blocking suggestion about documenting finalizeTimeout’s pre-deadline use remains optional and unaddressed, but is not merge-blocking. Verdict: APPROVE.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
| SC1 — mismatch is classified, not just reported. | Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:251-336 adds export type HeadShaMismatchKind and export function classifyHeadShaMismatch(...) returning "push-pending" | "divergent-prefix" | null; payload expectedHeadShaUnreached now includes non-nullable classification at :1121-1149 (right-side). |
SC2 — the divergent-prefix case does not wait out the timeout. |
Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1326-1350 (right-side) early-returns via finalizeTimeout() when mismatchKind() === "divergent-prefix", with explanatory log, preventing a full-timeout wait. |
| SC3 — the message names the likely cause and the remedy | Met | src/adapters/shared/commands/session/pr-wait-for-review-command.ts:112-156 (right-side) branches on classification; for divergent-prefix it replaces the generic two-cause text with specific cause/remedy: do not pad to 40 chars; pass commitHash verbatim; cites mt#4039, mt#4046. Associated test at src/adapters/shared/commands/session/pr-wait-for-review-command.test.ts:239-281 (right-side). |
| SC4 — the discriminator's false-positive rate is argued, not assumed. | Met | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:289-325 carries an in-code rationale (16^7 = 268,435,456) and explains threshold choice; tests pin the threshold at packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:77-95 (right-side). |
SC5 — push-pending BEHAVIOUR is unchanged. (Amended: exact-match assertions may be updated to include classification.) |
Met | packages/domain/src/session/commands/pr-wait-for-review-push-lag.test.ts:174-180 and :238-242 (right-side) updated to include classification: "push-pending" while preserving poll-to-deadline assertions; domain timeout path still constructed via buildTimeoutResult when kind !== "divergent-prefix" at packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1118-1160, 1353-1400 (right-side). |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| packages/domain/src/session/commands/pr-wait-for-review-subcommand.classifyHeadShaMismatch | function | packages/domain/src/session/commands/pr-wait-for-review-subcommand.ts:1135 — used indirectly via local mismatchKind() in the poll loop, packages/domain/src/session/commands/pr-wait-for-review-sha-classification.test.ts:16 — imported and unit-tested directly |
Adopted | Production path calls it via the local closure; tests import the export for focused coverage. |
Documentation impact
- no-update-needed — No new CLI flags or parameters were added; only the timeout payload gained an internal
classificationfield and the adapter’sformatTimeoutMessagerenders a more specific message for one case. The spec’s Scope section already noted no docs indocs/assert the previous behavior. I spot-checked the in-repo user-surface that mirrors help text (src/adapters/shared/commands/session/session-parameters.tsandsrc/generated/completion-manifest.json) and both were updated in this PR to describe the new classification behavior, keeping user-facing descriptions accurate.
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.
Summary
session_pr_wait-for-reviewtreated everyexpectedHeadShamismatch identically: poll until thetimeout, then report
expectedHeadShaUnreached. That is correct for the case it was built for — apush still in flight — and wrong for a sha the caller constructed, which can never arrive.
The two are separable on the first poll that observes a remote head. A push-lag mismatch has an
entirely different commit as the observed head. A caller-error mismatch shares a long common prefix
and then diverges, because the caller started from a real abbreviated sha and extended it.
This matters beyond the wasted time: a wait that returns nothing after its full budget is, per
/implement-task§9, the documented lead-in to the bypass ladder. The failure mode is an agentreading its own mis-armed wait as reviewer silence.
Originating incident (2026-09-04, PR #3635 / mt#4897):
session_commitreturnedcommitHash: "f76e55628"(9 chars). The caller passedf76e556285ff4d6a4e0d21b0ba1e0a54ba7d2e0f— the 9 real characters plus 31 invented ones —believing the parameter required a full 40-character sha. It does not; prefix matching is by design
(mt#4039). The real head was
f76e556281b76e51949a057834f279e73d03a8e0. Both existing boundarychecks admitted the value: it is hexadecimal and well over the 7-character floor.
Key changes
classifyHeadShaMismatch(pr-wait-for-review-subcommand.ts) returnsdivergent-prefixwhen the two share
>= MIN_ABBREVIATED_SHA_LENGTH(7) characters and then differ,push-pendingotherwise, andnullwhen there is nothing to compare.expectedHeadShaUnreachedcarriesclassification. The field is populated from the sameheadShaclosure the payload reports, so the verdict can never describe a different observationthan the
lastObservedHeadShaprinted beside it.divergent-prefixreturns immediately, throughfinalizeTimeoutrather than a barebuildTimeoutResult. That path is already bounded by its own short budget and still attaches thefresh reviews list and
reviewerCheckRunState; it cannot mis-report a review as a match herebecause its
finalMatchis gated onremoteIsServingExpectedHead()— the predicate that isfalse. Verified by reading it, not assumed.
divergent-prefixthe generic "two causes, oppositeremedies" line is replaced — one of its two causes has been ruled out by evidence, so leaving it
in asks the reader to weigh a possibility already eliminated. The new text names the padding
mistake and the remedy (pass
commitHashverbatim; do not extend it to 40 characters).completion-manifest.jsonupdated to match.Judgment calls
Gate (g) collision, resolved as coordinate rather than wait. Open PR #3412 (mt#4639, the
614-site
getLoggableErrorSummaryconversion) also touchespr-wait-for-review-subcommand.ts. I read its actual changed-file list and its patch for this filerather than judging by title: the overlap is +3 −2 — one import line and two
getErrorMessagesubstitutions inside catch-block
log.debugcalls (~L703, ~L1166). No hunk overlaps the matcher,the result type, or the poll loop, and the test file is untouched. Blocking a two-file change on an
eight-day-old 100+-file mechanical conversion was the worse trade. The
parallel-work-open-prguardfired at
session_startand I cleared it with an audit-loggedgrant-guard-override.tsgrant carrying this reasoning. Whoever lands second rebases.SC5 needed two assertion edits, and the criterion says "tests pass untouched". Adding a field to
expectedHeadShaUnreachedbreaks two exact-matchtoEqualassertions inpr-wait-for-review-push-lag.test.ts. I updated both to includeclassification: "push-pending".The semantics SC5 protects are unchanged — those tests still assert the wait polls on and times
out — but the payload shape changed, so the criterion is met in substance and not literally. Calling
that out rather than letting it read as untouched.
A known false negative, deliberately not fixed. A fabricated sha extending an older head's
abbreviation, on a PR whose head has since advanced, shares ~0 characters with the current head and
is reported as
push-pending. That degrades to exactly today's behaviour rather than to a wrongverdict; catching it would require retaining head history this wait does not keep. Recorded in the
code and in the spec's planning audit. The same reasoning covers a sha stranded by a rebase
(mem#1013) — five recorded incidents, cause fixed upstream by mt#4046, and not the shared-prefix
shape.
Testing
New file
pr-wait-for-review-sha-classification.test.ts(a sibling, not an extension ofpr-wait-for-review-push-lag.test.ts, which sits near the 400-linemax-linesWARN threshold thatthis repo's zero-tolerance warning gate makes unshippable — mem#833's extract-a-sibling precedent).
Execution evidence:
AT1 — a 40-char value sharing its first 9 characters with the observed head, diverging after,
classified
divergent-prefix, returning on poll 1 of a 600s budget; message names the cause(the AT1 message half is asserted in
pr-wait-for-review-command.test.ts, "names theextended-abbreviation cause"). AT2 — a true abbreviated prefix still matches (mt#4039 regression).
AT3 — a sha sharing fewer than 7 characters classified
push-pendingand polls >50 times.AT4 — a backend with no
getPullRequestHeadShais unchanged and reports nothing new.SC1/SC2/SC5 are the AT1-vs-AT3 contrast above; SC3 is the adapter message test; SC4 is the
threshold test plus the fixture check named in its own test title.
Negative control: neutralized the discriminator itself (
>= MIN_ABBREVIATED_SHA_LENGTH→>= 999), a full revert of the decision logic rather than just deleting the early return, per mt#4512. Result: 4 fail / 8 pass — the originating-incident test, the threshold test, the case/whitespace test and AT1 all went red, while AT2, AT3 and AT4 stayed green. The control therefore discriminates the new claim from the behaviour SC5 requires preserved, rather than merely proving the file can fail.Typecheck: 0 errors across 8 projects (
.,packages/domain,packages/shared,services/reviewer,services/site,src/cockpit/web,tsconfig.hooks.json,tsconfig.scripts.json);infra/skippedfor uninstalled deps. Lint: 0 errors, 0 warnings over 4382 files.
format:checkclean.Deploy verification: this PR touches deploy surface — I initially tagged the commit
[no-deploy-impact]from assumption and thecommit-msgguard correctly denied it, naming all 7staged files as deploy surface. The tag is removed and the claim retracted here. After merge I will
run
deployment_wait-for-latestforminsky-mcpwithnotBeforeset to the merge timestamp andexpectCommitShaset to the merge SHA, and readbuildIdentityrather than treating SUCCESS aloneas sufficient.
Live verification
Not a structural change under
/implement-task§7a — no new persistence path, model-output channel,external-system probe, deploy-target wiring, or schema migration; the behaviour is fully determined
by the injected
SessionPrWaitForReviewDependenciesseam and is covered by the tests above,including a negative control. No new external-system integration, so no live-exercise requirement.