Repository navigation
Description
Problem
The attended merge scripts judge a saved check rollup with the shell function mp_judge_checks in packages/agents/source/skills/merge-pr/shared/scripts/pull-request.sh, using jq, while the unattended merge path judges it with the Go evaluator through agentrepo checks evaluate (ma_gates in merge-agent.sh). Two judges for the same data can disagree, and a disagreement can turn a failing check green. This is feature 3 of #588. The input classes where they differ, and the contract that settles each, are recorded by the Spike #720; this issue applies that contract to the callers and deletes the shell judge.
The callers depend on the shell judge's behaviour. preflight.sh refuses with exit 75 when a review is required, runs the unattended gates, and then reports each check's state and name sorted by name, the count of superseded runs and a summary; failing or still-running checks do not themselves make that section refuse. squash-merge.sh prints the failing or still-running checks as <state> <name> lines on standard error and then refuses with a message that depends on the outcome. Both parse the saved response before they judge it. The Go command prints a JSON summary of the non-successful names only, with exit statuses 0 to 6, and neither lists every check's state nor counts superseded runs.
Acceptance criteria
- AC1: Given a saved rollup that the old shell judge reported ok for every check, when the merge path judges it, then the result is green, unless the Spike's contract lists that input class as a deliberate change toward refusal.
- AC2: Given a saved rollup in which the old shell judge reported any check failing or still running, including a repeated status context whose older entry fails and two runs whose start times order differently as text and as instants, when the merge path judges it, then the result is never green.
- AC3: Given an empty rollup, a missing or null rollup, truncated or malformed JSON, or a rollup with an unknown state (on the latest run or a superseded run, as the contract decides), when the squash-merge step runs, then it is refused, no merge write is made, and the message and exit status are the ones the contract names for that input; and when preflight runs on the same input, it prints the report and exit status the contract names for preflight.
- AC4: Given the evaluator command is absent, exits with a status the contract does not map, prints output that is not its result, prints a valid-looking success result with a nonzero exit status, or exits zero with a result that lacks its required fields, when the squash-merge step runs, then it is refused and no merge write is made; and when preflight runs, it gives the outcome the contract's second table names for preflight.
- AC5: Given a rollup with ok, still-running and failing checks and superseded runs, when preflight runs, then its check section prints one line per check with its state and name, sorted by name, the "(N earlier run(s) left out: a later run of the same check replaced each)" line with the same N as today when something was replaced and no such line otherwise, and the same summary sentence, unless the contract lists a change; for an empty or unreadable rollup it prints "no checks reported" or "the check rollup could not be read".
- AC6: Given a pull request that still requires a review, when preflight runs, then it exits 75 with its present message before it reports any check, and every other refusal that precedes the check section keeps its exit status and message.
- AC7: Given a failing, a still-running, an empty and an unreadable rollup, when the squash-merge step runs, then it prints the failing or still-running checks as
<state> <name>lines on standard error, refuses with the same message as today unless the contract lists the change, exits non-zero and makes no merge write. - AC8: Given the unattended merge path with a set of required checks, when it evaluates any rollup, then its outcome, exit status, messages and skipped-check hint are unchanged, except for the cases the contract lists as tightening.
- AC9: Given any rollup of the contract's corpus, the same required check names and the same expected head, when the attended and the unattended merge paths judge it, then the shared rollup evaluation gives the same verdict, that verdict equals the outcome and exit status the contract records for the row, and the only differences between the paths are the ones the contract lists (the unattended path's required-name matching and its stricter handling of skipped and neutral conclusions).
- AC10: After the change, the committed generated skills are exactly what the canonical source generates.
- AC11: Non-functional: judging a saved 500-check rollup takes under 1 s at the median of 20 runs, measured from the saved-rollup command invocation through response validation to the caller's verdict, with the Go command already built, or under the limit the Spike records.
- AC12: Consumers can read what changed: the adoption documentation names the
agentreporelease the attended merge now requires, how to install it, what the merge does with an older release already accepted by the shared loader and without anyagentrepo, and the contract's deliberate changes, anddocs/checks-evaluator.mddescribes the output and exit statuses the merge scripts rely on.
Scope and non-goals
In scope: the check judge, its two callers in the merge-pr scripts (preflight and the squash-merge step), the committed contract and corpus taken from the Spike, and the documentation named in AC12.
Non-goal: re-deciding the contract; a row that proves wrong goes back to the Spike's findings and is corrected there first.
Non-goal: review-verdict parsing (#589), approval selection (#590), policy inspection and merge execution (later features of #588).
Non-goal: removing jq from the rest of the merge path; only the judge's use of it goes.
Non-goal: changing which checks a repository requires, or the unattended path's required-check matching.
Out of scope: producing the Go binary as a release asset (#333), installing it in consumer repositories (#544), and the owner-approved protection of the replacement Go paths (AC8 of #588).
Approach
Commit the Spike's contract as a table and make the same table the Go test fixture, with the Spike's rollups and their recorded shell outputs as the corpus. Extend internal/checks only where the contract chooses the stricter behaviour, and give agentrepo checks evaluate a listing of every check's state and name with the count of superseded runs; existing callers' JSON and exit statuses stay as they are. Make the callers run the command and map its exit status and output as the contract's second table says, keeping the two caller contracts apart: preflight's check section reports, the squash-merge step refuses. Delete the shell judge and mp_superseded in the same change. Regenerate skills in order, mise run generate-skills, then mise run skill-tokens, then apm install, never by hand. Add the consumer steps to the adoption documentation.
Dependencies
Native blocked-by: #720 (the Spike that settles the contract and corpus), #333 (the binary release assets) and #544 (consumers install those assets). Attended merging refuses wherever the command is absent or too old, which AC4 makes safe but not harmless, so the change does not merge ahead of the installation path. The owner-approved protection update for the replacement Go enforcement paths (AC8 of #588) is a rollout hold: the callers are not deployed until the epic records it. The change edits the same generated merge-pr output as #590, so regenerate the shared token metadata from the combined tree if both land close together. #713 is ordered after this issue.
Size
M. One PR, if the diff stays within the size limits. If the corpus and the caller changes together exceed them, this issue becomes an Epic and splits into two refined issues: the contract fixtures with the evaluator listing, then the caller migration with the shell judge's deletion and the documentation.
Verify plan
- AC1: a Go table test that replays each corpus rollup the shell judged ok through the merge-path judge and requires green, except rows marked deliberate.
- AC2: the same table with the rollups the shell judged failing or unfinished, including a superseded cancelled run with no later run, a tied start time, a repeated status context whose older entry fails, and the offset pair whose text and instant orders disagree; none may come back green.
- AC3: Bats cases for empty, null, missing, truncated, malformed and unknown-state rollups. For the squash-merge step each asserts the contract's message and exit status, a refusal, and that a stub
ghrecorded no merge call; for preflight each asserts the report and exit status the contract names. Parser-stop cases (truncated, malformed) deliver the response raw to the caller, bypassing the test helper'sghstub that parses and rebuilds responses with jq; all other cases pass a valid review stamp and every earlier prerequisite so that they reach the judge boundary, which each case proves by asserting the evaluation invocation was recorded. - AC4, absent evaluator: Bats cases that remove the evaluator from both places the caller can resolve it (the shared loader's installed location and the search path), so no evaluator runs and nothing can be recorded. Each asserts the squash-merge refusal and its exit status, that a stub
ghrecorded no merge call, and the preflight outcome the contract's second table names for an absent evaluator, with standard error captured separately. The cases assert that no evaluation invocation exists, not that one was recorded. - AC4, executable-result failures: Bats cases with a stub evaluator that answers the loader's version request and the unattended context's help request normally and records the evaluation invocation, and then exits with an unmapped status, prints garbage, prints a valid-looking success result and exits 1 and then 2, and exits 0 with a result lacking its fields. Each asserts the squash-merge refusal with no merge call, the preflight outcome named by the contract, and the recorded evaluation invocation, with standard error captured separately.
- AC5: Bats cases for preflight with all-ok, failing, still-running, empty, unreadable, one replaced run, several replaced runs and no replaced run, comparing output with the recorded results.
- AC6: the existing regression at
tests/skills/merge-pr/scripts/merge-workflow.bats:385, a case asserting no check line is printed before the exit, and one replay of each refusal in the Spike's third table, asserting the recorded exit status and message (the authentication and quota regressions in the same suite are examples). - AC7: Bats cases for the squash-merge step on the failing, still-running, empty and unreadable rollups, asserting the standard error lines, the refusal message, the exit status and that no merge call was made.
- AC8: the
tests/skills/agent-merge-pr/suites, plus a case that runs the unattended evaluation over the corpus with required names and compares status, messages and hint with a recording made on the old head, with each difference matched to a row the contract lists as tightening. - AC9: a test that feeds every corpus rollup, with the same required names and expected head, to both paths and compares each verdict and exit status with the contract's recorded row, and with the listed unattended-only differences accounted for.
- AC10:
mise run generate-skills,mise run skill-tokensandapm installin that order leave a clean tree, andmise run check-generated-skillspasses. - AC11: a timing script that runs the saved-rollup command, validates its response and adapts the verdict 20 times over a generated 500-check rollup, with the build outside the timing, and fails when the median is at or over the limit.
- AC12: the documentation diff; a test or recorded run of the attended merge with the compatible release, with the oldest release the shared loader accepts (v0.16.0), and with no
agentrepo, each matching what the documentation says. - Completion checks (not ACs):
mise run check,mise run cover-go-checkandmise run cover-py-checkgreen; gitleaks and the repository's leak check with the private-strings list clean over the diff and each post; the pull request's Verification cites every AC ID above.
Risks
Treating a parser or API ambiguity as evidence that a merge is safe, which AC2 to AC4 guard against. Changing the callers' output or exit statuses silently, which AC5 to AC7 pin. Tightening the unattended path and refusing a merge that used to pass, which AC8 and the contract's deliberate rows make visible. Deploying the callers before a compatible binary is installed or before the protection update, which the Dependencies hold prevents. The undo is reverting the PR, which restores the shell judge.
Operator authority
None
Metadata
Metadata
Assignees
Labels
Type
Fields
Priority
Work type
Projects
- StatusShow more project fieldsBacklog
Activity
tbhb-agent commentedon Oct 25, 2026
Refinement: NOT READY — verdict parity and preflight refusal requirements conflict
Checked
Read #723, #720 and all its comments, including the former Feature’s round 3 verdict, #588, #713, #590, and native dependencies. #723 is blocked by #720, #333 and #544; #713 is blocked by #723.
Code references are to
mainatd41369e7fd0c84a878991c84a305af1d721e6305. No files, checks, builds, or external writes were made.packages/agents/source/skills/merge-pr/shared/scripts/pull-request.sh:49,packages/agents/source/skills/merge-pr/shared/scripts/preflight.sh:94,packages/agents/source/skills/merge-pr/shared/scripts/squash-merge.sh:203,internal/agentrepo/command.go:47).Nonegrants no additional authority; the separate owner-protection hold is explicit.Gaps
AC9 requires parity between evaluations with different inputs. Attended judging accepts
SKIPPEDandNEUTRAL; unattended judging rejects them when required. Missing required names also produce a distinct refusal (internal/checks/checks.go:164,internal/checks/checks.go:195). Existing fixtures require those differences (tests/skills/agent-merge-pr/workflow.bats:233,tests/skills/agent-merge-pr/workflow.bats:242,tests/skills/agent-merge-pr/workflow.bats:266).Define parity for identical required names and expected-head inputs, or explicitly distinguish common rollup evaluation from additional unattended requirements. Comparing both paths on the rollup alone cannot satisfy AC9 while preserving AC8 and the required-check non-goal.
AC3’s verification conflicts with preflight’s reporting contract. Verify AC3 demands refusal at both entry points for empty rollups. AC5 and Approach preserve preflight’s reporting behavior, which prints “no checks reported” and continues (
packages/agents/source/skills/merge-pr/shared/scripts/preflight.sh:101). Failing checks likewise leave preflight successful (tests/skills/merge-pr/scripts/merge-workflow.bats:86).Require merge execution to refuse, and require preflight to produce the contract’s report and exit status. Make the same distinction explicit for AC4’s evaluator failures. Also qualify AC3’s blanket unknown-state refusal by the Spike’s latest/superseded-entry decision; the Feature must consume that decision without imposing a conflicting rule.
The corpus checks do not establish every chosen contract result. AC1 verifies formerly green rows, except deliberate changes; AC2 verifies only “never green.” AC9 verifies agreement, which can pass when both paths are wrong. None explicitly requires every row—including formerly empty/unreadable inputs and deliberately changed rows—to match its recorded chosen outcome and exit status.
Require exhaustive comparison against the Spike’s expected results, with caller diagnostics checked against its entry-point table. Verify AC3 also omits its explicit missing-rollup case. Preserve unattended expectations except where the contract records a change; the old-head comparison in Verify AC8 currently lacks that exception.
The Bats plan needs explicit control of the injection boundary. The existing
ghstub parses and reconstructs responses with jq before returning them (tests/skills/merge-pr/test_helper.bash:69,tests/skills/merge-pr/test_helper.bash:90). A truncated fixture can therefore fail inside the stub rather than at the caller parser stop promised by AC3.Evaluator stubs can also fail before check evaluation: the loader requests
--version, and unattended context requests--help(packages/agents/source/lib/lib.sh:151,packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:28). The existing unusable-runner fixture deliberately tests that earlier refusal (tests/skills/agent-merge-pr/workflow.bats:12).Specify raw response delivery for parser-stop cases, and evaluator stubs that preserve prerequisite commands while recording the actual evaluation invocation. AC4 and AC7 should prove the intended boundary was reached, with a valid review stamp and otherwise passing prerequisites, and capture stderr separately where required.
AC6’s verification proves only part of AC6. The cited regression proves exit 75 for required reviews, and the added case proves ordering. Neither proves “every other refusal that precedes the check section is unchanged.” Enumerate and replay the relevant stops from the Spike’s third table, asserting their existing statuses and messages. Existing authentication and quota regressions provide examples (
tests/skills/merge-pr/scripts/merge-workflow.bats:530,tests/skills/merge-pr/scripts/merge-workflow.bats:539).AC wording still embeds verification machinery, and AC12 permits bypassing necessary adoption guidance. AC8’s reference to “existing fixtures” and AC10’s regeneration commands/check invocation belong in Verify; state compatibility and reproducible generated artifacts as the acceptance outcomes.
Remove AC12’s “No consumer impact” alternative: requiring a new evaluator interface is explicitly consumer-visible. Verify the compatible release and an older, previously accepted release as well as absence. The current floor accepts v0.16.0, while adoption guidance permits operation without
agentrepo(packages/agents/source/lib/lib.sh:140,docs/agent-skills-migration.md:9). Missing-field stubs alone do not prove compatibility with that supported installation path.AC11’s measurement boundary remains ambiguous. “Through the merge path” could mean the adapter or the full workflow; Verify merely says the script “judges” the rollup. State that timing covers the same saved-rollup command invocation, response validation, and verdict adaptation used by the caller, with the build outside the measurement. Otherwise timing the evaluator alone could pass without proving AC11.
Split
No further split required. Keep the caller migration, contract fixtures, evaluator changes, generated output, and adoption guidance together. Retain the existing distribution, installation, and protection prerequisites.
Definition of done
Ticks indicate coverage in the proposed AC and Verify plan, not completed implementation evidence.
check-leaks.sh, and leak review of posts are absent.provenance:agent-assistedlabel are unspecified.Status:comment naming the merge commit are unspecified.mise run check,mise run cover-go-check, andmise run cover-py-checkare absent.Round: 1 of 3
tbhb-agent commentedon Oct 25, 2026
Refinement: NOT READY — the absent-evaluator check requires an invocation that cannot occur
Checked
Read #723 and its round-one verdict, #720 and all comments, #588, #713, #590, and native blockers. #723 is blocked by #720, #333 and #544.
Code references are to
mainatd41369e7fd0c84a878991c84a305af1d721e6305.packages/agents/source/skills/merge-pr/shared/scripts/pull-request.sh:49,packages/agents/source/skills/merge-pr/shared/scripts/preflight.sh:94,packages/agents/source/skills/merge-pr/shared/scripts/squash-merge.sh:203,internal/agentrepo/command.go:47).internal/checks/checks.go:75,internal/checks/checks.go:148,docs/checks-evaluator.md:9).Nonegrants no additional authority. The separate owner-protection hold remains explicit.Gaps
Verify AC4 requires the absent evaluator to record its evaluation invocation. It specifies a stub that answers prerequisite requests, “then is absent at that call,” and requires the recorded invocation in every case. Once the executable is absent, it cannot record that invocation. A legitimate adapter that detects absence and refuses before execution also cannot satisfy this assertion.
This distinction matters in the existing paths: the loader tolerates no binary, while unattended context refuses an absent binary before evaluation (
packages/agents/source/lib/lib.sh:149,packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:28). The existing unusable-runner test intentionally covers an earlier stop (tests/skills/agent-merge-pr/workflow.bats:12).Separate actual absence from executable-result failures. For absence, isolate both binary-resolution sources and assert each caller’s contract outcome and no merge write; require no evaluator invocation. For executable stubs, retain prerequisite handling and the recorded evaluation invocation. If disappearance after resolution is also intended, specify caller-side evidence of the attempted execution separately.
Split
No mandatory split identified. Keep the evaluator extension, contract fixtures, caller migration, generated output and documentation together. The stated conversion to an Epic and two refined issues is appropriate if the resulting work exceeds one PR’s limits.
Definition of done
Ticks indicate coverage by the proposed AC and Verify plan, not completed implementation evidence.
provenance:agent-assistedlabel remain unspecified.Status:comment remain unspecified.mise run check,mise run cover-go-checkandmise run cover-py-check.Round: 2 of 3
tbhb-agent commentedon Oct 25, 2026
Refinement: READY — the absent-evaluator verification gap is closed
Checked
Read #723, its round-two verdict, #720 and all comments, #588, #713, #590, and native dependency links. Code was read from main at
d41369e7fd0c84a878991c84a305af1d721e6305.packages/agents/source/skills/merge-pr/shared/scripts/pull-request.sh:49,packages/agents/source/skills/merge-pr/shared/scripts/preflight.sh:101,packages/agents/source/skills/merge-pr/shared/scripts/squash-merge.sh:203,internal/agentrepo/command.go:47).internal/checks/checks.go:75,internal/checks/checks.go:148,internal/checks/checks.go:195).tests/skills/merge-pr/test_helper.bash:69,tests/skills/merge-pr/test_helper.bash:90).Nonegrants no additional authority. The separate owner-protection hold remains explicit.Gaps
None requiring another refinement.
The round-two gap is closed. Verify AC4 now removes the evaluator from both resolution sources and asserts caller outcomes, no merge write, and no evaluation invocation. Executable-result failures separately retain prerequisite responses and recorded evaluation calls.
That distinction matches the code: resolution checks the installed location and then PATH; absence is tolerated by the loader, while unattended context can refuse before evaluation (
packages/agents/source/lib/lib.sh:141,packages/agents/source/lib/lib.sh:149,packages/agents/source/skills/merge-pr/shared/scripts/merge-agent.sh:28). The revised cases can prove their stated outcomes without requiring an absent executable to record anything. No new gap was introduced.Split
No mandatory split. Keep the evaluator extension, contract fixtures, caller migration, generated output, and documentation together. Use the stated two-issue split if the resulting work exceeds one PR’s limits.
Definition of done
Ticks indicate coverage by the proposed AC and Verify plan, not completed implementation evidence.
provenance:agent-assistedlabel remain unspecified.Status:comment remain unspecified.mise run check,mise run cover-go-check, andmise run cover-py-check.Round: 3 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement amendment for the shell-to-Go epic (#588), covering compatibility and protection. The acceptance criteria stay as written, except where noted.
agentrepo checks evaluatewhen the listing is supported, and the existing shell judge otherwise. A supported evaluator that fails or returns an invalid result refuses the merge, as AC4 already requires. The shell judge andmp_supersededstay until the fallback issue. The contract corpus lands first in its own issue (the check contract corpus and evaluator listing).