Skip to content

Repository navigation

refactor(agents): port reviewer execution and verdict integration to Go #771

New issue
New issue
Open
Chore
Open
refactor(agents): port reviewer execution and verdict integration to Go#771
Chore
Labels
refinement:readyPassed the Definition of Ready; may be picked up

Description

@tbhb-agent
tbhb-agent
opened on Oct 26, 2026

Problem

Reviewer selection, the runner and dt_review in packages/agents/source/lib/lib.sh execute the independent review and stamp the result. The Go internal/review package already parses verdicts, but the execution boundary is shell, and a PASS followed by a failed or timed-out reviewer must never stamp.

Part of #588.

Acceptance criteria

  1. AC1: Given a PASS verdict followed by a nonzero reviewer exit, when the review runs with an existing stamp present, then the existing stamp is removed, no new stamp is written, the status is 4, and the diagnostic is on standard error.
  2. AC2: Given a reviewer that exceeds its deadline after printing a PASS, when the review runs, then the status is 124, no stamp exists afterward, and the reviewer process is cancelled and cleaned up.
  3. AC3: Given a review that passes, when it completes, then it prints the shell path's distinct success output, writes the stamp and persistent state the shell path writes, and a rejected review prints the distinct reject output with the same state effects as the shell path.
  4. AC4: Given conflicting repository and machine reviewer settings, when a review runs, then only the machine setting selects the reviewer, with the environment precedence the shell path applies, and the repository setting never executes.
  5. AC5: Given no explicit reviewer, when a review runs, then the default reviewers run read-only, as in the shell path.
  6. AC6: Given a consumer whose binary lacks the replacement, when it runs after this change, then it sees today's behaviour. Given a replacement that is present and fails, then the caller refuses with the replacement's nonzero status and a diagnostic on standard error. Given a replacement that returns partial output and exits 0 with no diagnostic, then the caller refuses with a nonzero status and a standard-error diagnostic and writes no stamp. In both cases the shell path never runs and nothing runs twice.
  7. AC7: Generated skill output is regenerated from the canonical source in the same pull request, and mise run check-generated-skills passes.

Scope and non-goals

Port reviewer selection and execution so that verdict parsing and the persistent redesign state behave exactly as they do today. Keep the machine-only reviewer configuration.

Non-goal: changing any behaviour a consumer on an older pinned binary relies on. Non-goal: removing the shell fallback, which a follow-up issue filed when this lands owns.

Approach

Meet AC1 to AC7. The mechanism is the implementer's choice within those outcomes.

Dependencies

Blocked by the trusted policy port (#763), the stamp port (#769) and the bounded-state port (#770), which supply prompts, bindings and bounds. Part of #588. Touches a gate path.

Size

S. One pull request.

Verify plan

  • AC1: start with an existing stamp, run a reviewer that prints PASS and exits nonzero; assert status 4, stderr text, stamp absent.
  • AC2: a reviewer that prints PASS then sleeps past the deadline; assert status 124, no stamp, and no surviving child process.
  • AC3: a passing and a rejecting reviewer; compare standard output, status, stamp and persistent state with the shell path.
  • AC4: conflicting repository and machine settings plus environment overrides; assert which one executes.
  • AC5: no reviewer configured; assert the default reviewers' read-only invocation.
  • AC6: stub binaries for absent, present-and-failing (nonzero) and present-with-partial-output-exiting-0-without-diagnostic; assert the legacy result for the absent case, then for each supported case the refusal status, the standard-error diagnostic, no stamp, zero fallback and zero repeat execution. The replacement path runs, not the legacy one.
  • AC7: mise run generate-skills leaves no diff and mise run check-generated-skills passes.
  • Definition of done: D1 each AC is verified at the merged head, with the commands and results cited in the pull request's Verification. D2 an independent review approves the exact merged head, and every finding has a recorded disposition. D3 required checks pass with no gate loosened. D4 the leak scans the repository specifies are clean, and every post made for this work (comments, descriptions) passes the per-post leak review before it is posted. D5 every commit is signed. D6 the documentation matches, or the description says No doc impact with the reason. D7 every commit carries an Assisted-by trailer and the pull request carries the provenance:agent-assisted label. D8 the change stays within the pull request size limits. D9 follow-up work, including deferred review findings, is filed as issues, each with a Problem section and numbered acceptance criteria and linking this issue; the pull request's Related section lists them, and any code that defers work links its issue. D10 the merge closes this issue through a closing reference, the project board shows the state Done, and a final Status: comment names the merge commit.

Risks

A fallback that ran after a supported command failed would mask an enforcement error, and a looser port would weaken a gate. AC1, AC2 and AC6 pin both. Undo: revert the change.

Operator authority

A standing rule names the operator: the canonical packages/agents/source/lib/lib.sh and the generated scripts/lib.sh copies are golden, constitutional or owner paths (.agents/agent-skills.conf:4,5,9), so the pull request needs the owner's exact-head review at merge, and it carries forward #588's protection of the Go enforcement path.

Activity

  1. github-project-automation
    added this to devtoolson Oct 26, 2026
  2. github-project-automation
    moved this to Backlog in devtoolson Oct 26, 2026

  3. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    ContributorAuthor

    Refinement: NOT READY — observable parity, verification, and owner authority need refinement
    Model: gpt-6.1-sol

    Checked

    Gaps

    1. High — Operator authority contradicts protected-path policy.
      Operator authority: None is incorrect. The canonical library and generated scripts/lib.sh copies match golden, constitutional, and owner-path protections (.agents/agent-skills.conf:4, .agents/agent-skills.conf:5, .agents/agent-skills.conf:9). Replace “None” with the required owner review, including exact-head approval where required. Carry forward Replace complex shell with tested Go, starting with gates and merging #588’s requirement that replacement Go enforcement paths receive owner protection before operational rollout.

    2. High — Shell parity is referenced through tests rather than stated as observable requirements.
      AC1 specifies no new stamp, but does not explicitly require removal of an existing stamp, preserved status, or diagnostic stream. Current integration removes an old stamp, returns 124 for timeout, maps other reviewer failures to 4, and reports failure on stderr (packages/agents/source/lib/lib.sh:559, packages/agents/source/lib/lib.sh:593). Successful and rejected verdicts also have distinct output, status, and persistent-state effects (packages/agents/source/lib/lib.sh:625).
      State the applicable observable outcomes, including read-only default reviewers, configured deadlines and cancellation cleanup, and legacy-versus-supported-command behavior. Preserve the existing security boundary (packages/agents/source/lib/lib.sh:433, internal/review/run_unix.go:57). Keep each acceptance criterion focused on one outcome.

    3. High — The Verify plan does not prove every AC.
      “Using the same fixtures as AC1” does not establish machine-only reviewer selection or regenerated-output correctness.

      • AC1: Assert the status, stderr diagnostic, and absence of a stamp after starting with an existing stamp; the current timeout and cancellation fixtures demonstrate those checks (tests/skills/source/lib/lib.bats:770, tests/skills/source/lib/lib.bats:779).
      • AC2: Introduce conflicting repository and machine reviewer settings; assert which reviewer actually executes, including supported environment precedence.
      • AC3: Identify the relevant cases and show they exercise the replacement path rather than succeeding through legacy fallback.
      • AC4: Require regeneration followed by mise run check-generated-skills; the latter directly checks generated consistency (mise.toml:470).

      Add absent-command, supported-failure, and partial-output checks that assert legacy behavior or refusal, with zero fallback after supported failure.

    4. Medium — The body prescribes design outside Operator authority.
      “Reusing the existing verdict parser,” “behind an agentrepo subcommand,” and the specified probing mechanism prescribe implementation choices. Replace these with requirements for strict verdict interpretation, compatibility with older binaries, a capability check without side effects, and refusal after supported-command failure. Leave implementation choices to the PR.

    5. Medium — Completion coverage is incomplete.
      The Verify plan lacks the merged-head evidence and completion obligations shown below. Add explicit coverage for the unchecked items. refactor(agents): port trusted review policy and prompt assembly to Go #763’s completion paragraph belongs to that dependency; it does not establish completion requirements for refactor(agents): port reviewer execution and verdict integration to Go #771.

    Split

    No split required. Keep policy assembly, stamp lifecycle, and bounded-state implementation in their named blockers. Refine this issue around reviewer execution and verdict integration.

    Definition of done

    The definition of done for #771 (Feature). Tick each item and give its evidence.

    Ticks indicate planning coverage, not completed work.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
      Evidence: AC IDs appear in the Verify plan, but merged-head verification and PR evidence are unspecified.

    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
      Evidence: No completion requirement covers independent approval and disposition of findings.

    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
      Evidence: Named parent Replace complex shell with tested Go, starting with gates and merging #588 AC8 requires applicable checks; its Operator authority retains owner protections and exact-head approval. Gap 1 concerns applying that authority explicitly here.

    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
      Evidence: Replace complex shell with tested Go, starting with gates and merging #588 mentions scanner gates generally; the complete listed coverage is unspecified.

    • D5 The author signed each commit, the squash included.
      Evidence: No completion requirement covers signing this change’s commits and squash.

    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.
      Evidence: Documentation disposition is unspecified.

    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
      Evidence: Provenance requirements are unspecified.

    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
      Evidence: “S. One pull request” estimates size but does not cover the limits or exception.

    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
      Evidence: Follow-up recording requirements are unspecified.

    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
      Evidence: Closure, board state, and final status evidence are unspecified.

    Round: 1 of 3

  4. tbhb-agent
    added the
    Chore
    issue type on Oct 26, 2026

  5. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    ContributorAuthor

    Refinement: NOT READY — partial-output refusal and one design prescription remain
    Model: gpt-6.1-sol

    Checked

    Gaps

    1. High — AC6 leaves successful partial output without a clear refusal status.
      AC6 requires refusal “with the replacement’s status and diagnostic” even when the replacement returns partial output. If that replacement exits 0 without a diagnostic, preserving those results does not communicate refusal. The Verify plan repeats the ambiguity without specifying the partial-output exit status.

      Require a nonzero refusal status and a diagnostic on standard error for partial output returned with status 0, with no stamp, fallback or repeat execution. Add that explicit fixture and those assertions to AC6’s verification. Named dependency refactor(agents): port trusted review policy and prompt assembly to Go #763 AC5 already distinguishes this case. The current review integration treats an unreadable verdict as status 4 with a stderr diagnostic (packages/agents/source/lib/lib.sh:640).

    2. Medium — Scope still prescribes parser reuse outside Operator authority.
      “Reusing the existing verdict parser” selects an implementation component. Its existence is factual (internal/review/verdict.go:15), but requiring its reuse conflicts with the rule that issues hold requirements and design belongs in the PR.

      Replace that prescription with the observable requirement to preserve strict verdict interpretation. Keep parser reuse as a PR design choice.

    Split

    No split required. With policy, stamp lifecycle and bounded-state implementation retained in their named blockers, reviewer execution and verdict integration plausibly fit one PR.

    Definition of done

    The definition of done for #771 (Feature). Tick each item and give its evidence.

    Ticks indicate planning coverage, not completed work.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
      Evidence: The Verify plan’s D1 explicitly requires merged-head verification and commands/results for every AC ID. Gap 1 concerns the AC6 contract those checks must prove.

    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
      Evidence: Verify-plan D2 requires exact-head independent approval and recorded dispositions; D9 covers deferred findings.

    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
      Evidence: Verify-plan D3 requires passing checks with no gate loosened; Operator authority requires exact-head owner review and carries forward Replace complex shell with tested Go, starting with gates and merging #588’s protection requirement.

    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
      Evidence: Verify-plan D4 requires the repository-specified scans and per-post leak review. Scan entry points include .devtools/tasks/security.toml:24 and .devtools/tasks/leaks.toml:12.

    • D5 The author signed each commit, the squash included.
      Evidence: Verify-plan D5 requires every commit to be signed.

    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.
      Evidence: Verify-plan D6 requires matching documentation or a documented “No doc impact” explanation.

    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
      Evidence: Verify-plan D7 requires the trailer on every commit and the specified PR label.

    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
      Evidence: Verify-plan D8 requires staying within the PR size limits.

    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
      Evidence: Verify-plan D9 explicitly covers issue content, Related entries and code links; fallback removal is identified as follow-up work.

    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
      Evidence: Verify-plan D10 explicitly requires all three outcomes.

    Round: 2 of 3

  6. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    ContributorAuthor

    Refinement: NOT READY — Scope still prescribes parser reuse
    Model: gpt-6.1-sol

    Checked

    Gaps

    1. Medium — Scope prescribes an implementation component.

      “Reusing the existing verdict parser” remains a design requirement outside Operator authority. The parser’s existence is factual (internal/review/verdict.go:15), but requiring its reuse conflicts with the rule that issues hold requirements and design belongs in the PR.

      Replace that phrase with an observable requirement to preserve strict verdict interpretation. Leave parser reuse to the PR’s design decision.

    Split

    No split required. Keep policy assembly, stamp lifecycle and bounded-state implementation in their named blockers.

    Definition of done

    The definition of done for #771 (Feature). Tick each item and give its evidence.

    Ticks indicate planning coverage, not completed work.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
      Evidence: Verify-plan D1 explicitly requires merged-head verification and commands/results for each AC ID.

    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
      Evidence: Verify-plan D2 requires exact-head independent approval and recorded dispositions; D9 covers deferred findings.

    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
      Evidence: Verify-plan D3 requires passing checks with no gate loosened; Operator authority requires exact-head owner review.

    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
      Evidence: Verify-plan D4 requires repository-specified scans and per-post leak review. Scan entry points are .devtools/tasks/security.toml:24 and .devtools/tasks/leaks.toml:12.

    • D5 The author signed each commit, the squash included.
      Evidence: Verify-plan D5 requires every commit to be signed.

    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.
      Evidence: Verify-plan D6 requires matching documentation or a documented “No doc impact” explanation.

    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
      Evidence: Verify-plan D7 requires the trailer on every commit and the specified PR label.

    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
      Evidence: Verify-plan D8 requires staying within the PR size limits.

    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
      Evidence: Verify-plan D9 explicitly covers issue content, Related entries and code links; fallback removal is identified as follow-up work.

    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
      Evidence: Verify-plan D10 explicitly requires all three outcomes.

    Round: 3 of 3

  7. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    ContributorAuthor

    Refinement: READY — reviewer execution parity and refusal behavior are sufficiently defined
    Model: gpt-6.1-sol

    Checked

    Gaps

    None.

    Split

    No split required. With policy assembly, stamp lifecycle and bounded-state implementation retained in the named blockers, reviewer execution and verdict integration fit one PR.

    Definition of done

    The definition of done for #771 (Feature). Tick each item and give its evidence.

    Ticks indicate planning coverage, not completed work.

    • D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
      Evidence: Verify-plan D1 explicitly requires merged-head verification and commands/results for every AC ID.

    • D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
      Evidence: Verify-plan D2 requires exact-head independent approval and recorded dispositions; D9 covers deferred findings.

    • D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
      Evidence: Verify-plan D3 requires passing checks with no gate loosened; Operator authority requires exact-head owner review.

    • D4 The leak scans come back clean: gitleaks, devtools' check-leaks.sh with the repository's private-strings list, and the leak review of each post.
      Evidence: Verify-plan D4 requires repository-specified scans and per-post leak review. Scan entry points include .devtools/tasks/security.toml:24 and .devtools/tasks/leaks.toml:12.

    • D5 The author signed each commit, the squash included.
      Evidence: Verify-plan D5 requires every commit to be signed, encompassing the squash commit.

    • D6 The docs match the change (README, docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.
      Evidence: Verify-plan D6 requires matching documentation or a documented “No doc impact” explanation.

    • D7 The commits and the squash have Assisted-by trailers, and the pull request has the provenance:agent-assisted label.
      Evidence: Verify-plan D7 requires the trailer on every commit and the specified PR label.

    • D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
      Evidence: Verify-plan D8 requires staying within the PR size limits.

    • D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
      Evidence: Verify-plan D9 explicitly covers issue content, Related entries and code links; fallback removal is identified as follow-up work.

    • D10 The merge closes the issue through a closing reference and the board shows Done. The final Status: comment names the merge commit.
      Evidence: Verify-plan D10 explicitly requires all three outcomes.

    Round: 1 of 3

  8. tbhb-agent
    added
    refinement:readyPassed the Definition of Ready; may be picked up
    on Oct 26, 2026
  9. tbhb-agent
    marked this as blocked by 3 issues on Oct 26, 2026
  10. tbhb-agent
    marked this as blocking 6 issues on Oct 31, 2026
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

    Type

    Chore

    Fields

    Priority

    None yet

    Work type

    None yet

    Projects

    • devtools
      Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions