feat(cli): add migrate-customizations command

[ggallen]

Summary

Implements the fullsend agent migrate-customizations command per ADR-0064 (deprecate customized/ directory overlay).

Closes #2971

What it does

• Scans customized/ directory and classifies each override:
  • Dead overrides (agent already in config) → delete harness files, move non-harness files
  • Custom agents (not in upstream scaffold) → move to regular paths, register in config.yaml
  • Modified standard agents → compute base: composition harness via DiffHarness, register with pinned URL
• Creates a PR via GitHub API with all changes in a single atomic commit
• --dry-run flag previews changes without creating a PR

New infrastructure

• internal/harness/diff.go: Harness diff engine — computes minimal child from base/customized pair
• internal/forge/forge.go: TreeFile.Delete field for API-based file deletion

Documentation

• Deprecation notices added to: customizing-agents.md, building-custom-agents.md, customizing-with-skills.md, running-agents-locally.md
• CLI command tree updated in cli-internals.md

Test plan

• go test ./internal/harness/ — 36 diff tests covering all field types, removal warnings, security fields
• go test ./internal/cli/ — 17 migration tests covering dry-run, PR creation, per-repo mode, mixed scenarios
• go test ./internal/scaffold/ — baseurl and content hash tests
• go vet ./... — clean

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:20 PM UTC · Completed 5:38 PM UTC
Commit: 5a1f018 · View workflow run →

[github-actions]

Site preview

Preview: https://56eb4bf0-site.fullsend-ai.workers.dev

Commit: e619d04f76f20071d2d1822f90d4e353b683a662

[qodo-code-review]

PR Summary by Qodo

Add CLI command to migrate customized/ overrides into config-driven agents

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add fullsend admin migrate-customizations to convert customized/ overrides into config-driven
 agents.
• Auto-handle dead overrides, custom agents, and modified scaffold agents using base composition.
• Introduce harness diffing utilities and add thorough unit test coverage.

Diagram

graph TD
  cmd["CLI: migrate-customizations"] --> scan["Scan customized/"] --> plan["Plan per-agent migrations"] --> apply["Move/delete files"] --> writecfg["Update config.yaml"]
  plan --> scaffold["Scaffold embedded harness"] --> diff["harness.DiffHarness()"] --> apply

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Always rewrite full harness without base composition

• ➕ Simpler mental model (no reverse-merge diffing)
• ➕ Avoids base-composition limitations like slice removals
• ➖ Larger, noisier harness files and harder future upgrades
• ➖ Does not leverage ADR-0045/0058 composition model

2. Use structural YAML diff/patch (RFC 6902 / jsonpatch-like)

• ➕ General-purpose diffing across maps/slices with explicit removals
• ➕ Could represent deletions more naturally than current warnings
• ➖ Runtime still needs to understand/apply patches (bigger design change)
• ➖ Harder to align with existing mergeBaseIntoChild semantics

3. Interactive/manual migration (report-only + guided steps)

• ➕ Lower risk of unintended file moves/deletions
• ➕ Lets users resolve ambiguous associations (e.g., shared scripts)
• ➖ Slower adoption; more operator effort
• ➖ Harder to automate at scale across many repos

Recommendation: The PR’s approach (plan migrations + generate minimal base-composition harnesses that match mergeBaseIntoChild) is the best fit for the ADR-defined config-driven model and keeps overrides small and maintainable. The main limitation—slice removals not being representable—is correctly surfaced as warnings/abort for DiffHarness(), which is preferable to silently producing an incorrect composition.

Files changed (6) +1365 / -0

Enhancement (4) +814 / -0

admin.goRegister migrate-customizations under admin CLI +1/-0

Register migrate-customizations under admin CLI

• Adds the new 'migrate-customizations' subcommand to the admin command tree so it becomes available as 'fullsend admin migrate-customizations'.

internal/cli/admin.go

migrate.goImplement customized/ → config-driven agent migration command +449/-0

Implement customized/ → config-driven agent migration command

• Introduces a new Cobra command and end-to-end migration workflow: scan customized files, categorize agents (dead/custom/modified), move/delete files, generate base-composition harnesses for modified scaffold agents, and update/validate/write config.yaml. Supports dry-run mode and both org vs per-repo customized directory layouts.

internal/cli/migrate.go

diff.goAdd DiffHarness to compute minimal base-composition overrides +350/-0

Add DiffHarness to compute minimal base-composition overrides

• Implements a reverse-merge diff that produces the minimal child harness needed to recreate a customized harness when composed with a base via mergeBaseIntoChild semantics. Handles scalars, concatenated slices (additions only), host_files/api_servers deduping, maps/env, pointer structs, and forge blocks, with warnings/abort behavior when removals would be required.

internal/harness/diff.go

baseurl.goExpose embedded scaffold harness YAML content loader +14/-0

Expose embedded scaffold harness YAML content loader

• Adds 'scaffold.HarnessContent()' to read the embedded scaffold harness YAML bytes by name with validation, enabling tooling (like migration) to diff against upstream templates without fetching remotely.

internal/scaffold/baseurl.go

Tests (2) +551 / -0

migrate_test.goAdd tests for migration planning and filesystem moves +308/-0

Add tests for migration planning and filesystem moves

• Adds unit tests covering missing/empty customized directories, dead override deletion, custom agent registration, dry-run behavior, per-repo customized path handling, standalone file moves, file association heuristics, and moveFile behavior.

internal/cli/migrate_test.go

diff_test.goAdd comprehensive DiffHarness test suite +243/-0

Add comprehensive DiffHarness test suite

• Adds tests for identical harnesses, scalar/slice/map/env/host_files/forge differences, customized-file retention behavior, multi-field diffs, new forge platforms, and slice-removal warning/abort semantics.

internal/harness/diff_test.go

[codecov]

Codecov Report

❌ Patch coverage is 83.00330% with 103 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/migrate.go	76.96%	41 Missing and 35 partials ⚠️
internal/harness/diff.go	95.56%	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 (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 54 rules

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

1. Moved scripts lose exec ✓ Resolved 🐞 Bug ☼ Reliability

Description

moveFile rewrites migrated files with mode 0644, dropping executable bits on scripts that are
executed directly via exec.Command. After migration, pre_script/post_script files moved from
customized/ can become non-executable and fail at runtime with permission errors.

Code

internal/cli/migrate.go[R308-323]

+func moveFile(customizedBase, destBase, relPath string) error {
+	src := filepath.Join(customizedBase, relPath)
+	dst := filepath.Join(destBase, relPath)
+
+	if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil {
+		return err
+	}
+
+	data, err := os.ReadFile(src)
+	if err != nil {
+		return err
+	}
+	if err := os.WriteFile(dst, data, 0o644); err != nil {
+		return err
+	}
+	return os.Remove(src)

Relevance

⭐⭐⭐ High

Team previously fixed exec-bit loss on copied scripts (PR #343); preserving file mode is
prioritized.

PR-#343

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The migration helper writes files as 0644, but runtime executes pre_script by path, which will fail
if the executable bit was removed.

internal/cli/migrate.go[306-323]
internal/cli/run.go[598-609]

Agent prompt

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

### Issue description
`moveFile` copies file contents and writes the destination with hard-coded permissions `0644`. This strips executable bits from scripts (e.g., pre/post scripts), which are later executed directly.

### Issue Context
`runAgent` uses `exec.Command(h.PreScript)` which requires the script to be executable on Unix-like systems.

### Fix Focus Areas
- internal/cli/migrate.go[306-324]
- internal/cli/run.go[598-609]

### Implementation notes
- Prefer `os.Rename(src, dst)` when possible (same filesystem), to preserve metadata.
- Otherwise:
 - `info, err := os.Stat(src)`
 - write `dst` with `info.Mode() & 0o777` (or write then `os.Chmod(dst, info.Mode() & 0o777)`)
 - then remove `src`.
- Add/adjust tests to assert executable bit preservation for a migrated `scripts/*.sh` file.

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

---

2. Ignored diff abort ✓ Resolved 🐞 Bug ≡ Correctness

Description

migrateModifiedAgent treats DiffHarness returning Child=nil as “harnesses are identical” and writes
a base-only wrapper even when DiffHarness aborted due to an unrepresentable diff (e.g., slice
removal). This can drop user customizations while also deleting the original customized harness
file.

Code

internal/cli/migrate.go[R395-427]

+	// Generate the composition harness YAML.
+	var outputHarness *harness.Harness
+	if diffResult.Child == nil {
+		// Harnesses are identical — create a minimal wrapper with just base:.
+		outputHarness = &harness.Harness{}
+	} else {
+		outputHarness = diffResult.Child
+	}
+	outputHarness.Base = baseURL
+
+	outputData, err := yaml.Marshal(outputHarness)
+	if err != nil {
+		return fmt.Errorf("marshaling composition harness: %w", err)
+	}
+
+	// Write the composition harness.
+	harnessPath := filepath.Join(absDir, "harness", agentName+".yaml")
+	if err := os.MkdirAll(filepath.Dir(harnessPath), 0o755); err != nil {
+		return err
+	}
+	if err := os.WriteFile(harnessPath, outputData, 0o644); err != nil {
+		return fmt.Errorf("writing composition harness: %w", err)
+	}
+
+	// Move non-harness customized files to regular directories.
+	for f := range customizedFilesSet {
+		if filepath.Dir(f) == "harness" {
+			// Remove the old customized harness (replaced by composition harness).
+			if err := os.Remove(filepath.Join(customizedBase, f)); err != nil && !os.IsNotExist(err) {
+				return fmt.Errorf("removing old customized harness: %w", err)
+			}
+			continue
+		}

Relevance

⭐⭐⭐ High

Harness composition correctness is closely reviewed; base/child merge edge cases routinely addressed
in harness PRs.

PR-#2180
PR-#2690

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DiffHarness intentionally returns Child=nil when it encounters an unrepresentable removal, but
migrateModifiedAgent interprets Child=nil as “identical” and overwrites/deletes the customized
harness anyway, which can lose behavior.

internal/harness/diff.go[98-104]
internal/harness/diff_test.go[53-64]
internal/cli/migrate.go[363-403]
internal/cli/migrate.go[419-425]

Agent prompt

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

### Issue description
`harness.DiffHarness` can return `Child=nil` for two different reasons:
1) base and child are identical, or
2) the diff is **not representable** with base composition (it returns early with warnings).

`migrateModifiedAgent` currently treats *any* `Child=nil` as “identical” and proceeds to overwrite the harness with a `base:`-only wrapper, then deletes the customized harness. This is lossy migration.

### Issue Context
- `DiffHarness` explicitly returns `Child=nil` + warning on slice removal.
- Migration should **stop** (or fall back to a non-composition migration) when the diff cannot be expressed.

### Fix Focus Areas
- internal/cli/migrate.go[363-427]
- internal/harness/diff.go[98-125]

### Implementation notes
- If `len(diffResult.Warnings) > 0` and `diffResult.Child == nil`, return an error and **do not** remove any customized files.
- Alternatively, fall back to migrating the agent as a local-path harness (move files + register `harness/<name>.yaml` without `base:`) rather than generating an incorrect composition harness.
- Consider adding a dedicated signal in `DiffResult` (e.g., `Aborted bool`) so `Child=nil` is not overloaded.

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

---

3. Config allowlist ignored ✓ Resolved 🐞 Bug ≡ Correctness

Description

migrateModifiedAgent only appends the base URL prefix to config.yaml’s allowed_remote_resources, but
runtime fetch enablement is controlled by harness YAML fields (allow_runtime_fetch /
allowed_remote_resources). Because DiffHarness never carries harness.AllowedRemoteResources into the
generated composition harness, migrating a customized harness that enabled fetching via
harness.allowed_remote_resources will change runtime behavior even though config.yaml was updated.

Code

internal/harness/diff.go[R31-170]

+func DiffHarness(base, child *Harness, customizedFiles map[string]bool) *DiffResult {
+	result := &DiffResult{Child: &Harness{}}
+	hasAny := false
+
+	// Scalar strings — keep if child differs from base, or if the
+	// referenced file is customized.
+	if diffScalarFile(&result.Child.Agent, base.Agent, child.Agent, customizedFiles) {
+		hasAny = true
+	}
+	if diffScalarFile(&result.Child.Doc, base.Doc, child.Doc, customizedFiles) {
+		hasAny = true
+	}
+	if child.Description != base.Description {
+		result.Child.Description = child.Description
+		hasAny = true
+	}
+	if child.Role != base.Role {
+		result.Child.Role = child.Role
+		hasAny = true
+	}
+	if child.Slug != base.Slug {
+		result.Child.Slug = child.Slug
+		hasAny = true
+	}
+	if child.Image != base.Image {
+		result.Child.Image = child.Image
+		hasAny = true
+	}
+	if diffScalarFile(&result.Child.Policy, base.Policy, child.Policy, customizedFiles) {
+		hasAny = true
+	}
+	if child.Model != base.Model {
+		result.Child.Model = child.Model
+		hasAny = true
+	}
+	if diffScalarFile(&result.Child.PreScript, base.PreScript, child.PreScript, customizedFiles) {
+		hasAny = true
+	}
+	if diffScalarFile(&result.Child.PostScript, base.PostScript, child.PostScript, customizedFiles) {
+		hasAny = true
+	}
+	if diffScalarFile(&result.Child.AgentInput, base.AgentInput, child.AgentInput, customizedFiles) {
+		hasAny = true
+	}
+
+	// Scalar ints
+	if child.TimeoutMinutes != base.TimeoutMinutes {
+		result.Child.TimeoutMinutes = child.TimeoutMinutes
+		hasAny = true
+	}
+	if child.SandboxTimeoutSeconds != base.SandboxTimeoutSeconds {
+		result.Child.SandboxTimeoutSeconds = child.SandboxTimeoutSeconds
+		hasAny = true
+	}
+
+	// Scalar bool
+	if child.AllowRuntimeFetch != base.AllowRuntimeFetch {
+		result.Child.AllowRuntimeFetch = child.AllowRuntimeFetch
+		hasAny = true
+	}
+
+	// Scalar *int
+	if !reflect.DeepEqual(child.MaxRuntimeFetches, base.MaxRuntimeFetches) {
+		result.Child.MaxRuntimeFetches = child.MaxRuntimeFetches
+		hasAny = true
+	}
+
+	// String slices (concatenated by mergeBaseIntoChild) — keep only extras
+	if extras, removed := diffStringSlice(base.Skills, child.Skills, customizedFiles); len(extras) > 0 || removed {
+		if removed {
+			result.Warnings = append(result.Warnings, "skills: child removes items from base; cannot express with base: composition")
+			result.Child = nil
+			return result
+		}
+		result.Child.Skills = extras
+		hasAny = true
+	}
+	if extras, removed := diffStringSlice(base.Plugins, child.Plugins, customizedFiles); len(extras) > 0 || removed {
+		if removed {
+			result.Warnings = append(result.Warnings, "plugins: child removes items from base; cannot express with base: composition")
+			result.Child = nil
+			return result
+		}
+		result.Child.Plugins = extras
+		hasAny = true
+	}
+	if extras, removed := diffStringSlice(base.Providers, child.Providers, customizedFiles); len(extras) > 0 || removed {
+		if removed {
+			result.Warnings = append(result.Warnings, "providers: child removes items from base; cannot express with base: composition")
+			result.Child = nil
+			return result
+		}
+		result.Child.Providers = extras
+		hasAny = true
+	}
+
+	// HostFiles — keep entries in child not in base (by dest)
+	if extras := diffHostFiles(base.HostFiles, child.HostFiles); len(extras) > 0 {
+		result.Child.HostFiles = extras
+		hasAny = true
+	}
+
+	// APIServers — keep entries in child not in base (by name)
+	if extras := diffAPIServers(base.APIServers, child.APIServers); len(extras) > 0 {
+		result.Child.APIServers = extras
+		hasAny = true
+	}
+
+	// Maps — keep keys where child value differs from base
+	if diff := diffStringMap(base.RunnerEnv, child.RunnerEnv); len(diff) > 0 {
+		result.Child.RunnerEnv = diff
+		hasAny = true
+	}
+
+	// Env — diff sub-maps independently
+	if envDiff := diffEnvConfig(base.Env, child.Env); envDiff != nil {
+		result.Child.Env = envDiff
+		hasAny = true
+	}
+
+	// Pointer structs — keep if non-nil and different
+	if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) {
+		result.Child.ValidationLoop = child.ValidationLoop
+		hasAny = true
+	}
+	if child.Security != nil && !reflect.DeepEqual(child.Security, base.Security) {
+		result.Child.Security = child.Security
+		hasAny = true
+	}
+
+	// Forge — diff per platform
+	if forgeDiff := diffForge(base.Forge, child.Forge); len(forgeDiff) > 0 {
+		result.Child.Forge = forgeDiff
+		hasAny = true
+	}
+
+	if !hasAny {
+		result.Child = nil
+	}
+	return result

Relevance

⭐⭐ Medium

No clear prior reviews on diffing/retaining harness allowed_remote_resources; related fetch gating
work exists but not this case.

PR-#2270

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The runtime decides to start the fetch service based on harness-level fields, but DiffHarness never
emits the harness allowlist field, while migration only updates config.yaml’s allowlist; this
combination can change migrated harness behavior.

internal/cli/migrate.go[439-446]
internal/harness/harness.go[237-266]
internal/cli/run.go[1320-1331]
internal/harness/diff.go[31-170]

Agent prompt

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

### Issue description
The migration updates **config.yaml** allowlist prefixes for the new `base:` URL, but it does not preserve the **harness-level** fetch configuration that actually drives runtime fetch service behavior:
- `Harness.AllowedRemoteResources`
- `Harness.AllowRuntimeFetch`
- `Harness.MaxRuntimeFetches`

`DiffHarness` never includes `AllowedRemoteResources` at all, so a customized harness that previously opted into runtime fetch via `allowed_remote_resources:` can lose that setting in the migrated composition harness.

### Issue Context
`shouldStartFetchService` starts runtime fetching based on harness fields (not config.yaml). Config allowlisting is necessary for validation/security, but it is not sufficient to preserve harness-driven fetch enablement.

### Fix Focus Areas
- internal/harness/diff.go[31-170]
- internal/harness/harness.go[237-266]
- internal/cli/run.go[1320-1331]
- internal/cli/migrate.go[439-446]

### Implementation notes
- Extend `DiffHarness` to treat these security/fetch fields as **base-independent** (since base composition does not merge them):
 - If `len(child.AllowedRemoteResources) > 0`, copy it into `result.Child.AllowedRemoteResources`.
 - If `child.AllowRuntimeFetch == true`, set it in the diff output.
 - If `child.MaxRuntimeFetches != nil`, copy it.
- Consider adding tests that verify a customized harness with `allowed_remote_resources` round-trips through `DiffHarness` + `mergeBaseIntoChild` without behavior change.

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

---

4. HarnessContent() missing unit tests ✓ Resolved 📘 Rule violation ▣ Testability

Description

internal/scaffold/baseurl.go adds the new exported function HarnessContent() without adding or
updating any _test.go coverage in the internal/scaffold package. This violates the requirement
that new or modified Go logic must be accompanied by tests, increasing regression risk.

Code

internal/scaffold/baseurl.go[R65-77]

+// HarnessContent returns the raw YAML bytes of an embedded scaffold harness
+// template. This is the same content served by raw.githubusercontent.com for
+// the release commit the CLI was built from.
+func HarnessContent(harnessName string) ([]byte, error) {
+	if !validHarnessName.MatchString(harnessName) {
+		return nil, fmt.Errorf("invalid harness name %q: must match %s", harnessName, validHarnessName.String())
+	}
+	data, err := content.ReadFile("fullsend-repo/harness/" + harnessName + ".yaml")
+	if err != nil {
+		return nil, fmt.Errorf("unknown harness %q: %w", harnessName, err)
+	}
+	return data, nil
+}

Relevance

⭐ Low

Scaffold/test-coverage nitpicks are often rejected; several “add/update scaffold tests” suggestions
were declined.

PR-#586
PR-#2346

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rule requires tests for new or modified Go logic. The diff shows a new exported function
HarnessContent() was introduced in internal/scaffold/baseurl.go in this PR, but no corresponding
test file change is present to cover it.

Rule 1062049: Require tests for new or modified Go logic
internal/scaffold/baseurl.go[65-77]

Agent prompt

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

## Issue description
A new exported function `HarnessContent()` was added to `internal/scaffold/baseurl.go` but no tests were added/updated in this PR to validate its behavior.

## Issue Context
The compliance requirement mandates tests for new or modified Go logic. `HarnessContent()` is a new public API that validates harness names and reads embedded scaffold harness YAML; it should be covered for both success and error paths.

## Fix Focus Areas
- internal/scaffold/baseurl.go[65-77]
- internal/scaffold/baseurl_test.go[1-220]

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

---

[fullsend-ai-review]

Looks good to me

Findings

Low

• [edge-case] internal/cli/migrate.go:462 — For migrateModified agents where the customized harness is identical to upstream (DiffResult.Child is nil and no warnings), buildModifiedAgentFiles creates a harness with only base: set. This opts the agent into upstream tracking via base: composition, which differs from the original customized/ overlay behavior. This is arguably the correct behavior since there are no customizations to preserve.

• [edge-case] internal/cli/migrate.go:335 — The planMigrations file-association heuristic tries exact stem match before prefix-stripped variants, but when exact stem doesn't match any harness agent and prefix-stripped succeeds, the file is silently associated with the stripped agent. In practice this matches the intended pre-/post- script naming convention.

• [TOCTOU] internal/cli/migrate.go:537 — readTreeFile has a TOCTOU window between Lstat (symlink check / mode read) and ReadFile. Practical impact is negligible for a local CLI tool with path containment defense in depth.

• [stale-reference] docs/agents/triage.md:99 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill without a deprecation notice. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/agents/review.md:68 — Same as above — references customized/skills/issue-labels/SKILL.md without a deprecation notice.

• [stale-reference] docs/ADRs/0053-agent-driven-branch-targeting.md:160 — Mentions repos overriding via .fullsend/customized/ without referencing the deprecation. As an accepted ADR, this is a historical record; a cross-reference annotation to ADR-0064 would suffice.

Previous run

Looks good to me

Findings

Low

• [logic-error] internal/harness/diff.go:285 — diffEnvConfig returns (nil, false) when child is nil, even if base has populated Runner/Sandbox maps. A customized harness that intentionally omits the env: block is treated as "no difference" rather than a removal. After migration to base: composition, mergeBaseIntoChild will inject the base env into the child. Practical impact is low: this edge case (customized harness explicitly removing env) is unlikely in migration scenarios, and diffStringMap exhibits the same short-circuit behavior for empty maps.

• [edge-case] internal/cli/migrate.go:335 — The planMigrations file-association heuristic strips prefixes (pre-, post-, validate-output-) from the stem without first trying the original stem. A file named scripts/pre-review.sh would associate with agent review even if an agent named pre-review exists.

• [edge-case] internal/cli/migrate.go:462 — For migrateModified agents where the customized harness is identical to upstream, buildModifiedAgentFiles creates a harness with only base: set. This opts the agent into upstream tracking (future upstream changes will be inherited), which differs from the original customized/ overlay behavior of whole-file replacement. This is intended per ADR-0064.

• [TOCTOU] internal/cli/migrate.go:537 — readTreeFile has a TOCTOU window between Lstat (symlink check / mode read) and ReadFile. Practical impact is negligible for a local CLI tool with the path containment defense providing defense in depth.

• [stale-reference] docs/agents/triage.md:99 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill. The customized/ mechanism is deprecated (ADR-0064) but still functional. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/agents/review.md:68 — Same as above — references customized/skills/issue-labels/SKILL.md without a deprecation notice.

• [stale-reference] docs/ADRs/0053-agent-driven-branch-targeting.md:160 — Mentions repos overriding via .fullsend/customized/ without referencing the automated migration command.

Previous run (2)

Review

Findings

Medium

• [logic-error] internal/cli/migrate.go:167 — For migrateDead overrides, harness files are deleted, but non-harness associated files (e.g., agents/review.md, scripts/pre-review.sh) are moved to the destination directory while the customized copy is deleted. The dead agent is already registered in config pointing to a different source (e.g., a URL). Moving associated files to the regular directory may overwrite files that the registered agent's harness actually references, or introduce unreferenced orphan files. The warning acknowledges this, but proceeding silently for a dead override could leave the repo in an inconsistent state.
  Remediation: For dead overrides, consider deleting all associated files (not just harness files), or at minimum prompt for user confirmation before moving non-harness files. Alternatively, skip non-harness files for dead overrides and warn the user to clean them up manually.

Low

• [edge-case] internal/cli/migrate.go:335 — The planMigrations file-association heuristic strips prefixes (pre-, post-, validate-output-) greedily — it tries each prefix in order and breaks on the first match. A file named post-code.sh would only associate with the code agent if code has a harness entry in harnessAgents. If the agent's harness is NOT in customized/, the file becomes standalone and is moved independently. No data loss occurs, but may produce surprising migration output.

• [TOCTOU] internal/cli/migrate.go:537 — readTreeFile has a TOCTOU window between Lstat (symlink check / mode read) and ReadFile. Practical impact is negligible for a local CLI tool with the path containment defense providing defense in depth.

• [edge-case] internal/cli/migrate.go:462 — For migrateModified agents where the harness is identical to upstream, buildModifiedAgentFiles creates an empty harness with only base: set. This registers a config entry and creates a composition harness file for an agent that requires no customization. Semantically correct but adds unnecessary config entries.

• [stale-reference] docs/agents/triage.md:99 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill. The customized/ mechanism is deprecated (ADR-0064) but still functional. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/agents/review.md:68 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill. The customized/ mechanism is deprecated (ADR-0064) but still functional. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/guides/user/customizing-with-agents-md.md:122 — Contains a link reference to customized/skills/ overlay mechanism. The target section (customizing-with-skills.md) was updated in this PR with deprecation warnings, but this referencing page was not.

Previous run (3)

Review

Findings

Low

• [test-adequacy] internal/harness/diff_test.go — No round-trip test verifies that mergeBaseIntoChild(base, DiffHarness(base, child).Child) reproduces child. The ADR-0045 annotation added in this PR states "changes to merge rules must be reflected in both directions." A round-trip test would mechanically enforce this invariant and catch drift if a new field is added to the Harness struct but only handled in one direction.

• [stale-reference] docs/agents/triage.md:99 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill. The customized/ mechanism is deprecated (ADR-0064) but still functional. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/agents/review.md:68 — References customized/skills/issue-labels/SKILL.md as the path for overriding the built-in issue-labels skill. The customized/ mechanism is deprecated (ADR-0064) but still functional. Consider adding a deprecation notice in a follow-up PR.

• [stale-reference] docs/runtimes.md:97 — Agent rule layering diagram mentions customized/skills/ overrides without noting that this mechanism is deprecated per ADR-0064.

• [incomplete-doc] docs/plans/agent-registration.md:133 — Plan document lists agent subcommands (add, list, update, remove) but does not mention the new migrate-customizations subcommand. The PR already updates architecture.md, cli/README.md, cli-internals.md, and operations.md to include migrate-customizations.

• [TOCTOU] internal/cli/migrate.go:537 — readTreeFile has a TOCTOU window between Lstat and ReadFile. Practical impact is negligible for a local CLI tool with path containment defense in depth.

• [parameter-ordering] internal/cli/migrate.go:72 — Parameter order in runMigrateCustomizations differs slightly from agent.go patterns, though the pattern is not fully consistent within agent.go itself.

• [error-messages] internal/cli/migrate.go:56 — Minor error message formatting differences from agent.go patterns, though most messages follow the established lowercase verb + colon + %w convention.

Previous run (4)

Review

Findings

High

• [logic-error] internal/harness/diff.go — diffAPIServers computes extras by Name (treating entries with the same Name but different fields as overrides), but mergeBaseIntoChild uses simple concatenation for APIServers (no dedup by Name). When the diff child is composed with the base, both the original base entry and the diff's modified entry will appear, producing duplicate APIServer entries instead of reproducing the original customized harness. For example: base has [{Name: "proxy", Script: "start.sh"}], customized child has [{Name: "proxy", Script: "start-v2.sh"}, {Name: "metrics", Script: "metrics.sh"}]. diffAPIServers returns both entries. After composition via mergeBaseIntoChild (concatenation), the result is [{proxy, start.sh}, {proxy, start-v2.sh}, {metrics, metrics.sh}] — a duplicate proxy entry.
  Remediation: Either (a) change diffAPIServers to only return entries whose Name is NOT in base (treat any Name-level modification as an unrepresentable change that triggers a warning, similar to slice removal), or (b) align mergeBaseIntoChild to use Name-based dedup for APIServers. Option (a) is safer and more conservative. Update TestDiffHarness_APIServerDifference accordingly.

Low

• [edge-case] internal/cli/migrate.go:55 — When --dry-run is true, forge client initialization errors (forgeErr) are silently ignored and forgeClient may be nil. The error is discarded without logging. In dry-run mode forgeClient is never used (the function returns early), so this is not a bug, but logging the error would improve UX when users troubleshoot subsequent non-dry-run runs.

• [test-adequacy] internal/harness/diff_test.go — TestDiffHarness_APIServerDifference asserts len(result.Child.APIServers) == 2, which validates the current (incorrect) diffAPIServers behavior where modified-by-Name entries are treated as extras. Update this test once the high-severity diffAPIServers logic is fixed.

• [TOCTOU] internal/cli/migrate.go:537 — readTreeFile uses os.Stat and os.ReadFile, both of which follow symlinks. There is a TOCTOU window between walkCustomized (which skips symlinks) and readTreeFile. A regular file could be replaced with a symlink between these operations. Practical impact is negligible since this is a local CLI tool and the path containment check (strings.HasPrefix on absolute paths) provides defense in depth.

• [incomplete-doc] docs/plans/agent-registration.md:132 — The agent registration implementation plan documents the command structure for fullsend agent but does not include migrate-customizations. The information exists in the correct plan document (docs/plans/deprecate-customized-directory-overlay.md, updated in this PR); a cross-reference could improve discoverability.

Previous run (5)

Review

Findings

Medium

• [naming-consistency] internal/cli/migrate.go:301 — Command constructor named newMigrateCustomizationsCmd() breaks naming pattern. All other agent subcommands use newAgent*Cmd() prefix: newAgentAddCmd, newAgentListCmd, newAgentUpdateCmd, newAgentRemoveCmd.
  Remediation: Rename function to newAgentMigrateCustomizationsCmd() to match the newAgent*Cmd() pattern used by sibling commands.

Low

• [edge-case] internal/cli/migrate.go:320 — runMigrateCustomizations silently swallows forge client initialization errors. When defaultForgeClient() fails, forgeClient is nil and the error is discarded. In non-dry-run mode, the nil forgeClient is caught later with a generic "forge client required" error that loses the original initialization error context, making debugging harder.

• [test-adequacy] internal/harness/diff_test.go — Missing test for nil base.Env with non-nil child.Env, and vice versa. diffEnvConfig handles these paths but they lack test coverage.

Previous run (6)

Review

Findings

Medium

• [missing-adr-cross-reference] internal/harness/diff.go — The harness diff engine (438 lines) implements field-level harness composition diffing with complex semantics (scalars override, slices concatenate, security fields always kept, removal detection). No ADR describes this inverse composition model. ADR-0045 describes forward composition (base merged into child) but not the diff/decomposition direction. The diff engine makes policy decisions tightly coupled to mergeBaseIntoChild semantics (e.g., "security fields are never merged from base, so always include them in diff"), yet this logic isn't documented in an ADR.
  Remediation: Add a section to ADR-0045 describing bidirectional composition (forward merge + backward diff), or create a new ADR for the migration diff engine.

Low

• [scope-creep-beyond-authorization] internal/cli/migrate.go — The migrate-customizations command is authorized by issue feat: add migrate-customizations command (ADR-0064) #2971, but neither ADR-0064 nor its implementation plan explicitly describes an automated migration command. ADR-0064 states users "must migrate" and that "deprecation warnings... will guide migration." A brief mention in ADR-0064 acknowledging the migration tool would strengthen traceability.

• [missing-reference] docs/ADRs/0064-deprecate-customized-directory-overlay.md — ADR-0064 states users must migrate but does not mention the migrate-customizations command that automates this. Adding a sentence to the Consequences section would improve discoverability.

• [missing-reference] docs/plans/deprecate-customized-directory-overlay.md — The implementation plan for the deprecation does not reference the companion migration CLI command.

• [missing-usage-guidance] docs/guides/dev/cli-internals.md — The CLI internals doc lists migrate-customizations with its flags but provides no usage examples. Consider adding a user-facing migration guide with workflow examples.

• [edge-case] internal/harness/diff.go:383 — diffForgeConfig continues processing subsequent fields (ValidationLoop, RunnerEnv, Env) after detecting a skill removal and adding a warning, producing a partial ForgeConfig alongside warnings. The parent DiffHarness discards all results when warnings are present, so there is no runtime impact, but the control flow is misleading for future maintainers.

• [error-handling] internal/cli/migrate.go:372 — In the migrateDead case, non-harness files are moved from customized/ to the regular directory. If a file already exists at the destination in the repo, the commit silently overwrites it with no warning.

• [path-traversal] internal/cli/migrate.go:716 — readTreeFile discards errors from filepath.Abs() on both baseDir and the joined full path (lines 716–717). If filepath.Abs fails (requires os.Getwd failure — extremely unlikely), the strings.HasPrefix check could pass vacuously. Defense-in-depth gap; walkCustomized already filters .. paths.

• [function-structure] internal/cli/migrate.go — buildModifiedAgentFiles mutates the cfg parameter as a side effect (registering agents and updating allowed_remote_resources). The godoc documents this behavior, but returning the modifications for the caller to apply would be more consistent with other CLI helpers.

• [error-wrapping] internal/cli/migrate.go:270 — Error from loadAgentConfig is returned bare without context wrapping.

• [printer-usage] internal/cli/migrate.go:276 — Uses StepDone for informational messages ("No customized/ directory found") when nothing actually happened. StepInfo would be more semantically appropriate, though the codebase is inconsistent on this pattern.

---

Labels: PR adds Go CLI command and harness diff engine alongside documentation updates.

Previous run (7)

Review

Findings

Medium

• [logic-error] internal/cli/migrate.go:237 — For per-repo mode, customizedBase is computed as filepath.Join(absDir, ".fullsend", "customized"). Since --fullsend-dir conventionally points to the .fullsend/ directory itself (as shown by the agent add example: --fullsend-dir .fullsend), this produces .fullsend/.fullsend/customized/ instead of the correct .fullsend/customized/. The per-repo test masks this by placing config.yaml at the temp dir root rather than under .fullsend/, creating a non-realistic layout where the double nesting coincidentally resolves to the manually created test directory.
  Remediation: For per-repo mode, use customizedBase = filepath.Join(absDir, "customized") (same as org mode), since absDir already points to the directory containing config.yaml. Update the per-repo tests to use a realistic directory layout.

Low

• [edge-case] internal/harness/diff.go:383 — diffForgeConfig continues processing subsequent fields (RunnerEnv, Env, ValidationLoop) after detecting a skill removal and adding a warning, producing a partial ForgeConfig alongside warnings. The partial result is ultimately discarded by DiffHarness (which nulls out Child when forge warnings exist), so there is no runtime impact. The control flow is fragile — a future caller of diffForgeConfig that checks fc != nil without also checking warnings would mishandle this case.

• [test-inadequate] internal/cli/migrate_test.go:999 — TestMigrateCustomizations_PerRepoMode_CreatesPR and TestMigrateCustomizations_PerRepoMode_DryRun use a directory structure where config.yaml is at the temp dir root while customized files are nested under .fullsend/customized/. This does not match the real per-repo layout where --fullsend-dir points to .fullsend/ and config.yaml is at .fullsend/config.yaml. See also: [logic-error] finding above.

• [path-traversal] internal/cli/migrate.go:681 — readTreeFile discards errors from filepath.Abs() on both baseDir and the joined full path (lines 681–682). If filepath.Abs fails (extremely unlikely — requires os.Getwd failure), the strings.HasPrefix check could pass vacuously. Practical risk is negligible, but discarding security-relevant errors is a defense-in-depth gap.

• [missing-command-documentation] docs/cli/README.md:26 — The CLI overview lists fullsend agent as managing add, list, update, remove subcommands but omits the new migrate-customizations subcommand. The detailed CLI tree in docs/guides/dev/cli-internals.md (part of this PR) does document it.

• [missing-command-documentation] docs/guides/getting-started/operations.md:74 — The operations guide's standalone commands table lists the four existing fullsend agent subcommands but does not document the new migrate-customizations command. This is a one-time migration utility rather than a recurring operational task, so low severity.

• [missing-command-documentation] docs/architecture.md:245 — The Agent Registry section documents CLI management as fullsend agent add/list/update/remove but omits the new migrate-customizations command.

• [missing-reference] docs/ADRs/0058-agent-registration.md:62 — ADR-0058 documents the fullsend agent CLI subcommand group as having add, list, update, remove operations but the new migrate-customizations subcommand is not mentioned. However, ADRs are point-in-time records — adding migrate-customizations (an ADR-0064 concern) to an already-accepted ADR-0058 would violate the immutability convention.

---

Labels: PR modifies 5 documentation files with deprecation notices alongside the CLI and harness changes.

Previous run (8)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:349 — diffForge only iterates over platforms present in child, so it never detects when the child removes a forge platform that exists in base. After base: composition, mergeForgeBlocks (compose.go:1163-1169) inherits base platforms not present in the child, meaning the migrated harness would silently re-acquire a forge platform the customized version intentionally removed. The composition model supports explicit removal via a nil-valued key (compose.go:1171), but the diff engine does not produce nil entries for removed platforms.
  Remediation: After iterating child platforms, iterate base platforms and check if any are missing from child. For each missing base platform, either emit a warning (like other removal cases) or emit a nil-valued entry in the diff map so the child explicitly nulls it out during composition.

Low

• [architectural-coherence] internal/harness/diff.go — The harness diff engine lives in internal/harness alongside the composition logic it mirrors. The diff functions operate on harness.Harness structs and intimately understand composition semantics. While the placement is architecturally sound (avoids exporting internals), consider whether the diffing subsystem has long-term utility beyond migration or should be documented as migration-only.

• [scope-creep] internal/cli/migrate.go — ADR-0064 prescribes deprecation of customized/ overlays but does not explicitly describe an automated migration command. The PR introduces the command as migration tooling for the deprecation. A brief mention in ADR-0064 would document the migration path.

• [package-comment-missing] internal/harness/diff.go — diff.go adds 431 lines of harness diffing capability but lacks a file-level comment explaining the diffing subsystem's purpose and relationship to base composition.

• [edge-case] internal/harness/diff.go:383 — diffForgeConfig continues processing subsequent fields after detecting a removal, producing a partial ForgeConfig alongside warnings. The top-level DiffHarness aborts on any warnings so the partial result is unused, but the function contract is misleading.

• [test-inadequate] internal/harness/diff_test.go — No test for forge platform removal. A test exercising base with github + gitlab and child with only github would reveal the diffForge bug in finding docs: Add agent-compatible code problem document #1.

• [path-traversal] internal/cli/migrate.go:681 — readTreeFile includes path-traversal validation via strings.HasPrefix, but errors from filepath.Abs() are silently discarded. Negligible practical risk.

Previous run (9)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:252 — diffStringMap (used for RunnerEnv and Env sub-maps) silently drops map key removals. If a user's customized harness deliberately removed env vars present in the upstream base, the migration produces a diff that omits those removals. After base: composition, the removed keys will reappear from the base. Unlike slice diffing (diffStringSlice, diffHostFiles, diffAPIServers) which detects removals and emits a warning/abort, map key removals produce no warning. This can cause silent behavior changes after migration.
  Remediation: Add removal detection to diffStringMap (check for keys in base but not in child). When removals are detected, emit a warning analogous to the slice removal warnings so the user is informed that the migration cannot fully represent their customization.

• [missing-authorization] — PR feat(cli): add migrate-customizations command #2954 has no linked issue. The PR adds 2300+ lines implementing a fullsend admin migrate-customizations command and harness diffing infrastructure. This is substantial new functionality that warrants a tracking issue.
  Remediation: File a tracking issue that authorizes this migration command feature.

• [scope-creep] internal/cli/migrate.go — The migration command (542 lines) and harness diffing infrastructure (405 lines) represent a significant architectural addition. ADR-0064 prescribes deprecation of customized/ overlays but does not explicitly describe an automated migration command. This PR introduces automated conversion that arguably warrants its own ADR or an update to ADR-0064.
  Remediation: Update ADR-0064 to document the automated migration path, or write a focused ADR for the migration tooling.

• [stale-doc] docs/guides/user/building-custom-agents.md:16 — Guide's architecture overview and all instructions are built on the deprecated customized/ directory overlay. The guide teaches users to create .fullsend/customized/ subdirectories. No deprecation notice appears in this 550-line guide.
  Remediation: Add a prominent deprecation notice at the top of the guide directing users to use base: composition and config-driven agent registration.

• [stale-doc] docs/guides/user/customizing-with-skills.md:120 — The "Overriding built-in skills" section instructs users to use customized/ overlay with explicit reference to deprecated ADR-0035. Teaches creating customized/skills/ directories without mentioning the deprecation.
  Remediation: Add deprecation notice referencing ADR-0064 and migrate-customizations command.

• [stale-doc] docs/guides/user/customizing-agents.md:150 — The "How Override Resolution Works" section provides detailed instructions on using the deprecated customized/ pattern. While a deprecation notice was added at line ~97, this later section continues teaching the deprecated approach without its own warning.
  Remediation: Add a deprecation callout at the start of the "How Override Resolution Works" subsection.

Low

• [architectural-coherence] internal/harness/diff.go — The harness diff implementation introduces substantial new internal/harness package surface area tightly coupled to the migration command's use case. If it's one-time migration scaffolding, consider co-locating with the migration command.

• [intent-tier-mismatch] — The PR body describes implementing ADR-0058 migration, but ADR-0058 does NOT authorize a migration command. The migration work implements ADR-0064's deprecation. Update the PR body to reference ADR-0064 as the primary authorization.

• [architectural-coherence] internal/forge/forge.go — Addition of Delete field to forge.TreeFile is a structural change to a core abstraction. The implementation is correct and well-documented, but the rationale for embedding delete semantics in TreeFile (vs. a separate API) could be documented.

• [path-traversal] internal/cli/migrate.go:608 — readTreeFile joins baseDir with relPath using filepath.Join without independent path-traversal validation. All current callers pass relPath from walkCustomized which filters .. and symlinks, so not exploitable through existing call chain. Defense-in-depth gap.

• [data-exposure] internal/cli/migrate.go:613 — readTreeFile reads any file under customized/ and includes content in PR commit without checking for sensitive file patterns (.env, credentials). User explicitly runs this command and PR targets their own repo.

• [test-inadequate] internal/forge/fake.go:571 — FakeClient.CommitFilesToBranch does not handle TreeFile.Delete. When Delete: true, the fake stores nil Content instead of removing the entry from FileContents map. Current PR tests avoid this by inspecting TreeFile slices directly, but future tests may be misled.

• [test-inadequate] internal/cli/migrate_test.go:596 — TestMigrateCustomizations_ModifiedAgent_CreatesPR validates composition harness contains base: and URL, but doesn't verify diff content (e.g., that model: sonnet appears). A bug in DiffHarness silently dropping changes would not be caught by this integration test.

• [stale-doc] docs/guides/user/customizing-agents.md:210 — The "Customizing Pre-commit Tool Dependencies" section references customized/scripts/.pre-commit-tools.yaml without deprecation notice.

• [stale-doc] docs/guides/user/running-agents-locally.md:279 — Instructions include copying customized/ directories from org config repo, assuming the deprecated overlay mechanism.

• [incomplete-doc] docs/guides/user/customizing-agents.md:34 — Deprecation notice added in this PR references migrate-customizations command and base: composition but provides no explanation of what base: composition is or how to use it.

Previous run (10)

Review

Findings

High

• [logic-error] internal/cli/migrate.go:289 — In per-repo mode (cfg.isOrg == false), the updated config.yaml is written to the git tree at path "config.yaml" (repo root). However, the per-repo install convention (confirmed in internal/cli/github.go:239, internal/cli/admin.go:683, and internal/config/config.go:406) writes config to .fullsend/config.yaml. The migration PR will create a stray config.yaml at the repo root instead of updating the actual per-repo config.
  Remediation: Compute the config path conditionally: use ".fullsend/config.yaml" when !cfg.isOrg, and "config.yaml" otherwise.

• [logic-error] internal/cli/migrate.go:525 — In per-repo mode, buildModifiedAgentFiles writes the composition harness to "harness/" + m.name + ".yaml" (repo root). Per-repo harness files should live under .fullsend/harness/. Similarly, readTreeFile returns relPath as the tree file path without .fullsend/ prefix. The migrateCustom branch at line 247 has the same issue. All moved files land at repo root instead of under .fullsend/ in per-repo mode.
  Remediation: Thread a destPrefix (empty for org mode, ".fullsend/" for per-repo mode) through buildModifiedAgentFiles, readTreeFile callers, and the migrateCustom branch. Prepend it to all non-delete tree file paths and to the AgentEntry.Source value.

Medium

• [logic-error] internal/harness/diff.go:332 — diffForgeConfig silently discards the removed return value from diffStringSlice for forge skills: extras, _ := diffStringSlice(base.Skills, child.Skills, nil). If the child's forge config removes a skill present in the base, the removal is silently dropped. This is inconsistent with the top-level DiffHarness which checks removed and aborts with a warning for skills, plugins, and providers.
  Remediation: Check the removed return value and either return nil with a warning (matching the top-level behavior) or propagate a warning through the DiffResult.

• [stale-doc] docs/guides/user/building-custom-agents.md:16 — The guide instructs users to create custom agents using the deprecated .fullsend/customized/ directory structure without mentioning the deprecation from ADR-0064, the base: composition mechanism, or the migrate-customizations command. The customized/ approach still functions but is deprecated.
  Remediation: Add a deprecation notice at the top of the guide pointing to ADR-0064 and the base: composition approach.

• [stale-doc] docs/guides/user/customizing-with-skills.md:120 — The section "Overriding built-in skills" instructs users to use the deprecated customized/skills/ overlay mechanism, contradicting the deprecation notice already added to customizing-agents.md.
  Remediation: Add a deprecation notice to the "Overriding built-in skills" section.

Low

• [test-inadequate] internal/cli/migrate_test.go:900 — TestMigrateCustomizations_PerRepoMode_CreatesPR validates tree file paths without the .fullsend/ prefix (asserts pathMap["harness/my-agent.yaml"] and pathMap["config.yaml"]), confirming the buggy per-repo path behavior from the high-severity findings above.

• [path-traversal] internal/cli/migrate.go:569 — readTreeFile joins baseDir with relPath using filepath.Join but performs no independent path traversal validation. Its only caller passes relPath values from walkCustomized, which filters .. and symlinks, so current usage is safe. Defense-in-depth would add an independent guard.

• [stale-doc] docs/guides/user/customizing-agents.md:152 — The section "How Override Resolution Works" teaches the deprecated customized/harness/ directory pattern. The deprecation notice at line 35 covers the same document, but this section actively teaches the deprecated approach without a cross-reference.

• [stale-doc] docs/guides/user/running-agents-locally.md:279 — The section "Simulating Fullsend's real customization layers" instructs users to copy customized/ directories to simulate the deprecated overlay mechanism. The mechanism still functions, so this is incomplete rather than incorrect.

• [incomplete-doc] docs/guides/user/customizing-agents.md:35 — The deprecation notice references the migrate-customizations command but does not explain what base: composition is or provide any examples. The ADR itself should contain the detailed explanation.

• [missing-reference] docs/guides/user/customizing-agents.md:210 — The section on pre-commit tool dependencies mentions the deprecated customized/scripts/.pre-commit-tools.yaml without a deprecation notice.

Previous run (11)

Review

Findings

High

• [logic-error] internal/cli/migrate.go:289 — In per-repo mode (cfg.isOrg == false), the updated config.yaml is written to the git tree at path "config.yaml" (repo root). However, the per-repo install convention (admin.go line 683) writes config to .fullsend/config.yaml. The migration PR will create a stray config.yaml at the repo root instead of updating the actual per-repo config.
  Remediation: Compute the config tree path based on cfg.isOrg: use "config.yaml" for org mode and ".fullsend/config.yaml" for per-repo mode.

• [logic-error] internal/cli/migrate.go:525 — In per-repo mode, buildModifiedAgentFiles writes the composition harness to "harness/" + m.name + ".yaml" (repo root). Per-repo harness files live under .fullsend/harness/. Similarly, readTreeFile returns relPath as the tree file path without .fullsend/ prefix. The migrateCustom case (line 237) and standalone file handling also use readTreeFile directly. All moved files land at repo root instead of under .fullsend/ in per-repo mode.
  Remediation: Compute a destPrefix (empty for org mode, ".fullsend/" for per-repo mode) and prepend it to all destination tree file paths.

Medium

• [logic-error] internal/harness/diff.go:332 — diffForgeConfig silently discards the removed return value from diffStringSlice for forge skills: extras, _ := diffStringSlice(base.Skills, child.Skills, nil). If the child's forge config removes a skill present in the base, the removal is silently dropped. Since mergeForgeConfigInto concatenates base + child forge skills, the composed harness will include the removed skill. Compare to the top-level DiffHarness which checks removed and aborts with a warning.
  Remediation: Check the removed return value and either emit a warning in DiffResult.Warnings or abort the diff for that forge platform, matching the top-level slice diff behavior.

Low

• [path-traversal] internal/cli/migrate.go:569 — readTreeFile joins baseDir with relPath using filepath.Join but performs no independent path traversal validation. Its only caller passes relPath values from walkCustomized, which filters .. and symlinks, so current usage is safe. Defense-in-depth would add an independent guard.

• [test-inadequate] internal/cli/migrate_test.go:900 — TestMigrateCustomizations_PerRepoMode_CreatesPR validates tree file paths without the .fullsend/ prefix (e.g., asserts pathMap["harness/my-agent.yaml"] and pathMap["config.yaml"]). The test confirms the buggy per-repo path behavior from the high-severity findings above.

• [test-inadequate] internal/cli/migrate_test.go:1157 — TestMigrateCustomizations_ModifiedAgent_CreatesPR verifies the composition harness contains base: and a raw.githubusercontent.com URL, but does not verify that the actual diff content is correct (e.g., that the changed model field appears in the output harness). A malformed diff could pass this test.

• [cross-repo-contracts] internal/forge/fake.go:544 — FakeClient.CommitFiles and CommitFilesToBranch do not handle the new TreeFile.Delete field. When Delete=true, the fake still adds Content to FileContents map instead of removing the path. The real GitHub client correctly handles deletion. This only affects test fidelity.

• [data-exposure] internal/cli/migrate.go:573 — readTreeFile reads file content from customized/ and uploads it via the forge API without checking for sensitive file patterns (.env, credentials). Risk is minimal since these files are already in the repo's git history.

• [naming-convention] internal/cli/migrate.go:52 — newMigrateCustomizationsCmd() is more verbose than established patterns like newForeignCmd(), newEnableCmd(). The verbosity is justified by the specificity of the operation but deviates from the terse naming convention.

Previous run (12)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:213 — diffStringSlice returns items present in both base and child as extras when customizedFiles[s] is true (line: !baseSet[s] || customizedFiles[s]). Since mergeBaseIntoChild concatenates base + child skills/plugins/providers without deduplication, a customized skill that already exists in the base will appear twice in the composed result. For example, if base has ["skills/a", "skills/b"] and child has the same but skills/b is customized, DiffHarness produces child skills ["skills/b"], and composition yields ["skills/a", "skills/b", "skills/b"].
  Remediation: Either deduplicate during composition (in mergeBaseIntoChild), or don't include customized-but-matching items as extras in diffStringSlice for skills/plugins/providers slices (file-path override semantics only apply meaningfully to scalar fields, not concatenated slices).

• [missing-reference] docs/guides/user/customizing-agents.md:91 — The customizing-agents.md guide extensively documents the customized/ directory overlay mechanism but does not mention that this is deprecated per ADR-0064 or reference the migration tool. Users reading this guide will continue to use the deprecated pattern.
  Remediation: Add a deprecation notice at the top of the "Layered Configuration Resolution" section pointing to ADR-0064 and the fullsend admin migrate-customizations command.

Low

• [logic-error] internal/harness/diff.go:332 — diffForgeConfig silently discards the removed return value from diffStringSlice for forge skills: extras, _ := diffStringSlice(base.Skills, child.Skills, nil). If the child forge config removes skills present in the base forge config, the removal is silently ignored and the removed skills reappear after composition. Unlike the top-level skills/plugins/providers handling which aborts with a warning on removal, forge skill removal produces no diagnostic.

• [test-inadequate] internal/cli/migrate_test.go:632 — TestMigrateCustomizations_ModifiedAgent_CreatesPR only verifies that a base: URL is present in the output and the config is updated, but does not verify the diff content itself (e.g., that only the changed model field appears in the output harness). It also does not test the allowed_remote_resources allowlist update that buildModifiedAgentFiles performs.

• [migration-guide-missing] docs/ADRs/0064-deprecate-customized-directory-overlay.md:91 — ADR-0064 tells users they "must migrate" but no user-facing migration guide exists yet. The command has --help and --dry-run, and the deprecation plan sequences documentation as a follow-up PR, but users arriving at the ADR have no clear path to the tool.

• [missing-authorization] — PR has no linked GitHub issue. This is a ~2100-line feature PR implementing a migration command referenced in ADR-0064 and the deprecation plan, but lacks formal authorization via an issue.

Previous run (13)

Review

Findings

High

• [logic-error] internal/harness/diff.go:289 — DiffHarness treats AllowRuntimeFetch, MaxRuntimeFetches, and AllowedRemoteResources as normal diffable fields, but mergeBaseIntoChild (compose.go:495-498) explicitly does NOT merge these from base to child (security: prevent privilege escalation). When both base and customized harness have AllowRuntimeFetch=true, DiffHarness omits it from the diff (values are equal). At composition time, mergeBaseIntoChild won't copy it from base either, so the composed harness will have AllowRuntimeFetch=false. This silently disables runtime skill fetching for migrated agents. Same issue applies to MaxRuntimeFetches (when both match) and AllowedRemoteResources (entirely missing from DiffHarness).
  Remediation: For fields not merged by mergeBaseIntoChild (AllowRuntimeFetch, MaxRuntimeFetches, AllowedRemoteResources), always include non-zero values from the customized child in the diff output, regardless of whether they match the base.

Medium

• [path-traversal] internal/cli/migrate.go:283 — walkCustomized uses filepath.Walk which follows symlinks by default. A symlink inside customized/ pointing outside the fullsend directory would be followed, and the target file's content would be read via os.ReadFile and uploaded to the forge API as a TreeFile. The codebase has an established containedLocalPath pattern (in run.go) that resolves symlinks and checks containment, but it is not used here.
  Remediation: Either use filepath.WalkDir and skip symlinks (info.Type()&fs.ModeSymlink != 0), or resolve paths with filepath.EvalSymlinks and verify containment similar to containedLocalPath in run.go.

• [migration-guide-missing] docs/ADRs/0064-deprecate-customized-directory-overlay.md:91 — ADR-0064 tells users they "must migrate" from customized/ directories but no migration guide exists. The PR adds the migrate-customizations command but does not document how to use it. While the command has --help text and the deprecation plan has a separate PR for documentation (PR4), shipping the tool without basic usage documentation creates a gap.
  Remediation: Add a migration guide documenting command usage, flags (--fullsend-dir, --repo, --dry-run), migration paths (dead/custom/modified), and examples for both per-org and per-repo modes.

• [stale-doc] docs/guides/dev/cli-internals.md:9 — The admin command tree does not include the new migrate-customizations subcommand added in this PR.
  Remediation: Add migrate-customizations to the CLI command tree with flag documentation.

Low

• [test-inadequate] internal/harness/diff_test.go:243 — No tests cover DiffHarness behavior for AllowRuntimeFetch, MaxRuntimeFetches, or AllowedRemoteResources — fields with special merge semantics requiring different diffing logic.

• [test-inadequate] internal/cli/migrate_test.go:632 — The migrateModified code path (buildModifiedAgentFiles) has limited test coverage: no test exercises the agents-repo URL pinning path, DiffHarness warning/nil handling, or error paths when commitSHA is "dev".

• [path-traversal] internal/cli/migrate.go:197 — No validation that relative paths from walkCustomized don't contain .. components before being used as destination paths in the Git Trees API.

• [data-exposure] internal/cli/migrate.go:225 — Standalone files are read and uploaded without content inspection. No warning for potentially sensitive file patterns (.env, credentials).

• [missing-authorization] — PR has no linked GitHub issue. This is a non-trivial feature referencing ADR-0058 and ADR-0064 but has no formal issue for work tracking.

• [incomplete-deprecation-path] docs/plans/deprecate-customized-directory-overlay.md:218 — The implementation plan's PR4 says to "Add migration guidance" but this PR implements the command before that phase. Plan not updated to reflect sequencing.

Previous run (14)

Review

Findings

High

• [logic-error] internal/harness/diff.go:289 — DiffHarness treats AllowRuntimeFetch, MaxRuntimeFetches, and AllowedRemoteResources as normal diffable fields, but mergeBaseIntoChild (compose.go:495-498) explicitly does NOT merge these from base to child (security: prevent privilege escalation). When both base and customized harness have AllowRuntimeFetch=true, DiffHarness omits it from the diff (values are equal). At composition time, mergeBaseIntoChild won't copy it from base either, so the composed harness will have AllowRuntimeFetch=false. This silently disables runtime skill fetching for migrated agents. Same issue applies to MaxRuntimeFetches (when both match) and AllowedRemoteResources (entirely missing from DiffHarness).
  Remediation: For fields not merged by mergeBaseIntoChild (AllowRuntimeFetch, MaxRuntimeFetches, AllowedRemoteResources), always include non-zero values from the customized child in the diff output, regardless of whether they match the base.

Medium

• [path-traversal] internal/cli/migrate.go:283 — walkCustomized uses filepath.Walk which follows symlinks by default. A symlink inside customized/ pointing outside the fullsend directory would be followed, and the target file's content would be read via os.ReadFile and uploaded to the forge API as a TreeFile. The codebase has an established containedLocalPath pattern (in run.go) that resolves symlinks and checks containment, but it is not used here.
  Remediation: Either use filepath.WalkDir and skip symlinks (info.Type()&fs.ModeSymlink != 0), or resolve paths with filepath.EvalSymlinks and verify containment similar to containedLocalPath in run.go.

• [migration-guide-missing] docs/ADRs/0064-deprecate-customized-directory-overlay.md:91 — ADR-0064 tells users they "must migrate" from customized/ directories but no migration guide exists. The PR adds the migrate-customizations command but does not document how to use it. While the command has --help text and the deprecation plan has a separate PR for documentation (PR4), shipping the tool without basic usage documentation creates a gap.
  Remediation: Add a migration guide documenting command usage, flags (--fullsend-dir, --repo, --dry-run), migration paths (dead/custom/modified), and examples for both per-org and per-repo modes.

• [stale-doc] docs/guides/dev/cli-internals.md:9 — The admin command tree does not include the new migrate-customizations subcommand added in this PR.
  Remediation: Add migrate-customizations to the CLI command tree with flag documentation.

Low

• [test-inadequate] internal/harness/diff_test.go:243 — No tests cover DiffHarness behavior for AllowRuntimeFetch, MaxRuntimeFetches, or AllowedRemoteResources — fields with special merge semantics requiring different diffing logic.

• [test-inadequate] internal/cli/migrate_test.go:632 — The migrateModified code path (buildModifiedAgentFiles) has limited test coverage: no test exercises the agents-repo URL pinning path, DiffHarness warning/nil handling, or error paths when commitSHA is "dev".

• [path-traversal] internal/cli/migrate.go:197 — No validation that relative paths from walkCustomized don't contain .. components before being used as destination paths in the Git Trees API.

• [data-exposure] internal/cli/migrate.go:225 — Standalone files are read and uploaded without content inspection. No warning for potentially sensitive file patterns (.env, credentials).

• [missing-authorization] — PR has no linked GitHub issue. This is a non-trivial feature referencing ADR-0058 and ADR-0064 but has no formal issue for work tracking.

• [incomplete-deprecation-path] docs/plans/deprecate-customized-directory-overlay.md:218 — The implementation plan's PR4 says to "Add migration guidance" but this PR implements the command before that phase. Plan not updated to reflect sequencing.

Previous run (15)

Review

Findings

High

• [migration-guide-missing] docs/ADRs/0064-deprecate-customized-directory-overlay.md:91 — ADR-0064 tells users they "must migrate" from customized/ to base: composition, but no documentation explains how to use the new migrate-customizations command. Shipping a migration tool without a migration guide leaves users without actionable instructions.
  Remediation: Add a migration guide (e.g., docs/guides/admin/migrate-customizations.md) that walks users through the command, or update the deprecation plan to reference the command with usage examples and expected outcomes.

Medium

• [logic-error] internal/cli/migrate.go:145 — printer.StepDone(...) is called unconditionally outside the if !dryRun block in all three migration branches (migrateDead at line 145, migrateCustom at line 161, migrateModified at line 177). In dry-run mode, users see completion messages like "Removed dead override for ..." and "Registered custom agent ..." even though no files were modified. This contradicts the --dry-run contract and could lead users to believe the migration already happened.
  Remediation: Move each StepDone call inside the if !dryRun block, or use StepInfo("Would remove/register/migrate ...") for dry-run mode (matching the pattern in moveStandaloneFiles).

• [edge-case] internal/cli/migrate.go:338 — moveFile hardcodes 0o644 permissions via os.WriteFile(dst, data, 0o644), discarding the source file's permission bits. Files associated with agents via pre-, post-, and validate-output- prefixes are typically executable scripts referenced by harness pre_script/post_script fields. After migration, these scripts lose their executable bit and will fail at runtime with permission denied errors.
  Remediation: Read the source file's mode with os.Stat before reading content, then pass the original mode to os.WriteFile.

• [stale-doc] docs/guides/dev/cli-internals.md:9 — The admin command tree lists install, uninstall, analyze, enable, disable, and foreign but does not include the new migrate-customizations subcommand. The file also describes internal/cli/admin.go capabilities without mentioning migration.
  Remediation: Add migrate-customizations to the admin command tree and the admin.go capabilities summary in cli-internals.md.

Low

• [edge-case] internal/cli/migrate.go:328 — moveFile writes to the destination without checking if a file already exists, silently overwriting. While overwriting is the expected semantic for migration (customized files override regular ones), a warning would prevent surprise data loss when users have manually placed files at the destination path.

• [test-inadequate] internal/cli/migrate_test.go — The migrateModifiedAgent code path — the most complex branch involving DiffHarness, scaffold content loading, forge client fallback, base URL generation, composition harness writing, and config allowlist updates — has no test coverage. The other two paths (migrateDead, migrateCustom) are well-tested.

• [path-traversal] internal/cli/migrate.go — moveFile() and os.Remove() operate on paths derived from filepath.Walk without symlink resolution or containment checks. The codebase has established patterns for path containment (containedLocalPath in run.go). While the risk is low (paths come from admin-controlled directories, not user input), following existing security patterns improves defense in depth.

• [error-handling] internal/cli/migrate.go:341 — moveFile is not atomic: if os.WriteFile succeeds but os.Remove(src) fails, the file exists in both locations. Re-running migration would attempt to move it again, potentially overwriting edits made to the destination.

---

Labels: PR adds a new admin CLI command and harness diff utility

Previous run (16)

Review

Findings

High

• [logic-error] internal/cli/migrate.go:289 — In per-repo mode (cfg.isOrg == false), the updated config.yaml is written to the git tree at path "config.yaml" (repo root). However, the per-repo install convention (admin.go line 683) writes config to .fullsend/config.yaml. The migration PR will create a stray config.yaml at the repo root instead of updating the actual per-repo config.
  Remediation: Compute the config tree path based on cfg.isOrg: use "config.yaml" for org mode and ".fullsend/config.yaml" for per-repo mode.

• [logic-error] internal/cli/migrate.go:525 — In per-repo mode, buildModifiedAgentFiles writes the composition harness to "harness/" + m.name + ".yaml" (repo root). Per-repo harness files live under .fullsend/harness/. Similarly, readTreeFile returns relPath as the tree file path without .fullsend/ prefix. The migrateCustom case (line 237) and standalone file handling also use readTreeFile directly. All moved files land at repo root instead of under .fullsend/ in per-repo mode.
  Remediation: Compute a destPrefix (empty for org mode, ".fullsend/" for per-repo mode) and prepend it to all destination tree file paths.

Medium

• [logic-error] internal/harness/diff.go:332 — diffForgeConfig silently discards the removed return value from diffStringSlice for forge skills: extras, _ := diffStringSlice(base.Skills, child.Skills, nil). If the child's forge config removes a skill present in the base, the removal is silently dropped. Since mergeForgeConfigInto concatenates base + child forge skills, the composed harness will include the removed skill. Compare to the top-level DiffHarness which checks removed and aborts with a warning.
  Remediation: Check the removed return value and either emit a warning in DiffResult.Warnings or abort the diff for that forge platform, matching the top-level slice diff behavior.

Low

• [path-traversal] internal/cli/migrate.go:569 — readTreeFile joins baseDir with relPath using filepath.Join but performs no independent path traversal validation. Its only caller passes relPath values from walkCustomized, which filters .. and symlinks, so current usage is safe. Defense-in-depth would add an independent guard.

• [test-inadequate] internal/cli/migrate_test.go:900 — TestMigrateCustomizations_PerRepoMode_CreatesPR validates tree file paths without the .fullsend/ prefix (e.g., asserts pathMap["harness/my-agent.yaml"] and pathMap["config.yaml"]). The test confirms the buggy per-repo path behavior from the high-severity findings above.

• [test-inadequate] internal/cli/migrate_test.go:1157 — TestMigrateCustomizations_ModifiedAgent_CreatesPR verifies the composition harness contains base: and a raw.githubusercontent.com URL, but does not verify that the actual diff content is correct (e.g., that the changed model field appears in the output harness). A malformed diff could pass this test.

• [cross-repo-contracts] internal/forge/fake.go:544 — FakeClient.CommitFiles and CommitFilesToBranch do not handle the new TreeFile.Delete field. When Delete=true, the fake still adds Content to FileContents map instead of removing the path. The real GitHub client correctly handles deletion. This only affects test fidelity.

• [data-exposure] internal/cli/migrate.go:573 — readTreeFile reads file content from customized/ and uploads it via the forge API without checking for sensitive file patterns (.env, credentials). Risk is minimal since these files are already in the repo's git history.

• [naming-convention] internal/cli/migrate.go:52 — newMigrateCustomizationsCmd() is more verbose than established patterns like newForeignCmd(), newEnableCmd(). The verbosity is justified by the specificity of the operation but deviates from the terse naming convention.

Previous run (17)

Review

Findings

Medium

• [logic-error] internal/harness/diff.go:213 — diffStringSlice returns items present in both base and child as extras when customizedFiles[s] is true (line: !baseSet[s] || customizedFiles[s]). Since mergeBaseIntoChild concatenates base + child skills/plugins/providers without deduplication, a customized skill that already exists in the base will appear twice in the composed result. For example, if base has ["skills/a", "skills/b"] and child has the same but skills/b is customized, DiffHarness produces child skills ["skills/b"], and composition yields ["skills/a", "skills/b", "skills/b"].
  Remediation: Either deduplicate during composition (in mergeBaseIntoChild), or don't include customized-but-matching items as extras in diffStringSlice for skills/plugins/providers slices (file-path override semantics only apply meaningfully to scalar fields, not concatenated slices).

• [missing-reference] docs/guides/user/customizing-agents.md:91 — The customizing-agents.md guide extensively documents the customized/ directory overlay mechanism but does not mention that this is deprecated per ADR-0064 or reference the migration tool. Users reading this guide will continue to use the deprecated pattern.
  Remediation: Add a deprecation notice at the top of the "Layered Configuration Resolution" section pointing to ADR-0064 and the fullsend admin migrate-customizations command.

Low

• [logic-error] internal/harness/diff.go:332 — diffForgeConfig silently discards the removed return value from diffStringSlice for forge skills: extras, _ := diffStringSlice(base.Skills, child.Skills, nil). If the child forge config removes skills present in the base forge config, the removal is silently ignored and the removed skills reappear after composition. Unlike the top-level skills/plugins/providers handling which aborts with a warning on removal, forge skill removal produces no diagnostic.

• [test-inadequate] internal/cli/migrate_test.go:632 — TestMigrateCustomizations_ModifiedAgent_CreatesPR only verifies that a base: URL is present in the output and the config is updated, but does not verify the diff content itself (e.g., that only the changed model field appears in the output harness). It also does not test the allowed_remote_resources allowlist update that buildModifiedAgentFiles performs.

• [migration-guide-missing] docs/ADRs/0064-deprecate-customized-directory-overlay.md:91 — ADR-0064 tells users they "must migrate" but no user-facing migration guide exists yet. The command has --help and --dry-run, and the deprecation plan sequences documentation as a follow-up PR, but users arriving at the ADR have no clear path to the tool.

• [missing-authorization] — PR has no linked GitHub issue. This is a ~2100-line feature PR implementing a migration command referenced in ADR-0064 and the deprecation plan, but lacks formal authorization via an issue.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 5:42 PM UTC · Ended 5:49 PM UTC
Commit: 0a95cac · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:50 PM UTC · Completed 6:07 PM UTC
Commit: c9996a2 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:35 PM UTC · Ended 6:50 PM UTC
Commit: 0a95cac · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:50 PM UTC · Completed 7:18 PM UTC
Commit: 19e24d1 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

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

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:18 PM UTC · Ended 9:19 PM UTC
Commit: 0a95cac · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:20 PM UTC · Completed 9:34 PM UTC
Commit: 1b54776 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:47 PM UTC · Completed 10:00 PM UTC
Commit: fc679f5 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:04 PM UTC · Completed 10:18 PM UTC
Commit: 410a569 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:55 PM UTC · Completed 11:09 PM UTC
Commit: e619d04 · View workflow run →