Skip to content

Replace the shell check judge in the merge scripts with the Go evaluator #723

Description

@tbhb-agent

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

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. 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".
  6. 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.
  7. 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.
  8. 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.
  9. 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).
  10. AC10: After the change, the committed generated skills are exactly what the canonical source generates.
  11. 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.
  12. AC12: Consumers can read what changed: the adoption documentation names the agentrepo release 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 any agentrepo, and the contract's deliberate changes, and docs/checks-evaluator.md describes 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 gh recorded 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's gh stub 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 gh recorded 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-tokens and apm install in that order leave a clean tree, and mise run check-generated-skills passes.
  • 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-check and mise run cover-py-check green; 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

Activity

  1. updated Priority Standard and Work type Planned on Oct 25, 2026
  2. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    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 main at d41369e7fd0c84a878991c84a305af1d721e6305. No files, checks, builds, or external writes were made.

    • Problem: Supported. The shell emits per-check rows and counts superseded runs; Go emits summary JSON with outcomes 0–6. Preflight reports checks after review and unattended gates; squash execution refuses failures (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).
    • Acceptance criteria: Twelve correctly numbered entries; AC11 covers performance. Conflicts and verification prescriptions remain below.
    • Scope and non-goals: Bounded to the judge migration and its supporting artifacts, with contract decisions delegated to Settle the check judge contract and divergence corpus before porting it #720.
    • Approach: Addresses listings, superseded counts, caller adaptation, deletion, and ordered generation. Some unconditional compatibility wording needs alignment with the Spike contract.
    • Dependencies: Native blockers match the body. Installation and the separate owner-protection rollout hold are explicit.
    • Size: M, one PR, is plausible; no mandatory split identified.
    • Verify plan: Every AC ID appears, but several checks cannot prove the stated outcome.
    • Risks: Covers unsafe acceptance, compatibility drift, and rollout order; reverting supplies an undo.
    • Operator authority: None grants no additional authority; the separate owner-protection hold is explicit.

    Gaps

    1. AC9 requires parity between evaluations with different inputs. Attended judging accepts SKIPPED and NEUTRAL; 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.

    2. 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.

    3. 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.

    4. The Bats plan needs explicit control of the injection boundary. The existing gh stub 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.

    5. 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).

    6. 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.

    7. 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.

    • D1 Pending: AC-linked checks exist, but merged-head verification and evidence in the PR’s Verification section are unspecified.
    • D2 Pending: independent approval of the exact merged head and finding disposition are unspecified.
    • D3 Pending: required-check results and exact-head owner approval for loosened gates are unspecified.
    • D4 Unmet in plan: gitleaks, private-string check-leaks.sh, and leak review of posts are absent.
    • D5 Pending: signing every commit, including the squash, is unspecified.
    • D6 AC12 and its Verify entry cover adoption and evaluator documentation; remove the bypass identified above.
    • D7 Pending: Assisted-by trailers and the provenance:agent-assisted label are unspecified.
    • D8 Pending: M estimates scope but does not verify size limits or an exact-head owner exception.
    • D9 Pending: follow-up issue requirements, Related links, and code references are unspecified.
    • D10 Pending: closing reference, Done state, and final Status: comment naming the merge commit are unspecified.
    • devtools-1 Unmet in plan: mise run check, mise run cover-go-check, and mise run cover-py-check are absent.
    • devtools-2 Approach requires ordered generation → token measurement → deployment; AC10 verifies reproducibility and generated consistency.
    • devtools-3 AC12 requires consumer installation and behavior guidance in adoption documentation, subject to removing its bypass.

    Round: 1 of 3

  3. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    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 main at d41369e7fd0c84a878991c84a305af1d721e6305.

    • Problem: Supported. Shell produces per-check rows and superseded counts; preflight reports them, while squash execution refuses failed or pending checks. Go supplies summary JSON and statuses 0–6 (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).
    • Acceptance criteria: Twelve correctly numbered criteria. Their outcomes are testable against the forthcoming contract; related messages, statuses and absence of writes describe the same caller result. AC11 supplies a bounded non-functional outcome.
    • Scope and non-goals: Explicit and consistent with consuming Settle the check judge contract and divergence corpus before porting it #720’s findings rather than deciding them here.
    • Approach: Covers evaluator listings, superseded counts, distinct caller adapters, shell deletion, ordered generation and documentation. Existing Go grouping, validation and required-check semantics support the stated migration risks (internal/checks/checks.go:75, internal/checks/checks.go:148, docs/checks-evaluator.md:9).
    • Dependencies: Native blockers match the body. Installation and owner-protection rollout holds are explicit; overlap with Port exact-head merge approval selection from shell to Go #590 and ordering before Port the gate runner and pass cache from Python to Go #713 are addressed.
    • Size: M and one PR remain plausible, subject to the eventual corpus and generated diff.
    • Verify plan: Every AC has a corresponding check. Round-one gaps 1–3 and 5–7 are closed. Gap 4 is substantially addressed, with the absence-case defect below remaining.
    • Risks: Covers unsafe acceptance, diagnostic drift, unattended tightening and rollout order; reverting supplies an undo.
    • Operator authority: None grants no additional authority. The separate owner-protection hold remains explicit.

    Gaps

    1. 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.

    • D1 Pending: AC-linked PR evidence is required; verification specifically on the merged head remains unspecified.
    • D2 Pending: exact merged-head independent approval and disposition of findings remain unspecified.
    • D3 Pending: completion checks are planned; required checks and exact-head owner approval for loosened gates remain unspecified.
    • D4 Partially covered: gitleaks and private-string scans cover the diff and posts; independent leak review of each post remains unspecified.
    • D5 Pending: signing every commit, including the squash, remains unspecified.
    • D6 AC12 and Verify AC12 require adoption and evaluator documentation matching the changed interface.
    • D7 Pending: Assisted-by trailers and the provenance:agent-assisted label remain unspecified.
    • D8 Pending: Size requires staying within limits or splitting; actual size evidence remains pending.
    • D9 Pending: follow-up issue structure, Related links and references from deferred code remain unspecified.
    • D10 Pending: closing reference, Done state and final merge-commit Status: comment remain unspecified.
    • devtools-1 Completion checks explicitly require mise run check, mise run cover-go-check and mise run cover-py-check.
    • devtools-2 Approach requires ordered generation → token measurement → deployment; AC10 and Verify AC10 require committed consistency and a clean regeneration.
    • devtools-3 AC12 requires consumer release, installation and compatibility guidance in adoption documentation.

    Round: 2 of 3

  4. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    ContributorAuthor

    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.

    • Problem: Supported. Shell emits per-check states and superseded counts; preflight reports them, while squash execution refuses failed or pending checks. Go emits summary JSON with statuses 0–6 (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).
    • Acceptance criteria: Twelve correctly numbered criteria, each expressing an observable compatibility, caller, generation, performance, or documentation outcome. Messages, statuses, and absence of writes describe the corresponding caller result. AC11 supplies a bounded non-functional requirement.
    • Scope and non-goals: Explicit and bounded. Replace the shell check judge in the merge scripts with the Go evaluator #723 consumes Settle the check judge contract and divergence corpus before porting it #720’s future contract and corpus; it does not need those findings to exist before refinement.
    • Approach: Covers evaluator listings and superseded counts, separate caller adapters, shell deletion, ordered generation, and adoption documentation. Existing validation, grouping, and required-check behavior substantiate the migration risks (internal/checks/checks.go:75, internal/checks/checks.go:148, internal/checks/checks.go:195).
    • Dependencies: Native blockers are Settle the check judge contract and divergence corpus before porting it #720, Ship the Go binaries as release assets installed through mise's github backend #333, and feat(distribution): migrate consumer tool pins to release assets #544, matching the body. The owner-protection rollout hold, overlap with Port exact-head merge approval selection from shell to Go #590, and ordering before Port the gate runner and pass cache from Python to Go #713 are explicit.
    • Size: M and one PR remain plausible, subject to the eventual corpus and generated diff.
    • Verify plan: Every AC has a corresponding proof. AC1–AC2 replay compatibility cases; AC3–AC7 cover caller boundaries and diagnostics; AC8–AC9 cover unattended compatibility and contract results; AC10–AC12 cover generation, timing, and documented compatibility. Raw malformed responses bypass the helper’s jq reconstruction, while valid-input cases require prerequisites and invocation evidence (tests/skills/merge-pr/test_helper.bash:69, tests/skills/merge-pr/test_helper.bash:90).
    • Risks: Covers unsafe acceptance, diagnostic drift, unattended tightening, and rollout order; reverting supplies an undo.
    • Operator authority: None grants 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.

    • D1 Pending: AC-linked PR evidence is required; verification specifically on the merged head remains unspecified.
    • D2 Pending: exact merged-head independent approval and finding disposition remain unspecified.
    • D3 Pending: completion checks are planned; required-check evidence and exact-head owner approval for loosened gates remain unspecified.
    • D4 Partially covered: gitleaks and private-string scans cover the diff and posts; leak review of each post remains unspecified.
    • D5 Pending: signing every commit, including the squash, remains unspecified.
    • D6 AC12 and Verify AC12 require adoption and evaluator documentation matching the changed interface.
    • D7 Pending: Assisted-by trailers and the provenance:agent-assisted label remain unspecified.
    • D8 Pending: Size requires staying within limits or splitting; actual size evidence remains pending.
    • D9 Pending: follow-up issue structure, Related links, and references from deferred code remain unspecified.
    • D10 Pending: closing reference, Done state, and final merge-commit Status: comment remain unspecified.
    • devtools-1 Completion checks require mise run check, mise run cover-go-check, and mise run cover-py-check.
    • devtools-2 Approach requires ordered generation, token measurement, and deployment; AC10 and Verify AC10 require committed consistency and clean regeneration.
    • devtools-3 AC12 requires consumer release, installation, and compatibility guidance in adoption documentation.

    Round: 3 of 3

  5. added
    refinement:readyPassed the Definition of Ready; may be picked up
    and removed
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026
  6. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    ContributorAuthor

    Refinement amendment for the shell-to-Go epic (#588), covering compatibility and protection. The acceptance criteria stay as written, except where noted.

    1. The callers run agentrepo checks evaluate when 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 and mp_superseded stay until the fallback issue. The contract corpus lands first in its own issue (the check contract corpus and evaluator listing).
    2. Fallback rule from docs: state the N-2 compatibility policy #661, applied here: the caller probes for the Go command without writes. When the command is absent (an older pinned binary), the existing shell implementation runs unchanged. When the command is present and fails, prints partial output or returns an invalid result, the caller refuses and never falls back to repeat a protected action. Configuration, flags, streams, statuses and state formats stay as they are, and this change does not raise the binary floor. A separate XS issue removes the fallback after every supported release pin ships the command. Any acceptance criterion or step that demands deleting the shell logic, or a missing-binary refusal, moves to that issue.
    3. No new complex shell: the fallback is the frozen existing code, not a translation of it.
    4. The replacement Go package and its dispatcher registration are covered by the owner-approval protection patterns before any caller switches to them. That protection is its own Wave 1 issue and is a native dependency of this one.
    5. Generated skill output is regenerated in the same PR, in the documented order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refinement:readyPassed the Definition of Ready; may be picked up

    Fields

    Priority

    Standard

    Work type

    Planned

    Projects

    • Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions