Repository navigation
Verify merge skill directories against base lock hashes in Go #981
Description
Problem
The merge must prove that the merge-pr and agent-merge-pr skill directories it runs match what the pinned base commit's apm.lock.yaml says was deployed. Main has no such check: packages/agents/source/skills/merge-pr/shared/scripts/merge-policy.py on main ends its lock parser at line 181 and has no integrity command and no deployed_file_hashes reader. internal/mergepolicy holds only approval.go, whose Approved has a production caller at internal/agentrepo/merge_approval.go:34. The integrity routine exists only in open PR #600, as Python run through uv, under the shell freeze of #588.
This issue is the library half of what replaces the oversized #600, as #499's AC3, and it owns the one lock parser. Part of #324, within #588.
Ownership across #980, #981 and #982, identical in all three bodies:
| Concern | Owner | Others |
|---|---|---|
Pinned-base settings reader and agentrepo config repository --base |
#980 | #864 and #865 call it for the gates and squash lint, #872 for cleanup's disposable_ignored; each gains #980 as a native blocker and a divergent-checkout AC |
Pinned-binary selection and refusal (no pinned agentrepo, or below the floor, exits 2 with a reason) |
#1010, a native blocker of #874, #875 and #876; its logic is tested Go with thin forwarding shims, and its lib.sh change needs owner exact-head review after #753, a Compatibility: deprecates declaration and a migration note |
None of #980, #981 and #982 selects a binary |
apm.lock.yaml parser, internal/mergepolicy/lock.go, including dependency, deployment and active_owner fields |
#981 | #859 is blocked by #981, consumes the parser and keeps only the ownership layer and its command |
| Expected map, directory walk and comparison library | #981 | Tested directly; no production caller until #982 |
Base snapshot, agentrepo merge integrity and snapshot cleanup on every exit path |
#982 | #864 (first gate pass) and #870 (final check) call the command, run helpers from the snapshot and remove it |
| Consumer contracts and existing package callers | internal/mergepolicy.Approved and its caller in internal/agentrepo/merge_approval.go stay as they are; the new APIs gain callers only in #859, #982, #864 and #870 |
#864 and #870 each gain #982 as a blocker and an AC for integrity refusal, snapshot use and cleanup; live head and base re-reads stay with them |
Post-merge cleanup read of disposable_ignored (squash-merge.sh:372) |
#872 | Calls the #980 reader |
| Retirement of PR #600 | Closed as superseded, unmerged, once the pull requests for #980, #981 and #982 have all merged; it is not rebased and no replacement takes code from it | The note on #600 cites those three; #499 stays open for the binary-refusal issue, its AC5 documentation follow-up and the caller issues |
| Documentation of merge authority and settings (#499 AC5) | A follow-up issue filed by #982 under its D9 duty | Not a blocker for any of the three |
Parity oracle: the Python merge-policy.py from PR #600's tree at commit 25df88552963de13e06c6d28380b94e783f31a7d.
- Lines 71 to 190 (
deployment_path,lock_scalar,lock_list,lock_records,lock_record,parse_lock) read the same as main's parser, one line later, so they are the parser parity reference. - Lines 779 to 850 (
skill_owner,skill_hashes,integrity) are new behavior, and the reference for the expected-map, walk and refusal cases.
Expected values come from running that file once in a throwaway harness outside the repository, recorded as fixture files. The harness is not committed and adds no production Python or shell.
Acceptance criteria
- AC1: Given an
apm.lock.yamlblob, when it is parsed, then each record yields its scalars, inline lists, block lists anddeployed_file_hashesmap, with quoted values, comments and empty sections read as the oracle's parser reads them. - AC2: Given a lock with a duplicate section, a duplicate field or an invalid scalar, when it is parsed, then parsing returns an error naming the defect.
- AC3: Given a skill directory, when its expected map is built, then exactly one lock record must own it, and zero or several owners refuse.
- AC4: Given a local package, when its expected map is built, then each hash comes from the base blob at the same path, and an unreadable base object or an absent lock refuses.
- AC5: Given a remote package, when its expected map is built, then each hash comes from the lock's
sha256entries under that directory, and a malformed or duplicate hash refuses. - AC6: Given an expected map that is empty, when the check runs, then it refuses, so an empty remote hash set or an empty local base directory never passes with an empty directory on disk.
- AC7: Given a skill root that is missing, is not a directory or is a symlink, when the check runs, then it refuses naming the root.
- AC8: Given a walk of
merge-prandagent-merge-pr, when an entry is a symlink or other non-regular file, then it refuses naming the path. - AC9: Given the walked files, when compared with the expected map, then equal maps pass, and a changed, missing or extra file refuses naming the path.
- AC10: Non-functional: reading base blobs and hashing files runs no repository content (a fixture hook or helper that would create a sentinel file never does),
uvis not needed, and the tests pass on macOS, Linux and Windows in CI. - AC11: The change adds no new shell logic and no Python, and leaves the existing
Approvedcaller untouched; no production code calls the newly exported parser or integrity APIs, though tests do, until The merge policy reads the APM lockfile and base-owned deployments in Go #859 and Run the skill-directory integrity gate in the merge entry points #982 consume them. - AC12: Given a lock record, when it is parsed, then the dependency fields (
repo_url,source,local_path,virtual_path,deployed_files) and the deployment fields (kind,value,active_owner) are exported as typed values, so The merge policy reads the APM lockfile and base-owned deployments in Go #859 interprets ownership without a second parser.
Scope and non-goals
In scope: in internal/mergepolicy, the exported lock parser (lock.go, with exported types for scalars, lists, records, dependency and deployment fields, and deployed_file_hashes), the expected map, the directory walk and the comparison, with tests and fixture files.
Non-goal: the snapshot, agentrepo merge integrity, the base snapshot, the final re-check and the entry scripts (#982).
Non-goal: pinned configuration reads (#980) and binary selection or refusal (the binary-refusal issue in the table); this issue touches neither.
Non-goal: interpreting lock ownership for the loosened-gate scan, deployments and active owners (#859, which consumes this parser).
Non-goal: wrapper cutovers (#874, #875, #876), live machine-authority adoption, unchanged commit and PR overrides, and authority or migration documentation from #499, which a follow-up issue filed by #982 covers.
Non-goal: deleting merge-policy.py or any shell, which belongs to the separately authorized #588 cutover.
Approach
Fail the tests first on fixtures taken from the oracle. The package is the single owner of lock parsing: #859 imports it and adds only the ownership layer and its command over it. New exported functions are added beside Approved in internal/mergepolicy and leave it unchanged.
Dependencies
Blocked by: none. #981 lands first and owns the parser.
Blocks #982, where the native link is already in place.
Blocks #859, which is edited before this issue is marked ready to gain a native blocked-by #981 beside its existing #333 blocker, with its scope, approach and Verify plan changed to consume this parser. #859 has no PR, so nothing is duplicated.
#600 is closed as superseded, unmerged, once the pull requests for #980, #981 and #982 have merged, as the table states.
Size
M. One pull request. Estimate in code lines including tests: lock.go about 150, expected map and base reads about 120, walk and comparison about 80, tests about 350, for roughly 700. Fixture files and generated output are excluded; any executable fixture or harness code counts as code. If the built diff passes 800, split before opening: the parser with its tests as this issue, then the expected map, walk and comparison as a new issue depending on it.
Verify plan
- AC1, AC2: table-driven parser tests on lock fixtures recorded from the oracle, covering quoted, commented, inline, block and empty forms, duplicates and invalid scalars.
- AC3, AC4, AC5: tests in a scratch repository with a base commit, local and remote package records, no owner, two owners, a failed base read, an absent lock, and malformed and duplicate hashes.
- AC6: tests with an empty remote hash set and an empty local base directory, each against an empty directory on disk.
- AC7: tests with a missing root, a regular file as root and a symlink root.
- AC8: tests with a symlink inside the directory on every platform, plus a FIFO case run on Linux and macOS only. On Windows the symlink and not-a-directory cases carry the refusal.
- AC9: tests for equal, changed, missing and extra files.
- AC10: the CI matrix run, a test with
uvabsent fromPATH, and a fixture repository holding an executable hook and helper that would create a sentinel file, asserting it never appears. - AC11: the diff shows no shell or Python change, and a search of non-test Go files finds the new exported symbols referenced nowhere outside
internal/mergepolicy, while theApprovedcaller is unchanged. - AC12: parser tests assert each dependency and deployment field, including
active_owner, on the oracle fixtures. - Definition of done: D1 each AC verified on the merged head with commands and results cited in the PR's Verification. D2 independent review of the exact merged head. D3 required checks,
mise run check,mise run cover-go-checkandmise run cover-py-check. D4check-leaks.shwith the private-strings list and gitleaks on the diff and each post. D5 signed commits. D6 doc impact: "No doc impact", an unused internal library. D7Assisted-bytrailers and theprovenance:agent-assistedlabel. D8 the line count against 800. D9 follow-ups filed with a Problem and numbered ACs and listed under Related in the PR. D10Closes #981, the board showing Done, and a finalStatus:comment naming the merge commit. devtools-2 does not apply to a Go library. devtools-3: the PR says "No consumer impact" because the library is unused.
Risks
A parser that reads a lock differently from the oracle could accept a directory the base does not own, and #859 would inherit that. Oracle fixtures are the control, and no production code calls the new APIs until #859 and #982. Undo: revert; no caller depends on it. No consumer impact: an unused internal library.
Operator authority
None.
Activity
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — settle parser ownership, fail-closed cases, and the cutover boundary
Checked
Gaps
Resolve The merge policy reads the APM lockfile and base-owned deployments in Go #859/Verify merge skill directories against base lock hashes in Go #981 parser ownership before implementation. Both issues claim the common scalar/list/record parser, while The merge policy reads the APM lockfile and base-owned deployments in Go #859 also needs deployments and active-owner fields and Verify merge skill directories against base lock hashes in Go #981 needs deployed_file_hashes. “Whichever merges first” plus rebasing does not assign a single deliverable or settle the field contract. Choose the parser owner, update both scopes and verification, and add the corresponding native blocked-by relationship. Retain the separate consumers: scan ownership in The merge policy reads the APM lockfile and base-owned deployments in Go #859 and integrity in Verify merge skill directories against base lock hashes in Go #981.
Name the actual parity oracle and settle the competing work. The “frozen helper” on main has neither integrity nor deployed_file_hashes parsing; its selected-field parser ends at packages/agents/source/skills/merge-pr/shared/scripts/merge-policy.py:181. Identify the exact PR fix(merge): pin gate policy and require machine authority #600 commit/cases used to record hash expectations, distinguish unchanged parser behavior from new integrity behavior, and record how fix(merge): pin gate policy and require machine authority #600 is superseded so it cannot also land the same behavior as new shell/Python. Deriving hash maps from main as written is not possible. Keep fixture extraction disposable; add no production shell logic.
Require a nonempty trusted file set for each skill. AC3 permits one owning record, and AC7 says equal maps pass; together these allow an empty expected map and empty directory to succeed. Specify refusal for a missing/non-directory skill root and unreadable/missing required lock or base objects. Verify empty remote hashes, an empty local base directory, a symlink root, missing root, and failed base reads alongside the equality cases. PR fix(merge): pin gate policy and require machine authority #600's proposed integrity routine rejects an empty expected map; preserve that boundary explicitly.
Strengthen AC8 verification. Absence of uv proves only that uv is unnecessary; it does not prove “no repository content runs.” Add a malicious hook/helper sentinel fixture and assert no sentinel is created while base blobs are read and files hashed. State how the FIFO test is platform-scoped and what exercises non-regular/root refusal on Windows; an unconditional FIFO fixture cannot establish the promised three-platform run. Keep every AC ID traced after these edits.
Reconcile the split with existing Epic children and merge gates take authority and gate settings from the environment and the working tree #499. Run the skill-directory integrity gate in the merge entry points #982 AC8 replaces all merge entry scripts, but The lint, populate-description, squash-message and review scripts become wrappers over agentrepo merge #874/The squash-merge and merge scripts become wrappers over agentrepo merge #875/The preflight scripts become wrappers over agentrepo merge #876 already own wrapper cutovers with binary-distribution, protection and N-2 prerequisites; Run the skill-directory integrity gate in the merge entry points #982 currently blocks only on Verify merge skill directories against base lock hashes in Go #981. Main implements only merge approval (internal/agentrepo/merge_approval.go:53), so Run the skill-directory integrity gate in the merge entry points #982 cannot assume a complete transaction exists. Bound Run the skill-directory integrity gate in the merge entry points #982 to integrity integration, tie it natively to the required transaction/cutover children, and keep protected Python deletion in the separately authorized Replace complex shell with tested Go, starting with gates and merging #588 cutover. Identify owners for merge gates take authority and gate settings from the environment and the working tree #499's live machine-authority adoption, pinned-setting adoption, unchanged commit/PR overrides, and authority/migration documentation: feat(config): read merge authority from machine configuration #942's reader and Read gate settings from the pinned base in Go #980's accessor alone do not complete those outcomes. Keep that work outside Verify merge skill directories against base lock hashes in Go #981.
Establish the size before dispatch. M is plausible for an isolated library, but “if the built package measures over 800” is a late split and does not bound PR code including tests. Add a bounded estimate for the shared parser, expected-map/base reads, walk/comparison and tests, with only permitted generated exclusions. If it exceeds 800 code lines, split/refine the parser first and make integrity depend on it. Settle parser ownership before estimating.
Split
Keep #980 for pinned configuration/binary selection, #981 for the integrity library, and #982 for bounded integration into an available Go merge path. Reconcile #859's parser once, preserve the existing wrapper/deprecation owners, and retire #600's competing implementation. The current bodies do not yet demonstrate a non-overlapping, gap-free split under 800 code lines per PR. #981 can remain one M PR if the inclusive estimate establishes that bound; otherwise split parser from map/walk before coding.
Definition of done
Ticks indicate coverage in the proposed ACs/Verify plan, not completed implementation or merged-head evidence. Partial or absent coverage stays unchecked.
Round: 1 of 3
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — finish the parser handoff and make the no-caller criterion satisfiable
Checked
Gaps
Complete the parser handoff rather than describe a future dependency. Live native links show Verify merge skill directories against base lock hashes in Go #981 blocks only Run the skill-directory integrity gate in the merge entry points #982; The merge policy reads the APM lockfile and base-owned deployments in Go #859 is still blocked only by Ship the Go binaries as release assets installed through mise's github backend #333. Its body still assigns lock parsing to itself and says nothing about consuming Verify merge skill directories against base lock hashes in Go #981. Edit The merge policy reads the APM lockfile and base-owned deployments in Go #859's scope, approach and Verify plan to consume the shared parser, retaining only its ownership/command layer, and add native blocked-by Verify merge skill directories against base lock hashes in Go #981 alongside Ship the Go binaries as release assets installed through mise's github backend #333. State that the shared parser supplies the dependency and deployment fields The merge policy reads the APM lockfile and base-owned deployments in Go #859 needs, including active_owner; The merge policy reads the APM lockfile and base-owned deployments in Go #859 still owns interpreting those fields. Verify merge skill directories against base lock hashes in Go #981 can have no blockers. This is the remaining round-1 ownership gap.
Correct AC11 and its Verify bullet. “Nothing calls the new package” / “a search finds no caller of the package” cannot pass: internal/mergepolicy is an existing package with a production caller at internal/agentrepo/merge_approval.go:34. Require instead that no production caller invokes the newly added parser or integrity APIs until their consuming issues land; tests may invoke them. Verify references to the new exported symbols while preserving the existing Approved caller. Keep the no-new-shell/no-Python outcome.
Reconcile the competing PR's retirement across Read gate settings from the pinned base in Go #980/Verify merge skill directories against base lock hashes in Go #981/Run the skill-directory integrity gate in the merge entry points #982. Verify merge skill directories against base lock hashes in Go #981 says fix(merge): pin gate policy and require machine authority #600 closes after all three merge; Read gate settings from the pinned base in Go #980 says it closes unmerged when the first replacement PR merges and includes a separately proposed binary-selection issue. Run the skill-directory integrity gate in the merge entry points #982 describes yet another completion boundary including remaining merge gates take authority and gate settings from the environment and the working tree #499 work. Choose one supersession rule in all three, explicitly preventing fix(merge): pin gate policy and require machine authority #600 from landing its Python/shell implementation under Replace complex shell with tested Go, starting with gates and merging #588. Update Verify merge skill directories against base lock hashes in Go #981's statement that Read gate settings from the pinned base in Go #980 owns binary selection: Read gate settings from the pinned base in Go #980 now expressly excludes it. This needs issue/dependency bookkeeping, not an expansion of Verify merge skill directories against base lock hashes in Go #981 into wrappers, authority adoption or documentation.
Split
Keep #981 as the parser plus integrity library in one M PR, #859 as its ownership consumer, #982 as the snapshot/subcommand, and #980 as the pinned-settings reader. Keep wrapper cutovers and protected deletion with their existing owners. No additional split is required by the current estimate; count any executable fixture or harness code as code, and split before publication if the actual code diff reaches the 800-line boundary.
Definition of done
Ticks mean the issue's ACs/Verify plan cover the item, not that implementation has completed it. Unchecked base items are coverage notes; DoD rollout is warn-only and these alone do not determine this verdict.
Round: 2 of 3
tbhb-dev-reviews commentedon Oct 31, 2026
Refinement: NOT READY — complete the parser handoff and binary-refusal prerequisite
Checked
Gaps
Complete the The merge policy reads the APM lockfile and base-owned deployments in Go #859 handoff before marking Verify merge skill directories against base lock hashes in Go #981 ready. Its live body still owns lock reading/parsing, derives its own ownership fixtures, and lists only Ship the Go binaries as release assets installed through mise's github backend #333 as its blocker; it does not mention Verify merge skill directories against base lock hashes in Go #981 or consuming its parser. Edit its Scope, Approach and Verify plan to retain only the ownership layer/command over Verify merge skill directories against base lock hashes in Go #981's exported parser, and add native blocked-by Verify merge skill directories against base lock hashes in Go #981 beside Ship the Go binaries as release assets installed through mise's github backend #333. AC12 resolves the field contract, but a promise to edit the consumer is not the completed dependency correction. The D9 allowance to defer that edit also conflicts with the explicit before-ready requirement; remove that deferral.
Satisfy the explicit before-ready condition for binary refusal. The shared table says a separate issue must be filed and linked as a native blocker of The lint, populate-description, squash-message and review scripts become wrappers over agentrepo merge #874/The squash-merge and merge scripts become wrappers over agentrepo merge #875/The preflight scripts become wrappers over agentrepo merge #876 before any of Read gate settings from the pinned base in Go #980/Verify merge skill directories against base lock hashes in Go #981/Run the skill-directory integrity gate in the merge entry points #982 is ready. No issue number is supplied and no such blocker appears on those wrappers. File/link the bounded tested-Go behavior, thin forwarding boundary, build(agents): protect replacement Go enforcement paths #753/owner exact-head requirement, deprecation declaration and migration note as specified. If an existing issue owns it, name that issue and reconcile its scope and native links. Keep this outside Verify merge skill directories against base lock hashes in Go #981's implementation and add no logic to the frozen lib.sh resolver.
Split
Keep #981 as one M PR containing the shared parser and expected-map/walk/comparison library, #859 as its ownership consumer, #980 as the settings reader, and #982 as the snapshot/subcommand. No further split is justified by the current estimate. Count executable fixtures/harnesses as code and enforce the requested under-800 boundary before publication. The remaining gaps concern completing the declared handoffs. This is the third NOT READY round; the owner should rethink the unresolved handoff/readiness arrangement under the three-round rule before dispatching implementation.
Definition of done
Ticks mean coverage in #981's ACs/Verify plan, not completed implementation or merged-head evidence. Partial wording is identified below; these coverage notes are separate from the two readiness gaps.
Round: 3 of 3