Repository navigation
refactor(agents): port trusted review policy and prompt assembly to Go #763
Description
Problem
dt_policy_*, dt_review_policy and dt_review_prompt in packages/agents/source/lib/lib.sh read the repository review policy from the trusted remote tracking ref and assemble the reviewer prompt. A failed blob read or a branch-edited policy must never reach the reviewer.
Part of #588.
Acceptance criteria
-
AC1: Given a policy file changed on the reviewed branch, when the prompt is assembled, then built-in rules alone apply and the branch's text does not reach the reviewer.
-
AC2: Given a non-regular or executable blob, an untrusted ref, or no common base, when the policy is read, then the outcome (refusal or built-in rules alone) and its status and message equal the shell path's.
-
AC3: Given a policy blob whose read or digest fails, when the policy is read, then the review is refused with a nonzero status and a diagnostic on standard error, and no partial policy text reaches the reviewer. This is an intentional correction: the shell path continues and marks the policy applied.
-
AC4: Given a trusted policy, when the prompt is assembled, then the policy text precedes the credentials floor, the audit line reports the path, digest, status, ref and commit, and the bytes equal the shell path's prompt.
-
AC5: Given a consumer on an older pinned binary that 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 diagnostic on standard error. Given a replacement that returns partial output with status 0, then the caller refuses with a nonzero status and a diagnostic, and the partial output is excluded from every downstream input. In both cases the shell path never runs and nothing runs twice.
-
AC6: Generated skill output (the copies of
lib.shunder the generated skill directories) is regenerated from the canonical source in the same pull request, andmise run check-generated-skillspasses. -
AC7: Given an unrelated default-branch commit fetched while a review runs, when the review finishes, then it stays valid and is stamped if the policy digest, status and authority ref are unchanged, as required by fix(agents): keep a review valid when only unrelated commits move the trusted ref #776 AC1.
-
AC8: Given policy addition, removal, editing or replacement by a non-regular file on the trusted ref during review, when the reviewer returns, then review fails with status 5 and the existing diagnostic and writes no stamp, as required by fix(agents): keep a review valid when only unrelated commits move the trusted ref #776 AC2.
-
AC9: Given a trusted ref moving to an unreadable or untrusted policy state during review, when review finishes, then it fails with status 5 and writes no stamp, as required by fix(agents): keep a review valid when only unrelated commits move the trusted ref #776 AC3.
-
AC10: Given a review stamp preserved across an unrelated trusted-ref commit, when commit or publication validates it, then unchanged draft, tree, head, configuration, policy digest, policy status and authority bindings allow it; changed policy digest, status or authority refuse it, as required by fix(agents): keep a review valid when only unrelated commits move the trusted ref #776 AC6. The audit retains the starting trusted commit.
Scope and non-goals
Port the trusted-policy reader and prompt assembly. Keep the authority rules: a regular non-executable blob on the trusted default ref, built-in rules alone when the reviewed tree changes the policy path, and the credentials floor after the policy text.
Non-goal: changing policy authority rules. The intended freshness contract is #776 AC1 to AC3 and AC6; unchanged-policy freshness follows that correction rather than the earlier commit-ID comparison. Older pinned binaries retain their existing fallback behaviour. Non-goal: removing the shell fallback, which is deferred to a follow-up issue filed under D9 when this lands.
Approach
Meet the acceptance criteria with failing cases first. The mechanism is the implementer's choice within those outcomes.
Dependencies
Blocked by #753 and by #755. Part of #588. The fallback removal is deferred to a follow-up issue filed when this lands. Touches a gate path.
Size
S. One pull request.
Verify plan
- AC1: a branch that edits the policy file; compare the assembled prompt with the built-in prompt byte for byte.
- AC2: fixtures for an executable blob, a symlink, a missing ref and no common base; compare outcome, status and message with the shell path.
- AC3: a fixture whose blob read fails and one whose digest fails; assert the nonzero status, the stderr diagnostic and that no policy text appears in the reviewer input; show the shell path's contrasting behaviour in the failing-first case.
- AC4: a trusted policy fixture; compare the whole prompt and the audit line with the shell path's.
- AC5: stub binaries for absent, present-and-failing (nonzero), and present-with-partial-output-and-status-0; assert the legacy result, then for each supported case the refusal status, the stream and text of the diagnostic, exclusion of the partial output, zero fallback and zero repeat execution.
- AC7 to AC10: carry the fix(agents): keep a review valid when only unrelated commits move the trusted ref #776 regression fixtures for unrelated trusted commits, changed policy content and status, unreadable or untrusted authority, and later stamp validation into the Go implementation. Verify the starting audit commit remains recorded.
- AC6:
mise run generate-skillsleaves no diff andmise run check-generated-skillspasses. - 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 impactwith the reason. D7 every commit carries anAssisted-bytrailer and the pull request carries theprovenance:agent-assistedlabel. 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 finalStatus:comment names the merge commit.
Risks
A branch-edited policy reaching the reviewer would let a change write its own review criteria. AC1 pins it. 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), so the pull request needs the owner's exact-head review at merge.
Activity
tbhb-agent commentedon Oct 26, 2026
Refinement: NOT READY — behavior contract, verification, and owner authority need refinement
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools; checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/lib.sh:292,packages/agents/source/lib/lib.sh:309, andpackages/agents/source/lib/lib.sh:346.Gaps
High — State observable behavior instead of suite success. AC1 combines branch-policy suppression and failed-read refusal; AC2 primarily requires existing tests to pass. Separate those outcomes and state parity for successful results, output streams, statuses, and side effects, except the intentional failed-read correction. Include observable outcomes for an absent command, a successful supported command, and failed or partial supported output without fallback. Relevant existing evidence includes policy reporting at
packages/agents/source/lib/lib.sh:332, prompt precedence atpackages/agents/source/lib/lib.sh:388, and failure assertions attests/skills/source/lib/masked-status.bats:56andtests/skills/source/lib/masked-status.bats:117.High — Map verification to checks that prove each outcome. Reusing AC1 fixtures cannot prove AC3’s generated-output requirement. Replace the generic failing-first bullets with explicit cases and assertions for each AC. For AC2, identify the policy and prompt cases exercised through the Go path, including partial-output refusal and absence of reviewer invocation or a stamp after failure. For AC3, specify regeneration followed by
mise run check-generated-skills; the generator compares generated content and modes attools/generate-skills.sh:144. Parent Replace complex shell with tested Go, starting with gates and merging #588 already requires generation and applicable integration checks.High — Correct Operator authority. “None” omits the standing owner-review requirement. Name
packages/agents/source/lib/lib.shand its generatedscripts/lib.shcopies as protected paths, and require the owner’s exact-head review at merge. They match the golden, constitutional, and owner patterns in.agents/agent-skills.conf:4,.agents/agent-skills.conf:5, and.agents/agent-skills.conf:9. Retain build(agents): protect replacement Go enforcement paths #753 as the prerequisite protecting replacement Go enforcement paths. This records review requirements without granting implementation or write authority.Medium — Remove prescribed design from requirements. The Approach section mandates placement “behind an
agentreposubcommand” and a particular shell probing mechanism outside Operator authority. Keep the required Go migration and observable compatibility/refusal outcomes; move command placement and probing design into the pull request.Split
No additional split is needed. The trusted-policy and prompt boundary fits one PR, with #753 and #755 retained as blockers. Fallback removal remains outside scope.
Definition of done
The definition of done for #763 (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 names AC IDs, but does not require merged-head evidence and has the coverage gaps above.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: The named issues do not cover this complete requirement.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: Replace complex shell with tested Go, starting with gates and merging #588 AC8 requires applicable checks to pass; its Operator authority requires exact-head owner approval for protected changes.
D4 The leak scans come back clean: gitleaks, devtools'
check-leaks.shwith 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, but does not specify this complete set.
D5 The author signed each commit, the squash included.
Evidence: Not covered by the issue or named requirements.
D6 The docs match the change (README,
docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.Evidence: Generated skill output is required; documentation impact is not addressed.
D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: Not covered by the issue or named requirements.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: “S” estimates scope but does not cover size-limit verification or an 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: Fallback removal is deferred, but the complete follow-up requirement is absent.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: Not covered by the issue or named requirements.
Round: 1 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: NOT READY — failure parity and fallback ownership remain unresolved
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools. Checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/lib.sh:292andpackages/agents/source/lib/lib.sh:309. Prompt precedence and the credentials floor appear atpackages/agents/source/lib/lib.sh:388..agents/agent-skills.conf:4,.agents/agent-skills.conf:5and.agents/agent-skills.conf:9.tools/generate-skills.sh:121. No prescribed layout or library choice remains.Gaps
High — AC2 combines incompatible failure requirements. It requires exact shell status/message parity while forbidding partial policy text. The current reader does not check the blob-read or digest status before marking policy applied (
packages/agents/source/lib/lib.sh:327). In a conditional context, those failures can be masked—the context explicitly documented attests/skills/source/lib/masked-status.bats:2. State failed-read refusal as an intentional correction, with observable status, diagnostic stream and exclusion of partial policy text. Retain parity for the other cases. Update AC2’s verification to assert that correction directly; comparison with the shell alone cannot prove it.High — AC4 leaves successful-status partial output undefined. A replacement can return partial output with status 0. “Refuses with the replacement’s status” would then report success. Separate nonzero replacement failure from incomplete output returned with status 0. Specify the latter’s nonzero refusal status, diagnostic stream and exclusion of partial text from reviewer input. Extend the partial-output fixture to assert those outcomes alongside zero fallback and zero repeat execution.
Medium — refactor(agents): remove the shell fallback for discovery and worktree readiness #772 does not own this fallback’s removal. refactor(agents): port trusted review policy and prompt assembly to Go #763 assigns its fallback removal to refactor(agents): remove the shell fallback for discovery and worktree readiness #772, but refactor(agents): remove the shell fallback for discovery and worktree readiness #772 limits removal to discovery and worktree readiness from refactor(agents): port skill capability and worktree context discovery to Go #755 and explicitly excludes fallbacks owned by other ports. Correct that attribution and name an actual follow-up covering trusted-policy and prompt fallback removal, or explicitly leave that follow-up unassigned. This is deferred work, not a blocker on implementing the port.
Split
No further implementation split is needed. Policy reading and prompt assembly form one bounded PR. Keep #753 and #755 as prerequisites; correct the separate fallback-removal ownership.
Definition of done
The definition of done for #763 (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 each AC. The verification defects above remain readiness gaps.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: The Verify plan’s D2 requires independent approval of the exact merged head and a recorded disposition for every finding.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: The Verify plan’s 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.shwith the repository's private-strings list, and the leak review of each post.Evidence: D4 refers generally to repository-specified scans; the body and named issues do not explicitly cover this complete set.
D5 The author signed each commit, the squash included.
Evidence: The Verify plan’s D5 requires every commit to be signed, which includes 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: D6 covers matching documentation or a
No doc impactstatement, but omits the explanation and its placement in Verification.D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: D7 names generic provenance trailers and a provenance label without specifying these exact requirements.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: The Verify plan’s D8 requires staying within the pull request 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: D9 requires follow-up issues but does not cover their required content, Related listing or code links. Fallback ownership also remains unresolved.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: D10 covers closure and the final merge-commit comment, but does not require a closing reference or board status Done.
Round: 2 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: READY — policy-port requirements and verification are sufficiently defined
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools. Checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/lib.sh:292andpackages/agents/source/lib/lib.sh:309.packages/agents/source/lib/lib.sh:327. Conditional invocation can mask failures, as documented attests/skills/source/lib/masked-status.bats:2.packages/agents/source/lib/lib.sh:332andpackages/agents/source/lib/lib.sh:388.tools/generate-skills.sh:121..agents/agent-skills.conf:4,.agents/agent-skills.conf:5and.agents/agent-skills.conf:9.Gaps
None.
Split
No additional split is needed. Keep #753 and #755 as blockers; retain fallback removal as a separately filed follow-up.
Definition of done
The definition of done for #763 (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.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: D2 requires independent exact-head approval and recorded dispositions; D9 requires deferred findings to become issues.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: 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.shwith the repository's private-strings list, and the leak review of each post.Evidence: D4 requires clean repository-specified leak scans and per-post leak review. Repository scan entry points include
.devtools/tasks/security.toml:24and.devtools/tasks/leaks.toml:12.D5 The author signed each commit, the squash included.
Evidence: D5 requires every commit to be signed, including the squash.
D6 The docs match the change (README,
docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.Evidence: D6 requires matching documentation or an explained
No doc impactstatement in the pull request description.D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: D7 explicitly requires
Assisted-byon every commit and the exact label.D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: D8 requires staying within the pull request 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: D9 explicitly covers issue content, deferred findings, Related entries and code links; scope assigns fallback removal to this process.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: D10 explicitly requires the closing reference, board state Done and final merge-commit comment.
Round: 3 of 3