Repository navigation
refactor(agents): port skill signing verification to Go #764
Description
Problem
packages/agents/source/lib/signing.sh (93 lines) verifies that a landed commit has the reviewed tree, parents and an SSH signature. The Go signing package signs, but nothing in Go asserts the landed commit, so the check stays in shell.
Part of #588.
Acceptance criteria
- AC1: Given a landed commit whose tree, parents, branch, message or signature differs from what was reviewed, or that has no valid signature, or whose valid signature comes from a different key than the configured one, when it is verified, then it is rejected with the shell path's status and standard-error diagnostic and nothing on standard output, and a matching commit is accepted with the shell path's success output on standard output and standard error.
- AC2: Given a hook that rewrote the commit after review, when it is verified, then the mismatch is reported and the commit and review evidence are retained, as the shell path does.
- AC3: Given the commit this invocation created, when it is verified, then the check compares it with the reviewed content and not with a different commit that merely shares the branch, and verification of an unrelated commit is rejected.
- AC4: 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 or returns partial output, then the caller refuses with a nonzero status and a standard-error diagnostic and never runs the shell path.
- AC5: Generated skill output (the copies of
lib.shand ofsigning.shthattools/generate-skills.shwrites under the generated skill directories) is regenerated from the canonical source in the same pull request, andmise run check-generated-skillspasses.
Scope and non-goals
Port the landed-commit verification: tree, parents, branch and signature must match what was reviewed.
Non-goal: changing the signing key handling or any behaviour a consumer on an older pinned binary relies on. 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 (the Git context). 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: disposable keys; a commit with a wrong tree, wrong parents, wrong branch, a message changed after review, a missing signature, an invalid signature, and a valid signature from a second disposable key, plus a matching commit; compare status, standard output and standard error (success output and every refusal diagnostic) with the shell path (the signing cases in
tests/skills/agent-commit/workflow.bats). - AC2: a rewriting hook fixture; assert the mismatch report and retained evidence.
- AC3: a second commit on the same branch; assert it is rejected and the invocation's commit is accepted.
- AC4: stub binaries for absent, present-and-failing and present-with-partial-output; assert the legacy result, then status and diagnostic, zero fallback and zero repeat execution.
- AC5:
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 weaker check would let an unreviewed or unsigned commit count as published. AC1 to AC3 pin it. Undo: revert the change.
Operator authority
A standing rule names the operator: the canonical packages/agents/source/lib/lib.sh, the canonical signing.sh and the generated scripts/lib.sh and scripts/signing.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 — parity, verification, and owner protection need clarification
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools. Checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/signing.sh:55; the existing signing entry point delegates to signing atinternal/signing/run.go:33.Gaps
High — Observable behavior parity is incomplete. AC1 covers tree, parents, and signatures; scope also mentions branch identity. Existing verification additionally identifies this invocation’s commit, compares its reviewed message, requires the configured signing key, preserves recovery evidence, and reports success or refusal through defined streams and statuses (
packages/agents/source/lib/signing.sh:53,packages/agents/source/lib/signing.sh:62,packages/agents/source/lib/signing.sh:74,packages/agents/source/lib/signing.sh:89). Parent selection also differs for amend, merge, and unborn history (packages/agents/source/lib/signing.sh:65). State these observable outcomes and the compatibility outcomes explicitly, including older-command fallback and supported-command failure or partial-output refusal. Map verification to each outcome, comparing output, streams, status, and side effects.High — “Operator authority: None” omits protected-path requirements. The canonical signing library and its generated copies are constitutional paths under
.agents/agent-skills.conf:5; those changes produce protection findings atpackages/agents/source/skills/merge-pr/shared/scripts/merge-policy.py:523. Parent Replace complex shell with tested Go, starting with gates and merging #588 also requires owner protection for replacement enforcement code before rollout. Name the affected protected paths, required owner review of the exact head, and how replacement protection will be satisfied before callers switch. Preserve the distinction between required review and implementation authority.Medium — The Verify plan does not prove every AC. AC3’s “same fixtures as AC1” cannot demonstrate that generated outputs match canonical sources. Replace that entry with the documented generation sequence and freshness check (
tools/generate-skills.sh:30,mise.toml:470). AC1’s plan also omits its explicit missing-signature case. AC2 should identify the relevant cases and demonstrate that they actually exercise Go; current cases include message mutation, tree mutation, and branch movement (tests/skills/agent-commit/workflow.bats:201,tests/skills/agent-commit/workflow.bats:251,tests/skills/agent-commit/workflow.bats:261).Medium — The body prescribes design outside Operator authority. “On top of
internal/signing” and “behind anagentreposubcommand” choose implementation boundaries. Move those choices to the pull request, retaining the required Go migration and observable compatibility behavior. If they are operator decisions, record them in Operator authority.Low — The claimed fallback-removal follow-up is unnamed. The non-goal says a separate XS issue owns removal, but supplies no issue reference. Name the actual follow-up, or describe removal solely as excluded scope without claiming established ownership. Do not make later fallback removal a blocker for this port.
Split
No additional split is needed. Landed-commit verification and its generated caller updates form one coherent small PR. Keep eventual fallback removal outside this scope.
Definition of done
The definition of done for #764 (Feature). Tick each item and give its evidence.
Checkboxes describe planning coverage, not completed work. No item is fully covered by the live requirements and named issues.
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 in the pull request.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: No explicit requirement covers this.
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, but complete exact-head coverage is unstated.
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: The complete scan and post-review requirement is unstated.
D5 The author signed each commit, the squash included.
Evidence: AC1 concerns the verifier’s behavior, rather than signatures on every delivery commit 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: No documentation requirement or justified exemption is stated.
D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: No explicit requirement covers these delivery properties.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: Size is estimated as S; limits and exact-head exception approval are unstated.
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: The fallback-removal follow-up is unnamed; the remaining requirements are unstated.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: No explicit closure, board, or final-status requirement is stated.
Round: 1 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: NOT READY — verification parity and signing-path coverage remain incomplete
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools. Checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/signing.sh:55; the existing Go entry point performs signing atinternal/signing/run.go:33.Gaps
High — Observable parity and its verification remain incomplete. AC1 explicitly covers tree, parents, branch and signature, but does not clearly require rejection of a changed reviewed message or a valid signature from a different key. The shell enforces both (
packages/agents/source/lib/signing.sh:74,packages/agents/source/lib/signing.sh:80,packages/agents/source/lib/signing.sh:82). AC2’s generic rewriting-hook case does not establish those outcomes.State these rejection requirements explicitly. Also require parity of standard output and standard error, including successful verification output and refusal diagnostics (
packages/agents/source/lib/signing.sh:89,packages/agents/source/lib/signing.sh:92). Extend the Verify plan with message mutation, a valid signature from another key, and stream comparisons. Cover parent expectations for ordinary, amend, merge and unborn-history commits, whose observable acceptance behavior differs (packages/agents/source/lib/signing.sh:65). These are behavioral requirements, without prescribing implementation.Medium — Generated-output and operator-authority statements omit the signing files being ported. AC5 identifies generated output as copies of
lib.sh; Operator authority likewise names only canonical and generatedlib.sh. The generator separately copiessigning.shinto generated skills (tools/generate-skills.sh:112,tools/generate-skills.sh:114). Canonicalpackages/agents/source/lib/signing.shand generatedscripts/signing.shcopies are constitutional paths under.agents/agent-skills.conf:5.Correct AC5 to cover all affected generated outputs, including signing copies, and name those protected signing paths in Operator authority with their exact-head owner-review requirement. Retain build(agents): protect replacement Go enforcement paths #753 as the prerequisite protecting replacement enforcement code. No layout or implementation choice is needed.
Split
No additional split is needed. Verification, compatibility behavior and affected generated outputs form one coherent small PR. Keep eventual fallback removal outside this issue.
Definition of done
The definition of done for #764 (Feature). Tick each item and give its evidence.
Ticks indicate planning coverage, not completed work. The live Verify plan’s Definition of done paragraph supplies these delivery obligations; the AC verification gaps above still need refinement.
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: D1 explicitly requires merged-head verification of every AC and commands/results in the pull request’s Verification.
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 approval of the exact merged head and a recorded disposition for every finding; D9 requires issues for deferred findings.
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 and prohibits gate loosening. 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 the repository-specified leak scans to be clean and leak review before every post.
D5 The author signed each commit, the squash included.
Evidence: 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: D6 requires matching documentation or a reasoned
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 requires the trailer on every commit and the specified pull request 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 pull request size limits; no exception is proposed.
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 references and code links; fallback removal is included as deferred 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: D10 explicitly requires the closing reference, board state and final comment naming the merge commit.
Round: 2 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: READY — verification parity, compatibility, and owner protection are covered
Model: gpt-6.1-sol
Checked
tbhb-dev/devtools. Checkedorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/signing.sh:55; the existing Go signing entry point calls signing atinternal/signing/run.go:33.packages/agents/source/lib/signing.sh:60,packages/agents/source/lib/signing.sh:74andpackages/agents/source/lib/signing.sh:89.packages/agents/source/lib/signing.sh:65). AC1’s parity requirement and Replace complex shell with tested Go, starting with gates and merging #588’s requirement to port relevant behavior cover these cases. Existing evidence includes amend verification attests/skills/agent-commit/workflow.bats:225and merge verification attests/skills/commit/scripts/commit-workflow.bats:232.tools/generate-skills.sh:112. Its freshness check is defined atmise.toml:470..agents/agent-skills.conf:4,.agents/agent-skills.conf:5).Gaps
None.
Split
No additional split is needed. Verification, compatibility and generated-output updates form one coherent PR. Fallback removal remains deferred through D9.
Definition of done
The definition of done for #764 (Feature). Tick each item and give its evidence.
Ticks indicate planning coverage, not completed work. Evidence below comes from the live body’s Verify plan and named parent requirements.
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: D1 explicitly requires merged-head verification of every AC with commands and results in the pull request’s Verification.
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 exact-head independent approval and a recorded disposition for every finding; 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: D3 requires passing checks and prohibits gate loosening. 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 leak review before every post.
D5 The author signed each commit, the squash included.
Evidence: D5 requires every commit to be signed, including 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 requires matching documentation or a reasoned
No doc impactstatement in the pull request description; D1 places verification evidence in Verification.D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: D7 requires the trailer on every commit and the specified pull request 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 pull request size limits; no exception is proposed.
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 requires issue content, Related references and code links, including a follow-up for fallback removal.
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, Done board state and final comment naming the merge commit.
Round: 3 of 3