feat(cli): add --all flag to fullsend lock for batch harness locking by ggallen · Pull Request #2262 · fullsend-ai/fullsend

ggallen · 2026-06-12T19:43:40Z

Summary

Add --all flag to fullsend lock to lock every harness in the directory in a single pass
Extract lockOneAgent helper so single-agent and batch modes share the same resolution logic
Extract resolveHarnessPath helper shared by lock.go and run.go for consistent .yaml/.yml fallback with dual-extension warnings
Add discoverHarnessNames using filepath.Glob for *.yaml and *.yml files
Write the combined lock file once at the end (not per-agent)
Prune stale lock entries for harnesses that no longer have remote dependencies or were removed from the directory

Cross-command consistency fix (discovered gap): run.go lacked the .yml extension fallback and dual-extension warning that lockOneAgent had. Both now use the shared resolveHarnessPath helper, and run.go uses absFullsendDir consistently.

ADR-0045 Phase 2 PR 5 — parallel with PRs 1–3, feeds into PR 6.

Test plan

🤖 Generated with Claude Code

github-actions · 2026-06-12T19:45:15Z

Site preview

Preview: https://fd765a21-site.fullsend-ai.workers.dev

Commit: acecfc33ad3f3a3dc2ce64cce81707719aab8c17

fullsend-ai-review · 2026-06-12T19:47:23Z

🤖 Finished Review · ✅ Success · Started 7:47 PM UTC · Completed 8:00 PM UTC
Commit: 7d1cd39 · View workflow run →

codecov · 2026-06-12T19:47:26Z

Codecov Report

❌ Patch coverage is 84.72906% with 31 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/lock.go	84.42%	22 Missing and 9 partials ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-06-12T20:00:22Z

Looks good to me

Previous run

Review

Findings

Medium

[internal-consistency] docs/plans/adr-0045-forge-portable-harness-phase2.md:95 — Line 95 (in the PR 1/DiscoverAgents section, not touched by this diff) says: "Contrast with lock --all (PR 5), which uses all-or-nothing semantics because writing an incomplete lock file would silently leave some dependencies unpinned." The diff updates lines 327 and 341 of the same document from "all-or-nothing" to "partial-progress semantics", making line 95's contrast statement factually incorrect.
Remediation: Update line 95 to reflect that lock --all now uses partial-progress semantics.

Low

[design-document-divergence] docs/plans/adr-0045-forge-portable-harness-phase2.md:320 — The plan specifies cobra.MaximumNArgs(1) but the implementation uses cobra.RangeArgs(0, 1). Functionally equivalent, but the plan text is not updated to match.
[unauthorized-change] docs/plans/adr-0045-forge-portable-harness-phase2.md:54 — The PR changes failure semantics from all-or-nothing to partial-progress. The plan document is updated at lines 327 and 341 to reflect this, and the PR description documents the change, so the deviation is self-documenting. The remaining issue is the stale reference at line 95 (see medium finding above).
[scope-creep] internal/cli/run.go — The run.go changes (integrating resolveHarnessPath) extend beyond the stated PR 5 scope. The PR description calls this out as a "discovered gap" and "cross-command consistency fix", which is reasonable — without it, lock --all would discover .yml harnesses that run would fail to load.
[error-message-consistency] internal/cli/lock.go:113 — The mutual-exclusion error --all must not be combined with a positional agent name differs from the established --X and --Y are mutually exclusive pattern in admin.go.
[error-message-consistency] internal/cli/lock.go:117 — The required-argument error --all or a positional agent name is required differs from the established must specify X or use Y pattern in admin.go.

Info

[edge-case] internal/cli/lock.go — In runLockAll, when lockOneAgent returns nil (up-to-date), isHarnessUpToDate re-reads the harness file and recomputes its SHA256 — duplicating work already done inside lockOneAgent. Functionally correct; negligible overhead since it only occurs on the fast path (no network calls).
[design-evolution] docs/plans/adr-0045-forge-portable-harness-phase2.md:54 — The partial-progress save on failure is a reasonable design evolution from the original all-or-nothing plan. The implementation correctly saves partial progress only when at least one harness was successfully locked.

Previous run (2)

Review

Reason: tool-failure

This PR was NOT reviewed. Do not count this as an approval.

Previous run (3)

Review

Findings

Medium

[design-document-divergence] internal/cli/lock.go — runLockAll implements partial-save-on-failure semantics (writing successfully locked harnesses to disk before returning the error), but the implementation plan at docs/plans/adr-0045-forge-portable-harness-phase2.md explicitly specifies all-or-nothing: "If any individual file fails to parse, the entire lock --all invocation fails with no partial lock file written." The partial-save behavior is arguably better UX (avoids re-resolving already-completed harnesses on retry), but it contradicts the referenced specification. Either update the plan to reflect the partial-save decision, or align the implementation with the spec.

Low

[stale-doc] docs/ADRs/0038-universal-harness-access.md:341 — Shows fullsend lock <harness> with required angle brackets, but the PR makes the argument optional ([agent-name]) with --all support. ADR is a decision record rather than a living CLI reference, so impact is low.
[error-message-consistency] internal/cli/lock.go — The mutual-exclusion error messages ("--all and a positional agent name are mutually exclusive", "specify an agent name or use --all") are clear but use a different pattern from the existing numeric-constraint validations (--max-depth must be >= 0, got %d). Minor stylistic inconsistency.

Info

[edge-case] internal/cli/lock.go — In runLockAll, when lockOneAgent returns nil (up-to-date), isHarnessUpToDate re-reads the harness file and recomputes its SHA256 — duplicating work already done inside lockOneAgent. Functionally correct; minor inefficiency.
[stale-doc] docs/plans/universal-harness-access-phase3.md:59 — Shows the old usage pattern fullsend lock <agent-name> with required argument. This is a completed implementation plan, not a living reference — the PR already updates the actual user-facing docs.

Previous run (4)

Review

Findings

Low

[test-adequacy] internal/cli/lock_all_test.go — No test verifies that fullsend run works with a .yml harness extension via the shared resolveHarnessPath helper. The test suite covers the lock path comprehensively (including TestResolveHarnessPath_FallsBackToYml, TestLockOneAgent_YMLFallback), but the cross-command consistency fix in run.go lacks its own integration test. Since resolveHarnessPath is well unit-tested, the residual risk is low.

Info

[missing-authorization] internal/cli/lock.go — Non-trivial PR has no linked issue, but the PR references ADR-0045 Phase 2 PR 5 which provides sufficient architectural authorization. The cross-command consistency fix in run.go is explicitly called out as a discovered gap.
[edge-case] internal/cli/lock.go — In runLockAll, when lockOneAgent returns nil (up-to-date), isHarnessUpToDate re-reads the harness file and recomputes its SHA256 — duplicating work already done by lockOneAgent. A theoretical TOCTOU gap exists if the file is modified between the two reads, but this requires concurrent modification during a lock run, making it extremely unlikely in practice.
[stale-doc] docs/plans/universal-harness-access-phase3.md — Shows the old usage pattern fullsend lock <agent-name> with required argument, but the PR changes this to [agent-name] (optional) with --all support. This is a completed implementation plan, not a living reference — the PR already updates the actual user-facing docs.

Previous run (5)

Review

Findings

Low

[test-adequacy] internal/cli/lock_all_test.go — No test verifies that fullsend run works with a .yml harness extension via the shared resolveHarnessPath helper. The test suite covers the lock path comprehensively (including TestResolveHarnessPath_FallsBackToYml, TestLockOneAgent_YMLFallback), but the cross-command consistency fix in run.go lacks its own integration test. Since resolveHarnessPath is well unit-tested, the residual risk is low.

Info

[resolved] internal/cli/lock.go — Prior finding [edge-case] about resolveHarnessPath addressed: the function now properly stats .yml before returning and produces a combined error (harness file not found: tried X.yaml and X.yml) when neither extension exists.
[missing-authorization] internal/cli/lock.go — Non-trivial PR has no linked issue, but the PR references ADR-0045 Phase 2 PR 5 which provides sufficient architectural authorization. The cross-command consistency fix in run.go is explicitly called out as a discovered gap.
[edge-case] internal/cli/lock.go — In runLockAll, when lockOneAgent returns nil (up-to-date), isHarnessUpToDate re-reads the harness file and recomputes its SHA256 — duplicating work already done by lockOneAgent. A theoretical TOCTOU gap exists if the file is modified between the two reads, but this requires concurrent modification during a lock run, making it extremely unlikely in practice.
[stale-doc] docs/plans/universal-harness-access-phase3.md — Shows the old usage pattern fullsend lock <agent-name> with required argument, but the PR changes this to [agent-name] (optional) with --all support. This is a completed implementation plan, not a living reference — the PR already updates the actual user-facing docs.

Previous run (6)

Review

Findings

Low

[edge-case] internal/cli/lock.go — resolveHarnessPath returns the .yml path without verifying it exists when .yaml is not found. If neither extension exists, the downstream os.ReadFile error says "reading harness file: open .../agent.yml: no such file or directory" without indicating that .yaml was also checked. Consider adding an os.Stat check for .yml before returning, and producing a combined error mentioning both extensions were tried.
[test-adequacy] internal/cli/lock_all_test.go — No test verifies that fullsend run works with a .yml harness extension via the new resolveHarnessPath helper. The test suite comprehensively covers the lock path but the cross-command consistency fix in run.go lacks its own test.

Info

[resolved] internal/cli/lock.go — Prior finding [code-duplication] addressed: resolveHarnessPath is now a shared helper used by both lockOneAgent in lock.go and runAgent in run.go, eliminating the duplication.
[resolved] internal/cli/lock.go — Prior finding [variable-naming] addressed: the variable is now named lockAll, matching the shorter boolean flag naming convention used elsewhere in the codebase.
[missing-authorization] internal/cli/lock.go — Non-trivial PR has no linked issue, but the PR references ADR-0045 Phase 2 PR 5 which provides sufficient architectural authorization. The cross-command consistency fix in run.go is explicitly called out as a discovered gap.
[edge-case] internal/cli/lock.go — In runLockAll, when lockOneAgent returns nil (up-to-date), isHarnessUpToDate re-reads the harness file and recomputes its SHA256 — duplicating work already done by lockOneAgent. A theoretical TOCTOU gap exists if the file is modified between the two reads, but this requires concurrent modification during a lock run, making it extremely unlikely in practice.
[edge-case] internal/cli/lock.go — The PR correctly fixes the Source field in HarnessLock to use filepath.Base(harnessPath) instead of hardcoding .yaml, so .yml harnesses now get the correct source path in the lock file.

Previous run (7)

Review

Findings

Medium

[code-duplication] internal/cli/run.go:143 — The harness file discovery logic (check .yaml, fall back to .yml, warn on dual extensions) is duplicated between runAgent in run.go and lockOneAgent in lock.go. The pattern is structurally identical: os.Stat for .yaml, if os.IsNotExist fall back to .yml, else check for .yml coexistence and warn. Consider extracting into a shared helper like resolveHarnessPath(dir, agentName, printer) (string, error) to reduce duplication and ensure consistent behavior across commands.

Low

[scope-creep] internal/cli/run.go:143 — The run.go changes (.yml extension fallback, dual-extension warning, fullsendDir → absFullsendDir fix) are sensible but extend beyond the PR's stated scope of "fullsend lock --all" (Phase 2 PR 5). The implementation plan does not mention run.go changes. Consider calling out these changes explicitly in the PR description as a discovered gap from Phase 1. See also: [code-duplication] finding at this location.
[variable-naming] internal/cli/lock.go:48 — The variable allHarnesses is slightly verbose compared to existing boolean flag names in the codebase (update, offline, keepSandbox, noPostScript). Consider lockAll for consistency.

Info

[resolved] internal/cli/lock.go:479 — Prior finding addressed: isHarnessUpToDate now emits printer.StepWarn on os.Stat and os.ReadFile errors before returning true, resolving the prior concern about zero diagnostic output on permission errors.
[resolved] internal/cli/run.go:143 — Prior finding addressed: harnessPath construction now uses absFullsendDir (resolved absolute path) instead of the raw fullsendDir input, matching the convention in lockOneAgent.

Previous run (8)

Review

Findings

Low

[edge-case] internal/cli/lock.go:479 — isHarnessUpToDate silently returns true (treating the harness as up-to-date) when os.Stat fails with a non-IsNotExist error (e.g., permission denied), and also when os.ReadFile fails. This prevents pruning but gives the user zero diagnostic output. A user running fullsend lock --all with permission issues on some harness files would see those entries silently counted as "up to date" with no warning. The main lockOneAgent call handles these errors properly, so the impact is limited to the pruning path when a harness was previously locked but now has no remote deps.
[inconsistency] internal/cli/run.go:143 — The new .yml fallback in runAgent constructs harnessPath using the un-resolved fullsendDir (relative path as provided by the user), while lockOneAgent uses absFullsendDir. No runtime bug since paths are resolved downstream, but a minor convention inconsistency between two functions that share identical fallback logic.

Previous run (9)

Review

Findings

Low

[edge-case] internal/cli/lock.go:491 — isHarnessUpToDate does not handle non-IsNotExist errors from os.Stat (e.g., permission denied). If os.Stat fails for a non-file-not-found reason, the function returns false, triggering pruning of a potentially valid lock entry. lockOneAgent correctly distinguishes IsNotExist from other stat errors. The degradation is safe (over-prune rather than silently keep stale data), but the inconsistency could cause surprising behavior for users with permission issues.

Info

[stale-doc] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Completed implementation plan, not a living reference.

Previous run (10)

Review

Findings

Low

[edge-case] internal/cli/lock.go:491 — isHarnessUpToDate does not handle non-IsNotExist errors from os.Stat (e.g., permission denied). If os.Stat fails for a non-file-not-found reason, the function returns false, triggering pruning of a potentially valid lock entry. lockOneAgent correctly distinguishes IsNotExist from other stat errors. The degradation is safe (over-prune rather than silently keep stale data), but the inconsistency could cause surprising behavior for users with permission issues.

Info

[stale-doc] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Completed implementation plan, not a living reference.

Previous run (11)

Review

Findings

Low

[inconsistency] internal/cli/run.go:143 — The .yml fallback in run.go does not warn when both .yaml and .yml files exist for the same agent, unlike lockOneAgent in lock.go which logs a StepWarn. Minor consistency gap between fullsend lock and fullsend run.
[missing-authorization] — Non-trivial PR has no linked issue. The PR references ADR-0045 Phase 2 PR 5, which provides architectural authorization but is not an explicit issue ticket.

Info

[stale-doc] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Completed implementation plan, not a living reference.

Previous run (12)

Review

Findings

Low

[logic-correctness] internal/cli/lock.go — In runLockAll, when some harnesses are up to date and others have no remote dependencies, the upToDate counter only tracks harnesses with existing lock entries. A harness that previously had remote deps but no longer does will not increment upToDate (its lock entry is stale and won't match the current hash), so the summary message may slightly undercount. The two-branch message logic ("All N harnesses already up to date" vs "No harnesses have remote dependencies") is a significant improvement over the prior version and correctly distinguishes the common cases.
[edge-case] internal/cli/lock.go — In runLockAll, when all harnesses return nil from lockOneAgent (all up-to-date or local-only), the function returns early without writing the lock file. If a harness was previously locked and its remote dependencies were later removed, the stale lock entry is never pruned from lock.yaml. Pre-existing behavior in the single-agent path but more visible with --all.
[missing-authorization] — Non-trivial PR has no linked issue. The PR references ADR-0045 Phase 2 PR 5, which provides architectural authorization but is not an explicit issue ticket.

Previous run (13)

Review

Findings

Medium

[logic-correctness] internal/cli/lock.go:395 — In runLockAll, when all harnesses are already up to date (lockOneAgent returns nil because !entry.IsStale), locked remains empty and the function prints "No harnesses have remote dependencies — nothing to lock". This message is incorrect: the harnesses do have remote dependencies, they are just already locked. The nil return from lockOneAgent conflates two distinct states: (1) no remote dependencies and (2) lock entry already current. The single-agent runLock path handles this correctly via the per-harness "Lock entry for X is up to date (N dependencies)" message, but the batch summary loses this distinction.
Remediation: Have lockOneAgent return a richer result that distinguishes "already up to date" from "no remote deps", or track skipped-because-current harnesses separately in runLockAll so the summary message is accurate (e.g., "All N harnesses already up to date").

Low

[edge-case] internal/cli/lock.go:395 — In runLockAll, when all harnesses return nil from lockOneAgent (all up-to-date or local-only), the function returns early without writing the lock file. If a harness was previously locked and its remote dependencies were later removed, the stale lock entry is never pruned. Pre-existing behavior in the single-agent path but more visible with --all.
[missing-authorization] — Non-trivial PR has no linked issue. The PR references ADR-0045 Phase 2 PR 5, which provides architectural authorization but is not an explicit issue ticket.

Previous run (14)

Review

Findings

Medium

[logic-correctness] internal/cli/lock.go:395 — In runLockAll, when all harnesses are already up to date (lockOneAgent returns nil because !entry.IsStale), locked remains empty and the function prints "No harnesses have remote dependencies — nothing to lock". This message is incorrect: the harnesses do have remote dependencies, they are just already locked. The nil return from lockOneAgent conflates two distinct states: (1) no remote dependencies and (2) lock entry already current. The single-agent runLock path handles this correctly via the per-harness "Lock entry for X is up to date (N dependencies)" message, but the batch summary loses this distinction.
Remediation: Have lockOneAgent return a richer result that distinguishes "already up to date" from "no remote deps", or track skipped-because-current harnesses separately in runLockAll so the summary message is accurate (e.g., "All N harnesses already up to date").

Low

[edge-case] internal/cli/lock.go:395 — In runLockAll, when all harnesses return nil from lockOneAgent (all up-to-date or local-only), the function returns early without writing the lock file. If a harness was previously locked and its remote dependencies were later removed, the stale lock entry is never pruned. Pre-existing behavior in the single-agent path but more visible with --all.
[missing-authorization] — Non-trivial PR has no linked issue. The PR references ADR-0045 Phase 2 PR 5, which provides architectural authorization but is not an explicit issue ticket.

Previous run (15)

Review

Findings

Medium

[logic-correctness] internal/cli/lock.go:163 — lockOneAgent adds .yml fallback so fullsend lock review works when only review.yml exists. However, run.go line 143 still hardcodes agentName+".yaml" with no .yml fallback. A user who names their harness review.yml can lock it successfully but fullsend run review will fail with a confusing "harness not found" error. The two commands have inconsistent file-extension handling.
Remediation: Add the same .yml fallback logic from lockOneAgent to runAgent in run.go, or extract a shared helper like resolveHarnessPath.

Low

[logic-correctness] internal/cli/lock.go:395 — In runLockAll, when all harnesses return nil from lockOneAgent (all up-to-date or local-only), locked remains empty and the function returns early without writing the lock file. If a harness previously had remote dependencies and those were removed, the stale lock entry is never pruned. Pre-existing behavior in the single-agent path but more visible with --all.
[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. Consider adding a TODO comment referencing the future harness.DiscoverAgents() to make the planned refactor explicit.
[stale-usage-pattern] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Verification example at line 81 also does not mention the --all flag. Low priority since this is a completed implementation plan, not a living reference.
[edge-case] internal/cli/lock.go:413 — discoverHarnessNames iterates *.yaml before *.yml, and lockOneAgent stats .yaml first — both prefer .yaml when dual extensions exist. The behavior is correct but relies on these two functions maintaining aligned iteration order independently.

Previous run (16)

Review

Findings

Low

[edge-case] internal/cli/lock.go:136 — In lockOneAgent, when os.Stat(harnessPath) returns a non-IsNotExist error (e.g., permission denied), the error is silently ignored and the .yml fallback does not trigger. The subsequent os.ReadFile will fail with a less informative error message. Handle the non-IsNotExist stat error explicitly.
[edge-case] internal/cli/lock.go:136 — The .yml extension fallback in lockOneAgent is inconsistent with run.go which hardcodes .yaml only. A user who names their harness foo.yml can use fullsend lock foo successfully but fullsend run foo will fail.
[logic-correctness] internal/cli/lock.go:398 — When lockOneAgent returns nil (up-to-date or no remote deps), stale lock entries for harnesses that previously had remote dependencies are not pruned. With --all, a harness converted to local-only retains its old lock entry indefinitely. Pre-existing behavior but more visible with --all.
[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. Consider adding a TODO comment referencing the future harness.DiscoverAgents() to make the planned refactor explicit.
[missing-authorization] internal/cli/lock.go — PR claims "ADR-0045 Phase 2 PR 5" but the --all flag for batch locking is not explicitly authorized in ADR-0045 or its implementation plan. Phase 2 covers adoption (scaffold templates, harness discovery), not batch locking. This is an additive CLI convenience feature, not an architectural deviation, but the PR metadata is inaccurate.
[stale-usage-pattern] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Low priority since this is a completed implementation plan, not a living reference.
[incomplete-cli-reference] docs/guides/user/running-agents-locally.md:208 — Lock file section shows only single-harness locking example. Does not mention the --all flag for batch operations.

Previous run (17)

Review

Findings

Medium

[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. When Phase 2 adoption work begins, this CLI-layer implementation will need to be reconciled with the planned harness-package version.
Remediation: If batch locking is approved long-term, consider implementing harness discovery in internal/harness/ and calling it from the CLI layer.
[error-handling] internal/cli/lock.go:387 — In runLockAll, if lockOneAgent fails mid-iteration (e.g., 3rd of 5 harnesses), previously resolved results stored via SetHarness are never written to disk. For batch operations with network-intensive resolution, this means all progress is lost and must be repeated on retry.
[missing-flag-documentation] docs/guides/dev/cli-internals.md:34 — CLI command tree documentation shows lock <agent-name> but does not document the new --all flag. The usage pattern is now lock [agent-name] where agent-name is optional when --all is used.
Remediation: Update to show lock [agent-name] and add --all flag to the command tree.

Low

[missing-authorization] internal/cli/lock.go — PR claims "ADR-0045 Phase 2 PR 5" but the --all flag for batch locking is not explicitly authorized in ADR-0045 or its implementation plan. Phase 2 in the plan covers adoption (scaffold templates, harness discovery), not batch locking. This is an additive CLI convenience feature, not an architectural deviation, but the PR metadata is inaccurate.
[edge-case] internal/cli/lock.go:136 — In lockOneAgent, if both agent.yaml and agent.yml exist for the same stem name, .yaml is silently preferred with no warning. discoverHarnessNames deduplicates consistently (also preferring .yaml). Deterministic but may surprise users with both extensions present.
[harness-file-discovery] internal/cli/lock.go:144 — lockOneAgent now supports .yml fallback after .yaml, but run.go (the fullsend run command) still hardcodes .yaml only. Extension handling is inconsistent across commands.
[flag-documentation] internal/cli/lock.go:70 — Flag help text "lock all harnesses in the harness directory" could be more specific (e.g., "lock all harness files in the .fullsend/harness directory") to match the detail level of other --all flags in admin.go.
[logic-correctness] internal/cli/lock.go:398 — When lockOneAgent returns nil (up-to-date or no remote deps), stale lock entries for harnesses that previously had remote dependencies are not pruned. Pre-existing behavior but more visible with --all.
[incomplete-feature-documentation] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Low priority since this is a completed implementation plan, not a living reference.

Previous run (18)

Review

Findings

Medium

[logic-correctness] internal/cli/lock.go:163 — lockOneAgent adds .yml fallback so fullsend lock review works when only review.yml exists. However, run.go line 143 still hardcodes agentName+".yaml" with no .yml fallback. A user who names their harness review.yml can lock it successfully but fullsend run review will fail with a confusing "harness not found" error. The two commands have inconsistent file-extension handling.
Remediation: Add the same .yml fallback logic from lockOneAgent to runAgent in run.go, or extract a shared helper like resolveHarnessPath.

Low

[logic-correctness] internal/cli/lock.go:395 — In runLockAll, when all harnesses return nil from lockOneAgent (all up-to-date or local-only), locked remains empty and the function returns early without writing the lock file. If a harness previously had remote dependencies and those were removed, the stale lock entry is never pruned. Pre-existing behavior in the single-agent path but more visible with --all.
[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. Consider adding a TODO comment referencing the future harness.DiscoverAgents() to make the planned refactor explicit.
[stale-usage-pattern] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Verification example at line 81 also does not mention the --all flag. Low priority since this is a completed implementation plan, not a living reference.
[edge-case] internal/cli/lock.go:413 — discoverHarnessNames iterates *.yaml before *.yml, and lockOneAgent stats .yaml first — both prefer .yaml when dual extensions exist. The behavior is correct but relies on these two functions maintaining aligned iteration order independently.

Previous run (19)

Review

Findings

Low

[edge-case] internal/cli/lock.go:136 — In lockOneAgent, when os.Stat(harnessPath) returns a non-IsNotExist error (e.g., permission denied), the error is silently ignored and the .yml fallback does not trigger. The subsequent os.ReadFile will fail with a less informative error message. Handle the non-IsNotExist stat error explicitly.
[edge-case] internal/cli/lock.go:136 — The .yml extension fallback in lockOneAgent is inconsistent with run.go which hardcodes .yaml only. A user who names their harness foo.yml can use fullsend lock foo successfully but fullsend run foo will fail.
[logic-correctness] internal/cli/lock.go:398 — When lockOneAgent returns nil (up-to-date or no remote deps), stale lock entries for harnesses that previously had remote dependencies are not pruned. With --all, a harness converted to local-only retains its old lock entry indefinitely. Pre-existing behavior but more visible with --all.
[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. Consider adding a TODO comment referencing the future harness.DiscoverAgents() to make the planned refactor explicit.
[missing-authorization] internal/cli/lock.go — PR claims "ADR-0045 Phase 2 PR 5" but the --all flag for batch locking is not explicitly authorized in ADR-0045 or its implementation plan. Phase 2 covers adoption (scaffold templates, harness discovery), not batch locking. This is an additive CLI convenience feature, not an architectural deviation, but the PR metadata is inaccurate.
[stale-usage-pattern] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Low priority since this is a completed implementation plan, not a living reference.
[incomplete-cli-reference] docs/guides/user/running-agents-locally.md:208 — Lock file section shows only single-harness locking example. Does not mention the --all flag for batch operations.

Previous run (20)

Review

Findings

Medium

[architectural-trajectory] internal/cli/lock.go — discoverHarnessNames duplicates harness directory scanning that ADR-0045 Phase 2 plans as harness.DiscoverAgents(dir) in internal/harness/. When Phase 2 adoption work begins, this CLI-layer implementation will need to be reconciled with the planned harness-package version.
Remediation: If batch locking is approved long-term, consider implementing harness discovery in internal/harness/ and calling it from the CLI layer.
[error-handling] internal/cli/lock.go:387 — In runLockAll, if lockOneAgent fails mid-iteration (e.g., 3rd of 5 harnesses), previously resolved results stored via SetHarness are never written to disk. For batch operations with network-intensive resolution, this means all progress is lost and must be repeated on retry.
[missing-flag-documentation] docs/guides/dev/cli-internals.md:34 — CLI command tree documentation shows lock <agent-name> but does not document the new --all flag. The usage pattern is now lock [agent-name] where agent-name is optional when --all is used.
Remediation: Update to show lock [agent-name] and add --all flag to the command tree.

Low

[missing-authorization] internal/cli/lock.go — PR claims "ADR-0045 Phase 2 PR 5" but the --all flag for batch locking is not explicitly authorized in ADR-0045 or its implementation plan. Phase 2 in the plan covers adoption (scaffold templates, harness discovery), not batch locking. This is an additive CLI convenience feature, not an architectural deviation, but the PR metadata is inaccurate.
[edge-case] internal/cli/lock.go:136 — In lockOneAgent, if both agent.yaml and agent.yml exist for the same stem name, .yaml is silently preferred with no warning. discoverHarnessNames deduplicates consistently (also preferring .yaml). Deterministic but may surprise users with both extensions present.
[harness-file-discovery] internal/cli/lock.go:144 — lockOneAgent now supports .yml fallback after .yaml, but run.go (the fullsend run command) still hardcodes .yaml only. Extension handling is inconsistent across commands.
[flag-documentation] internal/cli/lock.go:70 — Flag help text "lock all harnesses in the harness directory" could be more specific (e.g., "lock all harness files in the .fullsend/harness directory") to match the detail level of other --all flags in admin.go.
[logic-correctness] internal/cli/lock.go:398 — When lockOneAgent returns nil (up-to-date or no remote deps), stale lock entries for harnesses that previously had remote dependencies are not pruned. Pre-existing behavior but more visible with --all.
[incomplete-feature-documentation] docs/plans/universal-harness-access-phase3.md:59 — Shows fullsend lock <agent-name> with agent-name as required; now optional when --all is used. Low priority since this is a completed implementation plan, not a living reference.

fullsend-ai-review · 2026-06-12T20:23:16Z

🤖 Review · ⚠️ Cancelled · Started 8:23 PM UTC · Ended 8:31 PM UTC
Commit: 4e21a60 · View workflow run →

fullsend-ai-review · 2026-06-12T20:35:24Z

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

fullsend-ai-review · 2026-06-12T21:00:45Z

🤖 Finished Review · ✅ Success · Started 9:00 PM UTC · Completed 9:15 PM UTC
Commit: bf50830 · View workflow run →

fullsend-ai-review · 2026-06-13T02:38:32Z

🤖 Finished Review · ✅ Success · Started 2:38 AM UTC · Completed 2:50 AM UTC
Commit: dacb065 · View workflow run →

fullsend-ai-review · 2026-06-13T03:29:04Z

🤖 Finished Review · ✅ Success · Started 3:29 AM UTC · Completed 3:40 AM UTC
Commit: dc4d05b · View workflow run →

fullsend-ai-review · 2026-06-13T18:26:34Z

🤖 Finished Review · ✅ Success · Started 6:26 PM UTC · Completed 6:37 PM UTC
Commit: bb95bdb · View workflow run →

fullsend-ai-review · 2026-06-15T14:03:44Z

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

fullsend-ai-review · 2026-06-15T14:22:07Z

🤖 Finished Review · ✅ Success · Started 2:22 PM UTC · Completed 2:33 PM UTC
Commit: 2bc7293 · View workflow run →

Signed-off-by: Greg Allen <gregoryallen1989@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

fullsend-ai-review · 2026-06-15T14:48:44Z

🤖 Finished Review · ✅ Success · Started 2:48 PM UTC · Completed 2:58 PM UTC
Commit: acecfc3 · View workflow run →

ralphbean

LGTM.

fullsend-ai-retro · 2026-06-16T02:11:04Z

🤖 Finished Retro · ✅ Success · Started 2:11 AM UTC · Completed 2:25 AM UTC
Commit: acecfc3 · View workflow run →

fullsend-ai-retro · 2026-06-16T02:25:30Z

Retro: PR #2262 — `feat(cli): add --all flag to fullsend lock for batch harness locking`

Verdict: Workflow went well. No new proposals — existing issues already cover all identified improvements.

Timeline

Jun 12: Human author (ggallen) opened PR with +1134/-64 across 8 files (ADR-0045 Phase 2 PR 5).
Jun 12–15: Author iterated through ~16 force-pushes, each triggering a review agent dispatch (15 total, 14 succeeded, 1 cancelled by cancel-in-progress).
Jun 15: Human reviewer (ralphbean) approved with "LGTM."
Jun 16: PR merged.

Review quality assessment

Strong. The review agent caught genuinely valuable issues across its ~20 review cycles:

.yml/.yaml extension inconsistency between lock.go and run.go → led to shared resolveHarnessPath helper
Missing stat error handling for non-IsNotExist errors
Stale lock entry pruning for removed harnesses
All-or-nothing vs partial-progress semantics inconsistency between plan doc and implementation
Missing test coverage for .yml fallback in run.go

All bot reviews submitted an APPROVED verdict (not changes_requested), with findings surfaced as inline comments. The author addressed every finding. This is the review agent working as intended.

Waste identified (all already tracked)

18 no-op pull_request_review workflow runs: Bot and human review submissions triggered fullsend.yaml runs that correctly matched no dispatch stage and exited. These are harmless but wasteful. Already tracked by #1271.
No debounce on rapid pushes: Each force-push immediately dispatched a review agent. The cancel-in-progress: true on review.yml mitigates this at the execution level, but a debounce delay could reduce dispatch volume. Already tracked by #1014, #1418, #1422.
Re-reviewing across iterations: The bot re-reviewed the entire diff on each push rather than focusing on what changed since the last review. Already tracked by #1500, #1285, #1552.

No new proposals filed — the existing issue backlog comprehensively covers all improvement opportunities observed in this workflow.

github-actions Bot deployed to site-preview June 12, 2026 19:45 View deployment

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 12, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from 7d1cd39 to ab6ffe6 Compare June 12, 2026 20:19

github-actions Bot deployed to site-preview June 12, 2026 20:21 View deployment

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from ab6ffe6 to e0f8d98 Compare June 12, 2026 20:31

github-actions Bot deployed to site-preview June 12, 2026 20:32 View deployment

fullsend-ai-review Bot approved these changes Jun 12, 2026

View reviewed changes

Comment thread internal/cli/lock.go

Comment thread internal/cli/lock.go

Comment thread internal/cli/lock.go

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 12, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from e0f8d98 to bf50830 Compare June 12, 2026 20:55

github-actions Bot deployed to site-preview June 12, 2026 20:57 View deployment

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 12, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from bf50830 to dacb065 Compare June 13, 2026 02:35

github-actions Bot deployed to site-preview June 13, 2026 02:36 View deployment

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 13, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from dacb065 to dc4d05b Compare June 13, 2026 03:25

github-actions Bot deployed to site-preview June 13, 2026 03:26 View deployment

fullsend-ai-review Bot approved these changes Jun 13, 2026

View reviewed changes

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 13, 2026

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 13, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from 6336a8c to bb95bdb Compare June 13, 2026 18:22

github-actions Bot deployed to site-preview June 13, 2026 18:24 View deployment

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 13, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from bb95bdb to cd28360 Compare June 15, 2026 14:00

github-actions Bot deployed to site-preview June 15, 2026 14:01 View deployment

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 15, 2026

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch 2 times, most recently from b1021dc to 2bc7293 Compare June 15, 2026 14:17

github-actions Bot deployed to site-preview June 15, 2026 14:19 View deployment

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jun 15, 2026

feat(cli): add --all flag to fullsend lock for batch harness locking

acecfc3

Signed-off-by: Greg Allen <gregoryallen1989@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>

ggallen force-pushed the worktree-adr-0045-phase2-pr5 branch from 2bc7293 to acecfc3 Compare June 15, 2026 14:44

github-actions Bot deployed to site-preview June 15, 2026 14:46 View deployment

fullsend-ai-review Bot approved these changes Jun 15, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jun 15, 2026

ralphbean approved these changes Jun 15, 2026

View reviewed changes

ggallen added this pull request to the merge queue Jun 16, 2026

Merged via the queue into main with commit 02b81c8 Jun 16, 2026
16 checks passed

ggallen deleted the worktree-adr-0045-phase2-pr5 branch June 16, 2026 02:05

Conversation

ggallen commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

github-actions Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

fullsend-ai-review Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fullsend-ai-review Bot commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Info

Review

Review

Findings

Medium

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Low

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Info

Review

Findings

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Low

ggallen commented Jun 12, 2026 •

edited

Loading

github-actions Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

codecov Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 13, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 15, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 16, 2026 •

edited

Loading

Retro: PR #2262 — `feat(cli): add --all flag to fullsend lock for batch harness locking`