From df9293b4a2423be606fb1201a73d840ff0ff8bd6 Mon Sep 17 00:00:00 2001 From: Greg Allen Date: Tue, 7 Jul 2026 20:34:33 -0400 Subject: [PATCH] feat(cli): add migrate-customizations command with PR creation Implements the `fullsend agent migrate-customizations` command per ADR-0064, converting customized/ directory overlays into config-driven agents with base: composition harnesses. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Claude Opus 4.6 Signed-off-by: Greg Allen --- AGENTS.md | 2 +- docs/ADRs/0033-per-repo-installation-mode.md | 2 +- .../0045-forge-portable-harness-schema.md | 5 + .../0056-per-repo-precommit-tools-registry.md | 2 +- docs/ADRs/0058-agent-registration.md | 3 +- ...-deprecate-customized-directory-overlay.md | 2 + docs/agents/review.md | 4 + docs/agents/triage.md | 4 + docs/architecture.md | 2 +- docs/cli/README.md | 2 +- docs/guides/dev/cli-internals.md | 32 +- docs/guides/getting-started/operations.md | 1 + docs/guides/user/building-custom-agents.md | 6 + docs/guides/user/customizing-agents.md | 30 +- .../guides/user/customizing-with-agents-md.md | 2 +- docs/guides/user/customizing-with-skills.md | 7 +- docs/guides/user/running-agents-locally.md | 5 + docs/plans/agent-registration.md | 6 + .../deprecate-customized-directory-overlay.md | 2 + docs/runtimes.md | 3 + internal/cli/agent.go | 1 + internal/cli/agent_test.go | 3 +- internal/cli/migrate.go | 648 ++++++++++ internal/cli/migrate_test.go | 1054 +++++++++++++++++ internal/forge/fake.go | 24 +- internal/forge/forge.go | 2 + internal/forge/github/github.go | 13 + internal/harness/diff.go | 455 +++++++ internal/harness/diff_test.go | 811 +++++++++++++ internal/scaffold/baseurl.go | 14 + internal/scaffold/baseurl_test.go | 31 + 31 files changed, 3152 insertions(+), 26 deletions(-) create mode 100644 internal/cli/migrate.go create mode 100644 internal/cli/migrate_test.go create mode 100644 internal/harness/diff.go create mode 100644 internal/harness/diff_test.go diff --git a/AGENTS.md b/AGENTS.md index 3ca9821021..0c1f996033 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -166,6 +166,6 @@ The term "tier" is used in multiple distinct contexts across this codebase. Alwa |---|---|---| | **credential delivery tier** | The four-tier model for how agents receive credentials: (1) prefetch + post-process, (2) providers + L7, (3) host-side REST server, (4) host files | [ADR 0025](docs/ADRs/0025-provider-credential-delivery-for-sandboxed-agents.md) | | **intent authorization tier** | The four-tier model for change authorization: (0) standing rules, (1) tactical/issue, (2) strategic, (3) organizational | [intent-representation.md](docs/problems/intent-representation.md) | -| **configuration tier** | The three-tier inheritance model for agent configuration: upstream defaults → org config → per-repo overrides | [ADR 0035](docs/ADRs/0035-layered-content-resolution.md) | +| **configuration tier** | The three-tier inheritance model for agent configuration: upstream defaults → org config → per-repo overrides. The `customized/` overlay mechanism (ADR-0035) is deprecated; use config-driven agent registration per [ADR 0064](docs/ADRs/0064-deprecate-customized-directory-overlay.md) | [ADR 0035](docs/ADRs/0035-layered-content-resolution.md) | **Do not** use bare "Tier N" or "tier" without a prefix — the same number means different things in different contexts (e.g., "Tier 2" could be provider-based credential delivery or strategic intent authorization). External tier references (e.g., "GitLab Free tier", "GitHub plan tiers") are exempt from this convention. diff --git a/docs/ADRs/0033-per-repo-installation-mode.md b/docs/ADRs/0033-per-repo-installation-mode.md index b62437772d..e6bd497776 100644 --- a/docs/ADRs/0033-per-repo-installation-mode.md +++ b/docs/ADRs/0033-per-repo-installation-mode.md @@ -137,7 +137,7 @@ Per-repo requirements: repo admin + org admin to install GitHub Apps on the repo target-repo/ ├── .github/workflows/fullsend.yml ← single workflow file (~70 lines, shim) ├── .fullsend/ ← in-repo config workspace (optional) -│ ├── customized/ ← user overrides (same convention as per-org) +│ ├── customized/ ← user overrides (deprecated by ADR-0064) │ │ ├── agents/ ← agent prompt overrides │ │ ├── harness/ ← harness config overrides │ │ ├── policies/ ← sandbox policies diff --git a/docs/ADRs/0045-forge-portable-harness-schema.md b/docs/ADRs/0045-forge-portable-harness-schema.md index 9358306bb8..e24be6ac5a 100644 --- a/docs/ADRs/0045-forge-portable-harness-schema.md +++ b/docs/ADRs/0045-forge-portable-harness-schema.md @@ -588,6 +588,11 @@ forge-specific artifact. The harness and agent definition are portable. removing an agent is deleting a file, adding one is creating a thin wrapper with `base:`. +- **Bidirectional composition.** The `base:` merge semantics have an + inverse (`DiffHarness`) used by [ADR 0064](0064-deprecate-customized-directory-overlay.md)'s + `migrate-customizations` command. Changes to merge rules must be + reflected in both directions. + - **Default URL allowlist for `base` composition.** `fullsend install` sets `allowed_remote_resources` in `config.yaml` to include the fullsend scaffold URL prefix diff --git a/docs/ADRs/0056-per-repo-precommit-tools-registry.md b/docs/ADRs/0056-per-repo-precommit-tools-registry.md index f2167db15f..262ea106c3 100644 --- a/docs/ADRs/0056-per-repo-precommit-tools-registry.md +++ b/docs/ADRs/0056-per-repo-precommit-tools-registry.md @@ -26,7 +26,7 @@ Related: [#1270](https://github.com/fullsend-ai/fullsend/issues/1270) PR #1055 introduced `.pre-commit-tools.yaml` — a registry mapping pre-commit hooks to the system tools they require. The registry can be fully replaced at the org level via `customized/scripts/` (L1 override, -[ADR 0035](0035-layered-content-resolution.md)), but repos needing one extra tool must copy the entire file. +[ADR 0035](0035-layered-content-resolution.md); `customized/` deprecated by [ADR 0064](0064-deprecate-customized-directory-overlay.md)), but repos needing one extra tool must copy the entire file. ## Decision diff --git a/docs/ADRs/0058-agent-registration.md b/docs/ADRs/0058-agent-registration.md index ee52c0b7c9..aef2d469c4 100644 --- a/docs/ADRs/0058-agent-registration.md +++ b/docs/ADRs/0058-agent-registration.md @@ -59,7 +59,8 @@ scaffold-discovered agents; collision is keyed by agent name (explicit favor of config, enabling gradual migration. Once all first-party agents are extracted, config becomes authoritative and the scaffold fallback is removed. -A `fullsend agent` CLI subcommand (`add`, `list`, `update`, `remove`) +A `fullsend agent` CLI subcommand (`add`, `list`, `update`, `remove`; +plus `migrate-customizations` per [ADR 0064](0064-deprecate-customized-directory-overlay.md)) manages entries (single-user CLI operations; no concurrency guard on config read/write) and auto-pins URLs to a commit SHA with an integrity hash. Per-repo config gains `allowed_remote_resources` so per-repo diff --git a/docs/ADRs/0064-deprecate-customized-directory-overlay.md b/docs/ADRs/0064-deprecate-customized-directory-overlay.md index 06a0bd9f9d..98c4fca468 100644 --- a/docs/ADRs/0064-deprecate-customized-directory-overlay.md +++ b/docs/ADRs/0064-deprecate-customized-directory-overlay.md @@ -90,6 +90,8 @@ implemented and in production. - Users who placed files in `customized/` must migrate to `base:` composition, URL references, or config-based registration. + `fullsend agent migrate-customizations` automates this conversion and + delivers the changes via pull request. - Deprecation warnings during install and updated documentation will guide migration. - The reusable workflows become simpler — no overlay loop, no diff --git a/docs/agents/review.md b/docs/agents/review.md index 37dead7371..b9a90260e6 100644 --- a/docs/agents/review.md +++ b/docs/agents/review.md @@ -68,6 +68,10 @@ You can also overload it at the org level in your `.fullsend` config repo at `customized/skills/issue-labels/SKILL.md`. At runtime, your version replaces the upstream default -- no other configuration needed. +> **Deprecated (ADR-0064):** The `customized/` overlay is deprecated. Use +> config-driven agent registration instead. Run `fullsend agent migrate-customizations` +> to migrate existing overrides. + See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and [Customizing with Skills](../guides/user/customizing-with-skills.md). diff --git a/docs/agents/triage.md b/docs/agents/triage.md index 42fdbbbfe4..cda473b12d 100644 --- a/docs/agents/triage.md +++ b/docs/agents/triage.md @@ -99,6 +99,10 @@ You can also overload it at the org level in your `.fullsend` config repo at `customized/skills/issue-labels/SKILL.md`. At runtime, your version replaces the upstream default — no other configuration needed. +> **Deprecated (ADR-0064):** The `customized/` overlay is deprecated. Use +> config-driven agent registration instead. Run `fullsend agent migrate-customizations` +> to migrate existing overrides. + Here's an example that encodes domain-specific labeling rules: ```markdown diff --git a/docs/architecture.md b/docs/architecture.md index c13b2b704d..db32650f50 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -252,7 +252,7 @@ Fullsend provides a base set of agent definitions. The adopting organization's * - Config-level agent registration: an `agents` list in both `OrgConfig` and `PerRepoConfig` declares agent harness sources as pinned URLs or local paths, replacing compiled-in agent discovery ([ADR 0058](ADRs/0058-agent-registration.md)). - Runtime resolution: `fullsend run ` resolves agents in three tiers: (1) config entries from `OrgConfig.Agents` (highest priority), (2) runtime fallback to the `fullsend-ai/agents` repository for known first-party agents not in config, (3) scaffold-embedded harnesses on disk. The agents-repo fallback is a transitional mechanism for the [agent extraction](plans/agent-extraction-to-agents-repo.md); it will be removed once all users have migrated to config-driven registration (ADR 0058 Phase 5). - Additive merge: config entries overlay scaffold-discovered agents (config wins on name collision), enabling gradual extraction of first-party agents without disrupting existing installations. Builds on [ADR 0045](ADRs/0045-forge-portable-harness-schema.md) harness identity model. -- CLI management: `fullsend agent add/list/update/remove` manages config entries and auto-pins URLs to a commit SHA with an integrity hash. +- CLI management: `fullsend agent add|list|update|remove|migrate-customizations` manages config entries and auto-pins URLs to a commit SHA with an integrity hash. **Open questions:** diff --git a/docs/cli/README.md b/docs/cli/README.md index 60e000d612..f980ef3cc7 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -23,7 +23,7 @@ Download the latest binary from [GitHub Releases](https://github.com/fullsend-ai | Command | Description | |---------|-------------| | `fullsend run` | Execute an agent locally in a sandbox. See [running agents locally](../guides/user/running-agents-locally.md). | -| `fullsend agent` | Manage agent registrations in config (add, list, update, remove) | +| `fullsend agent` | Manage agent registrations in config. Subcommands: `add`, `list`, `update`, `remove` (CRUD) and `migrate-customizations` (one-time migration of `customized/` overrides to config-driven agents per ADR-0064) | | `fullsend lock [agent-name]` | Pin remote dependencies to `lock.yaml` | | `fullsend scan` | Run security scanners on agent input/output | diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index 79c04618ea..849d354959 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -42,7 +42,11 @@ fullsend │ ├── add # Register an agent (URL auto-pinned) │ ├── list # List registered agents │ ├── update [sha] # Re-pin URL agent to new commit SHA -│ └── remove # Unregister agent from config +│ ├── remove # Unregister agent from config +│ └── migrate-customizations # Migrate customized/ → config agents +│ ├── --fullsend-dir # Base directory with .fullsend layout +│ ├── --repo # Target repo for migration PR +│ └── --dry-run # Preview changes without PR ├── lock [agent-name] # Pin remote deps to lock.yaml │ ├── --all # Lock all harnesses in the harness directory │ ├── --fullsend-dir # Base directory with .fullsend layout @@ -85,6 +89,28 @@ fullsend └── --role # Agent role for minting (required with --mint-url) ``` +### Migrate Customizations + +The `fullsend agent migrate-customizations` command converts `customized/` directory overlays (deprecated by [ADR-0064](../../ADRs/0064-deprecate-customized-directory-overlay.md)) into config-driven agents with `base:` composition harnesses. It scans the local `customized/` directory, classifies each override, and delivers changes via PR: + +```bash +# Preview what would change (no PR created) +fullsend agent migrate-customizations --fullsend-dir .fullsend --dry-run + +# Create a migration PR +fullsend agent migrate-customizations --fullsend-dir .fullsend --repo owner/repo +``` + +Migration actions per agent: + +| Override type | Detection | Action | +|---------------|-----------|--------| +| Dead | Agent already registered in config | Delete customized files | +| Custom | Not in upstream scaffold | Move files, register local path in config | +| Modified | Standard scaffold agent, not in config | Compute `base:` composition harness via `DiffHarness`, register in config | + +The diff engine (`internal/harness/diff.go`) computes the minimal child harness that reproduces the customized version when composed with the upstream base. It mirrors `mergeBaseIntoChild` semantics: scalar overrides, slice concatenation extras, map merge deltas, and security fields always included. + ### Command Decomposition The `mint`, `inference`, and `github` subcommands decompose setup into role-specific operations for organizations that separate GCP and GitHub responsibilities: @@ -186,7 +212,8 @@ Both per-org and per-repo modes share the same core pipeline. The code follows t │ ┌────────────────────────────────────────────────────────────┐ │ │ │ Phase 5: Write scaffold + config files │ │ │ │ │ │ -│ │ Both modes: write workflow files + customized/ dirs │ │ +│ │ Both modes: write workflow files (customized/ deprecated │ │ +│ │ by ADR-0064; use migrate-customizations to convert) │ │ │ │ CommitScaffoldFiles() delivery modes: │ │ │ │ Default (PR): create feature branch → commit → open PR │ │ │ │ --direct: try CommitFiles (default branch) │ │ @@ -549,6 +576,7 @@ var executableFiles = map[string]struct{}{ |------|-------|---------| | `internal/cli/root.go` | ~34 | CLI entry point, command registration | | `internal/cli/admin.go` | ~2415 | Install/uninstall/analyze/enable/disable | +| `internal/cli/migrate.go` | ~520 | Migrate customized/ overrides to config-driven agents | | `internal/cli/mint.go` | ~1022 | Mint deploy/enroll/unenroll/status | | `internal/cli/inference.go` | ~408 | Inference WIF provision/status | | `internal/cli/github.go` | ~966 | GitHub setup/set/status/uninstall/sync-scaffold/enroll/unenroll | diff --git a/docs/guides/getting-started/operations.md b/docs/guides/getting-started/operations.md index b6d4deafaa..917449f89b 100644 --- a/docs/guides/getting-started/operations.md +++ b/docs/guides/getting-started/operations.md @@ -75,6 +75,7 @@ For organizations that separate GCP and GitHub responsibilities across teams, fu | Developer | `fullsend agent list` | List registered agents and their sources | | Developer | `fullsend agent update [sha]` | Re-pin a URL agent to a new commit SHA | | Developer | `fullsend agent remove ` | Unregister an agent from config | +| Developer | `fullsend agent migrate-customizations` | Migrate `customized/` overlays to config-driven agents via PR | The typical handoff: a GCP admin runs `mint deploy` + `mint enroll` + `inference provision`, then passes the mint URL and WIF provider resource name to a GitHub maintainer who runs `github setup --mint-url=... --inference-wif-provider=...`. diff --git a/docs/guides/user/building-custom-agents.md b/docs/guides/user/building-custom-agents.md index 3d0c37cedf..981029a173 100644 --- a/docs/guides/user/building-custom-agents.md +++ b/docs/guides/user/building-custom-agents.md @@ -1,5 +1,11 @@ # Building custom agents +> **Deprecated:** This guide uses the `customized/` directory overlay, which is +> deprecated per [ADR-0064](../../ADRs/0064-deprecate-customized-directory-overlay.md). +> For new custom agents, register them in `config.yaml` with a local `source:` +> path instead. Run `fullsend agent migrate-customizations --dry-run` to +> preview migrating existing customizations. + This guide walks through creating a new custom agent from scratch on a per-repo fullsend installation. For customizing existing agents (overriding harnesses, skills, or policies), see [Customizing agents](customizing-agents.md). diff --git a/docs/guides/user/customizing-agents.md b/docs/guides/user/customizing-agents.md index ab9f605515..a2358396f9 100644 --- a/docs/guides/user/customizing-agents.md +++ b/docs/guides/user/customizing-agents.md @@ -95,11 +95,22 @@ security: # Security is enabled by default with fail_mode ## Layered Configuration Resolution +> **Deprecated:** The `customized/` directory overlay mechanism described +> below is deprecated per [ADR-0064](../../ADRs/0064-deprecate-customized-directory-overlay.md). +> Use `base:` composition instead: register agents in `config.yaml` with a +> `base:` URL pointing to the upstream harness, and override only the fields +> that differ. See [ADR-0045](../../ADRs/0045-forge-portable-harness-schema.md) +> for the composition model and [ADR-0058](../../ADRs/0058-agent-registration.md) +> for config-driven registration. +> Run `fullsend agent migrate-customizations --dry-run` to preview the +> migration, then `fullsend agent migrate-customizations --repo owner/repo` +> to apply it. + Fullsend uses a three-tier configuration inheritance model for all configuration: agent definitions, skills, policies, harness definitions, and guardrails. Each configuration tier can extend or override the one below it. ``` ┌──────────────────────────────────────────────────────────────┐ -│ Configuration Layering (ADR 0035) │ +│ Configuration Layering (ADR 0035, deprecated by ADR 0064) │ ├──────────────────────────────────────────────────────────────┤ │ │ │ Priority (highest wins): │ @@ -149,15 +160,18 @@ For per-repo mode, the same structure lives at `.fullsend/customized/` within th ### How Override Resolution Works +> **Deprecated:** This section describes the deprecated `customized/` overlay. +> See the [deprecation notice above](#layered-configuration-resolution). + **File-level replacement, not field-level merging.** When you place a file in `customized/harness/code.yaml`, it completely replaces the upstream `harness/code.yaml`. There is no YAML field merging. **Example: Adding a skill to the code agent** -To add a custom skill to the code agent's harness: +To add a custom skill to the code agent's harness (deprecated — use `base:` composition instead): 1. **Copy the full upstream harness** from `fullsend-ai/fullsend` to your customization directory: ```bash - # Get upstream harness + # ⚠ Deprecated: use `fullsend agent migrate-customizations` to convert to config-driven agents curl -o .fullsend/customized/harness/code.yaml \ https://raw.githubusercontent.com/fullsend-ai/fullsend/main/internal/scaffold/fullsend-repo/harness/code.yaml ``` @@ -190,6 +204,10 @@ To add a custom skill to the code agent's harness: ### Customizing Pre-commit Tool Dependencies +> **Note:** The `customized/scripts/.pre-commit-tools.yaml` L1 overlay path +> referenced below uses the deprecated `customized/` mechanism. +> The per-repo L2 path (`.pre-commit-tools.yaml` at repo root) is unaffected. + Fullsend auto-detects and installs tools required by a target repo's pre-commit hooks. The resolver reads `.pre-commit-config.yaml`, matches hooks against a tools registry, and installs missing dependencies before the authoritative pre-commit check runs. Only hooks that pre-commit **cannot self-serve** need registry entries: @@ -300,7 +318,8 @@ Each agent role has its own identity, permissions, and purpose: ### Adding a Custom Skill -Create `.fullsend/customized/skills/my-skill/SKILL.md` in your config repo: +Create `.fullsend/customized/skills/my-skill/SKILL.md` in your config repo +(deprecated — use config-driven agent registration instead): ```markdown # My Custom Skill @@ -316,7 +335,8 @@ The skill will be automatically available to all agents that include `skills/my- ### Overriding an Agent Definition -Create `.fullsend/customized/agents/code.md` to override the default code agent with org-specific instructions: +Create `.fullsend/customized/agents/code.md` to override the default code agent +with org-specific instructions (deprecated — use `base:` composition instead): ```markdown # Code Agent (Customized) diff --git a/docs/guides/user/customizing-with-agents-md.md b/docs/guides/user/customizing-with-agents-md.md index 30431caa2c..018f31e4b3 100644 --- a/docs/guides/user/customizing-with-agents-md.md +++ b/docs/guides/user/customizing-with-agents-md.md @@ -120,7 +120,7 @@ takes precedence. cannot write files regardless of what AGENTS.md says) - Remove or replace built-in skills — use [`customized/skills/`](customizing-with-skills.md#overriding-built-in-skills) - for that + for that (deprecated per ADR-0064; use config-driven agent registration instead) - Change the agent's model or execution parameters ### Injection handling diff --git a/docs/guides/user/customizing-with-skills.md b/docs/guides/user/customizing-with-skills.md index 50bab45a83..83e8e6ae5c 100644 --- a/docs/guides/user/customizing-with-skills.md +++ b/docs/guides/user/customizing-with-skills.md @@ -116,6 +116,10 @@ when available. ## Overriding built-in skills +> **Deprecated:** The `customized/` overlay described below is deprecated per +> [ADR-0064](../../ADRs/0064-deprecate-customized-directory-overlay.md). +> Use `base:` composition and config-driven agent registration instead. + To intentionally **replace** a built-in skill with your own version, use the `customized/` overlay ([ADR 0035](../../ADRs/0035-layered-content-resolution.md)). This replaces the skill at the config layer before the agent starts — the @@ -134,7 +138,8 @@ engine, not through project-level skill discovery. ### Built-in skills -These skills ship with fullsend and can be overridden via `customized/skills/`: +These skills ship with fullsend and can be overridden via `customized/skills/` +(deprecated per ADR-0064 — use config-driven agent registration instead): | Agent | Skill | Purpose | |-------|-------|---------| diff --git a/docs/guides/user/running-agents-locally.md b/docs/guides/user/running-agents-locally.md index 9056a3e819..7f2a3fb4e5 100644 --- a/docs/guides/user/running-agents-locally.md +++ b/docs/guides/user/running-agents-locally.md @@ -274,6 +274,11 @@ cp -r /tmp/fullsend-ai_fullsend/internal/scaffold/fullsend-repo/. /tmp/agents/ Then apply your organization customizations, if any: +> **Note:** The `customized/` overlay mechanism is deprecated per +> [ADR-0064](../../ADRs/0064-deprecate-customized-directory-overlay.md). +> Orgs that have migrated to config-driven agents should skip these +> `cp -r customized/` steps and use the registered harness paths directly. + ```bash git clone --depth 1 https://github.com/{org}/.fullsend.git /tmp/org-fullsend/ cp -r /tmp/org-fullsend/customized/. /tmp/agents/ diff --git a/docs/plans/agent-registration.md b/docs/plans/agent-registration.md index eb39c76b31..40fe4c77e9 100644 --- a/docs/plans/agent-registration.md +++ b/docs/plans/agent-registration.md @@ -450,3 +450,9 @@ PRs 2 and 3 can be developed in parallel after PR 1 merges. PR 4 is the cleanup that depends on everything else. Phase 5 is a follow-up tracked by a GitHub issue, filed once all first-party agents have been extracted from the scaffold. + +**Related:** `fullsend agent migrate-customizations` (implemented in +ADR-0064 / PR #2932) migrates existing `customized/` overrides into +config-driven agents. It uses `DiffHarness` to compute minimal `base:` +composition harnesses and registers agents via the same config schema +defined in Phase 1. diff --git a/docs/plans/deprecate-customized-directory-overlay.md b/docs/plans/deprecate-customized-directory-overlay.md index 181d4169f8..ca74c136d9 100644 --- a/docs/plans/deprecate-customized-directory-overlay.md +++ b/docs/plans/deprecate-customized-directory-overlay.md @@ -216,6 +216,8 @@ directories. The scaffold embed no longer contains them. - Remove all references to `customized/` directories. - Add examples of thin harness wrappers with `base:` URLs. - Add migration guidance for users who had files in `customized/`. +- Reference the `fullsend agent migrate-customizations` CLI command + that automates the conversion and delivers changes via PR. **`docs/agents/triage.md`, `docs/agents/review.md`:** diff --git a/docs/runtimes.md b/docs/runtimes.md index fb7f09f038..3783191ba9 100644 --- a/docs/runtimes.md +++ b/docs/runtimes.md @@ -96,6 +96,9 @@ slots: │ fullsend wins, repo version shadowed) │ │ Repo skills extend the agent; customized/skills/ │ │ overrides at the config layer before upload │ +│ ⚠ customized/ is deprecated per ADR-0064; use │ +│ config-driven agents instead (see `fullsend agent │ +│ migrate-customizations`) │ └────────────────────────────────────────────────────────┘ ``` diff --git a/internal/cli/agent.go b/internal/cli/agent.go index 855ba1e27f..8359c7e9c9 100644 --- a/internal/cli/agent.go +++ b/internal/cli/agent.go @@ -102,6 +102,7 @@ func newAgentCmd() *cobra.Command { cmd.AddCommand(newAgentListCmd()) cmd.AddCommand(newAgentUpdateCmd()) cmd.AddCommand(newAgentRemoveCmd()) + cmd.AddCommand(newAgentMigrateCustomizationsCmd()) return cmd } diff --git a/internal/cli/agent_test.go b/internal/cli/agent_test.go index f26531c8fd..c0f54ef5b7 100644 --- a/internal/cli/agent_test.go +++ b/internal/cli/agent_test.go @@ -1182,7 +1182,7 @@ allowed_remote_resources: func TestNewAgentCmd_HasSubcommands(t *testing.T) { cmd := newAgentCmd() - assert.Len(t, cmd.Commands(), 4) + assert.Len(t, cmd.Commands(), 5) names := make([]string, len(cmd.Commands())) for i, c := range cmd.Commands() { names[i] = c.Name() @@ -1191,4 +1191,5 @@ func TestNewAgentCmd_HasSubcommands(t *testing.T) { assert.Contains(t, names, "list") assert.Contains(t, names, "update") assert.Contains(t, names, "remove") + assert.Contains(t, names, "migrate-customizations") } diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go new file mode 100644 index 0000000000..bb6e0e3351 --- /dev/null +++ b/internal/cli/migrate.go @@ -0,0 +1,648 @@ +package cli + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + "gopkg.in/yaml.v3" + + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/harness" + "github.com/fullsend-ai/fullsend/internal/layers" + "github.com/fullsend-ai/fullsend/internal/scaffold" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +type migrationAction int + +const ( + migrateDead migrationAction = iota // agent already in config — delete customized files + migrateCustom // unknown agent — move files, register local path + migrateModified // scaffold agent not in config — base: composition +) + +type agentMigration struct { + name string + action migrationAction + files []string // relative paths under customized/ (e.g., "harness/triage.yaml") +} + +func newAgentMigrateCustomizationsCmd() *cobra.Command { + var fullsendDir string + var repoFlag string + var dryRun bool + + cmd := &cobra.Command{ + Use: "migrate-customizations", + Short: "Migrate customized/ overrides to config-driven agents", + Long: `Scan the customized/ directory and migrate each override: + + - Dead overrides (agent already in config) are deleted. + - Custom agents (not in upstream scaffold) are moved to regular + directories and registered as local paths in config.yaml. + - Modified standard agents are converted to base: composition + harnesses and registered in config.yaml. + +Changes are committed to a branch and delivered via pull request. +Use --dry-run to preview changes without creating a PR.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + printer := ui.New(os.Stdout) + forgeClient, forgeErr := defaultForgeClient() + if forgeErr != nil { + if !dryRun { + return fmt.Errorf("initializing forge client: %w", forgeErr) + } + printer.StepWarn(fmt.Sprintf("forge client unavailable: %v (not needed for dry-run)", forgeErr)) + } + return runMigrateCustomizations(cmd.Context(), fullsendDir, repoFlag, dryRun, forgeClient, printer) + }, + } + cmd.Flags().StringVar(&fullsendDir, "fullsend-dir", "", "base directory containing the .fullsend layout") + cmd.Flags().StringVar(&repoFlag, "repo", "", "target repository (owner/repo) for the migration PR") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "show what would change without creating a PR") + _ = cmd.MarkFlagRequired("fullsend-dir") + return cmd +} + +func runMigrateCustomizations(ctx context.Context, fullsendDir, repoFlag string, dryRun bool, forgeClient forge.Client, printer *ui.Printer) error { + absDir, err := filepath.Abs(fullsendDir) + if err != nil { + return fmt.Errorf("resolving fullsend dir: %w", err) + } + + configPath := filepath.Join(absDir, "config.yaml") + cfg, err := loadAgentConfig(configPath) + if err != nil { + return fmt.Errorf("reading config %s: %w", configPath, err) + } + + customizedBase := filepath.Join(absDir, "customized") + + if _, err := os.Stat(customizedBase); os.IsNotExist(err) { + printer.StepInfo("No customized/ directory found — nothing to migrate") + return nil + } + + files, err := walkCustomized(customizedBase) + if err != nil { + return fmt.Errorf("scanning customized directory: %w", err) + } + if len(files) == 0 { + printer.StepInfo("No customized files found — nothing to migrate") + return nil + } + + scaffoldNames, err := scaffold.HarnessNames() + if err != nil { + return fmt.Errorf("listing scaffold harnesses: %w", err) + } + scaffoldSet := make(map[string]bool, len(scaffoldNames)) + for _, n := range scaffoldNames { + scaffoldSet[n] = true + } + + migrations := planMigrations(files, cfg, scaffoldSet) + standaloneFiles := findStandaloneFiles(files, migrations) + + if len(migrations) == 0 && len(standaloneFiles) == 0 { + printer.StepInfo("No migrations needed") + return nil + } + + // Dry-run: report planned actions and return. + if dryRun { + for _, m := range migrations { + switch m.action { + case migrateDead: + printer.StepInfo(fmt.Sprintf("Would remove dead override: %s", m.name)) + case migrateCustom: + printer.StepInfo(fmt.Sprintf("Would register custom agent: %s", m.name)) + case migrateModified: + printer.StepInfo(fmt.Sprintf("Would convert to base: composition: %s", m.name)) + } + } + for _, f := range standaloneFiles { + printer.StepInfo(fmt.Sprintf("Would move standalone file: %s", f)) + } + return nil + } + + if repoFlag == "" { + return fmt.Errorf("--repo is required when not using --dry-run") + } + if forgeClient == nil { + return fmt.Errorf("forge client required for PR creation (set GITHUB_TOKEN)") + } + + parts := strings.SplitN(repoFlag, "/", 2) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("--repo must be in owner/repo format") + } + owner, repoName := parts[0], parts[1] + + // Determine the repo-relative prefix for customized paths and + // the destination prefix for moved files. + customizedPrefix := "customized/" + destPrefix := "" + if !cfg.isOrg { + customizedPrefix = ".fullsend/customized/" + destPrefix = ".fullsend/" + } + + var treeFiles []forge.TreeFile + configChanged := false + var prBodyParts []string + + for _, m := range migrations { + switch m.action { + case migrateDead: + printer.StepInfo(fmt.Sprintf("Dead override: %s (already registered in config)", m.name)) + for _, f := range m.files { + treeFiles = append(treeFiles, forge.TreeFile{ + Path: customizedPrefix + f, + Delete: true, + }) + } + prBodyParts = append(prBodyParts, fmt.Sprintf("- Removed dead override for **%s**", m.name)) + + case migrateCustom: + printer.StepInfo(fmt.Sprintf("Custom agent: %s → register in config", m.name)) + for _, f := range m.files { + tf, readErr := readTreeFile(customizedBase, f) + if readErr != nil { + return fmt.Errorf("reading custom agent file %s: %w", f, readErr) + } + if filepath.Dir(f) == "harness" && strings.HasSuffix(f, ".yaml") { + rewritten, rwErr := rewriteHarnessContent(tf.Content) + if rwErr != nil { + return fmt.Errorf("rewriting paths in %s: %w", f, rwErr) + } + tf.Content = rewritten + } + tf.Path = destPrefix + tf.Path + printer.StepWarn(fmt.Sprintf("Moving %s → %s (review PR to verify no unintended overwrites)", customizedPrefix+f, tf.Path)) + treeFiles = append(treeFiles, tf) + treeFiles = append(treeFiles, forge.TreeFile{ + Path: customizedPrefix + f, Delete: true, + }) + } + entry := config.AgentEntry{Source: "harness/" + m.name + ".yaml"} + if _, found := findAgentByName(cfg.agents(), m.name); !found { + cfg.setAgents(append(cfg.agents(), entry)) + configChanged = true + } + prBodyParts = append(prBodyParts, fmt.Sprintf("- Registered custom agent **%s**", m.name)) + + case migrateModified: + printer.StepInfo(fmt.Sprintf("Modified standard agent: %s → base: composition", m.name)) + agentFiles, buildErr := buildModifiedAgentFiles(customizedBase, customizedPrefix, destPrefix, m, cfg, printer) + if buildErr != nil { + return fmt.Errorf("building modified agent %s files: %w", m.name, buildErr) + } + treeFiles = append(treeFiles, agentFiles...) + configChanged = true + prBodyParts = append(prBodyParts, fmt.Sprintf("- Converted **%s** to `base:` composition", m.name)) + } + } + + // Move standalone files. + for _, f := range standaloneFiles { + tf, readErr := readTreeFile(customizedBase, f) + if readErr != nil { + return fmt.Errorf("reading standalone file %s: %w", f, readErr) + } + tf.Path = destPrefix + tf.Path + printer.StepWarn(fmt.Sprintf("Moving %s → %s (review PR to verify no unintended overwrites)", customizedPrefix+f, tf.Path)) + treeFiles = append(treeFiles, tf) + treeFiles = append(treeFiles, forge.TreeFile{ + Path: customizedPrefix + f, Delete: true, + }) + prBodyParts = append(prBodyParts, fmt.Sprintf("- Moved standalone file `%s`", f)) + } + + // Add updated config.yaml if agents were registered. + if configChanged { + if err := cfg.validate(); err != nil { + return fmt.Errorf("config validation failed after migration: %w", err) + } + data, marshalErr := cfg.marshal() + if marshalErr != nil { + return fmt.Errorf("marshaling config: %w", marshalErr) + } + cfgPath := "config.yaml" + if !cfg.isOrg { + cfgPath = ".fullsend/config.yaml" + } + treeFiles = append(treeFiles, forge.TreeFile{ + Path: cfgPath, Content: data, Mode: "100644", + }) + } + + if len(treeFiles) == 0 { + printer.StepInfo("No changes needed") + return nil + } + + if err := checkDuplicateDestinations(treeFiles); err != nil { + return err + } + + repo, err := forgeClient.GetRepo(ctx, owner, repoName) + if err != nil { + return fmt.Errorf("getting repo %s/%s: %w", owner, repoName, err) + } + + commitMsg := "chore: migrate customized/ overrides to config-driven agents" + prTitle := commitMsg + prBody := "## Migration Summary\n\n" + strings.Join(prBodyParts, "\n") + + "\n\nGenerated by `fullsend agent migrate-customizations`." + + _, err = layers.CommitFilesViaPR(ctx, forgeClient, printer, + owner, repoName, repo.DefaultBranch, + "fullsend/migrate-customizations", + commitMsg, prTitle, prBody, + treeFiles) + if err != nil { + return fmt.Errorf("creating migration PR: %w", err) + } + + return nil +} + +// walkCustomized walks the customized directory and returns relative paths +// of all non-.gitkeep files. +func walkCustomized(root string) ([]string, error) { + var files []string + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + if d.Type()&os.ModeSymlink != 0 { + return nil + } + if d.Name() == ".gitkeep" { + return nil + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + if strings.Contains(rel, "..") { + return nil + } + files = append(files, rel) + return nil + }) + return files, err +} + +// planMigrations groups customized files by agent name and determines the +// migration action for each. +func planMigrations(files []string, cfg *agentConfig, scaffoldSet map[string]bool) []agentMigration { + // Group files by agent name (derived from harness filename). + harnessAgents := make(map[string][]string) // agent name → list of all related files + var harnessNames []string + + for _, f := range files { + dir := filepath.Dir(f) + if dir != "harness" { + continue + } + base := filepath.Base(f) + if !strings.HasSuffix(base, ".yaml") { + continue + } + name := strings.TrimSuffix(base, ".yaml") + if _, exists := harnessAgents[name]; !exists { + harnessNames = append(harnessNames, name) + } + harnessAgents[name] = append(harnessAgents[name], f) + } + + // Associate non-harness files with agents by filename stem, but only + // for known per-agent directories to avoid false matches from files + // that coincidentally share a stem with an agent name. + perAgentDirs := map[string]bool{ + "agents": true, "scripts": true, "policies": true, + "schemas": true, "env": true, "skills": true, + } + for _, f := range files { + dir := filepath.Dir(f) + if dir == "harness" { + continue + } + if !perAgentDirs[dir] { + continue + } + base := filepath.Base(f) + stem := strings.TrimSuffix(base, filepath.Ext(base)) + if _, exists := harnessAgents[stem]; exists { + harnessAgents[stem] = append(harnessAgents[stem], f) + continue + } + for _, prefix := range []string{"pre-", "post-", "validate-output-"} { + if strings.HasPrefix(stem, prefix) { + stem = strings.TrimPrefix(stem, prefix) + break + } + } + if _, exists := harnessAgents[stem]; exists { + harnessAgents[stem] = append(harnessAgents[stem], f) + } + } + + var migrations []agentMigration + for _, name := range harnessNames { + m := agentMigration{ + name: name, + files: harnessAgents[name], + } + + if _, found := findAgentByName(cfg.agents(), name); found { + m.action = migrateDead + } else if scaffoldSet[name] { + m.action = migrateModified + } else { + m.action = migrateCustom + } + migrations = append(migrations, m) + } + return migrations +} + +// findStandaloneFiles returns customized files not associated with any +// migration (i.e., non-harness files without a matching agent). +func findStandaloneFiles(allFiles []string, migrations []agentMigration) []string { + migrated := make(map[string]bool) + for _, m := range migrations { + for _, f := range m.files { + migrated[f] = true + } + } + + var standalone []string + for _, f := range allFiles { + if !migrated[f] { + standalone = append(standalone, f) + } + } + return standalone +} + +// checkDuplicateDestinations detects conflicting writes where two migrations +// produce TreeFile entries targeting the same non-delete path. +func checkDuplicateDestinations(files []forge.TreeFile) error { + seen := make(map[string]bool, len(files)) + for _, f := range files { + if f.Delete { + continue + } + if seen[f.Path] { + return fmt.Errorf("migration conflict: multiple files target %s", f.Path) + } + seen[f.Path] = true + } + return nil +} + +// resolveBaseURL determines the base: URL for a composition harness. The diff +// is computed against the embedded scaffold, so the base URL must reference +// the same content — always the scaffold URL pinned to the CLI's commit SHA. +func resolveBaseURL(agentName string) (string, error) { + if commitSHA == "" || commitSHA == "dev" { + return "", fmt.Errorf("cannot determine base URL: no valid commit SHA (binary built without version info)") + } + return scaffold.HarnessBaseURLWithHash(agentName, commitSHA) +} + +// registerMigratedAgent adds the agent to cfg and ensures +// allowed_remote_resources covers the base URL prefix. +func registerMigratedAgent(cfg *agentConfig, agentName, baseURL string) { + entry := config.AgentEntry{Source: "harness/" + agentName + ".yaml"} + if _, found := findAgentByName(cfg.agents(), agentName); !found { + cfg.setAgents(append(cfg.agents(), entry)) + } + + prefix := allowlistPrefixForURL(baseURL) + if prefix != "" { + resources := cfg.allowedRemoteResources() + if !hasAllowlistPrefix(resources, prefix) { + cfg.setAllowedRemoteResources(append(resources, prefix)) + } + } +} + +// buildModifiedAgentFiles generates TreeFile entries for a modified standard +// agent. It computes a base: composition harness from the diff between the +// upstream scaffold and the customized version, then returns file entries for +// the new harness, deleted customized files, and moved associated files. +func buildModifiedAgentFiles( + customizedBase, customizedPrefix, destPrefix string, + m agentMigration, + cfg *agentConfig, + printer *ui.Printer, +) ([]forge.TreeFile, error) { + upstreamData, err := scaffold.HarnessContent(m.name) + if err != nil { + return nil, fmt.Errorf("loading upstream harness: %w", err) + } + var upstreamHarness harness.Harness + if err := yaml.Unmarshal(upstreamData, &upstreamHarness); err != nil { + return nil, fmt.Errorf("parsing upstream harness: %w", err) + } + + customizedPath := filepath.Join(customizedBase, "harness", m.name+".yaml") + customizedData, err := os.ReadFile(customizedPath) + if err != nil { + return nil, fmt.Errorf("reading customized harness: %w", err) + } + var customizedHarness harness.Harness + if err := yaml.Unmarshal(customizedData, &customizedHarness); err != nil { + return nil, fmt.Errorf("parsing customized harness: %w", err) + } + rewriteCustomizedPaths(&customizedHarness) + + customizedFilesSet := make(map[string]bool, len(m.files)) + for _, f := range m.files { + customizedFilesSet[f] = true + } + + diffResult := harness.DiffHarness(&upstreamHarness, &customizedHarness, customizedFilesSet) + if len(diffResult.Warnings) > 0 { + for _, w := range diffResult.Warnings { + printer.StepWarn(fmt.Sprintf("Agent %s: %s", m.name, w)) + } + if diffResult.Child == nil { + return nil, fmt.Errorf("agent %s: diff aborted due to unrepresentable changes (see warnings above)", m.name) + } + } + + baseURL, err := resolveBaseURL(m.name) + if err != nil { + return nil, fmt.Errorf("resolving base URL for %s: %w", m.name, err) + } + + var outputHarness *harness.Harness + if diffResult.Child == nil { + outputHarness = &harness.Harness{} + } else { + outputHarness = diffResult.Child + } + outputHarness.Base = baseURL + + outputData, err := yaml.Marshal(outputHarness) + if err != nil { + return nil, fmt.Errorf("marshaling composition harness: %w", err) + } + + var treeFiles []forge.TreeFile + + treeFiles = append(treeFiles, forge.TreeFile{ + Path: destPrefix + "harness/" + m.name + ".yaml", Content: outputData, Mode: "100644", + }) + + for _, f := range m.files { + if filepath.Dir(f) == "harness" { + treeFiles = append(treeFiles, forge.TreeFile{ + Path: customizedPrefix + f, Delete: true, + }) + continue + } + tf, readErr := readTreeFile(customizedBase, f) + if readErr != nil { + return nil, fmt.Errorf("reading customized file %s: %w", f, readErr) + } + tf.Path = destPrefix + tf.Path + treeFiles = append(treeFiles, tf) + treeFiles = append(treeFiles, forge.TreeFile{ + Path: customizedPrefix + f, Delete: true, + }) + } + + registerMigratedAgent(cfg, m.name, baseURL) + + return treeFiles, nil +} + +// rewriteCustomizedPaths strips the "customized/" prefix from path-bearing +// fields in a Harness so that internal references remain correct after files +// are moved out of the customized/ directory. Also rewrites env map values +// that embed "customized/" in variable-expanded paths (e.g. +// "${FULLSEND_DIR}/customized/schemas/..."). +func rewriteCustomizedPaths(h *harness.Harness) { + const prefix = "customized/" + strip := func(s string) string { + return strings.TrimPrefix(s, prefix) + } + h.Agent = strip(h.Agent) + h.Doc = strip(h.Doc) + h.Policy = strip(h.Policy) + h.PreScript = strip(h.PreScript) + h.PostScript = strip(h.PostScript) + h.AgentInput = strip(h.AgentInput) + for i := range h.Skills { + h.Skills[i] = strip(h.Skills[i]) + } + for i := range h.Plugins { + h.Plugins[i] = strip(h.Plugins[i]) + } + for i := range h.HostFiles { + h.HostFiles[i].Src = strip(h.HostFiles[i].Src) + } + for i := range h.APIServers { + h.APIServers[i].Script = strip(h.APIServers[i].Script) + } + if h.ValidationLoop != nil { + h.ValidationLoop.Script = strip(h.ValidationLoop.Script) + h.ValidationLoop.Schema = strip(h.ValidationLoop.Schema) + } + rewriteEnvMap(h.RunnerEnv) + if h.Env != nil { + rewriteEnvMap(h.Env.Runner) + rewriteEnvMap(h.Env.Sandbox) + } + for _, fc := range h.Forge { + if fc == nil { + continue + } + fc.PreScript = strip(fc.PreScript) + fc.PostScript = strip(fc.PostScript) + for i := range fc.Skills { + fc.Skills[i] = strip(fc.Skills[i]) + } + if fc.ValidationLoop != nil { + fc.ValidationLoop.Script = strip(fc.ValidationLoop.Script) + fc.ValidationLoop.Schema = strip(fc.ValidationLoop.Schema) + } + rewriteEnvMap(fc.RunnerEnv) + if fc.Env != nil { + rewriteEnvMap(fc.Env.Runner) + rewriteEnvMap(fc.Env.Sandbox) + } + } +} + +// rewriteEnvMap strips "customized/" path segments from env map values so +// that paths like "${FULLSEND_DIR}/customized/schemas/foo.json" become +// "${FULLSEND_DIR}/schemas/foo.json". Only strips the segment when preceded +// by "/" or at the start of the value to avoid corrupting unrelated substrings. +func rewriteEnvMap(m map[string]string) { + for k, v := range m { + v = strings.TrimPrefix(v, "customized/") + v = strings.ReplaceAll(v, "/customized/", "/") + m[k] = v + } +} + +// rewriteHarnessContent parses a harness YAML file, strips "customized/" +// prefixes from path fields, and returns the updated YAML bytes. +func rewriteHarnessContent(data []byte) ([]byte, error) { + var h harness.Harness + if err := yaml.Unmarshal(data, &h); err != nil { + return nil, fmt.Errorf("parsing harness for path rewrite: %w", err) + } + rewriteCustomizedPaths(&h) + return yaml.Marshal(&h) +} + +// readTreeFile reads a file from baseDir/relPath and returns a TreeFile with +// the correct git mode (100755 for executable files, 100644 otherwise). +func readTreeFile(baseDir, relPath string) (forge.TreeFile, error) { + full := filepath.Join(baseDir, relPath) + absBase, err := filepath.Abs(baseDir) + if err != nil { + return forge.TreeFile{}, fmt.Errorf("resolving base dir: %w", err) + } + absFull, err := filepath.Abs(full) + if err != nil { + return forge.TreeFile{}, fmt.Errorf("resolving file path: %w", err) + } + if !strings.HasPrefix(absFull, absBase+string(filepath.Separator)) { + return forge.TreeFile{}, fmt.Errorf("path %q escapes base directory", relPath) + } + info, err := os.Lstat(full) + if err != nil { + return forge.TreeFile{}, err + } + if info.Mode()&os.ModeSymlink != 0 { + return forge.TreeFile{}, fmt.Errorf("path %q is a symlink", relPath) + } + data, err := os.ReadFile(full) + if err != nil { + return forge.TreeFile{}, err + } + mode := "100644" + if info.Mode()&0o111 != 0 { + mode = "100755" + } + return forge.TreeFile{Path: relPath, Content: data, Mode: mode}, nil +} diff --git a/internal/cli/migrate_test.go b/internal/cli/migrate_test.go new file mode 100644 index 0000000000..8fdaace1fb --- /dev/null +++ b/internal/cli/migrate_test.go @@ -0,0 +1,1054 @@ +package cli + +import ( + "context" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/scaffold" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +// customizedReviewHarness returns the embedded review scaffold harness with +// the model field changed, producing a valid diff that only changes a scalar. +func customizedReviewHarness(t *testing.T, newModel string) string { + t.Helper() + data, err := scaffold.HarnessContent("review") + require.NoError(t, err) + return strings.Replace(string(data), "model: opus", "model: "+newModel, 1) +} + +func setupCustomizedDir(t *testing.T, dir string, files map[string]string) { + t.Helper() + customizedBase := filepath.Join(dir, "customized") + for relPath, content := range files { + full := filepath.Join(customizedBase, relPath) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } +} + +func fakeClientWithRepo(owner, repo string) *forge.FakeClient { + fc := forge.NewFakeClient() + fc.Repos = []forge.Repository{{ + FullName: owner + "/" + repo, + Name: repo, + DefaultBranch: "main", + }} + return fc +} + +func TestMigrateCustomizations_NoCustomizedDir(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) +} + +func TestMigrateCustomizations_EmptyCustomizedDir(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "customized", "harness"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "customized", "harness", ".gitkeep"), nil, 0o644)) + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) +} + +func TestMigrateCustomizations_DeadOverride_DryRun(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, `agents: + - source: "https://raw.githubusercontent.com/fullsend-ai/agents/abc123abc123abc123abc123abc123abc123abc1/harness/review.yaml#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +allowed_remote_resources: + - "https://raw.githubusercontent.com/fullsend-ai/agents/" +`) + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": "agent: agents/review.md\nmodel: opus\n", + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) + + // Dry-run should NOT delete the file. + _, err = os.Stat(filepath.Join(dir, "customized", "harness", "review.yaml")) + assert.NoError(t, err, "dry-run should not delete files") +} + +func TestMigrateCustomizations_DeadOverride_CreatesPR(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, `agents: + - source: "https://raw.githubusercontent.com/fullsend-ai/agents/abc123abc123abc123abc123abc123abc123abc1/harness/review.yaml#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +allowed_remote_resources: + - "https://raw.githubusercontent.com/fullsend-ai/agents/" +`) + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": "agent: agents/review.md\nmodel: opus\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + // Should have created a branch. + require.Len(t, fc.CreatedBranches, 1) + assert.Equal(t, "my-org/.fullsend/fullsend/migrate-customizations", fc.CreatedBranches[0]) + + // Should have committed files with a delete entry. + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + assert.Equal(t, "fullsend/migrate-customizations", record.Branch) + + var deletePaths []string + for _, f := range record.Files { + if f.Delete { + deletePaths = append(deletePaths, f.Path) + } + } + assert.Contains(t, deletePaths, "customized/harness/review.yaml") + + // Should have created a PR. + require.Len(t, fc.CreatedProposals, 1) + assert.Contains(t, fc.CreatedProposals[0].Title, "migrate customized/") +} + +func TestMigrateCustomizations_CustomAgent_DryRun(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + + setupCustomizedDir(t, dir, map[string]string{ + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: opus\n", + "agents/gh-classify.md": "You are gh-classify agent.\n", + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) + + // Dry-run: customized file should still exist. + _, err = os.Stat(filepath.Join(dir, "customized", "harness", "gh-classify.yaml")) + assert.NoError(t, err, "file should still exist in dry-run mode") + + // Config should NOT be modified. + cfgData, err := os.ReadFile(filepath.Join(dir, "config.yaml")) + require.NoError(t, err) + assert.NotContains(t, string(cfgData), "gh-classify") +} + +func TestMigrateCustomizations_CustomAgent_CreatesPR(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: opus\n", + "agents/gh-classify.md": "You are gh-classify agent.\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + // Should have committed files. + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + // Verify tree files: move harness + agent, delete both from customized/, update config. + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + // Harness moved to regular dir. + harnessFile, ok := pathMap["harness/gh-classify.yaml"] + require.True(t, ok, "harness should be added at regular path") + assert.Contains(t, string(harnessFile.Content), "agents/gh-classify.md") + assert.False(t, harnessFile.Delete) + + // Agent prompt moved to regular dir. + agentFile, ok := pathMap["agents/gh-classify.md"] + require.True(t, ok, "agent prompt should be added at regular path") + assert.Contains(t, string(agentFile.Content), "gh-classify agent") + assert.False(t, agentFile.Delete) + + // Customized copies deleted. + assert.True(t, pathMap["customized/harness/gh-classify.yaml"].Delete) + assert.True(t, pathMap["customized/agents/gh-classify.md"].Delete) + + // Config updated with agent registration. + cfgFile, ok := pathMap["config.yaml"] + require.True(t, ok, "config should be updated") + assert.Contains(t, string(cfgFile.Content), "harness/gh-classify.yaml") + + // PR created. + require.Len(t, fc.CreatedProposals, 1) +} + +func TestMigrateCustomizations_StandaloneFiles_CreatesPR(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "env/common.env": "SHARED_KEY=value\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + // File moved to regular dir. + envFile, ok := pathMap["env/common.env"] + require.True(t, ok) + assert.Equal(t, "SHARED_KEY=value\n", string(envFile.Content)) + assert.False(t, envFile.Delete) + + // Customized copy deleted. + assert.True(t, pathMap["customized/env/common.env"].Delete) +} + +func TestMigrateCustomizations_RequiresRepoFlag(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: opus\n", + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", false, nil, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "--repo is required") +} + +func TestMigrateCustomizations_RequiresForgeClient(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: opus\n", + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, nil, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "forge client required") +} + +func TestMigrateCustomizations_InvalidRepoFormat(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: opus\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "invalid-repo", false, fc, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "owner/repo format") +} + +func TestMigrateCustomizations_StandaloneFiles_DryRun(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "env/common.env": "SHARED_KEY=value\n", + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) + + // Dry-run: file should still be in customized/. + _, err = os.Stat(filepath.Join(dir, "customized", "env", "common.env")) + assert.NoError(t, err, "dry-run should not move files") +} + +func TestMigrateCustomizations_PerRepoMode_DryRun(t *testing.T) { + dir := t.TempDir() + // Realistic per-repo layout: --fullsend-dir points to .fullsend/. + fullsendDir := filepath.Join(dir, ".fullsend") + require.NoError(t, os.MkdirAll(fullsendDir, 0o755)) + writePerRepoConfig(t, fullsendDir, "") + + customizedBase := filepath.Join(fullsendDir, "customized") + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "harness"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "harness", "my-agent.yaml"), + []byte("agent: agents/my-agent.md\n"), + 0o644, + )) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), fullsendDir, "", true, nil, printer) + require.NoError(t, err) + + // Dry-run: file should still exist. + _, err = os.Stat(filepath.Join(customizedBase, "harness", "my-agent.yaml")) + assert.NoError(t, err) +} + +func TestMigrateCustomizations_PerRepoMode_CreatesPR(t *testing.T) { + dir := t.TempDir() + // Realistic per-repo layout: --fullsend-dir points to .fullsend/. + fullsendDir := filepath.Join(dir, ".fullsend") + require.NoError(t, os.MkdirAll(fullsendDir, 0o755)) + writePerRepoConfig(t, fullsendDir, "") + + customizedBase := filepath.Join(fullsendDir, "customized") + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "harness"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "harness", "my-agent.yaml"), + []byte("agent: agents/my-agent.md\n"), + 0o644, + )) + + fc := fakeClientWithRepo("my-org", "my-repo") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), fullsendDir, "my-org/my-repo", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + // Per-repo prefix is .fullsend/customized/. + assert.True(t, pathMap[".fullsend/customized/harness/my-agent.yaml"].Delete) + + // File moved to regular path under .fullsend/. + _, ok := pathMap[".fullsend/harness/my-agent.yaml"] + assert.True(t, ok) + + // Config updated at per-repo path. + cfgFile, ok := pathMap[".fullsend/config.yaml"] + require.True(t, ok) + assert.Contains(t, string(cfgFile.Content), "harness/my-agent.yaml") +} + +func TestMigrateCustomizations_MixedDeadAndCustom(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, `agents: + - source: "https://raw.githubusercontent.com/fullsend-ai/agents/abc123abc123abc123abc123abc123abc123abc1/harness/review.yaml#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +allowed_remote_resources: + - "https://raw.githubusercontent.com/fullsend-ai/agents/" +`) + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": "agent: agents/review.md\nmodel: opus\n", + "harness/gh-classify.yaml": "agent: agents/gh-classify.md\nmodel: sonnet\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + // Dead override: only delete. + assert.True(t, pathMap["customized/harness/review.yaml"].Delete) + + // Custom agent: move + delete + config. + _, ok := pathMap["harness/gh-classify.yaml"] + assert.True(t, ok, "custom agent harness should be moved") + assert.True(t, pathMap["customized/harness/gh-classify.yaml"].Delete) + + // Config updated with custom agent. + cfgFile := pathMap["config.yaml"] + assert.Contains(t, string(cfgFile.Content), "harness/gh-classify.yaml") +} + +func TestWalkCustomized(t *testing.T) { + dir := t.TempDir() + files := map[string]string{ + "harness/.gitkeep": "", + "harness/review.yaml": "test", + "agents/review.md": "test", + "agents/.gitkeep": "", + "scripts/pre-review.sh": "#!/bin/sh", + } + for relPath, content := range files { + full := filepath.Join(dir, relPath) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o644)) + } + + result, err := walkCustomized(dir) + require.NoError(t, err) + + // .gitkeep files should be excluded. + for _, f := range result { + assert.NotEqual(t, ".gitkeep", filepath.Base(f)) + } + assert.Len(t, result, 3) +} + +func TestWalkCustomized_SkipsSymlinks(t *testing.T) { + dir := t.TempDir() + + // Create a real file and a symlink. + require.NoError(t, os.MkdirAll(filepath.Join(dir, "harness"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "harness", "real.yaml"), []byte("test"), 0o644)) + require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(dir, "harness", "symlink.yaml"))) + + result, err := walkCustomized(dir) + require.NoError(t, err) + assert.Len(t, result, 1) + assert.Equal(t, filepath.Join("harness", "real.yaml"), result[0]) +} + +func TestReadTreeFile_PreservesExecutableBit(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "scripts"), 0o755)) + + // Non-executable file. + require.NoError(t, os.WriteFile(filepath.Join(dir, "scripts", "config.yaml"), []byte("key: val"), 0o644)) + tf, err := readTreeFile(dir, "scripts/config.yaml") + require.NoError(t, err) + assert.Equal(t, "100644", tf.Mode) + + // Executable file. + require.NoError(t, os.WriteFile(filepath.Join(dir, "scripts", "run.sh"), []byte("#!/bin/sh"), 0o755)) + tf, err = readTreeFile(dir, "scripts/run.sh") + require.NoError(t, err) + assert.Equal(t, "100755", tf.Mode) + assert.Equal(t, "scripts/run.sh", tf.Path) + assert.Equal(t, "#!/bin/sh", string(tf.Content)) +} + +func TestMigrateCustomizations_ExecutableScriptMode(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + // Create an executable script in customized/. + customizedBase := filepath.Join(dir, "customized") + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "scripts"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "scripts", "deploy.sh"), + []byte("#!/bin/bash\necho deploy"), + 0o755, + )) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + for _, f := range record.Files { + if f.Path == "scripts/deploy.sh" { + assert.Equal(t, "100755", f.Mode, "executable script should preserve 100755 mode") + return + } + } + t.Fatal("scripts/deploy.sh not found in committed files") +} + +func TestBuildModifiedAgentFiles_DiffAbort(t *testing.T) { + origSHA := commitSHA + commitSHA = "abcdef1234567890abcdef1234567890abcdef12" + t.Cleanup(func() { commitSHA = origSHA }) + + dir := t.TempDir() + customizedBase := filepath.Join(dir, "customized") + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "harness"), 0o755)) + + // Create a customized harness that removes skills from the scaffold base. + // The review scaffold harness has skills, so an empty skills list triggers removal. + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "harness", "review.yaml"), + []byte("agent: agents/review.md\nmodel: opus\nimage: ghcr.io/fullsend-ai/fullsend-code:latest\nskills: []\n"), + 0o644, + )) + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte("version: \"1\"\ndispatch:\n platform: github-actions\ndefaults:\n roles: [fullsend]\nrepos: {}\n") + c, _ := config.ParseOrgConfig(data) + return c + }(), + } + + m := agentMigration{ + name: "review", + action: migrateModified, + files: []string{"harness/review.yaml"}, + } + + printer := ui.New(io.Discard) + _, err := buildModifiedAgentFiles(customizedBase, "customized/", "", m, cfg, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "diff aborted") +} + +func TestBuildModifiedAgentFiles_DevCommitSHAError(t *testing.T) { + origSHA := commitSHA + commitSHA = "dev" + t.Cleanup(func() { commitSHA = origSHA }) + + dir := t.TempDir() + customizedBase := filepath.Join(dir, "customized") + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "harness"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "harness", "review.yaml"), + []byte(customizedReviewHarness(t, "sonnet")), + 0o644, + )) + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte("version: \"1\"\ndispatch:\n platform: github-actions\ndefaults:\n roles: [fullsend]\nrepos: {}\n") + c, _ := config.ParseOrgConfig(data) + return c + }(), + } + + m := agentMigration{ + name: "review", + action: migrateModified, + files: []string{"harness/review.yaml"}, + } + + printer := ui.New(io.Discard) + _, err := buildModifiedAgentFiles(customizedBase, "customized/", "", m, cfg, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot determine base URL") +} + +func TestMigrateCustomizations_ModifiedAgent_DryRun(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": customizedReviewHarness(t, "sonnet"), + }) + + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "", true, nil, printer) + require.NoError(t, err) + + // Dry-run should not modify anything. + _, err = os.Stat(filepath.Join(dir, "customized", "harness", "review.yaml")) + assert.NoError(t, err, "dry-run should not delete files") +} + +func TestMigrateCustomizations_ModifiedAgent_CreatesPR(t *testing.T) { + // Set commitSHA to a valid value so scaffold URL fallback works. + origSHA := commitSHA + commitSHA = "abcdef1234567890abcdef1234567890abcdef12" + t.Cleanup(func() { commitSHA = origSHA }) + + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": customizedReviewHarness(t, "sonnet"), + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + // Should have a composition harness with base: URL and diff content. + harnessFile, ok := pathMap["harness/review.yaml"] + require.True(t, ok, "composition harness should be created") + harnessContent := string(harnessFile.Content) + assert.Contains(t, harnessContent, "base:") + assert.Contains(t, harnessContent, "raw.githubusercontent.com") + assert.Contains(t, harnessContent, "model: sonnet", "diff should include the changed model field") + assert.NotContains(t, harnessContent, "model: opus", "base model should not appear in diff") + + // Old customized harness should be deleted. + assert.True(t, pathMap["customized/harness/review.yaml"].Delete) + + // Config should have the agent registered. + cfgFile, ok := pathMap["config.yaml"] + require.True(t, ok) + assert.Contains(t, string(cfgFile.Content), "harness/review.yaml") + + // PR created. + require.Len(t, fc.CreatedProposals, 1) +} + +func TestBuildModifiedAgentFiles_WithAssociatedFiles(t *testing.T) { + origSHA := commitSHA + commitSHA = "abcdef1234567890abcdef1234567890abcdef12" + t.Cleanup(func() { commitSHA = origSHA }) + + dir := t.TempDir() + customizedBase := filepath.Join(dir, "customized") + + // Set up customized harness + associated agent prompt. + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "harness"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(customizedBase, "agents"), 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "harness", "review.yaml"), + []byte(customizedReviewHarness(t, "sonnet")), + 0o644, + )) + require.NoError(t, os.WriteFile( + filepath.Join(customizedBase, "agents", "review.md"), + []byte("Custom review agent prompt.\n"), + 0o644, + )) + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte(`version: "1" +dispatch: + platform: github-actions +defaults: + roles: [fullsend] +repos: {} +`) + c, _ := config.ParseOrgConfig(data) + return c + }(), + } + + m := agentMigration{ + name: "review", + action: migrateModified, + files: []string{"harness/review.yaml", "agents/review.md"}, + } + + printer := ui.New(io.Discard) + files, err := buildModifiedAgentFiles(customizedBase, "customized/", "", m, cfg, printer) + require.NoError(t, err) + + pathMap := make(map[string]forge.TreeFile) + for _, f := range files { + pathMap[f.Path] = f + } + + // Composition harness should have base: URL. + harnessFile, ok := pathMap["harness/review.yaml"] + require.True(t, ok) + assert.Contains(t, string(harnessFile.Content), "base:") + + // Old customized harness deleted. + assert.True(t, pathMap["customized/harness/review.yaml"].Delete) + + // Associated agent file moved. + agentFile, ok := pathMap["agents/review.md"] + require.True(t, ok) + assert.Equal(t, "Custom review agent prompt.\n", string(agentFile.Content)) + + // Customized agent file deleted. + assert.True(t, pathMap["customized/agents/review.md"].Delete) + + // Agent registered in config. + found := false + for _, a := range cfg.agents() { + if a.Source == "harness/review.yaml" { + found = true + } + } + assert.True(t, found, "agent should be registered in config") +} + +func TestPlanMigrations_Categories(t *testing.T) { + files := []string{ + "harness/review.yaml", // in config → dead + "harness/triage.yaml", // in scaffold, not in config → modified + "harness/my-custom.yaml", // not in scaffold → custom + } + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte(`version: "1" +dispatch: + platform: github-actions +defaults: + roles: [fullsend] +repos: {} +agents: + - source: "https://example.com/review.yaml#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +allowed_remote_resources: + - "https://example.com/" +`) + c, err := config.ParseOrgConfig(data) + require.NoError(t, err) + return c + }(), + } + + scaffoldSet := map[string]bool{ + "triage": true, + "code": true, + "fix": true, + "review": true, + "retro": true, + "prioritize": true, + } + + migrations := planMigrations(files, cfg, scaffoldSet) + require.Len(t, migrations, 3) + + byName := make(map[string]agentMigration) + for _, m := range migrations { + byName[m.name] = m + } + + assert.Equal(t, migrateDead, byName["review"].action) + assert.Equal(t, migrateModified, byName["triage"].action) + assert.Equal(t, migrateCustom, byName["my-custom"].action) +} + +func TestPlanMigrations_AssociatesNonHarnessFiles(t *testing.T) { + files := []string{ + "harness/review.yaml", + "agents/review.md", + "scripts/pre-review.sh", + "scripts/post-review.sh", + "policies/review.yaml", + } + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte(`version: "1" +dispatch: + platform: github-actions +defaults: + roles: [fullsend] +repos: {} +`) + c, _ := config.ParseOrgConfig(data) + return c + }(), + } + + scaffoldSet := map[string]bool{"review": true} + migrations := planMigrations(files, cfg, scaffoldSet) + require.Len(t, migrations, 1) + + m := migrations[0] + assert.Equal(t, "review", m.name) + + // Should have harness + associated files. + fileSet := make(map[string]bool) + for _, f := range m.files { + fileSet[f] = true + } + assert.True(t, fileSet["harness/review.yaml"]) + assert.True(t, fileSet["agents/review.md"]) + assert.True(t, fileSet["scripts/pre-review.sh"]) + assert.True(t, fileSet["scripts/post-review.sh"]) + assert.True(t, fileSet["policies/review.yaml"]) +} + +func TestPlanMigrations_NonPerAgentDirBecomesStandalone(t *testing.T) { + files := []string{ + "harness/review.yaml", + "docs/review.md", + "templates/review.yaml", + } + + cfg := &agentConfig{ + isOrg: true, + orgCfg: func() *config.OrgConfig { + data := []byte("version: \"1\"\ndispatch:\n platform: github-actions\ndefaults:\n roles: [fullsend]\nrepos: {}\n") + c, _ := config.ParseOrgConfig(data) + return c + }(), + } + + scaffoldSet := map[string]bool{"review": true} + migrations := planMigrations(files, cfg, scaffoldSet) + require.Len(t, migrations, 1) + + m := migrations[0] + assert.Equal(t, "review", m.name) + + fileSet := make(map[string]bool) + for _, f := range m.files { + fileSet[f] = true + } + assert.True(t, fileSet["harness/review.yaml"]) + assert.False(t, fileSet["docs/review.md"], "files in non-per-agent dirs should not be associated") + assert.False(t, fileSet["templates/review.yaml"], "files in non-per-agent dirs should not be associated") + + standalone := findStandaloneFiles(files, migrations) + standaloneSet := make(map[string]bool) + for _, f := range standalone { + standaloneSet[f] = true + } + assert.True(t, standaloneSet["docs/review.md"]) + assert.True(t, standaloneSet["templates/review.yaml"]) +} + +func TestCheckDuplicateDestinations_NoDuplicates(t *testing.T) { + files := []forge.TreeFile{ + {Path: "harness/review.yaml", Content: []byte("a"), Mode: "100644"}, + {Path: "harness/triage.yaml", Content: []byte("b"), Mode: "100644"}, + {Path: "customized/harness/review.yaml", Delete: true}, + } + assert.NoError(t, checkDuplicateDestinations(files)) +} + +func TestCheckDuplicateDestinations_Conflict(t *testing.T) { + files := []forge.TreeFile{ + {Path: "harness/review.yaml", Content: []byte("a"), Mode: "100644"}, + {Path: "harness/review.yaml", Content: []byte("b"), Mode: "100644"}, + } + err := checkDuplicateDestinations(files) + require.Error(t, err) + assert.Contains(t, err.Error(), "migration conflict") + assert.Contains(t, err.Error(), "harness/review.yaml") +} + +func TestCheckDuplicateDestinations_DeletesIgnored(t *testing.T) { + files := []forge.TreeFile{ + {Path: "customized/harness/review.yaml", Delete: true}, + {Path: "customized/harness/review.yaml", Delete: true}, + } + assert.NoError(t, checkDuplicateDestinations(files)) +} + +func TestResolveBaseURL_DevCommitSHA(t *testing.T) { + origSHA := commitSHA + commitSHA = "dev" + t.Cleanup(func() { commitSHA = origSHA }) + + _, err := resolveBaseURL("review") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot determine base URL") +} + +func TestResolveBaseURL_ValidCommitSHA(t *testing.T) { + origSHA := commitSHA + commitSHA = "abc123abc123abc123abc123abc123abc123abc1" + t.Cleanup(func() { commitSHA = origSHA }) + + url, err := resolveBaseURL("review") + require.NoError(t, err) + assert.Contains(t, url, commitSHA) + assert.Contains(t, url, "review") +} + +func TestRegisterMigratedAgent_AddsEntryAndAllowlist(t *testing.T) { + data := []byte("version: \"1\"\ndispatch:\n platform: github-actions\ndefaults:\n roles: [fullsend]\nrepos: {}\n") + orgCfg, err := config.ParseOrgConfig(data) + require.NoError(t, err) + cfg := &agentConfig{isOrg: true, orgCfg: orgCfg} + + baseURL := "https://raw.githubusercontent.com/fullsend-ai/agents/abc123/harness/triage.yaml" + registerMigratedAgent(cfg, "triage", baseURL) + + _, found := findAgentByName(cfg.agents(), "triage") + assert.True(t, found, "agent should be registered") + + resources := cfg.allowedRemoteResources() + assert.NotEmpty(t, resources, "allowlist should have an entry") +} + +func TestRegisterMigratedAgent_NoDuplicate(t *testing.T) { + data := []byte("version: \"1\"\ndispatch:\n platform: github-actions\ndefaults:\n roles: [fullsend]\nrepos: {}\nagents:\n - source: \"harness/review.yaml\"\n") + orgCfg, err := config.ParseOrgConfig(data) + require.NoError(t, err) + cfg := &agentConfig{isOrg: true, orgCfg: orgCfg} + + before := len(cfg.agents()) + registerMigratedAgent(cfg, "review", "https://example.com/review.yaml") + assert.Equal(t, before, len(cfg.agents()), "should not duplicate existing agent") +} + +func TestMigrateCustomizations_DeadOverrideWithNonHarnessFiles(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, `agents: + - source: "https://raw.githubusercontent.com/fullsend-ai/agents/abc123abc123abc123abc123abc123abc123abc1/harness/review.yaml#sha256=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +allowed_remote_resources: + - "https://raw.githubusercontent.com/fullsend-ai/agents/" +`) + setupCustomizedDir(t, dir, map[string]string{ + "harness/review.yaml": "agent: agents/review.md\nmodel: opus\n", + "agents/review.md": "Custom review agent prompt.\n", + "scripts/pre-review.sh": "#!/bin/sh\necho pre", + "scripts/post-review.sh": "#!/bin/sh\necho post", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + for _, f := range record.Files { + assert.True(t, f.Delete, "dead override file %s should be deleted, not moved", f.Path) + assert.Contains(t, f.Path, "customized/", "dead override deletes should target customized/ paths") + } +} + +func TestMigrateCustomizations_CustomAgent_RewritesPaths(t *testing.T) { + dir := t.TempDir() + writeOrgConfig(t, dir, "") + + setupCustomizedDir(t, dir, map[string]string{ + "harness/explore.yaml": `agent: customized/agents/explore.md +model: opus +pre_script: customized/scripts/pre-explore.sh +post_script: customized/scripts/post-explore.sh +skills: + - customized/skills/public-research +host_files: + - src: customized/env/explore-agent.env + dest: /sandbox/workspace/.env.d/explore-agent.env +`, + "agents/explore.md": "You are the explore agent.\n", + "scripts/pre-explore.sh": "#!/bin/sh\necho pre\n", + "scripts/post-explore.sh": "#!/bin/sh\necho post\n", + "env/explore-agent.env": "KEY=val\n", + }) + + fc := fakeClientWithRepo("my-org", ".fullsend") + printer := ui.New(io.Discard) + err := runMigrateCustomizations(context.Background(), dir, "my-org/.fullsend", false, fc, printer) + require.NoError(t, err) + + require.Len(t, fc.CommittedFilesToBranch, 1) + record := fc.CommittedFilesToBranch[0] + + pathMap := make(map[string]forge.TreeFile) + for _, f := range record.Files { + pathMap[f.Path] = f + } + + harnessFile, ok := pathMap["harness/explore.yaml"] + require.True(t, ok, "harness should be added at regular path") + content := string(harnessFile.Content) + + assert.NotContains(t, content, "customized/", "customized/ prefixes should be stripped from harness content") + assert.Contains(t, content, "agent: agents/explore.md") + assert.Contains(t, content, "pre_script: scripts/pre-explore.sh") + assert.Contains(t, content, "post_script: scripts/post-explore.sh") + assert.Contains(t, content, "skills/public-research") + assert.Contains(t, content, "src: env/explore-agent.env") +} + +func TestRewriteCustomizedPaths(t *testing.T) { + input := `agent: customized/agents/explore.md +doc: customized/agents/explore-doc.md +policy: customized/policies/explore.yaml +pre_script: customized/scripts/pre-explore.sh +post_script: customized/scripts/post-explore.sh +agent_input: customized/schemas/explore-input.json +skills: + - customized/skills/public-research + - customized/skills/jira-read +plugins: + - customized/plugins/my-plugin +host_files: + - src: customized/env/explore-agent.env + dest: /sandbox/workspace/.env.d/explore-agent.env +api_servers: + - name: test-server + script: customized/scripts/api-server.sh + port: 8080 +validation_loop: + script: customized/scripts/validate-output-explore.sh + schema: customized/schemas/explore-output.json + max_iterations: 3 +runner_env: + FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/customized/schemas/explore-result.schema.json + GH_TOKEN: "${GH_TOKEN}" +forge: + github: + pre_script: customized/scripts/gh-pre.sh + post_script: customized/scripts/gh-post.sh + skills: + - customized/skills/gh-only-skill + validation_loop: + script: customized/scripts/gh-validate.sh + schema: customized/schemas/gh-output.json + max_iterations: 2 + runner_env: + GH_SCHEMA: ${FULLSEND_DIR}/customized/schemas/gh.json +` + rewritten, err := rewriteHarnessContent([]byte(input)) + require.NoError(t, err) + content := string(rewritten) + + assert.NotContains(t, content, "customized/") + assert.Contains(t, content, "agent: agents/explore.md") + assert.Contains(t, content, "doc: agents/explore-doc.md") + assert.Contains(t, content, "policy: policies/explore.yaml") + assert.Contains(t, content, "pre_script: scripts/pre-explore.sh") + assert.Contains(t, content, "post_script: scripts/post-explore.sh") + assert.Contains(t, content, "agent_input: schemas/explore-input.json") + assert.Contains(t, content, "skills/public-research") + assert.Contains(t, content, "skills/jira-read") + assert.Contains(t, content, "plugins/my-plugin") + assert.Contains(t, content, "src: env/explore-agent.env") + assert.Contains(t, content, "script: scripts/api-server.sh") + assert.Contains(t, content, "script: scripts/validate-output-explore.sh") + assert.Contains(t, content, "schema: schemas/explore-output.json") + assert.Contains(t, content, "${FULLSEND_DIR}/schemas/explore-result.schema.json") + assert.Contains(t, content, "${GH_TOKEN}", "non-customized env values should be unchanged") + assert.Contains(t, content, "pre_script: scripts/gh-pre.sh", "forge pre_script should be rewritten") + assert.Contains(t, content, "post_script: scripts/gh-post.sh", "forge post_script should be rewritten") + assert.Contains(t, content, "skills/gh-only-skill", "forge skills should be rewritten") + assert.Contains(t, content, "script: scripts/gh-validate.sh", "forge validation_loop.script should be rewritten") + assert.Contains(t, content, "schema: schemas/gh-output.json", "forge validation_loop.schema should be rewritten") + assert.Contains(t, content, "${FULLSEND_DIR}/schemas/gh.json", "forge runner_env should be rewritten") +} + +func TestRewriteEnvMap_NoFalsePositive(t *testing.T) { + m := map[string]string{ + "SAFE": "https://example.com/customized/config", + "REWRITE": "${FULLSEND_DIR}/customized/schemas/foo.json", + "PREFIX": "customized/scripts/bar.sh", + } + rewriteEnvMap(m) + assert.Equal(t, "https://example.com/config", m["SAFE"]) + assert.Equal(t, "${FULLSEND_DIR}/schemas/foo.json", m["REWRITE"]) + assert.Equal(t, "scripts/bar.sh", m["PREFIX"]) +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 1a89087688..49aaecc0ee 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -568,12 +568,7 @@ func (f *FakeClient) CommitFiles(_ context.Context, owner, repo, message string, Files: files, }) - if f.FileContents == nil { - f.FileContents = make(map[string][]byte) - } - for _, file := range files { - f.FileContents[owner+"/"+repo+"/"+file.Path] = file.Content - } + f.applyFileContents(owner, repo, files) changed := f.CommitFilesChanged == nil || *f.CommitFilesChanged return changed, nil @@ -595,15 +590,24 @@ func (f *FakeClient) CommitFilesToBranch(_ context.Context, owner, repo, branch, Files: files, }) + f.applyFileContents(owner, repo, files) + + changed := f.CommitFilesChanged == nil || *f.CommitFilesChanged + return changed, nil +} + +func (f *FakeClient) applyFileContents(owner, repo string, files []TreeFile) { if f.FileContents == nil { f.FileContents = make(map[string][]byte) } for _, file := range files { - f.FileContents[owner+"/"+repo+"/"+file.Path] = file.Content + key := owner + "/" + repo + "/" + file.Path + if file.Delete { + delete(f.FileContents, key) + } else { + f.FileContents[key] = file.Content + } } - - changed := f.CommitFilesChanged == nil || *f.CommitFilesChanged - return changed, nil } func (f *FakeClient) getRefLocked(owner, repo, refPath string) (string, bool) { diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 42a65b3b3a..c379dc3f21 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -200,10 +200,12 @@ type UserIdentity struct { // TreeFile represents a file to be committed via the Git Trees API. // Mode controls file permissions: "100644" for regular files, // "100755" for executable files (e.g., shell scripts). +// When Delete is true, the file is removed from the tree. type TreeFile struct { Path string Content []byte Mode string // "100644" or "100755" + Delete bool // remove file from tree instead of adding/updating } // DirectoryEntry represents a file or subdirectory in a repository directory listing. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 0db4ed9b17..a42bfa057f 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -825,6 +825,19 @@ func (c *LiveClient) commitFilesTo(ctx context.Context, owner, repo, branch, mes // 4. Compute expected blob SHAs and filter to changed files. var changedEntries []map[string]any for _, f := range files { + if f.Delete { + if _, exists := existing[f.Path]; !exists { + continue + } + changedEntries = append(changedEntries, map[string]any{ + "path": f.Path, + "mode": "100644", + "type": "blob", + "sha": nil, + }) + continue + } + expectedSHA := blobSHA(f.Content) info, exists := existing[f.Path] if exists && info.sha == expectedSHA && info.mode == f.Mode { diff --git a/internal/harness/diff.go b/internal/harness/diff.go new file mode 100644 index 0000000000..df7115c894 --- /dev/null +++ b/internal/harness/diff.go @@ -0,0 +1,455 @@ +package harness + +import ( + "reflect" +) + +// DiffResult holds the minimal child harness and any warnings produced +// during diffing. +type DiffResult struct { + // Child is the minimal harness containing only fields that differ from + // the base. Nil if base and child are identical and no customized files + // require field overrides. + Child *Harness + + // Warnings lists non-fatal issues (e.g., slice items removed from base + // that cannot be expressed with base: composition). + Warnings []string +} + +// DiffHarness computes the minimal child harness that, when composed with +// the given base via mergeBaseIntoChild, reproduces the full child. +// +// customizedFiles is a set of relative paths (e.g., "agents/triage.md") +// that exist in the customized/ directory. When a file-referencing field +// in the child matches the base value but the referenced file has been +// customized, the field is kept in the diff so the local file overrides +// the base's URL-resolved version. +// +// Returns nil DiffResult.Child when base and child are identical and no +// file overrides are needed. +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 + } + + // Security/fetch fields — mergeBaseIntoChild does NOT merge these from + // base to child (prevents privilege escalation). The diff must always + // include non-zero child values so they survive composition. + if child.AllowRuntimeFetch { + result.Child.AllowRuntimeFetch = true + hasAny = true + } + if child.MaxRuntimeFetches != nil { + result.Child.MaxRuntimeFetches = child.MaxRuntimeFetches + hasAny = true + } + if len(child.AllowedRemoteResources) > 0 { + result.Child.AllowedRemoteResources = child.AllowedRemoteResources + hasAny = true + } + + // String slices (concatenated by mergeBaseIntoChild) — keep only extras. + // Pass nil for customizedFiles: file-path override semantics only apply + // to scalar fields, not concatenated slices (would cause duplication). + if extras, removed := diffStringSlice(base.Skills, child.Skills, nil); 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, nil); 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, nil); 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, hfRemoved := diffHostFiles(base.HostFiles, child.HostFiles); len(extras) > 0 || hfRemoved { + if hfRemoved { + result.Warnings = append(result.Warnings, "host_files: child removes items from base; cannot express with base: composition") + result.Child = nil + return result + } + result.Child.HostFiles = extras + hasAny = true + } + + // APIServers — keep entries in child not in base (by name) + if extras, asRemoved := diffAPIServers(base.APIServers, child.APIServers); len(extras) > 0 || asRemoved { + if asRemoved { + result.Warnings = append(result.Warnings, "api_servers: child removes items from base; cannot express with base: composition") + result.Child = nil + return result + } + result.Child.APIServers = extras + hasAny = true + } + + // Maps — keep keys where child value differs from base + if diff, mapRemoved := diffStringMap(base.RunnerEnv, child.RunnerEnv); len(diff) > 0 || mapRemoved { + if mapRemoved { + result.Warnings = append(result.Warnings, "runner_env: child removes keys from base; cannot express with base: composition") + result.Child = nil + return result + } + result.Child.RunnerEnv = diff + hasAny = true + } + + // Env — diff sub-maps independently + if envDiff, envRemoved := diffEnvConfig(base.Env, child.Env); envDiff != nil || envRemoved { + if envRemoved { + result.Warnings = append(result.Warnings, "env: child removes keys from base; cannot express with base: composition") + result.Child = nil + return result + } + result.Child.Env = envDiff + hasAny = true + } + + // Pointer structs — keep if non-nil and different; abort if child removes. + if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) { + result.Child.ValidationLoop = child.ValidationLoop + hasAny = true + } else if child.ValidationLoop == nil && base.ValidationLoop != nil { + result.Warnings = append(result.Warnings, "validation_loop: child removes block from base; cannot express with base: composition") + result.Child = nil + return result + } + if child.Security != nil && !reflect.DeepEqual(child.Security, base.Security) { + result.Child.Security = child.Security + hasAny = true + } else if child.Security == nil && base.Security != nil { + result.Warnings = append(result.Warnings, "security: child removes block from base; cannot express with base: composition") + result.Child = nil + return result + } + + // Forge — diff per platform + if forgeDiff, forgeWarnings := diffForge(base.Forge, child.Forge); len(forgeDiff) > 0 || len(forgeWarnings) > 0 { + if len(forgeWarnings) > 0 { + result.Warnings = append(result.Warnings, forgeWarnings...) + result.Child = nil + return result + } + result.Child.Forge = forgeDiff + hasAny = true + } + + if !hasAny { + result.Child = nil + } + return result +} + +// diffScalarFile sets *dst to childVal if it differs from baseVal, or if +// childVal is a path that appears in customizedFiles. Returns true if a +// difference was recorded. +func diffScalarFile(dst *string, baseVal, childVal string, customizedFiles map[string]bool) bool { + if childVal != baseVal { + *dst = childVal + return true + } + if childVal != "" && customizedFiles[childVal] { + *dst = childVal + return true + } + return false +} + +// diffStringSlice returns items in child that are not in base (extras), +// and whether any base items are missing from child (removed). +// Items whose path appears in customizedFiles are always kept as extras. +func diffStringSlice(base, child []string, customizedFiles map[string]bool) (extras []string, removed bool) { + baseSet := make(map[string]bool, len(base)) + for _, s := range base { + baseSet[s] = true + } + + childSet := make(map[string]bool, len(child)) + for _, s := range child { + childSet[s] = true + } + + for _, s := range base { + if !childSet[s] { + removed = true + break + } + } + + for _, s := range child { + if !baseSet[s] || customizedFiles[s] { + extras = append(extras, s) + } + } + return extras, removed +} + +// diffHostFiles returns HostFile entries in child whose Dest is not in base, +// or whose fields differ from the base entry with the same Dest. It also +// reports whether any base entries were removed from child. +func diffHostFiles(base, child []HostFile) (extras []HostFile, removed bool) { + baseByDest := make(map[string]HostFile, len(base)) + for _, hf := range base { + baseByDest[hf.Dest] = hf + } + + childByDest := make(map[string]bool, len(child)) + for _, hf := range child { + childByDest[hf.Dest] = true + } + + for _, hf := range base { + if !childByDest[hf.Dest] { + removed = true + break + } + } + + for _, hf := range child { + if bHF, exists := baseByDest[hf.Dest]; !exists || !reflect.DeepEqual(hf, bHF) { + extras = append(extras, hf) + } + } + return extras, removed +} + +// diffAPIServers returns APIServer entries in child whose Name is not in +// base (truly new entries). It also reports whether any base entries were +// removed or modified in child. mergeBaseIntoChild concatenates APIServers +// without dedup, so returning modified-by-Name entries would produce +// duplicates after composition. +func diffAPIServers(base, child []APIServer) (extras []APIServer, removed bool) { + baseByName := make(map[string]APIServer, len(base)) + for _, as := range base { + baseByName[as.Name] = as + } + + childByName := make(map[string]bool, len(child)) + for _, as := range child { + childByName[as.Name] = true + } + + for _, as := range base { + if !childByName[as.Name] { + removed = true + break + } + } + + for _, as := range child { + bAS, exists := baseByName[as.Name] + if !exists { + extras = append(extras, as) + } else if !reflect.DeepEqual(as, bAS) { + removed = true + } + } + return extras, removed +} + +// diffStringMap returns keys where child value differs from base value, +// or keys present in child but not in base. It also reports whether any +// base keys are missing from child (removed). +func diffStringMap(base, child map[string]string) (diff map[string]string, removed bool) { + for k := range base { + if _, ok := child[k]; !ok { + removed = true + break + } + } + if len(child) == 0 { + return nil, removed + } + diff = make(map[string]string) + for k, cv := range child { + if bv, ok := base[k]; !ok || cv != bv { + diff[k] = cv + } + } + if len(diff) == 0 { + return nil, removed + } + return diff, removed +} + +// diffEnvConfig returns an EnvConfig with only the runner/sandbox keys +// that differ between base and child, and whether any base keys were removed. +func diffEnvConfig(base, child *EnvConfig) (*EnvConfig, bool) { + if child == nil { + if base != nil && (len(base.Runner) > 0 || len(base.Sandbox) > 0) { + return nil, true + } + return nil, false + } + var baseRunner, baseSandbox map[string]string + if base != nil { + baseRunner = base.Runner + baseSandbox = base.Sandbox + } + + runnerDiff, runnerRemoved := diffStringMap(baseRunner, child.Runner) + sandboxDiff, sandboxRemoved := diffStringMap(baseSandbox, child.Sandbox) + removed := runnerRemoved || sandboxRemoved + + if runnerDiff == nil && sandboxDiff == nil { + return nil, removed + } + return &EnvConfig{Runner: runnerDiff, Sandbox: sandboxDiff}, removed +} + +// diffForge returns forge platforms/fields that differ between base and child, +// along with any warnings about unrepresentable changes. +func diffForge(base, child map[string]*ForgeConfig) (map[string]*ForgeConfig, []string) { + if len(child) == 0 && len(base) == 0 { + return nil, nil + } + + diff := make(map[string]*ForgeConfig) + var warnings []string + + for platform := range base { + childFC, ok := child[platform] + if !ok || childFC == nil { + warnings = append(warnings, "forge["+platform+"]: child removes platform from base; cannot express with base: composition") + } + } + + for platform, childFC := range child { + if childFC == nil { + continue + } + baseFC := base[platform] + if baseFC == nil { + diff[platform] = childFC + continue + } + fc, w := diffForgeConfig(baseFC, childFC, platform) + warnings = append(warnings, w...) + if fc != nil { + diff[platform] = fc + } + } + if len(diff) == 0 { + return nil, warnings + } + return diff, warnings +} + +// diffForgeConfig returns a ForgeConfig with only fields that differ, +// along with any warnings about unrepresentable changes. +func diffForgeConfig(base, child *ForgeConfig, platform string) (*ForgeConfig, []string) { + if child == nil { + return nil, nil + } + fc := &ForgeConfig{} + hasAny := false + var warnings []string + + if child.PreScript != base.PreScript { + fc.PreScript = child.PreScript + hasAny = true + } + if child.PostScript != base.PostScript { + fc.PostScript = child.PostScript + hasAny = true + } + if extras, removed := diffStringSlice(base.Skills, child.Skills, nil); len(extras) > 0 || removed { + if removed { + return nil, []string{"forge[" + platform + "].skills: child removes items from base; cannot express with base: composition"} + } + fc.Skills = extras + hasAny = true + } + if child.ValidationLoop != nil && !reflect.DeepEqual(child.ValidationLoop, base.ValidationLoop) { + fc.ValidationLoop = child.ValidationLoop + hasAny = true + } + if d, mapRemoved := diffStringMap(base.RunnerEnv, child.RunnerEnv); len(d) > 0 || mapRemoved { + if mapRemoved { + return nil, []string{"forge[" + platform + "].runner_env: child removes keys from base; cannot express with base: composition"} + } + fc.RunnerEnv = d + hasAny = true + } + if d, envRemoved := diffEnvConfig(base.Env, child.Env); d != nil || envRemoved { + if envRemoved { + return nil, []string{"forge[" + platform + "].env: child removes keys from base; cannot express with base: composition"} + } + fc.Env = d + hasAny = true + } + + if !hasAny { + return nil, warnings + } + return fc, warnings +} diff --git a/internal/harness/diff_test.go b/internal/harness/diff_test.go new file mode 100644 index 0000000000..f8f49de78f --- /dev/null +++ b/internal/harness/diff_test.go @@ -0,0 +1,811 @@ +package harness + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDiffHarness_Identical(t *testing.T) { + h := &Harness{ + Agent: "agents/triage.md", + Model: "opus", + TimeoutMinutes: 10, + Skills: []string{"skills/a"}, + } + result := DiffHarness(h, h, nil) + assert.Nil(t, result.Child) + assert.Empty(t, result.Warnings) +} + +func TestDiffHarness_ScalarDifference(t *testing.T) { + base := &Harness{ + Agent: "agents/triage.md", + Model: "opus", + Image: "ghcr.io/example:latest", + } + child := &Harness{ + Agent: "agents/triage.md", + Model: "sonnet", + Image: "ghcr.io/example:latest", + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, "sonnet", result.Child.Model) + assert.Empty(t, result.Child.Agent, "unchanged scalar should not appear in diff") + assert.Empty(t, result.Child.Image, "unchanged scalar should not appear in diff") +} + +func TestDiffHarness_SliceAddition(t *testing.T) { + base := &Harness{ + Skills: []string{"skills/a", "skills/b"}, + } + child := &Harness{ + Skills: []string{"skills/a", "skills/b", "skills/c"}, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, []string{"skills/c"}, result.Child.Skills) + assert.Empty(t, result.Warnings) +} + +func TestDiffHarness_SliceRemoval(t *testing.T) { + base := &Harness{ + Skills: []string{"skills/a", "skills/b", "skills/c"}, + } + child := &Harness{ + Skills: []string{"skills/a"}, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "removes items from base") +} + +func TestDiffHarness_CustomizedFileKeepsField(t *testing.T) { + base := &Harness{ + Agent: "agents/triage.md", + Policy: "policies/triage.yaml", + } + child := &Harness{ + Agent: "agents/triage.md", + Policy: "policies/triage.yaml", + } + customized := map[string]bool{ + "agents/triage.md": true, + } + result := DiffHarness(base, child, customized) + require.NotNil(t, result.Child) + assert.Equal(t, "agents/triage.md", result.Child.Agent, "customized file path should be kept") + assert.Empty(t, result.Child.Policy, "non-customized file should not appear") +} + +func TestDiffHarness_MapDifference(t *testing.T) { + base := &Harness{ + RunnerEnv: map[string]string{ + "KEY1": "val1", + "KEY2": "val2", + }, + } + child := &Harness{ + RunnerEnv: map[string]string{ + "KEY1": "val1", + "KEY2": "changed", + "KEY3": "new", + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, map[string]string{"KEY2": "changed", "KEY3": "new"}, result.Child.RunnerEnv) +} + +func TestDiffHarness_EnvDifference(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"A": "1"}, + Sandbox: map[string]string{"B": "2"}, + }, + } + child := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"A": "1", "C": "3"}, + Sandbox: map[string]string{"B": "2"}, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + require.NotNil(t, result.Child.Env) + assert.Equal(t, map[string]string{"C": "3"}, result.Child.Env.Runner) + assert.Nil(t, result.Child.Env.Sandbox, "identical sandbox should not appear") +} + +func TestDiffHarness_HostFileDifference(t *testing.T) { + base := &Harness{ + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + {Src: "b.txt", Dest: "/tmp/b"}, + }, + } + child := &Harness{ + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + {Src: "b-custom.txt", Dest: "/tmp/b"}, + {Src: "c.txt", Dest: "/tmp/c"}, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Len(t, result.Child.HostFiles, 2) + assert.Equal(t, "/tmp/b", result.Child.HostFiles[0].Dest) + assert.Equal(t, "/tmp/c", result.Child.HostFiles[1].Dest) +} + +func TestDiffHarness_ForgeDifference(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "scripts/pre.sh", + PostScript: "scripts/post.sh", + }, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "scripts/pre.sh", + PostScript: "scripts/custom-post.sh", + }, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + require.NotNil(t, result.Child.Forge["github"]) + assert.Equal(t, "scripts/custom-post.sh", result.Child.Forge["github"].PostScript) + assert.Empty(t, result.Child.Forge["github"].PreScript, "unchanged forge field should not appear") +} + +func TestDiffHarness_MultipleFieldTypes(t *testing.T) { + base := &Harness{ + Agent: "agents/code.md", + Model: "opus", + Skills: []string{"skills/a"}, + TimeoutMinutes: 10, + RunnerEnv: map[string]string{"K": "v"}, + } + child := &Harness{ + Agent: "agents/code.md", + Model: "sonnet", + Skills: []string{"skills/a", "skills/b"}, + TimeoutMinutes: 20, + RunnerEnv: map[string]string{"K": "v", "K2": "v2"}, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, "sonnet", result.Child.Model) + assert.Equal(t, []string{"skills/b"}, result.Child.Skills) + assert.Equal(t, 20, result.Child.TimeoutMinutes) + assert.Equal(t, map[string]string{"K2": "v2"}, result.Child.RunnerEnv) + assert.Empty(t, result.Child.Agent) +} + +func TestDiffHarness_ValidationLoopDifference(t *testing.T) { + base := &Harness{ + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + MaxIterations: 2, + }, + } + child := &Harness{ + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + MaxIterations: 5, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + require.NotNil(t, result.Child.ValidationLoop) + assert.Equal(t, 5, result.Child.ValidationLoop.MaxIterations) +} + +func TestDiffHarness_NewForgePlatform(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "pre.sh"}, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "pre.sh"}, + "gitlab": {PreScript: "gl-pre.sh"}, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Nil(t, result.Child.Forge["github"], "identical platform should not appear") + assert.NotNil(t, result.Child.Forge["gitlab"]) + assert.Equal(t, "gl-pre.sh", result.Child.Forge["gitlab"].PreScript) +} + +func TestDiffHarness_AllowRuntimeFetch_AlwaysKept(t *testing.T) { + base := &Harness{ + AllowRuntimeFetch: true, + Model: "opus", + } + child := &Harness{ + AllowRuntimeFetch: true, + Model: "opus", + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child, "AllowRuntimeFetch must be kept even when matching base") + assert.True(t, result.Child.AllowRuntimeFetch) +} + +func TestDiffHarness_MaxRuntimeFetches_AlwaysKept(t *testing.T) { + maxFetches := 20 + base := &Harness{ + MaxRuntimeFetches: &maxFetches, + } + child := &Harness{ + MaxRuntimeFetches: &maxFetches, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child, "MaxRuntimeFetches must be kept even when matching base") + require.NotNil(t, result.Child.MaxRuntimeFetches) + assert.Equal(t, 20, *result.Child.MaxRuntimeFetches) +} + +func TestDiffHarness_AllowedRemoteResources_AlwaysKept(t *testing.T) { + base := &Harness{ + AllowedRemoteResources: []string{"https://example.com/"}, + } + child := &Harness{ + AllowedRemoteResources: []string{"https://example.com/"}, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child, "AllowedRemoteResources must be kept even when matching base") + assert.Equal(t, []string{"https://example.com/"}, result.Child.AllowedRemoteResources) +} + +func TestDiffHarness_SecurityFields_OmittedWhenZero(t *testing.T) { + base := &Harness{ + AllowRuntimeFetch: false, + Model: "opus", + } + child := &Harness{ + AllowRuntimeFetch: false, + Model: "opus", + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "zero-value security fields should not force a diff") +} + +func TestDiffHarness_SecurityFields_Combined(t *testing.T) { + maxFetches := 50 + base := &Harness{ + AllowRuntimeFetch: true, + MaxRuntimeFetches: &maxFetches, + AllowedRemoteResources: []string{"https://example.com/"}, + Model: "opus", + } + child := &Harness{ + AllowRuntimeFetch: true, + MaxRuntimeFetches: &maxFetches, + AllowedRemoteResources: []string{"https://example.com/", "https://other.com/"}, + Model: "sonnet", + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.True(t, result.Child.AllowRuntimeFetch) + assert.Equal(t, 50, *result.Child.MaxRuntimeFetches) + assert.Equal(t, []string{"https://example.com/", "https://other.com/"}, result.Child.AllowedRemoteResources) + assert.Equal(t, "sonnet", result.Child.Model) +} + +func TestDiffHarness_DescriptionAndSlug(t *testing.T) { + base := &Harness{Description: "old", Slug: "old-slug", Role: "reviewer"} + child := &Harness{Description: "new", Slug: "new-slug", Role: "reviewer"} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, "new", result.Child.Description) + assert.Equal(t, "new-slug", result.Child.Slug) + assert.Empty(t, result.Child.Role) +} + +func TestDiffHarness_ImageAndRole(t *testing.T) { + base := &Harness{Image: "img:1", Role: "coder"} + child := &Harness{Image: "img:2", Role: "reviewer"} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, "img:2", result.Child.Image) + assert.Equal(t, "reviewer", result.Child.Role) +} + +func TestDiffHarness_PrePostScript(t *testing.T) { + base := &Harness{PreScript: "scripts/pre.sh", PostScript: "scripts/post.sh", AgentInput: "input.txt"} + child := &Harness{PreScript: "scripts/pre2.sh", PostScript: "scripts/post.sh", AgentInput: "input2.txt"} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, "scripts/pre2.sh", result.Child.PreScript) + assert.Empty(t, result.Child.PostScript) + assert.Equal(t, "input2.txt", result.Child.AgentInput) +} + +func TestDiffHarness_SandboxTimeout(t *testing.T) { + base := &Harness{SandboxTimeoutSeconds: 60} + child := &Harness{SandboxTimeoutSeconds: 120} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, 120, result.Child.SandboxTimeoutSeconds) +} + +func TestDiffHarness_SecurityDifference(t *testing.T) { + base := &Harness{ + Security: &SecurityConfig{FailMode: "closed"}, + } + child := &Harness{ + Security: &SecurityConfig{FailMode: "open"}, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + require.NotNil(t, result.Child.Security) + assert.Equal(t, "open", result.Child.Security.FailMode) +} + +func TestDiffHarness_SecurityIdentical(t *testing.T) { + sec := &SecurityConfig{FailMode: "closed"} + base := &Harness{Security: sec, Model: "opus"} + child := &Harness{Security: sec, Model: "opus"} + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child) +} + +func TestDiffHarness_APIServerModification(t *testing.T) { + base := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + }, + } + child := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start-v2.sh", Port: 8080}, + {Name: "metrics", Script: "metrics.sh", Port: 9090}, + }, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "modified api_server should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "api_servers") +} + +func TestDiffHarness_APIServerAdditionOnly(t *testing.T) { + base := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + }, + } + child := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + {Name: "metrics", Script: "metrics.sh", Port: 9090}, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Len(t, result.Child.APIServers, 1) + assert.Equal(t, "metrics", result.Child.APIServers[0].Name) +} + +func TestDiffHarness_PluginsRemoval(t *testing.T) { + base := &Harness{Plugins: []string{"a", "b"}} + child := &Harness{Plugins: []string{"a"}} + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child) + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "plugins") +} + +func TestDiffHarness_ProvidersRemoval(t *testing.T) { + base := &Harness{Providers: []string{"p1", "p2"}} + child := &Harness{Providers: []string{"p1"}} + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child) + assert.Contains(t, result.Warnings[0], "providers") +} + +func TestDiffHarness_ProvidersAddition(t *testing.T) { + base := &Harness{Providers: []string{"p1"}} + child := &Harness{Providers: []string{"p1", "p2"}} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, []string{"p2"}, result.Child.Providers) +} + +func TestDiffHarness_DocCustomizedFile(t *testing.T) { + base := &Harness{Doc: "agents/doc.md"} + child := &Harness{Doc: "agents/doc.md"} + customized := map[string]bool{"agents/doc.md": true} + result := DiffHarness(base, child, customized) + require.NotNil(t, result.Child) + assert.Equal(t, "agents/doc.md", result.Child.Doc) +} + +func TestDiffHarness_NilValidationLoopWarning(t *testing.T) { + base := &Harness{ + ValidationLoop: &ValidationLoop{Script: "validate.sh"}, + } + child := &Harness{} + result := DiffHarness(base, child, nil) + require.NotEmpty(t, result.Warnings) + assert.Contains(t, result.Warnings[0], "validation_loop: child removes block from base") + assert.Nil(t, result.Child, "Child should be nil when validation_loop is removed") +} + +func TestDiffHarness_NilSecurityWarning(t *testing.T) { + base := &Harness{ + Security: &SecurityConfig{FailMode: "closed"}, + } + child := &Harness{} + result := DiffHarness(base, child, nil) + require.NotEmpty(t, result.Warnings) + assert.Contains(t, result.Warnings[0], "security: child removes block from base") + assert.Nil(t, result.Child, "Child should be nil when security is removed") +} + +func TestDiffForge_NilPlatformWarning(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "echo hi"}, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": nil, + }, + } + result := DiffHarness(base, child, nil) + require.NotEmpty(t, result.Warnings) + assert.Contains(t, result.Warnings[0], "child removes platform from base") +} + +func TestDiffForgeConfig_AllFields(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "pre.sh", + PostScript: "post.sh", + Skills: []string{"skill/a"}, + RunnerEnv: map[string]string{"K": "v"}, + Env: &EnvConfig{Runner: map[string]string{"R": "1"}}, + ValidationLoop: &ValidationLoop{ + Script: "validate.sh", + MaxIterations: 3, + }, + }, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "pre.sh", + PostScript: "post-v2.sh", + Skills: []string{"skill/a", "skill/b"}, + RunnerEnv: map[string]string{"K": "v", "K2": "v2"}, + Env: &EnvConfig{Runner: map[string]string{"R": "1", "R2": "2"}}, + ValidationLoop: &ValidationLoop{ + Script: "validate-v2.sh", + MaxIterations: 5, + }, + }, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + fc := result.Child.Forge["github"] + require.NotNil(t, fc) + assert.Equal(t, "post-v2.sh", fc.PostScript) + assert.Empty(t, fc.PreScript) + assert.Equal(t, []string{"skill/b"}, fc.Skills) + assert.Equal(t, map[string]string{"K2": "v2"}, fc.RunnerEnv) + assert.Equal(t, map[string]string{"R2": "2"}, fc.Env.Runner) + assert.NotNil(t, fc.ValidationLoop) +} + +func TestDiffHarness_PluginsAddition(t *testing.T) { + base := &Harness{Plugins: []string{"a"}} + child := &Harness{Plugins: []string{"a", "b"}} + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + assert.Equal(t, []string{"b"}, result.Child.Plugins) +} + +func TestDiffHarness_HostFileRemoval(t *testing.T) { + base := &Harness{ + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + {Src: "b.txt", Dest: "/tmp/b"}, + }, + } + child := &Harness{ + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + }, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "host_files") +} + +func TestDiffHarness_APIServerRemoval(t *testing.T) { + base := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + {Name: "metrics", Script: "metrics.sh", Port: 9090}, + }, + } + child := &Harness{ + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + }, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "api_servers") +} + +func TestDiffHarness_RunnerEnvRemoval(t *testing.T) { + base := &Harness{ + RunnerEnv: map[string]string{"A": "1", "B": "2"}, + } + child := &Harness{ + RunnerEnv: map[string]string{"A": "1"}, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "env key removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "runner_env") +} + +func TestDiffHarness_EnvRunnerRemoval(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{Runner: map[string]string{"A": "1", "B": "2"}}, + } + child := &Harness{ + Env: &EnvConfig{Runner: map[string]string{"A": "1"}}, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "env runner key removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "env") +} + +func TestDiffHarness_ForgeSkillsRemoval(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + Skills: []string{"skill/a", "skill/b"}, + }, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": { + Skills: []string{"skill/a"}, + }, + }, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "forge skill removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "forge[github].skills") +} + +func TestDiffHarness_ForgePlatformRemoval(t *testing.T) { + base := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "pre.sh"}, + "gitlab": {PreScript: "gl-pre.sh"}, + }, + } + child := &Harness{ + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "pre.sh"}, + }, + } + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "forge platform removal should abort diff") + assert.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "forge[gitlab]") + assert.Contains(t, result.Warnings[0], "removes platform from base") +} + +func TestDiffHarness_CustomizedSliceItem_NoDuplication(t *testing.T) { + // Customized-file semantics don't apply to concatenated slices (skills, + // plugins, providers) — including a customized item that already exists + // in the base would cause duplication after composition. + base := &Harness{ + Skills: []string{"skills/a.yaml", "skills/b.yaml"}, + } + child := &Harness{ + Skills: []string{"skills/a.yaml", "skills/b.yaml"}, + } + customized := map[string]bool{ + "skills/b.yaml": true, + } + result := DiffHarness(base, child, customized) + assert.Nil(t, result.Child, "identical slices should not produce a diff even with customized files") +} + +func TestDiffHarness_NilBaseEnvWithChildEnv(t *testing.T) { + base := &Harness{} + child := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"A": "1"}, + Sandbox: map[string]string{"B": "2"}, + }, + } + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child) + require.NotNil(t, result.Child.Env) + assert.Equal(t, map[string]string{"A": "1"}, result.Child.Env.Runner) + assert.Equal(t, map[string]string{"B": "2"}, result.Child.Env.Sandbox) +} + +func TestDiffHarness_BaseEnvWithNilChildEnv(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"A": "1"}, + Sandbox: map[string]string{"B": "2"}, + }, + } + child := &Harness{} + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "env removal should abort diff") + require.Len(t, result.Warnings, 1) + assert.Contains(t, result.Warnings[0], "env: child removes keys from base") +} + +func TestDiffHarness_BaseEnvWithNilChildEnv_EmptyBase(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{}, + } + child := &Harness{} + result := DiffHarness(base, child, nil) + assert.Nil(t, result.Child, "empty base env with nil child env should produce no diff") + assert.Empty(t, result.Warnings) +} + +func TestDiffHarness_RoundTrip(t *testing.T) { + maxFetches := 10 + base := &Harness{ + Agent: "agents/triage.md", + Doc: "agents/triage-doc.md", + Description: "Triage agent", + Role: "reviewer", + Slug: "triage", + Image: "ghcr.io/example:v1", + Policy: "policies/default.yaml", + Model: "opus", + PreScript: "scripts/pre.sh", + PostScript: "scripts/post.sh", + AgentInput: "input.md", + TimeoutMinutes: 10, + SandboxTimeoutSeconds: 60, + AllowRuntimeFetch: true, + MaxRuntimeFetches: &maxFetches, + AllowedRemoteResources: []string{"https://example.com/"}, + Skills: []string{"skills/a", "skills/b"}, + Plugins: []string{"plugin-a"}, + Providers: []string{"prov-1"}, + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + {Src: "b.txt", Dest: "/tmp/b"}, + }, + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + }, + RunnerEnv: map[string]string{"K1": "v1", "K2": "v2"}, + Env: &EnvConfig{ + Runner: map[string]string{"R1": "1"}, + Sandbox: map[string]string{"S1": "2"}, + }, + ValidationLoop: &ValidationLoop{ + Script: "validate.sh", + MaxIterations: 3, + }, + Security: &SecurityConfig{FailMode: "closed"}, + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "gh-pre.sh", + PostScript: "gh-post.sh", + Skills: []string{"skill/gh-a"}, + RunnerEnv: map[string]string{"GH": "1"}, + }, + }, + } + + childMaxFetches := 20 + child := &Harness{ + Agent: "agents/triage.md", + Doc: "agents/triage-doc.md", + Description: "Custom triage agent", + Role: "reviewer", + Slug: "custom-triage", + Image: "ghcr.io/example:v2", + Policy: "policies/default.yaml", + Model: "sonnet", + PreScript: "scripts/pre.sh", + PostScript: "scripts/custom-post.sh", + AgentInput: "input.md", + TimeoutMinutes: 20, + SandboxTimeoutSeconds: 120, + AllowRuntimeFetch: true, + MaxRuntimeFetches: &childMaxFetches, + AllowedRemoteResources: []string{"https://example.com/", "https://other.com/"}, + Skills: []string{"skills/a", "skills/b", "skills/c"}, + Plugins: []string{"plugin-a", "plugin-b"}, + Providers: []string{"prov-1", "prov-2"}, + HostFiles: []HostFile{ + {Src: "a.txt", Dest: "/tmp/a"}, + {Src: "b-v2.txt", Dest: "/tmp/b"}, + {Src: "c.txt", Dest: "/tmp/c"}, + }, + APIServers: []APIServer{ + {Name: "proxy", Script: "start.sh", Port: 8080}, + {Name: "metrics", Script: "metrics.sh", Port: 9090}, + }, + RunnerEnv: map[string]string{"K1": "v1", "K2": "changed", "K3": "new"}, + Env: &EnvConfig{ + Runner: map[string]string{"R1": "1", "R2": "new"}, + Sandbox: map[string]string{"S1": "2"}, + }, + ValidationLoop: &ValidationLoop{ + Script: "validate-v2.sh", + MaxIterations: 5, + }, + Security: &SecurityConfig{FailMode: "open"}, + Forge: map[string]*ForgeConfig{ + "github": { + PreScript: "gh-pre.sh", + PostScript: "gh-post-v2.sh", + Skills: []string{"skill/gh-a", "skill/gh-b"}, + RunnerEnv: map[string]string{"GH": "1", "GH2": "2"}, + }, + "gitlab": { + PreScript: "gl-pre.sh", + }, + }, + } + + result := DiffHarness(base, child, nil) + require.NotNil(t, result.Child, "diff should be non-nil for differing harnesses") + require.Empty(t, result.Warnings, "no warnings expected for additive changes") + + // Compose: merge base into the diff child (mergeBaseIntoChild fills + // zero-value fields from base, concatenates slices, merges maps). + mergeBaseIntoChild(base, result.Child) + + assert.Equal(t, child.Agent, result.Child.Agent) + assert.Equal(t, child.Description, result.Child.Description) + assert.Equal(t, child.Model, result.Child.Model) + assert.Equal(t, child.Slug, result.Child.Slug) + assert.Equal(t, child.Image, result.Child.Image) + assert.Equal(t, child.PostScript, result.Child.PostScript) + assert.Equal(t, child.TimeoutMinutes, result.Child.TimeoutMinutes) + assert.Equal(t, child.SandboxTimeoutSeconds, result.Child.SandboxTimeoutSeconds) + assert.Equal(t, child.AllowRuntimeFetch, result.Child.AllowRuntimeFetch) + assert.Equal(t, *child.MaxRuntimeFetches, *result.Child.MaxRuntimeFetches) + assert.Equal(t, child.AllowedRemoteResources, result.Child.AllowedRemoteResources) + assert.Equal(t, child.Skills, result.Child.Skills) + assert.Equal(t, child.Plugins, result.Child.Plugins) + assert.Equal(t, child.Providers, result.Child.Providers) + assert.Equal(t, child.RunnerEnv, result.Child.RunnerEnv) + assert.Equal(t, child.Env, result.Child.Env) + assert.Equal(t, child.ValidationLoop, result.Child.ValidationLoop) + assert.Equal(t, child.Security, result.Child.Security) + assert.Equal(t, child.HostFiles, result.Child.HostFiles) + assert.Equal(t, child.APIServers, result.Child.APIServers) + assert.Equal(t, child.Forge, result.Child.Forge) +} diff --git a/internal/scaffold/baseurl.go b/internal/scaffold/baseurl.go index 62bd451be1..5a301414cf 100644 --- a/internal/scaffold/baseurl.go +++ b/internal/scaffold/baseurl.go @@ -62,6 +62,20 @@ func HarnessBaseURLWithHash(harnessName, commitSHA string) (string, error) { return base + "#sha256=" + hash, nil } +// 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 +} + // HarnessNames returns the sorted list of harness template names // available in the embedded scaffold (e.g., ["code", "fix", "triage"]). func HarnessNames() ([]string, error) { diff --git a/internal/scaffold/baseurl_test.go b/internal/scaffold/baseurl_test.go index 7e348f9296..8d17c4b867 100644 --- a/internal/scaffold/baseurl_test.go +++ b/internal/scaffold/baseurl_test.go @@ -169,6 +169,37 @@ func TestHarnessNames(t *testing.T) { }) } +func TestHarnessContent(t *testing.T) { + t.Run("returns valid YAML bytes", func(t *testing.T) { + data, err := HarnessContent("review") + require.NoError(t, err) + assert.True(t, len(data) > 0) + assert.Contains(t, string(data), "agent:") + }) + + t.Run("invalid name errors", func(t *testing.T) { + _, err := HarnessContent("INVALID") + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid harness name") + }) + + t.Run("unknown harness errors", func(t *testing.T) { + _, err := HarnessContent("nonexistent") + assert.Error(t, err) + assert.Contains(t, err.Error(), "unknown harness") + }) + + t.Run("matches content hash", func(t *testing.T) { + data, err := HarnessContent("triage") + require.NoError(t, err) + sum := sha256.Sum256(data) + actual := hex.EncodeToString(sum[:]) + expected, err := HarnessContentHash("triage") + require.NoError(t, err) + assert.Equal(t, expected, actual) + }) +} + func TestHarnessBaseURLWithHashAllHarnesses(t *testing.T) { sha := "abcdef0123456789abcdef0123456789abcdef01"