feat(cli): add migrate-customizations command for ADR-0064

[ggallen]

Summary

• Add fullsend agent migrate-customizations CLI command implementing ADR-0064's migration tooling
• Implements harness YAML diffing (internal/harness/diff.go) to detect customizations in override files
• Plans and executes migrations: promotes live customizations to the regular directory, cleans up dead overrides, and creates a PR with the results
• Adds ListBranches method to forge interface for branch existence checking

Found during review of fullsend-ai/agents#12 (agent extraction PR).

Changes

• internal/cli/migrate.go — new migrate-customizations subcommand with plan/execute/PR-creation logic
• internal/harness/diff.go — YAML-aware harness config diffing (env, model, tools, secrets, allowed resources, etc.)
• internal/forge/forge.go / internal/forge/github/github.go / internal/forge/fake.go — ListBranches method
• internal/scaffold/baseurl.go — helper for resolving scaffold base URLs
• Documentation updates across ADRs, guides, and architecture docs

Test plan

• migrate_test.go — unit tests for migration planning and execution
• diff_test.go — unit tests for harness YAML diffing
• baseurl_test.go — unit tests for scaffold base URL resolution
• agent_test.go — updated for new subcommand registration
• Pre-commit hooks pass

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:33 PM UTC · Completed 2:58 PM UTC
Commit: 074ab1f · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Fix review action string mismatch and harden push error output

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

AI Description

• Fix request-changes action handling to match the review-result schema enum.
• Ensure request-changes outcomes follow the intended label/decision logic.
• Sanitize git push stderr before emitting GitHub Actions ::error:: commands.

Diagram

graph TD
  A["GitHub Actions workflow"] --> B["post-review.sh"] --> C{"ACTION?"} --> D["gh pr edit"] --> E["PR labels"]
  A --> F["post-code.sh"] --> G["Sanitized ::error::"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use GitHub Actions official command-escaping

• ➕ Preserves push stderr content while preventing workflow-command injection
• ➕ Aligns with documented escaping rules (e.g., encode %, \r, \n for command data)
• ➖ Slightly more code/complexity than simple string stripping
• ➖ Requires careful adherence to the specific escaping rules for the chosen command context

2. Avoid workflow command syntax for raw stderr

• ➕ Eliminates injection risk by not interpolating untrusted text into ::error:: format
• ➕ Can log stderr as plain output and emit a separate static ::error:: line
• ➖ Push output may be less visible/structured in the UI
• ➖ Harder to associate exact stderr with the annotated error line

Recommendation: The enum mismatch fix is clearly correct and should be merged. For the push-output hardening, the current stripping approach addresses the immediate ::/newline-encoding injection vector, but consider switching to GitHub’s documented command-escaping (or logging stderr outside workflow commands) to retain diagnostic fidelity while providing stronger, more principled protection.

Files changed (3) +10 / -5

Enhancement (1) +6 / -1

post-code.shSanitize git push stderr before emitting GHA ::error:: output +6/-1

Sanitize git push stderr before emitting GHA ::error:: output

• Introduces a sanitized 'SAFE_PUSH' derived from 'PUSH_OUTPUT' by removing workflow-command delimiters and newline encodings. Uses the sanitized value when printing '::error::Push output:' to reduce GitHub Actions command-injection risk via crafted stderr.

internal/scaffold/fullsend-repo/scripts/post-code.sh

Bug fix (1) +1 / -1

post-review.shFix dead branch by matching 'request-changes' action string +1/-1

Fix dead branch by matching 'request-changes' action string

• Replaces the underscore form of the request-changes action with the hyphenated enum value. This makes the request-changes branch reachable when actions are schema-validated, ensuring expected (no-label) handling executes.

internal/scaffold/fullsend-repo/scripts/post-review.sh

Tests (1) +3 / -3

post-review-test.shAlign tests with 'request-changes' action enum +3/-3

Align tests with 'request-changes' action enum

• Updates the test helper and test case inputs/comments to use 'request-changes' (hyphen) instead of 'request_changes' (underscore). Ensures the test suite matches the schema-valid action value.

internal/scaffold/fullsend-repo/scripts/post-review-test.sh

[github-actions]

Site preview

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

Commit: df9293b4a2423be606fb1201a73d840ff0ff8bd6

[codecov]

Codecov Report

❌ Patch coverage is 84.45748% with 106 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/migrate.go	80.00%	43 Missing and 36 partials ⚠️
internal/harness/diff.go	95.75%	6 Missing and 5 partials ⚠️
internal/forge/github/github.go	0.00%	9 Missing and 1 partial ⚠️
internal/forge/fake.go	40.00%	4 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 61 rules

✅ Skills: writing-user-docs, writing-adrs

1. Unsanitized push output printed 🐞 Bug ⛨ Security

Description

post-code.sh still echoes raw git push output to the GitHub Actions log (including the initial `echo
"${PUSH_OUTPUT}" and the --force-with-lease` fallback output), so workflow-command strings in push
stderr can still be processed even though the ::error::Push output: annotation now uses SAFE_PUSH.
This makes the new sanitization incomplete for the stated threat model (crafted push stderr).

Code

internal/scaffold/fullsend-repo/scripts/post-code.sh[R419-425]

+    SAFE_PUSH="${PUSH_OUTPUT//::/}"
+    SAFE_PUSH="${SAFE_PUSH//%0A/}"
+    SAFE_PUSH="${SAFE_PUSH//%0a/}"
+    SAFE_PUSH="${SAFE_PUSH//%0D/}"
+    SAFE_PUSH="${SAFE_PUSH//%0d/}"
    echo "::error::Push failed with unexpected error (git push origin ${BRANCH})" >&2
-    echo "::error::Push output: ${PUSH_OUTPUT}" >&2
+    echo "::error::Push output: ${SAFE_PUSH}" >&2

Relevance

⭐ Low

Similar request to sanitize/avoid raw PUSH_OUTPUT in ::error was explicitly rejected in PR #2395
reviews.

PR-#2395
PR-#2306

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The script captures push stderr into PUSH_OUTPUT, then prints it verbatim and also streams fallback
push output directly; repo-local comments in post-retro.sh confirm that echoing unsanitized content
can lead to GitHub Actions workflow-command injection, which is exactly the threat this PR intends
to mitigate.

internal/scaffold/fullsend-repo/scripts/post-code.sh[404-427]
internal/scaffold/fullsend-repo/scripts/post-retro.sh[154-171]
PR-#2306

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`post-code.sh` sanitizes `PUSH_OUTPUT` only for the `::error::Push output: ...` annotation, but still prints unsanitized push output elsewhere (including an unconditional `echo "${PUSH_OUTPUT}"` and the non-fast-forward `git push --force-with-lease ... 2>&1` path that streams output directly). This leaves a remaining GitHub Actions workflow-command injection surface via crafted `git push` stderr.

## Issue Context
The repo already treats `echo` output as a potential GHA workflow-command injection vector and sanitizes `COMMENT_OUTPUT` before echoing `::warning::...(${SAFE_OUTPUT})` (and even before plain `echo` in the fatal branch).

## Fix Focus Areas
- internal/scaffold/fullsend-repo/scripts/post-code.sh[393-427]
 - Capture and sanitize *all* `git push` outputs before printing to logs.
 - Replace the unconditional `echo "${PUSH_OUTPUT}"` with `echo "${SAFE_PUSH}"` (or print nothing unless needed).
 - In the non-fast-forward fallback, capture `git push --force-with-lease` output into a variable, sanitize it, and print the sanitized version (on both success and failure).
 - (Optional, consistent hardening) Apply the same approach to the `git push origin --delete` output since it is also streamed to logs.

Implementation sketch:
- Factor a small helper like `sanitize_gha()` (strip `::`, `%0A/%0a`, `%0D/%0d`) and reuse it for all git push-related log emissions.
- Ensure no raw `git push` stderr/stdout is directly emitted without passing through the sanitizer.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

This re-review covers the changes since the prior review at 80ba8218. All prior medium-severity findings have been addressed:

• ✅ Forge-level path fields in rewriteCustomizedPaths — now iterates over h.Forge entries
• ✅ ValidationLoop/Security nil handling — now sets result.Child = nil and returns with warning
• ✅ diffForge nil-valued platform detection — now checks !ok || childFC == nil
• ✅ skills/ directory now in perAgentDirs
• ✅ Doc field now rewritten via strip()

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence in the remote repository. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

• [edge-case] internal/cli/migrate.go, rewriteEnvMap — strings.ReplaceAll(v, "/customized/", "/") replaces all occurrences of /customized/ with /. A value like https://example.com/customized/config would be corrupted to https://example.com/config. In practice, harness env values are overwhelmingly ${FULLSEND_DIR}/customized/... patterns, so the risk is low. The PR review gate provides additional mitigation.

• [path-traversal] internal/cli/migrate.go, readTreeFile — TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Between these calls, the file could be replaced with a symlink. In practice this requires local attacker control of the customized directory. Defense-in-depth: walkCustomized already skips symlinks and path-escape protection is present.

• [path-traversal] internal/cli/migrate.go, buildModifiedAgentFiles — calls os.ReadFile on the customized harness path without checking for symlinks (unlike readTreeFile). Same TOCTOU class, mitigated by walkCustomized filtering symlinks at scan time and path construction from validated agent names.

Previous run

Review

Findings

Medium

• [logic-error] internal/cli/migrate.go, rewriteCustomizedPaths — Forge-level path fields are not rewritten. The function strips customized/ prefixes from all top-level path-bearing Harness fields (Agent, Policy, PreScript, PostScript, AgentInput, Skills, Plugins, HostFiles, APIServers, ValidationLoop, env maps), but does not process any fields inside h.Forge. ForgeConfig (defined in internal/harness/forge.go:14-21) contains the same categories of path-bearing fields: PreScript, PostScript, Skills[], ValidationLoop.Script, ValidationLoop.Schema, RunnerEnv, and Env. If a customized harness has forge-level overrides like forge.github.pre_script: customized/scripts/gh-pre.sh, those paths survive unrewritten after migration. The resulting harness references customized/scripts/gh-pre.sh but the file has been moved to scripts/gh-pre.sh, breaking runtime resolution. No test covers a customized harness with customized/ paths in forge-level fields.
  Remediation: Add forge-level path rewriting to rewriteCustomizedPaths — iterate over h.Forge entries and apply the same strip() to PreScript, PostScript, Skills[], ValidationLoop.Script/Schema, RunnerEnv, and Env maps.

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence in the remote repository. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

• [edge-case] internal/cli/migrate.go, rewriteEnvMap — strings.ReplaceAll(v, "customized/", "") is a blind substring replacement, not a prefix strip. A value like https://example.com/customized/config would be corrupted to https://example.com/config. In practice, harness env values are overwhelmingly ${FULLSEND_DIR}/customized/... patterns, so the risk is low. The PR review gate provides additional mitigation.

• [edge-case] internal/cli/migrate.go, rewriteCustomizedPaths — The Doc field (yaml:"doc,omitempty") is not rewritten. While Doc is not used at runtime (lint-agent-docs only), a customized harness with doc: customized/agents/triage-doc.md would retain the stale prefix after migration.

Previous run (2)

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence in the remote repository. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

Previous run (3)

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence in the remote repository. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

Previous run (4)

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

Previous run (5)

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [edge-case] internal/cli/migrate.go — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

Previous run (6)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:197 — DiffHarness warns when the child nullifies ValidationLoop or Security (pointer structs that have non-nil base values), but does not set result.Child = nil. After mergeBaseIntoChild composition, the base's ValidationLoop/Security will be inherited into the child (since child fields are nil), producing a composed result that does not match the original child's intention to remove those blocks. Unlike slice-removal cases that abort by setting result.Child = nil, these cases only warn. The buildModifiedAgentFiles caller only aborts when diffResult.Child == nil, so the migration proceeds silently producing a harness that does not reproduce the customized version.
  Remediation: Either set result.Child = nil and return immediately when ValidationLoop/Security are removed (matching the pattern used for slices), or in buildModifiedAgentFiles, check len(diffResult.Warnings) > 0 independently of diffResult.Child == nil and abort or escalate to the user.

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go:96 — DiffHarness cannot distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits the field entirely, because both produce the Go zero value 0. When the base has a non-zero timeout and the child intentionally sets it to 0, the diff omits the field. After composition, mergeBaseIntoChild fills from the base, restoring the non-zero value rather than the intended 0. This is an inherent limitation of Go's zero-value scheme and omitempty YAML tags.

• [scope-creep] internal/cli/migrate.go — The migrate-customizations CLI command and supporting infrastructure (DiffHarness, HarnessContent, TreeFile.Delete) are not explicitly allocated to any PR in ADR-0064's implementation plan. ADR-0064's Consequences section does mention the command, providing architectural authorization.

• [incomplete-api-documentation] docs/guides/dev/cli-internals.md:189 — The Phase 5 diagram shows write workflow files + customized/ dirs but does not mention that customized/ is deprecated per ADR-0064.

• [path-traversal-toctou] internal/cli/migrate.go:462 — readTreeFile has a TOCTOU race between os.Lstat (symlink check) and os.ReadFile. Between these calls, the file could be replaced with a symlink. In practice this requires local attacker control of the customized directory. Defense-in-depth: walkCustomized already skips symlinks and path-escape protection is present.

• [edge-case] internal/cli/migrate.go:240 — planMigrations only groups files into agent migrations when their parent directory is exactly harness or one of the flat perAgentDirs (agents, scripts, policies, schemas, env). The skills/ directory is absent from perAgentDirs, so skill files under customized/skills/ become standalone files moved without agent association.

• [edge-case] internal/cli/migrate.go:106 — migrateCustom creates TreeFile entries that move files from customized/ to regular directories without verifying destination existence. checkDuplicateDestinations only catches conflicts within migration TreeFile entries, not with pre-existing files. Warning log and PR review gate partially mitigate.

Previous run (7)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:183 — DiffHarness silently drops the case where the child explicitly nullifies a pointer struct (ValidationLoop, Security) that has a non-nil base value. When child.ValidationLoop == nil and base.ValidationLoop != nil, the diff omits the field. After composition via mergeBaseIntoChild, the base's ValidationLoop is inherited — so the composed result differs from the original child. The code correctly handles analogous cases for slices and maps (emitting a warning and aborting), but misses this pattern for pointer structs.
  Remediation: Detect the nil-child/non-nil-base case for ValidationLoop and Security and emit a warning (like the slice/map removal warnings) rather than silently producing a diff that does not round-trip correctly.

• [incomplete-api-documentation] docs/ADRs/0058-agent-registration.md:62 — ADR-0058 lists fullsend agent subcommands as add, list, update, remove but does not mention the new migrate-customizations subcommand. The PR updates docs/architecture.md and docs/cli/README.md to include migrate-customizations but misses ADR-0058.
  Remediation: Add a cross-reference note to ADR-0058 mentioning migrate-customizations (e.g., "The migrate-customizations subcommand (ADR-0064) provides automated migration from the deprecated customized/ overlay.").

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 1-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [missing-authorization] This PR implements migrate-customizations and supporting infrastructure without a linked issue. ADR-0064 provides architectural authorization for deprecating customized/ and its Consequences section states users must migrate. Building CLI tooling to automate that migration is reasonable implementation of the authorized deprecation. Filing a linked issue is recommended as best practice.

• [edge-case] internal/cli/migrate.go:393 — planMigrations only considers files directly under harness/ for agent grouping. Files in nested paths (e.g., skills/my-skill/SKILL.md where dir="skills/my-skill") don't match the flat perAgentDirs check, so they become standalone files. The standalone file handling provides a reasonable fallback but this may surprise users with nested skill customizations.

• [edge-case] internal/cli/migrate.go:547 — migrateCustom creates TreeFile entries that move files without verifying whether a file already exists at the destination in the remote repository. Mitigated by checkDuplicateDestinations (within-migration conflicts), the printer.StepWarn messages, and the PR review gate.

• [edge-case] internal/harness/diff.go:82 — DiffHarness does not distinguish between a child that explicitly sets timeout_minutes: 0 and one that omits it. After composition, the base's timeout would be inherited. This is a known limitation of Go's YAML model where zero values are indistinguishable from absent fields.

• [path-traversal-toctou] internal/cli/migrate.go:964 — readTreeFile has a TOCTOU race between os.Lstat (symlink check) and os.ReadFile. An attacker with local filesystem access could replace the file with a symlink between the two calls. Low severity: this is a local CLI tool where the user already has filesystem access, and the code includes path-escape protection.

• [adr-modification-scope] docs/ADRs/0045-forge-portable-harness-schema.md — PR adds a Consequence bullet to accepted ADR-0045 about bidirectional composition. The addition is factual and documents a real constraint discovered during implementation. Minor annotation consistent with ADR living-document practices.

• [scope-creep] internal/forge/forge.go — PR extends TreeFile struct with Delete field and implements file deletion in github.go. This is a minimal extension (one bool field + ~15 lines) directly required by the migration command. The forge Client interface itself is unchanged.

Previous run (8)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:351 — diffForge does not detect the case where child explicitly nullifies a forge platform (e.g., "github": nil in the child's Forge map). The check if _, ok := child[platform]; !ok only catches missing keys, not nil-valued ones. When a child has forge: { github: null }, diffForgeConfig(baseFC, nil, platform) returns (nil, nil) — no diff, no warning. After composition via mergeForgeBlocks, the base platform config would be inherited, silently restoring a platform the user intended to nullify.
  Remediation: In diffForge, after the existing !ok check, add a check for child[platform] == nil when ok is true, and issue the same removal warning.

• [protected-path] AGENTS.md — This PR modifies a protected governance file. The change is a 2-line update to the configuration tier terminology table, adding a deprecation note referencing ADR-0064. ADR-0064 provides architectural authorization and the PR body explains the rationale. Human approval is always required for protected-path changes.

Low

• [logic-error] internal/harness/diff.go:157 — When child has nil ValidationLoop or Security but base has non-nil values, DiffHarness silently omits them from the diff. After composition, base values would be inherited, potentially changing behavior from the original customized harness. This is a design limitation of the YAML-based diff approach (nil is ambiguous: "not specified" vs "intentionally cleared").

• [edge-case] internal/cli/migrate.go:547 — migrateCustom creates TreeFile entries that move files without verifying whether a file already exists at the destination in the remote repository. Mitigated by checkDuplicateDestinations (within-migration conflicts), printer.StepWarn messages, and the PR review gate.

• [edge-case] internal/cli/migrate.go:287 — planMigrations stem-matching heuristic associates non-harness files with agents by filename stem after stripping known prefixes. Restricted to perAgentDirs which limits false matches, but coincidental stem collisions are possible.

• [edge-case] internal/cli/migrate.go:398 — registerMigratedAgent adds AgentEntry with Source: "harness/<name>.yaml" without verifying the harness file will exist at that path. Path is constructed consistently by the caller and the file creation and config update happen atomically in the same commit.

• [path-traversal] internal/cli/migrate.go:508 — readTreeFile has a TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Mitigated by walkCustomized filtering symlinks at scan time, path-escape protection via strings.HasPrefix, and the PR review gate.

• [breaking-api] internal/forge/forge.go:193 — TreeFile gains a Delete bool field. This is in an internal/ package so it has no cross-repo impact. Existing callers use named field syntax and the bool zero value (false) preserves existing behavior.

• [adr-modification-annotation] docs/ADRs/0045-forge-portable-harness-schema.md:591 — 5-line annotation added to Consequences section of Accepted ADR-0045, documenting the bidirectional composition relationship with DiffHarness. Cross-reference annotation within acceptable bounds.

• [adr-modification-annotation] docs/ADRs/0064-deprecate-customized-directory-overlay.md:93 — 2-line annotation added to Consequences section noting the CLI command automates the conversion.

• [missing-issue-link] — No linked issue in fullsend-ai/fullsend for this PR. ADR-0064 provides architectural authorization for the migration tooling. Consider filing a tracking issue for traceability.

• [stale-reference] docs/ADRs/0033-per-repo-installation-mode.md:140 — References customized/ overlay without noting deprecation by ADR-0064.

• [stale-reference] docs/ADRs/0056-per-repo-precommit-tools-registry.md:29 — References customized/scripts/ L1 override from ADR-0035 without deprecation note.

• [error-message-consistency] internal/cli/migrate.go:528 — Error message pattern differs slightly from established codebase conventions.

• [error-context] internal/cli/migrate.go:473 — Error wrapping "reading config: %w" could include the config path for better diagnostics.

Previous run (9)

Review

Findings

Medium

• [logic-error] internal/cli/migrate.go:389 — resolveBaseURL computes the diff against the embedded scaffold harness (scaffold.HarnessContent) but first tries to pin the base: URL to the fullsend-ai/agents repo via pinAgentURL. If the agents repo content diverges from the embedded scaffold content, the diff (computed against the scaffold) would produce incorrect composition results at runtime: mergeBaseIntoChild(agentsRepoHarness, diffChild) would not reproduce the original customized harness because the diff was computed against a different base. The scaffold fallback path is correct, but the primary agents-repo path has no content-equivalence check.
  Remediation: After pinAgentURL succeeds, compare its content (or hash) against scaffold.HarnessContent(agentName). If they differ, abort or fall through to the scaffold URL fallback.

Low

• [edge-case] internal/cli/migrate.go:547 — migrateCustom and standalone-file handling create TreeFile entries that move files to their destination paths without verifying whether a file already exists at the destination in the remote repository. Mitigated by checkDuplicateDestinations (within-migration conflicts), printer.StepWarn messages, and the PR review gate.

• [adr-modification-annotation] docs/ADRs/0045-forge-portable-harness-schema.md:591 — Adds a 4-line "Bidirectional composition" consequence to ADR-0045 (status: Accepted). This is a cross-reference annotation rather than a substantive rewrite, but could be placed in a References section instead of modifying the Consequences list.

• [function-organization] internal/cli/migrate.go:419 — buildModifiedAgentFiles has 8 parameters and spans ~90 lines handling harness reading, diff computation, URL resolution, marshaling, and agent registration. Consider extracting sub-operations into smaller helpers.

• [logic-error] internal/harness/diff.go:108 — When the child has a nil ValidationLoop or Security block and the base has a non-nil one, DiffHarness silently omits them from the diff. After mergeBaseIntoChild, the base's value is inherited. If the user intentionally removed the block, this intent is lost. This is a design limitation of the YAML-based diff approach.

• [error-handling] internal/cli/migrate.go:392 — When pinAgentURL fails, resolveBaseURL falls back to the scaffold URL using the CLI binary's embedded commitSHA. The base: URL points to the scaffold content at the CLI's build commit, which might differ from what the user originally customized against.

• [edge-case] internal/cli/migrate.go:287 — planMigrations stem-matching heuristic associates non-harness files with agents by filename stem after stripping known prefixes. Restricted to perAgentDirs which limits false matches, but coincidental stem collisions are possible.

• [edge-case] internal/harness/diff.go:153 — diffStringSlice uses set-based comparison and doesn't detect ordering changes. Consistent with mergeBaseIntoChild concatenation semantics.

• [edge-case] internal/cli/migrate.go:398 — registerMigratedAgent adds AgentEntry with Source: "harness/<name>.yaml" without verifying the harness file will exist at that path after the PR is applied. Path is constructed consistently by the caller.

• [path-traversal] internal/cli/migrate.go:508 — readTreeFile has a TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Mitigated by walkCustomized filtering symlinks at scan time, path-escape protection via strings.HasPrefix, and the PR review gate.

• [error-message-consistency] internal/cli/migrate.go:455 — Error message pattern "loading config" differs from established patterns ("reading config" in agent.go, "parsing config" elsewhere).

• [stale-reference] AGENTS.md:167 — The "configuration tier" terminology definition references ADR-0035 for the three-tier model. The conceptual model still applies, but the implementation mechanism changed per ADR-0064.

• [stale-reference] docs/guides/user/customizing-agents.md:102 — The diagram header says "Configuration Layering (ADR 0035)" without acknowledging supersession by ADR-0064. Deprecation notice is present nearby but the diagram header is untouched.

• [missing-issue-link] — No linked issue in fullsend-ai/fullsend for this PR. ADR-0064 provides architectural authorization for the migration tooling. Consider filing a tracking issue in this repo for traceability.

• [cross-repo-reference-clarity] — PR body references feat: extract code, fix, review, retro, prioritize agents from fullsend agents#12 as discovery context without explaining what was discovered there.

Previous run (10)

Review

Findings

Medium

• [edge-case] internal/cli/migrate.go:547 — migrateCustom creates TreeFile entries that move files to their destination paths without checking whether a file already exists at the destination in the remote repository. If a file already exists, the migration PR would silently overwrite it. The warning message acknowledges this ("review PR to verify no unintended overwrites") but no programmatic check is performed. Mitigated by the PR review gate.

Low

• [missing-authorization] — No linked issue in fullsend-ai/fullsend for this PR. The PR body references feat: extract code, fix, review, retro, prioritize agents from fullsend agents#12 as discovery context but that issue is in a different repo. ADR-0064 provides architectural authorization for the migration tooling. Consider filing a tracking issue in this repo for traceability.

• [logic-error] internal/harness/diff.go:108 — When the child has a nil ValidationLoop or Security block and the base has a non-nil one, DiffHarness silently omits them from the diff. After mergeBaseIntoChild, the base's value is inherited, meaning a customization that removes a ValidationLoop block entirely cannot be round-tripped through the diff engine. Narrow edge case — modifications are handled correctly, and the engine aborts on other unrepresentable changes.

• [code-organization] internal/cli/migrate.go:419 — buildModifiedAgentFiles has 8 parameters and spans ~90 lines handling harness reading, diff computation, URL resolution, marshaling, and agent registration. Consider extracting sub-operations into smaller helpers.

• [stale-reference] AGENTS.md:167 — The "configuration tier" terminology definition references ADR-0035 for the three-tier model. While the conceptual model still applies, the implementation mechanism changed from the customized/ overlay to config-driven agents per ADR-0064.

• [stale-reference] docs/architecture.md:389 — The "Configuration layering" section references ADR-0035 for the customized/ overlay; the PR added a deprecation note, but the migration command is not mentioned in this section.

• [error-handling] internal/cli/migrate.go:392 — When pinAgentURL fails, resolveBaseURL falls back to scaffold.HarnessBaseURLWithHash using the CLI binary's embedded commitSHA. An old CLI binary would produce a base URL pointing to a potentially outdated scaffold version.

• [edge-case] internal/cli/migrate.go:287 — planMigrations stem matching heuristic associates non-harness files with agents by filename stem after stripping known prefixes (pre-, post-, validate-output-). Restricted to perAgentDirs which limits false matches, but coincidental stem collisions are possible.

• [edge-case] internal/harness/diff.go:153 — diffStringSlice uses set-based comparison and doesn't detect ordering changes. If a customization reorders skills without adding/removing items, the ordering change is silently lost. Consistent with mergeBaseIntoChild concatenation semantics.

• [edge-case] internal/cli/migrate.go:398 — registerMigratedAgent adds AgentEntry with Source: "harness/<name>.yaml" without verifying the harness file will exist at that path after PR is applied. Path is constructed consistently by the caller, so this is mitigated in practice.

• [path-traversal] internal/cli/migrate.go:508 — readTreeFile has a TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Mitigated by walkCustomized filtering symlinks at scan time, path-escape protection via strings.HasPrefix, and the PR review gate.

• [missing-migration-guidance] docs/ADRs/0033-per-repo-installation-mode.md:164 — Describes the customized/ overlay as part of the per-repo setup with no indication of deprecation or migration guidance.

• [stale-reference] docs/ADRs/0044-deprecate-per-org-installation-mode.md:56 — References "layered content resolution from ADR 0035" as a current feature without noting the deprecation.

• [stale-reference] docs/ADRs/0056-per-repo-precommit-tools-registry.md:29 — References the L1 override mechanism via customized/scripts/ per ADR-0035, which is deprecated.

Previous run (11)

Review

Findings

Medium

• [edge-case] internal/cli/migrate.go — The migrateCustom handler creates TreeFile entries that move files from customized/ to the destination prefix without checking whether a file already exists at the destination path in the repository tree. If a file exists at the destination, the migration PR would silently overwrite it. The warning message asks for manual review but does not prevent the operation. Mitigated by the PR review gate.

• [code-organization] internal/cli/migrate.go — Function buildModifiedAgentFiles has 8 parameters and spans ~120 lines, handling diff computation, URL building, tree file generation, and config mutation. Consider extracting URL resolution and config registration into separate helper functions.

• [error-message-style] internal/cli/migrate.go — Warning message "Could not pin agents repo URL, falling back to scaffold: %v" uses capitalized first word, inconsistent with established lowercase error/warning patterns in the codebase.

Low

• [error-handling] internal/cli/migrate.go — When pinAgentURL fails in buildModifiedAgentFiles, the code falls through to a scaffold URL fallback using the CLI's build-time commitSHA. The fallback is internally consistent (the scaffold embed matches the CLI build) but the user may not realize the base URL is pinned to the CLI's build version rather than the current upstream.

• [edge-case] internal/cli/migrate.go — planMigrations associates non-harness files with agents using filename stem matching after stripping known prefixes (pre-, post-, validate-output-). The heuristic is reasonable but can produce false associations in edge cases. Mitigated by the restricted perAgentDirs set and the PR review gate.

• [edge-case] internal/harness/diff.go — diffStringSlice uses set-based comparison, so it does not detect ordering changes. If the child reorders skills/plugins/providers relative to the base, no diff is produced. Consistent with mergeBaseIntoChild's concatenation semantics.

• [edge-case] internal/cli/migrate.go — migrateCustom registers the agent entry with Source: "harness/<name>.yaml" unconditionally. The harness file is always among the moved files (planMigrations ensures this), but files referenced by the harness (e.g., agent prompt paths) are not verified to exist at the destination.

• [path-traversal] internal/cli/migrate.go — readTreeFile has a TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Practical exploitation requires local filesystem access and precise timing. Path-escape protection is present, and walkCustomized already skips symlinks during traversal.

• [path-traversal] internal/cli/migrate.go — buildModifiedAgentFiles calls os.ReadFile on the customized harness path without checking for symlinks (unlike readTreeFile). Same TOCTOU class, mitigated by walkCustomized filtering symlinks at scan time.

• [adr-amendment-scope] docs/ADRs/0045-forge-portable-harness-schema.md — Adds a 5-line "Bidirectional composition" note to the Consequences section of ADR-0045 (Accepted). This is a brief cross-reference annotation within acceptable bounds for accepted ADRs.

• [adr-amendment-scope] docs/ADRs/0064-deprecate-customized-directory-overlay.md — Adds a 2-line annotation to an existing Consequences bullet noting the CLI command automates the conversion. Brief implementation note, acceptable for accepted ADRs.

• [stale-doc] docs/ADRs/0035-layered-content-resolution.md — Migration guidance references manual file moves but does not mention the new migrate-customizations command. ADR-0035 is already superseded by ADR-0064.

• [documentation-scope] docs/cli/README.md — No guidance on what happens if users run migrate-customizations multiple times. The command exits early with "No customized files found" after a successful migration, but this is only discoverable by reading the code.

Previous run (12)

Review

Findings

Medium

• [adr-amendment-scope] docs/ADRs/0045-forge-portable-harness-schema.md — The PR adds an 11-line "Bidirectional composition" subsection to the Consequences section of ADR-0045, which has status Accepted on main. This documents a discovered consequence of the original composition decision (that mergeBaseIntoChild has an inverse in DiffHarness), and the Context, Decision, and Status sections are untouched. While this reads as a consequence annotation rather than a decision change, it is substantive enough (~11 lines of new analytical content) to warrant human review under the repo's ADR immutability rules.

• [edge-case] internal/cli/migrate.go — The migrateCustom handler moves files from customized/ to the destination prefix via forge TreeFile entries without checking whether a file already exists at the destination path in the repository. If a file at the destination already exists, it would be silently overwritten in the PR. The code emits a StepWarn ("review PR to verify no unintended overwrites") and the changes are delivered via PR, providing a review gate. Practical risk is low but the edge case is real.

• [error-handling] internal/cli/migrate.go — When pinAgentURL fails in buildModifiedAgentFiles, the code falls through to a scaffold URL fallback using the CLI's build-time commitSHA. This SHA may not correspond to the upstream harness version the user originally customized against, meaning the base: URL in the composition harness could point to a different version than intended. The StepWarn is appropriate but the version mismatch risk is real.

Low

• [adr-immutability-violation] docs/ADRs/0064-deprecate-customized-directory-overlay.md — Adds 2 lines to an existing Consequences bullet noting that fullsend agent migrate-customizations automates the conversion. This is a minor editorial annotation to an Accepted ADR and does not change the decision.

• [missing-authorization] — No linked issue in fullsend-ai/fullsend for this feature PR. The PR references feat: extract code, fix, review, retro, prioritize agents from fullsend agents#12 (different repo) as discovery context. ADR-0064 (Accepted on main) provides architectural authorization for the migration tooling.

• [edge-case] internal/cli/migrate.go — planMigrations associates non-harness files with agents using filename stem matching after stripping known prefixes (pre-, post-, validate-output-). Files not matched become standalone and are moved separately with their own warnings. The heuristic is reasonable but there is no user-facing warning when a file in a per-agent directory fails to associate.

• [edge-case] internal/harness/diff.go — diffStringSlice uses set-based comparison, so it does not detect ordering changes. If the child reorders skills relative to the base, the diff reports no change and composition uses the base ordering. This is intentional given mergeBaseIntoChild's concatenation semantics.

• [edge-case] internal/cli/migrate.go — migrateCustom registers the agent entry with Source: "harness/<name>.yaml" without verifying that all files referenced by the harness (e.g., agent prompt paths) exist at the destination. In practice, associated files are moved in the same commit, so references should resolve.

• [path-traversal] internal/cli/migrate.go — readTreeFile has a TOCTOU gap between os.Lstat (symlink check) and os.ReadFile. Practical exploitation requires local filesystem access and precise timing. Path-escape protection (strings.HasPrefix) is present, and walkCustomized already skips symlinks during traversal.

• [path-traversal] internal/cli/migrate.go — buildModifiedAgentFiles calls os.ReadFile on the customized harness without re-checking for symlinks. Same TOCTOU class as readTreeFile, mitigated by walkCustomized filtering symlinks at scan time.

• [stale-doc] docs/architecture.md — The layered content resolution section references customized/ and notes ADR-0064 deprecation but does not mention the migrate-customizations command that automates the migration.

• [documentation-scope] docs/cli/README.md — No guidance on what happens if users run migrate-customizations multiple times. The command would exit early with "No customized files found" after a successful migration removes the customized files.

Previous run (13)

Review

Findings

Low

• [edge-case] internal/cli/migrate.go:520 — The migrateCustom handler moves files from customized/ to the destination prefix without checking whether a file already exists at the destination path. A warning is printed ('review PR to verify no unintended overwrites') but no programmatic check prevents overwriting. Mitigated by the PR review gate.

• [edge-case] internal/cli/migrate.go:679 — planMigrations associates non-harness files with agents using filename stem matching after stripping known prefixes (pre-, post-, validate-output-). Edge cases remain if an agent name collides with a prefix-stripped stem, though the restricted perAgentDirs set limits practical risk.

• [edge-case] internal/harness/diff.go:222 — diffStringSlice uses set-based comparison, so it does not detect ordering changes. Practical impact is minimal since mergeBaseIntoChild concatenates slices rather than preserving child ordering.

• [error-handling] internal/cli/migrate.go:443 — When pinAgentURL fails, the code falls through to a scaffold URL fallback with only a StepWarn. The fallback produces a valid URL using commitSHA and returns a hard error if commitSHA is invalid.

• [path-traversal] internal/cli/migrate.go:500 — readTreeFile has a minor TOCTOU gap between os.Lstat symlink check and os.ReadFile. Practical exploitation is unrealistic since this is a locally-run CLI tool requiring local filesystem access. Path-escape protection is present.

• [comment-style] internal/cli/migrate.go:362 — Inline comments in the migrationAction const block use period termination, which is inconsistent with the codebase convention in internal/layers/layers.go where inline const comments omit periods.

• [stale-doc] docs/guides/user/customizing-agents.md:175 — The example code demonstrates creating files in .fullsend/customized/ which uses the deprecated pattern. A section-level deprecation notice exists at lines 150–151, but the example itself has no inline warning.

• [stale-doc] docs/guides/user/customizing-agents.md:210 — The three-layer resolution table documents L1 org replacement: customized/scripts/.pre-commit-tools.yaml as an active feature. A deprecation note was added at lines 193–196 covering this section, but the table row itself lacks annotation.

• [stale-doc] docs/guides/user/customizing-with-skills.md:119 — The 'Overriding built-in skills' section describes the deprecated customized/ overlay mechanism. The deprecation notice under the heading is present but the section body still presents the deprecated approach as active guidance.

• [stale-doc] docs/runtimes.md:97 — Lines 97–98 reference customized/skills/ as an active override mechanism. A deprecation warning was added at lines 99–101 in the same ASCII art box, but the original lines still describe it as current.

• [documentation-inconsistency] docs/architecture.md:74 — The CLI management line lists fullsend agent add/list/update/remove/migrate-customizations using slash-separated formatting that works for short subcommands but is visually inconsistent with the long hyphenated migrate-customizations.

• [incomplete-migration-guidance] docs/cli/README.md:26 — The command description lists migrate-customizations inline with CRUD subcommands without explaining its distinct purpose (one-time migration utility vs. ongoing management).

Previous run (14)

Review

Findings

Medium

• [technical-accuracy] docs/plans/agent-registration.md:445 — The added text references "PR feat(cli): add migrate-customizations command #2954" as the implementing PR, but this change is being delivered in PR feat(cli): add migrate-customizations command for ADR-0064 #2932. This will be a stale/incorrect cross-reference once merged.
  Remediation: Change "PR feat(cli): add migrate-customizations command #2954" to "PR feat(cli): add migrate-customizations command for ADR-0064 #2932" or remove the PR number entirely.

Low

• [edge-case] internal/cli/migrate.go:173 — The migrateCustom handler (and the standalone-files path at ~line 508) moves files from customized/ to the destination prefix without checking whether a file already exists at the destination path in the repo. If the repo already has a file at that path, it will be silently overwritten in the PR. The migrateCustom path emits a StepWarn ("review PR to verify no unintended overwrites") but the standalone-files path does not. Mitigated by the PR review gate.

• [edge-case] internal/cli/migrate.go:320 — planMigrations associates non-harness files with agents using filename stem matching after stripping known prefixes (pre-, post-, validate-output-). The exact stem match takes priority before prefix stripping, which handles most overlap cases, but edge cases remain if only one of two overlapping agent names has a harness file.

• [edge-case] internal/harness/diff.go:222 — diffStringSlice uses set-based comparison, so it does not detect ordering changes. If a child has the same items as the base but in a different order, the diff reports no change. Practical impact is minimal since mergeBaseIntoChild concatenates slices rather than preserving child ordering.

• [error-handling] internal/cli/migrate.go:443 — When pinAgentURL fails, the code falls through to a scaffold URL fallback with only a StepWarn. The migration PR will reference the scaffold URL (tied to the current CLI build) rather than the agents repo URL. The fallback is safe (returns a hard error if commitSHA is invalid), but the warning could be easily missed.

• [path-traversal] internal/cli/migrate.go — readTreeFile has a minor TOCTOU gap between os.Lstat symlink check and os.ReadFile. Practical exploitation is unrealistic since this is a locally-run CLI tool requiring local filesystem access. Path-escape protection is present.

• [stale-doc] docs/agents/review.md:68 — References customized/skills/issue-labels/SKILL.md without a deprecation notice per ADR-0064. Users may follow the deprecated mechanism.

• [stale-doc] docs/agents/triage.md:99 — References customized/skills/issue-labels/SKILL.md without a deprecation notice per ADR-0064.

• [stale-doc] docs/guides/user/customizing-with-agents-md.md:122 — References customized/skills/ without a deprecation notice per ADR-0064.

• [comment-style] internal/cli/migrate.go:315 — Inline comments in the migrationAction const block have inconsistent period termination relative to the rest of the codebase.

---

Labels: PR adds Go code across cli/forge/harness/scaffold packages and updates 13 documentation files.

Previous run (15)

Review

Findings

Low

• [missing-authorization] — No linked issue in fullsend-ai/fullsend for this 2600+ line feature addition. ADR-0064 provides architectural authorization, but a tracked issue in this repo would improve traceability. The PR references feat: extract code, fix, review, retro, prioritize agents from fullsend agents#12 (a different repo) as discovery context.

• [edge-case] internal/cli/migrate.go:320 — The planMigrations stem-matching heuristic strips prefixes (pre-, post-, validate-output-) from filenames to associate non-harness files with agents. If two agents have coincidentally overlapping names after prefix stripping, files could be incorrectly associated. Mitigated by the PR review gate.

• [edge-case] internal/cli/migrate.go:173 — In the migrateCustom handler, files moved from customized/ to the regular directory via the Git Trees API silently overwrite any existing file at the destination path. Mitigated by the PR review gate.

• [edge-case] internal/harness/diff.go:222 — diffStringSlice does not preserve ordering changes. If the child reorders slice items without adding or removing, the diff is empty and mergeBaseIntoChild reconstructs the base ordering rather than the child ordering.

• [error-handling] internal/cli/migrate.go:443 — When pinAgentURL fails, the code falls back to using the scaffold embed's commit SHA for the base URL. The warning is logged via StepWarn but could be easily missed by the operator.

• [stale-doc] docs/ADRs/0058-agent-registration.md:62 — ADR-0058 enumerates fullsend agent subcommands as add, list, update, remove without mentioning migrate-customizations. The omission may be intentional since migrate-customizations is a one-time migration utility rather than a CRUD operation. ADR-0058 has status Accepted; updating it requires care per immutability rules.

Previous run (16)

Review

Findings

Critical

• [intent-misrepresentation] PR #2932 — Complete mismatch between PR metadata and actual content. The PR title claims fix(scaffold): fix review action mismatch and sanitize push output (a bug fix), body describes fixes to post-review.sh and post-code.sh, and it carries the bug label. However, the actual diff contains 2710 additions across 24 files implementing ADR-0064's migration tooling (migrate.go, diff.go, etc.) — a substantial new feature, not a bug fix. The PR content was entirely replaced since the prior review but metadata was not updated. Per AGENTS.md, GoReleaser uses PR titles to build release notes — a fix() prefix would misclassify this feature in the changelog.
  Remediation: Update the PR title to feat(cli): add migrate-customizations command for ADR-0064, rewrite the body to describe the actual changes, and replace the bug label with feature.

High

• [scope-tier-mismatch] PR #2932 — The PR uses the fix() commit prefix and carries the bug label, but the content is a 2710-line feature implementing a new CLI command (fullsend agent migrate-customizations). GoReleaser uses PR titles to build release notes — this feature would incorrectly appear as a bug fix. The ready-for-merge label compounds the risk since the PR could merge with incorrect metadata.
  Remediation: Change the PR title prefix from fix() to feat(). Remove the bug label and add the feature label.

Medium

• [missing-authorization] PR #2932 — No linked issue in fullsend-ai/fullsend for this 2710-line feature addition. The PR body references fullsend-ai/agents#12 (a different repo) as discovery context. Non-trivial structural changes benefit from explicit authorization via a linked issue for traceability.
  Remediation: Create or link an issue in fullsend-ai/fullsend that authorizes the ADR-0064 migration tooling implementation.

• [logic-error] internal/cli/migrate.go:178 — In the migrateDead handler, non-harness files (e.g., agents/review.md, scripts/pre-review.sh) associated with a dead override are moved to the regular directory (destPrefix + tf.Path), potentially overwriting files that already exist at the destination. The code emits a warning but proceeds unconditionally. For a dead override (agent already registered in config), the existing config-registered agent may already have its own version of these files at the destination path.
  Remediation: Check whether the destination file already exists in the repo before overwriting, or only delete all associated files for dead overrides rather than moving them.

Low

• [edge-case] internal/harness/diff.go:339 — diffEnvConfig returns (nil, false) when child.Env is nil, regardless of base.Env contents. If a user intentionally omitted the env: block to clear environment variables, the diff cannot represent this removal — the composed harness will inherit the base's env. Unlike slice/map removals which trigger warnings, this case is silent. This is a design limitation of the YAML-based diff approach (nil Env is ambiguous: "not specified" vs "intentionally cleared").

• [edge-case] internal/cli/migrate.go:338 — The planMigrations stem-matching heuristic for associating non-harness files with agents strips prefixes (pre-, post-, validate-output-). Files like policies/review.yaml (stem review) would be associated with the review agent. If such files are shared by multiple agents, moving them could break other agents. Mitigated by the fact that changes are delivered via PR for human review.

• [stale-doc] docs/ADRs/0058-agent-registration.md:62 — ADR-0058 explicitly enumerates fullsend agent subcommands as add, list, update, remove but does not mention the new migrate-customizations subcommand. Note: ADR-0058 has status Accepted, so updating it requires care per immutability rules. The omission may be intentional since migrate-customizations is a one-time migration utility rather than a CRUD operation.

• [adr-annotation] docs/ADRs/0064-deprecate-customized-directory-overlay.md — ADR-0064 (status: Accepted on main) receives a two-line annotation to its Consequences section noting the new CLI command. Verified this is a minor annotation within the scope allowed for accepted ADRs — no immutability violation.

---

Labels: PR content is a new feature (ADR-0064 migration CLI), not a bug fix. Title prefix and labels are incorrect for GoReleaser release notes.

Previous run (17)

Looks good to me.

Previous run (18)

Low

• [injection] internal/scaffold/fullsend-repo/scripts/post-code.sh:422 — SAFE_BRANCH sanitization is incomplete compared to SAFE_PUSH. The BRANCH variable is stripped of :: sequences but not of %0A/%0a/%0D/%0d (URL-encoded newlines). SAFE_PUSH receives all five substitutions while SAFE_BRANCH receives only one. Git ref format rules prohibit control characters in branch names (via git branch --show-current), making exploitation practically infeasible, but the inconsistency weakens defense-in-depth.
  Remediation: Apply the same %0A/%0a/%0D/%0d stripping to SAFE_BRANCH that is applied to SAFE_PUSH.

• [missing-authorization] No linked issue in this repo for the bug fixes. The PR body references fullsend-ai/agents#12 as discovery context but has no issue in fullsend-ai/fullsend. Consider creating one for traceability.

Previous run (19)

Looks good to me.

Low

• [injection] internal/scaffold/fullsend-repo/scripts/post-code.sh:422 — SAFE_BRANCH sanitization is incomplete compared to SAFE_PUSH. The BRANCH variable is stripped of :: sequences but not of %0A/%0a/%0D/%0d (URL-encoded newlines). SAFE_PUSH receives all five substitutions while SAFE_BRANCH receives only one. Git ref format rules prohibit control characters in branch names (via git branch --show-current), making exploitation practically infeasible, but the inconsistency weakens defense-in-depth.
  Remediation: Apply the same %0A/%0a/%0D/%0d stripping to SAFE_BRANCH that is applied to SAFE_PUSH.

• [missing-authorization] No linked issue in this repo for the bug fixes. The PR body references fullsend-ai/agents#12 as discovery context but has no issue in fullsend-ai/fullsend. Consider creating one for traceability.

Previous run (20)

Review

Findings

Medium

• [injection] internal/scaffold/fullsend-repo/scripts/post-code.sh:419 — The BRANCH variable is interpolated unsanitized into the ::error:: workflow command on this line: echo "::error::Push failed with unexpected error (git push origin ${BRANCH})". BRANCH is set from git branch --show-current (line 140) without character validation. The PR correctly sanitizes PUSH_OUTPUT but does not sanitize BRANCH in the same code block. While git ref format restrictions make exploitation difficult, applying the same sanitization pattern to BRANCH would be consistent defense-in-depth.
  Remediation: Apply the same sanitization pattern to BRANCH before interpolating it into workflow commands (e.g., SAFE_BRANCH="${BRANCH//::/}" with %0A/%0D stripping).

---

Labels: PR fixes bugs in scaffold harness post-scripts

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:33 PM UTC · Completed 3:42 PM UTC
Commit: 882aea1 · View workflow run →

[ggallen]

/fs-fix rebase and fix all the review issues. Squash the commit.

[ggallen]

/fs-fix rebase and fix all the review issues. Squash the commit.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:26 PM UTC · Completed 5:36 PM UTC
Commit: 84f572a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:42 PM UTC · Completed 9:58 PM UTC
Commit: fc679f5 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:59 PM UTC · Completed 3:08 PM UTC
Commit: c4ff56e · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:11 PM UTC · Ended 8:13 PM UTC
Commit: e8381e3 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:14 PM UTC · Ended 8:22 PM UTC
Commit: e8381e3 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:22 PM UTC · Completed 8:34 PM UTC
Commit: 80ba821 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:35 AM UTC · Completed 12:44 AM UTC
Commit: df9293b · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:12 PM UTC · Completed 12:20 PM UTC
Commit: df9293b · View workflow run →

[fullsend-ai-retro]

PR #2932 added a large migrate-customizations CLI command (3152 additions, 31 files). The review agent ran 18 times and produced 23 findings (18 low, 5 medium). Nearly half (11/23) were dismissed as design limitations but kept being re-raised in subsequent rounds. The one real integration bug — ValidRoles() hardcoded list rejecting custom agent roles — was found by human reviewer ascerra, not the bot. The bot excelled at injection detection and nil-handling bugs but its signal-to-noise ratio was poor due to duplicate and low-value findings.

Proposals filed

• Evidence for #2959: 7x duplicate edge-case findings across 18 review runs on PR #2932 (in fullsend-ai/fullsend)
• Review agent should detect hardcoded-allowlist validation failures for newly-introduced dynamic inputs (in fullsend-ai/fullsend)