feat: add internal/lock package for harness dependency pinning

[ggallen]

Summary

Phase 3, PR 1 of ADR-0038 (Universal Harness Access). Introduces the lock file data model and I/O for .fullsend/lock.yaml.

• Adds internal/lock/ package: LockFile, HarnessLock, and DependencyEntry structs modeling the lock file schema from the design doc
• Load/Save with atomic writes (temp-file-then-rename with fsync), version validation, and auto-directory creation
• Lookup helpers: by harness name, by dependency URL (depth-first through transitive deps), staleness detection via source hash comparison
• 24 tests covering round-trip serialization, error cases, transitive lookups, nil safety, and atomic overwrites

No callers — pure data model + I/O. The fullsend lock CLI subcommand and resolver integration follow in Phase 3 PR 2.

Depends on: Phase 2 (all merged: #1857, #1923, #2022)

Test plan

• go test ./internal/lock/... — 24 tests pass
• go test ./internal/... — all existing tests pass (no regressions)
• go vet ./... — clean
• make lint — clean
• Review: lock file schema matches ADR-0038 §6 (lock file format) and design doc §Phase 3

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://3687caf3-site.fullsend-ai.workers.dev

Commit: 7fda456580b6380cbf7f776609b615c6107191ff

[fullsend-ai-review]

🤖 Review · Started 8:47 PM UTC
Commit: d0ac11b · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [schema-mismatch] docs/plans/universal-harness-access.md:1275 — The implementation plan's lock file schema (labeled "Strawman") uses a map-based dependency format (agent:, skills:) while the actual implementation uses a flat array with field properties. The ADR's schema example is updated in this PR to match the implementation, but the plan still shows the old strawman. Low priority since the plan labels it as a strawman and the ADR (authoritative document) is being corrected here.

• [incomplete-phase] internal/lock/lock.go — This PR introduces the lock file data model and I/O with no callers yet. The functionality is not integrated into the resolver or CLI until Phase 3 PR 2. This is expected for a phased rollout.

• [file-permissions-inconsistency] internal/lock/lock.go:103 — Lock files use 0o644 permissions while internal/fetch/cache.go uses 0o600. The code comment explicitly documents the rationale (lock files are committed to version control). This is an intentional, well-documented design choice.

Previous run

Review

Findings

Medium

• [missing-authorization] internal/lock/lock.go — This PR introduces 619 lines of new code creating a new package, which constitutes a non-trivial change. No linked issue is present for traceability, though the PR body references ADR-0038 and prior Phase 2 PRs (feat: add skill frontmatter parser for transitive dependency resolution #1857, feat: add transitive dependency resolution to URL resolver #1923, feat(cli): wire --max-depth and --max-resources flags for transitive resolution #2022) as authorization. Consider linking a tracking issue for ADR-0038 Phase 3.

• [schema-mismatch] docs/ADRs/0038-universal-harness-access.md:305 — Lock file schema summary in ADR-0038 shows dependencies as a nested map structure (dependencies.agent, dependencies.skills), but internal/lock/lock.go implements dependencies as a flat array of DependencyEntry objects with a Field property. The ADR defers to the implementation plan for the detailed schema, but the summary is misleading.
  Remediation: Update the ADR-0038 schema summary to match the implemented flat-array structure, or add a note that the summary is illustrative and the implementation plan is authoritative.

Low

• [test-adequacy] internal/lock/lock_test.go — No test exercises the maxLookupDepth guard (depth >= 32 in lookupInDeps). This is a correctness-relevant behavior — silently returning nil when a matching dependency exists beyond depth 32 — and should be covered to prevent regressions if the constant or recursion logic changes.

• [edge-case] internal/lock/lock.go:72 — Save mutates its input: when lf.Version == 0, it sets lf.Version = currentVersion, modifying the caller's struct as a side effect. This is not documented in the doc comment and could surprise callers. Consider either documenting this mutation or operating on a copy.

• [schema-mismatch] docs/plans/universal-harness-access.md:1275 — Implementation plan's lock file schema shows dependencies as a nested map structure, but the implementation uses a flat array. The plan explicitly labels this as a "Strawman lock file schema," so drift is expected, but updating it would prevent confusion.

• [incomplete-schema] docs/ADRs/0038-universal-harness-access.md:303 — ADR-0038 lock file schema summary is missing the source and harness-level sha256 fields present in the implementation. The implementation plan's strawman does include these fields, so the ADR summary is simply less complete than its authoritative reference.

Info

• [API-contract] internal/lock/lock.go — Lookup returns a pointer to a copy (snapshot semantics), while LookupDep returns a pointer into the live slice (reference semantics). This asymmetry is not a bug today (no callers yet), but LookupDep's doc comment should note reference semantics for consistency with Lookup's snapshot warning.

• [sub-agent-failure] The style-conventions and intent-coherence sub-agents could not access PR branch files (files not on main branch). Findings from these dimensions were not produced for this review.

Previous run (2)

Review

Findings

Medium

• [missing-authorization] internal/lock/lock.go — This PR introduces 619 lines of new code creating a new package, which constitutes a non-trivial change. No linked issue is present for traceability, though the PR body references ADR-0038 and prior Phase 2 PRs (feat: add skill frontmatter parser for transitive dependency resolution #1857, feat: add transitive dependency resolution to URL resolver #1923, feat(cli): wire --max-depth and --max-resources flags for transitive resolution #2022) as authorization. Consider linking a tracking issue for ADR-0038 Phase 3.

Low

• [file-permissions-documentation] internal/lock/lock.go:76 — The 0o644 permission choice is documented in the function-level doc comment, which differs from cache.go where it is documented inline at the Chmod call. The function-level approach is arguably superior; no change needed.

• [test-organization] internal/lock/lock_test.go — Nil receiver tests (TestSetHarness_NilLockFile, TestIsStale_NilHarnessLock, TestLookupDep_NilHarnessLock) could be grouped into a single table-driven test to reduce repetition. Minor style preference — current approach is consistent with the codebase.

• [missing-documentation] docs/guides/dev/cli-internals.md:7 — CLI command tree does not include fullsend lock, which ADR-0038 documents. This can be deferred until the CLI command is wired up in Phase 3 PR 2.

Info

• [edge-case] internal/lock/lock.go — All prior findings have been addressed: append aliasing resolved via explicit pre-allocation (lines 88-90), Lookup nil-receiver behavior documented (line 125), maxLookupDepth rationale comment added (lines 152-153). No new correctness issues found.

• [design-direction] internal/lock/lock.go — The internal/lock package introduction aligns with ADR-0038 (Universal Harness Access) Phase 3 requirements.

• [scope-coherence] internal/lock/lock.go — Pure data model + I/O with no callers is architecturally sound for incremental delivery, matching the Phase 3 two-PR plan.

• [naming-consistency] internal/lock/lock.go — Package name follows established conventions (singular nouns for data model packages). Lock file location .fullsend/lock.yaml matches ADR-0038 spec.

Previous run (3)

Review

Findings

Low

• [api-contract] internal/lock/lock.go:89 — content := append(header, data...) is safe today because header is freshly allocated by []byte(...) with len==cap, so append always allocates a new backing array. However, this is a fragile idiom — if the header construction is ever refactored to pre-allocate extra capacity or reuse a buffer, append would silently mutate the header's backing array. Using slices.Concat or explicit allocation would be more robust.

• [nil-receiver-documentation] internal/lock/lock.go:123 — Lookup handles nil receivers (returns nil) but does not document this in its doc comment, unlike LookupDep, IsStale, and SetHarness which all document their nil receiver behavior. Add "Returns nil for a nil receiver." to Lookup's doc comment for consistency.

• [depth-limit-pattern] internal/lock/lock.go:154 — maxLookupDepth = 32 lacks a rationale comment. Unlike resolve.go which documents its DefaultMaxDepth, this constant has no explanation. Add: // Guard against excessively deep or cyclic transitive deps.

• [missing-documentation] docs/guides/dev/cli-internals.md — CLI command tree does not include fullsend lock, which ADR-0038 documents. This can be deferred until the CLI command is wired up in Phase 3 PR 2.

Info

• [api-contract] internal/lock/lock.go:30 — The implementation uses a list of DependencyEntry structs with a field attribute instead of the map keyed by field name shown in the ADR. This is a reasonable design improvement (lists are more natural for ordered/indexed entries like skills[0]). The ADR's schema is labeled "Strawman" — consider noting the divergence when the ADR is next updated.

• [edge-case] Prior findings resolved: Lookup snapshot semantics are now documented (line 125), lookupInDeps has a depth cap of 32 (line 154), SetHarness has a nil receiver guard (line 172), and nil receiver tests are present for SetHarness, IsStale, and LookupDep.

• [design-direction] The internal/lock package introduction aligns with ADR-0038 (Universal Harness Access) and the design doc Phase 3 plan.

Previous run (4)

Review

Findings

Medium

• [nil-deref] internal/lock/lock.go:153 — SetHarness does not guard against a nil receiver, unlike Lookup (line 119) and HarnessNames (line 163) which both handle nil *LockFile gracefully. If a caller obtains a nil *LockFile from Load (file not found) and calls SetHarness, the method will panic with a nil pointer dereference.
  Remediation: Add a nil check at the top of SetHarness: if lf == nil { return }. Given that Lookup and HarnessNames already set the precedent of nil-safety, consistency argues for adding the guard.

Low

• [api-contract] internal/lock/lock.go:124 — Lookup copies the HarnessLock value out of the map and returns a pointer to that local copy. Mutations through the returned pointer do NOT update the LockFile map. The only safe mutation path is SetHarness, but nothing in the API surface signals this constraint. Consider adding a doc comment noting the returned pointer is a snapshot.

• [edge-case] internal/lock/lock.go:140 — lookupInDeps performs unbounded recursion through TransitiveDeps. If a lock file contains a cycle (possible via YAML anchors/aliases or manual editing), this will cause a stack overflow. A simple depth cap (e.g., 32) would suffice for practical dependency trees.

• [missing-test] internal/lock/lock_test.go — No test covers calling SetHarness on a nil *LockFile, nor LookupDep/IsStale on a nil *HarnessLock. Given that Lookup and HarnessNames are tested for nil receivers, the asymmetry suggests these cases were overlooked.

• [pattern-inconsistency] internal/lock/lock.go:97 — Lock file uses 0o644 permissions while fetch/cache.go uses 0o600 for cache files that also contain SHA256 integrity hashes. This is likely intentional (lock files are meant to be committed to version control), but the choice should be documented.

• [design-smell] internal/lock/lock.go:159 — SetHarness mutation API may not fully align with the "generated file" nature of lock files. Consider documenting that this is intended for use only by the lock file generator.

Info

• [api-contract] internal/lock/lock.go:30 — The implementation uses a list of DependencyEntry structs (with a field attribute) instead of the map keyed by field name shown in the ADR, and adds source/sha256 fields at the HarnessLock level absent from the ADR schema. These are reasonable design improvements; the divergence should be noted so the ADR stays accurate.

• [design-direction] internal/lock/lock.go — The internal/lock package introduction aligns with documented architecture in ADR-0038 and design doc Phase 3.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:47 PM UTC · Completed 8:58 PM UTC
Commit: d0ac11b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:37 AM UTC · Completed 10:50 AM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:03 PM UTC · Completed 12:13 PM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:41 PM UTC · Completed 12:51 PM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:13 PM UTC · Completed 3:26 PM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:41 PM UTC · Completed 3:47 PM UTC
Commit: ba204cb · View workflow run →

[fullsend-ai-retro]

Retro: PR #2049 — feat: add internal/lock package for harness dependency pinning

This human-authored PR (by ggallen, co-authored with Claude Code locally) introduced a new internal/lock/ package (+652 lines across 2 new Go files and 1 ADR update). It went through 5 review agent iterations over ~19 hours before merging.

What went well

• Iteration 1 caught a genuine nil-deref bug in SetHarness (Medium severity) — real value delivered.
• Iteration 2 identified a subtle append backing-array fragility and documentation gaps — also valuable.
• The human reviewer (rh-hemartin) approved early, and the author was responsive to all feedback.

What could go better

• Self-dismissing findings: The agent repeatedly raised findings it simultaneously acknowledged as non-issues (e.g., file permissions flagged 3 times, each time the agent itself concluded "intentional, well-documented"). Already tracked in #1881.
• No cross-iteration memory: The agent re-raised the same file-permissions finding in iterations 1, 3, and 5 despite acknowledging it as intentional each time. Already tracked in #1552.
• Non-deterministic severity on unchanged code: The same commit received different verdicts across runs, causing label flapping between requires-manual-review and ready-for-merge. Already tracked in #1389.
• False Medium for governance concern: The agent raised a Medium "missing-authorization" finding (no linked issue) despite clear ADR traceability in the PR body. Related to #1273 (incorporate PR description context when assessing scope).
• Diminishing returns: Iterations 1-2 produced all the real value. Iterations 3-5 were mostly noise and repetition. Already tracked in #1370.

Proposals

No new proposals — all identified improvement opportunities are already covered by existing open issues (#1881, #1552, #1389, #1273, #1370). This PR provides additional supporting evidence for those issues, particularly #1881 (self-dismissing findings) and #1389 (verdict instability on unchanged code).