From 6a3da6342f1918b4aaf489feccf31d939b8692f8 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Sun, 16 Aug 2026 23:08:41 -0400 Subject: [PATCH 01/17] feat(harness): implement CEL-guarded overlays (ADR 0088) Add an `overlays:` list field to the harness schema. Each entry has a `when:` CEL expression (same environment as `trigger:`) and the same override fields as `ForgeConfig`. At resolution time, all entries whose `when` evaluates to true are merged into the harness in declaration order using mergeForgeConfig semantics. Key changes: - OverlayEntry struct (When + inline ForgeConfig) in forge.go - validateOverlays: CEL compilation, field validation, mutual exclusion with forge - ResolveOverlays: evaluate when expressions, merge matches, nil out - LoadWithOpts/LoadWithBase pipeline: validateOverlays + ResolveOverlays inserted after their forge counterparts - mergeBaseIntoChild: overlay concatenation (base first, child appended) - Lint: forge deprecation warning recommending overlays - User docs: bring-your-own-agent guide updated to show overlays syntax forge: and overlays: cannot coexist in the same harness. forge: is deprecated but continues to work unchanged. Closes #2264 Closes #5989 Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/contributing/harness-composition.md | 13 +- docs/guides/user/bring-your-own-agent.md | 73 +++++----- internal/harness/compose.go | 32 ++++- internal/harness/compose_test.go | 107 +++++++++++++++ internal/harness/forge.go | 139 +++++++++++++++++++ internal/harness/forge_test.go | 168 +++++++++++++++++++++++ internal/harness/harness.go | 17 ++- internal/harness/harness_test.go | 62 +++++++++ internal/harness/lint.go | 8 ++ internal/harness/lint_test.go | 32 +++++ 10 files changed, 612 insertions(+), 39 deletions(-) diff --git a/docs/contributing/harness-composition.md b/docs/contributing/harness-composition.md index e009318591..5f9728313f 100644 --- a/docs/contributing/harness-composition.md +++ b/docs/contributing/harness-composition.md @@ -30,12 +30,21 @@ and update the others as needed. | Function | File | Purpose | |----------|------|---------| | `mergeBaseIntoChild` | `internal/harness/compose.go` | Merges base harness fields into child during `base:` composition | -| `mergeForgeConfig` | `internal/harness/forge.go` | Applies `forge.` overrides onto top-level harness fields | +| `mergeForgeConfig` | `internal/harness/forge.go` | Applies `forge.` or overlay overrides onto top-level harness fields | | `mergeForgeConfigInto` | `internal/harness/compose.go` | Merges base `ForgeConfig` fields into child `ForgeConfig` during `base:` composition | | `mergeSkills` | `internal/harness/compose.go` | Deduplicates skills by basename (base + child); merges file-level override maps when both define the same basename (child keys win) | | `mergeHostFiles` | `internal/harness/compose.go` | Deduplicates host files by dest path (base + child) | | `mergeForgeBlocks` | `internal/harness/compose.go` | Merges `forge:` maps key-by-key across base and child | +### Validation and resolution side + +| Function | File | Purpose | +|----------|------|---------| +| `validateForge` | `internal/harness/forge.go` | Validates `forge:` block keys and `ForgeConfig` field values | +| `validateOverlays` | `internal/harness/forge.go` | Validates `overlays:` entries — CEL `when` expressions and `ForgeConfig` field values; enforces mutual exclusion with `forge:` | +| `ResolveForge` | `internal/harness/forge.go` | Merges the selected forge platform's config into the harness and nils the forge map | +| `ResolveOverlays` | `internal/harness/forge.go` | Evaluates overlay `when` expressions against event data, merges matching entries in order, nils the overlays list | + ### How they correspond The merge functions define which fields participate in harness @@ -99,5 +108,7 @@ matching `_test.go` file. current overlay mechanism - [ADR-0064](../ADRs/0064-deprecate-customized-directory-overlay.md): Deprecate customized directory overlay +- [ADR-0088](../ADRs/0088-cel-guarded-overlays.md): CEL-guarded overlays — + generalizes forge-specific config with CEL expressions - Issue #5579: Harness field integration pipeline (complementary checklist covering the broader field addition workflow) diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index 12a4b9cff7..161c5e624c 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -170,17 +170,17 @@ validation_loop: timeout_minutes: 10 -forge: - github: - pre_script: scripts/pre-triage.sh - post_script: scripts/post-triage.sh - env: - runner: - GITHUB_ISSUE_URL: ${GITHUB_ISSUE_URL} - GH_TOKEN: ${GH_TOKEN} - sandbox: - GITHUB_ISSUE_URL: "${GITHUB_ISSUE_URL}" - GH_TOKEN: "${GH_TOKEN}" +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/pre-triage.sh + post_script: scripts/post-triage.sh + env: + runner: + GITHUB_ISSUE_URL: ${GITHUB_ISSUE_URL} + GH_TOKEN: ${GH_TOKEN} + sandbox: + GITHUB_ISSUE_URL: "${GITHUB_ISSUE_URL}" + GH_TOKEN: "${GH_TOKEN}" ``` Key patterns to note: @@ -188,7 +188,7 @@ Key patterns to note: - **`policy: policies/triage.yaml`** is a per-agent policy that includes filesystem, landlock, process, and network rules (via inline `network_policies`). This agent predates the provider-based pattern — new agents can use `providers:` instead (see [Minimum viable agent](#minimum-viable-agent)). - **`host_files`** copy credentials from the trusted runner into the sandbox. `expand: true` resolves `${VAR}` references before copying. - **`validation_loop.schema`** references the JSON schema file directly — the validation script checks agent output against it. -- **`forge.github`** scopes scripts, skills, providers, openshell, host_files, and env vars to GitHub. When running on GitLab, a `forge.gitlab` block would take effect instead. +- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Here, the overlay matches GitHub events. Multiple overlays can match a single event (e.g. one for the source system, another for the event type) — they are applied in declaration order. - **`common/env/gcp-vertex.env`** is referenced by relative path because both files live in the same repo. If your agent lives in a different repo, reference it by URL (see [Remote references](#referencing-resources-local-vs-remote)) or copy it locally. ## Harness field reference @@ -275,23 +275,23 @@ api_servers: # Host-side REST proxies exposed to sandbox env: # Env vars for the server process API_KEY: "${API_KEY}" -# ── Forge-specific overrides ────────────────────────────────── -forge: - github: - pre_script: scripts/pre-gh.sh - post_script: scripts/post-gh.sh - skills: [skills/github-specific] # Concatenated with top-level - providers: [providers/github.yaml] # Concatenated with top-level - openshell: - profiles: [profiles/github.yaml] # Concatenated with top-level - host_files: # Forge-specific host files - - src: env/github.env - dest: /run/secrets/forge.env - env: - runner: - GH_TOKEN: "${GH_TOKEN}" - gitlab: - pre_script: scripts/pre-gl.sh +# ── Conditional overrides (CEL-guarded) ────────────────────── +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/pre-gh.sh + post_script: scripts/post-gh.sh + skills: [skills/github-specific] # Concatenated with top-level + providers: [providers/github.yaml] # Concatenated with top-level + openshell: + profiles: [profiles/github.yaml] # Concatenated with top-level + host_files: # Overlay-specific host files + - src: env/github.env + dest: /run/secrets/forge.env + env: + runner: + GH_TOKEN: "${GH_TOKEN}" +- when: 'event.source.system == "jira"' + pre_script: scripts/pre-jira.sh # ── Security ────────────────────────────────────────────────── security: @@ -308,18 +308,25 @@ security: ### Deprecated fields +> **Deprecated:** `forge` is deprecated. Use `overlays` with CEL `when` +> expressions instead (see [ADR 0088](../../ADRs/0088-cel-guarded-overlays.md)). +> The `forge` field still works but emits a deprecation warning at lint time. +> Migration: each forge key becomes an overlay entry — e.g. `forge: github:` +> becomes `overlays: - when: 'event.source.system == "github"'`. +> `forge` and `overlays` cannot coexist in the same harness. + > **Deprecated:** `runner_env` is deprecated. Use `env.runner` > instead. The `runner_env` field still works but emits a deprecation warning > at runtime. Migration: move `runner_env:` entries under `env: runner:` and > delete the `runner_env:` block. -### Field merge rules (for `base` and `forge`) +### Field merge rules (for `base` and `overlays`) | Field type | Behavior | |-----------|----------| | Scalars (`model`, `pre_script`, `policy`, `image`, etc.) | Child wins if non-empty | | `skills` | Merged with deduplication by basename (child overrides base) | -| `providers`, `openshell.profiles` | Concatenated (base + child); also applies per-forge | +| `providers`, `openshell.profiles` | Concatenated (base + child); also applies per-overlay | | `plugins`, `api_servers` | Concatenated (base + child) | | `host_files` | Concatenated; child overrides by `dest` | | `env`, `runner_env` (deprecated) | Merged; child keys win | @@ -394,7 +401,7 @@ skills: timeout_minutes: 15 ``` -Base chains support up to 5 levels (`MaxBaseDepth` in `internal/harness/compose.go`). Circular references are detected and rejected. Resolution order: base chain → child overrides → forge selection. See [field merge rules](#field-merge-rules-for-base-and-forge) for how each field type combines. +Base chains support up to 5 levels (`MaxBaseDepth` in `internal/harness/compose.go`). Circular references are detected and rejected. Resolution order: base chain → child overrides → overlay resolution. See [field merge rules](#field-merge-rules-for-base-and-overlays) for how each field type combines. > **Note:** `allowed_remote_resources`, `allow_runtime_fetch`, and `max_runtime_fetches` are NOT inherited from base harnesses — the child must declare its own. This prevents a base harness from injecting arbitrary URL prefixes or enabling runtime fetching in the child. @@ -462,7 +469,7 @@ env: ### What you can configure -Any harness field can be overridden. The [field merge rules](#field-merge-rules-for-base-and-forge) determine how your overrides combine with the base: +Any harness field can be overridden. The [field merge rules](#field-merge-rules-for-base-and-overlays) determine how your overrides combine with the base: - **Change model, timeout, image, scripts** — scalars replace the base value. - **Add skills** — your entries are merged with the base's by basename; same-named skills override the base entry. **Add plugins or host_files** — your entries are concatenated with the base's. diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 41c6a77121..273a907260 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -76,6 +76,10 @@ type ComposeOpts struct { // for no-base harnesses. SourceURL string + // Event is the normalized event data for CEL overlay resolution (ADR 0088). + // If nil, ResolveOverlays is a no-op. + Event map[string]any + // allowSelfAllowlist permits using the child harness's own AllowedRemoteResources // when OrgAllowlist is empty. This is for testing only; production callers should // always provide OrgAllowlist from config.yaml. Unexported to prevent misuse. @@ -89,10 +93,10 @@ type ComposeOpts struct { // // Pipeline: // 1. LoadRaw(path) — preserves forge map -// 2. If base absent: resolve URL-sourced resources → ResolveForge → Validate → return +// 2. If base absent: resolve URL-sourced resources → ResolveForge → ResolveOverlays → Validate → return // 3. If base present: loadBaseChain recursively, then mergeBaseIntoChild // 4. Resolve remaining URL-sourced resources and scripts (child's own relative paths) -// 5. ResolveForge once on final merged result +// 5. ResolveForge and ResolveOverlays once on final merged result // 6. Validate // // When base is absent, this behaves identically to LoadWithOpts. @@ -151,9 +155,15 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, if err := child.validateForge(); err != nil { return nil, nil, fmt.Errorf("invalid harness: %w", err) } + if err := child.validateOverlays(); err != nil { + return nil, nil, fmt.Errorf("invalid harness: %w", err) + } if err := child.ResolveForge(opts.ForgePlatform); err != nil { return nil, nil, fmt.Errorf("resolving forge config: %w", err) } + if err := child.ResolveOverlays(opts.Event); err != nil { + return nil, nil, fmt.Errorf("resolving overlays: %w", err) + } if err := child.Validate(); err != nil { return nil, nil, fmt.Errorf("invalid harness: %w", err) } @@ -244,13 +254,19 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, deps = append(deps, pluginDeps...) } - // ResolveForge once on the merged result + // ResolveForge and ResolveOverlays once on the merged result if err := child.validateForge(); err != nil { return nil, nil, fmt.Errorf("invalid harness: %w", err) } + if err := child.validateOverlays(); err != nil { + return nil, nil, fmt.Errorf("invalid harness: %w", err) + } if err := child.ResolveForge(opts.ForgePlatform); err != nil { return nil, nil, fmt.Errorf("resolving forge config: %w", err) } + if err := child.ResolveOverlays(opts.Event); err != nil { + return nil, nil, fmt.Errorf("resolving overlays: %w", err) + } if err := child.Validate(); err != nil { return nil, nil, fmt.Errorf("invalid harness: %w", err) } @@ -650,6 +666,16 @@ func mergeBaseIntoChild(base, child *Harness) { if base.Forge != nil { child.Forge = mergeForgeBlocks(base.Forge, child.Forge) } + + // Overlays: concatenated (base first, child appended) — same as plugins, + // providers, api_servers. Declaration order matters: later entries override + // earlier ones for scalars, so child entries naturally take precedence. + if base.Overlays != nil { + merged := make([]OverlayEntry, 0, len(base.Overlays)+len(child.Overlays)) + merged = append(merged, base.Overlays...) + merged = append(merged, child.Overlays...) + child.Overlays = merged + } } // isFullsendCachePath reports whether p is a path already inside fullsend's diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 60da06d3cb..65a9f762f1 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -8325,3 +8325,110 @@ base: `+baseURL+` } assert.True(t, foundPlugin, "should have plugin dependency") } + +func TestLoadWithBase_OverlayConcatBothHaveOverlays(t *testing.T) { + dir := t.TempDir() + + baseContent := ` +agent: agents/test.md +role: fix +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/base-gh.sh +- when: 'event.source.system == "jira"' + pre_script: scripts/base-jira.sh +` + childContent := ` +base: base.yaml +overlays: +- when: 'event.entity.kind == "issue"' + post_script: scripts/child-issue.sh +` + require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseContent), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childContent), 0o644)) + + h, _, err := LoadWithBase(context.Background(), filepath.Join(dir, "child.yaml"), ComposeOpts{ + WorkspaceRoot: dir, + Event: map[string]any{"source": map[string]any{"system": "github"}, "entity": map[string]any{"kind": "issue"}}, + }) + require.NoError(t, err) + // Base overlay matched → pre_script set + assert.Equal(t, "scripts/base-gh.sh", h.PreScript) + // Child overlay matched → post_script set + assert.Equal(t, "scripts/child-issue.sh", h.PostScript) +} + +func TestLoadWithBase_OverlayConcatOnlyBaseHasOverlays(t *testing.T) { + dir := t.TempDir() + + baseContent := ` +agent: agents/test.md +role: fix +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/gh.sh +` + childContent := ` +base: base.yaml +` + require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseContent), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childContent), 0o644)) + + h, _, err := LoadWithBase(context.Background(), filepath.Join(dir, "child.yaml"), ComposeOpts{ + WorkspaceRoot: dir, + Event: map[string]any{"source": map[string]any{"system": "github"}}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) +} + +func TestLoadWithBase_OverlayConcatOnlyChildHasOverlays(t *testing.T) { + dir := t.TempDir() + + baseContent := ` +agent: agents/test.md +role: fix +pre_script: scripts/base.sh +` + childContent := ` +base: base.yaml +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/child-gh.sh +` + require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseContent), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childContent), 0o644)) + + h, _, err := LoadWithBase(context.Background(), filepath.Join(dir, "child.yaml"), ComposeOpts{ + WorkspaceRoot: dir, + Event: map[string]any{"source": map[string]any{"system": "github"}}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/child-gh.sh", h.PreScript) +} + +func TestLoadWithBase_OverlayResolution(t *testing.T) { + dir := t.TempDir() + + baseContent := ` +agent: agents/test.md +role: fix +pre_script: scripts/base.sh +` + childContent := ` +base: base.yaml +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/gh.sh +` + require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseContent), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childContent), 0o644)) + + h, _, err := LoadWithBase(context.Background(), filepath.Join(dir, "child.yaml"), ComposeOpts{ + WorkspaceRoot: dir, + Event: map[string]any{"source": map[string]any{"system": "github"}}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) + assert.Nil(t, h.Overlays) +} diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 96f2149e9c..c5c2871406 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -4,6 +4,8 @@ import ( "fmt" "sort" "strings" + + "github.com/google/cel-go/common/types" ) // ForgeConfig holds platform-specific harness configuration. @@ -24,6 +26,16 @@ type ForgeConfig struct { Env *EnvConfig `yaml:"env,omitempty"` } +// OverlayEntry is a CEL-guarded conditional config block (ADR 0088). +// Each entry carries a CEL expression in When (evaluated against the event +// variable, same environment as trigger:) and the same override fields as +// ForgeConfig. Entries whose When evaluates to true are merged into the +// harness in declaration order. +type OverlayEntry struct { + When string `yaml:"when"` + ForgeConfig `yaml:",inline"` +} + var validForgeKeys = map[string]bool{ "github": true, "gitlab": true, @@ -121,6 +133,133 @@ func (h *Harness) validateForge() error { return nil } +// validateOverlayForgeConfig validates a ForgeConfig embedded in an overlay +// entry, applying the same checks as validateForge per entry. +func validateOverlayForgeConfig(idx int, fc *ForgeConfig) error { + prefix := fmt.Sprintf("overlays[%d]", idx) + if fc.Policy != "" && IsURL(fc.Policy) { + if _, _, hasHash := ParseIntegrityHash(fc.Policy); !hasHash { + return fmt.Errorf("%s.policy URL must include #sha256=... integrity hash", prefix) + } + } + if fc.PreScript != "" && IsURL(fc.PreScript) { + return fmt.Errorf("%s.pre_script must be a local path, not a URL", prefix) + } + if fc.PostScript != "" && IsURL(fc.PostScript) { + return fmt.Errorf("%s.post_script must be a local path, not a URL", prefix) + } + for i, s := range fc.Skills { + if IsURL(s.Source) { + if _, _, hasHash := ParseIntegrityHash(s.Source); !hasHash { + return fmt.Errorf("%s.skills[%d] URL must include #sha256=... integrity hash", prefix, i) + } + } + } + if err := ValidateSkillOverrides(fc.Skills); err != nil { + return fmt.Errorf("%s: %w", prefix, err) + } + for i, p := range fc.Providers { + if IsURL(p) { + if _, _, hasHash := ParseIntegrityHash(p); !hasHash { + return fmt.Errorf("%s.providers[%d] URL must include #sha256=... integrity hash", prefix, i) + } + } + } + if fc.OpenShell != nil { + for i, p := range fc.OpenShell.Profiles { + if IsURL(p) { + if _, _, hasHash := ParseIntegrityHash(p); !hasHash { + return fmt.Errorf("%s.openshell.profiles[%d] URL must include #sha256=... integrity hash", prefix, i) + } + } + } + } + for i, hf := range fc.HostFiles { + if hf.Src == "" { + return fmt.Errorf("%s.host_files[%d]: src is required", prefix, i) + } + if hf.Dest == "" { + return fmt.Errorf("%s.host_files[%d]: dest is required", prefix, i) + } + if IsURL(hf.Src) { + return fmt.Errorf("%s.host_files[%d].src must be a local path, not a URL", prefix, i) + } + } + if fc.ValidationLoop != nil { + if fc.ValidationLoop.Script == "" { + return fmt.Errorf("%s.validation_loop.script is required when validation_loop is set", prefix) + } + if IsURL(fc.ValidationLoop.Script) { + return fmt.Errorf("%s.validation_loop.script must be a local path, not a URL", prefix) + } + if fc.ValidationLoop.Schema != "" && IsURL(fc.ValidationLoop.Schema) { + return fmt.Errorf("%s.validation_loop.schema must be a local path, not a URL", prefix) + } + } + return nil +} + +// validateOverlays checks that the overlays section is well-formed: +// mutual exclusion with forge, CEL expression compilation, and +// ForgeConfig field validation per entry. +func (h *Harness) validateOverlays() error { + if len(h.Overlays) == 0 { + return nil + } + if h.Forge != nil { + return fmt.Errorf("forge and overlays cannot coexist in the same harness; migrate forge entries to overlays") + } + for i, entry := range h.Overlays { + when := strings.TrimSpace(entry.When) + if when == "" { + return fmt.Errorf("overlays[%d].when is required", i) + } + env, err := NewTriggerEnv() + if err != nil { + return fmt.Errorf("overlays[%d]: creating CEL env: %w", i, err) + } + ast, issues := env.Compile(when) + if issues != nil && issues.Err() != nil { + return fmt.Errorf("overlays[%d].when: %w", i, issues.Err()) + } + if !ast.OutputType().IsExactType(types.BoolType) { + return fmt.Errorf("overlays[%d].when must evaluate to bool, got %v", i, ast.OutputType()) + } + fc := entry.ForgeConfig + if err := validateOverlayForgeConfig(i, &fc); err != nil { + return err + } + } + return nil +} + +// ResolveOverlays evaluates each overlay's When expression against event +// data and merges matching entries into the harness in declaration order. +// After resolution, h.Overlays is set to nil (consumed). When event is +// nil or h.Overlays is empty, this is a no-op. +func (h *Harness) ResolveOverlays(event map[string]any) error { + if len(h.Overlays) == 0 { + h.Overlays = nil + return nil + } + if event == nil { + h.Overlays = nil + return nil + } + for i, entry := range h.Overlays { + matched, err := EvaluateTrigger(entry.When, event) + if err != nil { + return fmt.Errorf("overlays[%d].when: %w", i, err) + } + if matched { + fc := entry.ForgeConfig + mergeForgeConfig(h, &fc) + } + } + h.Overlays = nil + return nil +} + // ResolveForge merges forge-specific overrides into the harness in place. // After merging, h.Forge is set to nil (consumed). If platform is empty or // h.Forge is nil, this is a no-op. If platform is not present in h.Forge, diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index da918bfe14..4cf3d4da2f 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1004,3 +1004,171 @@ pre_script: scripts/pre.sh assert.Nil(t, h.Forge) assert.Equal(t, "scripts/pre.sh", h.PreScript) } + +// --- Overlay tests --- + +func TestValidateOverlays_EmptyWhenRejected(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: "", ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].when is required") +} + +func TestValidateOverlays_NonBoolCELRejected(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: "event.source.system", ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.Error(t, err) + assert.Contains(t, err.Error(), "must evaluate to bool") +} + +func TestValidateOverlays_ValidCELAccepted(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.NoError(t, err) +} + +func TestValidateOverlays_InvalidScriptPathRejected(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "https://example.com/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].pre_script must be a local path, not a URL") +} + +func TestValidateOverlays_MutualExclusionWithForge(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "scripts/pre-gh.sh"}, + }, + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.Error(t, err) + assert.Contains(t, err.Error(), "forge and overlays cannot coexist") +} + +func TestValidateOverlays_NoOverlaysIsNoop(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + } + err := h.validateOverlays() + require.NoError(t, err) +} + +func TestResolveOverlays_SingleMatch(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "github"}} + err := h.ResolveOverlays(event) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) + assert.Nil(t, h.Overlays) +} + +func TestResolveOverlays_MultipleMatchesScalarLastWins(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "a.sh"}}, + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "b.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "github"}} + err := h.ResolveOverlays(event) + require.NoError(t, err) + assert.Equal(t, "b.sh", h.PreScript) +} + +func TestResolveOverlays_ListFieldsAccumulate(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Skills: []SkillEntry{{Source: "skills/base"}}, + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{Skills: []SkillEntry{{Source: "skills/gh"}}}}, + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{Skills: []SkillEntry{{Source: "skills/extra"}}}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "github"}} + err := h.ResolveOverlays(event) + require.NoError(t, err) + require.Len(t, h.Skills, 3) + assert.Equal(t, "skills/base", h.Skills[0].Source) + assert.Equal(t, "skills/gh", h.Skills[1].Source) + assert.Equal(t, "skills/extra", h.Skills[2].Source) +} + +func TestResolveOverlays_NoMatchUnchanged(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", + Overlays: []OverlayEntry{ + {When: `event.source.system == "jira"`, ForgeConfig: ForgeConfig{PreScript: "scripts/jira.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "github"}} + err := h.ResolveOverlays(event) + require.NoError(t, err) + assert.Equal(t, "scripts/common.sh", h.PreScript) + assert.Nil(t, h.Overlays) +} + +func TestResolveOverlays_NilEventNoop(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + }, + } + err := h.ResolveOverlays(nil) + require.NoError(t, err) + assert.Nil(t, h.Overlays) +} + +func TestResolveOverlays_EmptyOverlaysNoop(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", + } + err := h.ResolveOverlays(map[string]any{"source": map[string]any{"system": "github"}}) + require.NoError(t, err) + assert.Equal(t, "scripts/common.sh", h.PreScript) +} diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 08836763a7..c2e4c5c973 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -347,7 +347,8 @@ type Harness struct { AllowRuntimeFetch bool `yaml:"allow_runtime_fetch,omitempty"` // opt-in to runtime skill fetching (default: false) MaxRuntimeFetches *int `yaml:"max_runtime_fetches,omitempty"` // per-run fetch cap; nil = default (10), valid range 1-1000 Forge map[string]*ForgeConfig `yaml:"forge,omitempty"` - Trigger string `yaml:"trigger,omitempty"` // optional CEL boolean over normevent (ADR 0061) + Overlays []OverlayEntry `yaml:"overlays,omitempty"` // CEL-guarded conditional config (ADR 0088) + Trigger string `yaml:"trigger,omitempty"` // optional CEL boolean over normevent (ADR 0061) } // Load reads a harness YAML file from path, unmarshals it, and validates it. @@ -369,9 +370,10 @@ func Load(path string) (*Harness, error) { return &h, nil } -// LoadOpts configures forge-aware harness loading. +// LoadOpts configures forge-aware and overlay-aware harness loading. type LoadOpts struct { ForgePlatform string + Event map[string]any // event data for CEL overlay resolution (ADR 0088) } // LoadWithOpts reads a harness YAML file and applies forge resolution before @@ -389,10 +391,18 @@ func LoadWithOpts(path string, opts LoadOpts) (*Harness, error) { return nil, fmt.Errorf("invalid harness: %w", err) } + if err := h.validateOverlays(); err != nil { + return nil, fmt.Errorf("invalid harness: %w", err) + } + if err := h.ResolveForge(opts.ForgePlatform); err != nil { return nil, fmt.Errorf("resolving forge config: %w", err) } + if err := h.ResolveOverlays(opts.Event); err != nil { + return nil, fmt.Errorf("resolving overlays: %w", err) + } + if err := h.Validate(); err != nil { return nil, fmt.Errorf("invalid harness: %w", err) } @@ -523,6 +533,9 @@ func (h *Harness) Validate() error { if err := h.validateForge(); err != nil { return err } + if err := h.validateOverlays(); err != nil { + return err + } if err := ValidateTriggerExpression(h.Trigger); err != nil { return fmt.Errorf("trigger: %w", err) } diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index bfb3d2493e..c36f65935f 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -2272,3 +2272,65 @@ func TestHasURLDirResources(t *testing.T) { }) } } + +func TestLoadWithOpts_OverlayResolution(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/gh.sh +- when: 'event.source.system == "jira"' + pre_script: scripts/jira.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + Event: map[string]any{"source": map[string]any{"system": "github"}}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) + assert.Nil(t, h.Overlays) +} + +func TestLoadWithOpts_OverlayNoEvent(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/gh.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{}) + require.NoError(t, err) + // No event → overlays are a no-op and consumed + assert.Nil(t, h.Overlays) +} + +func TestLoadWithOpts_OverlayAndForgeReject(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +forge: + github: + pre_script: scripts/gh.sh +overlays: +- when: 'event.source.system == "github"' + pre_script: scripts/gh2.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + _, err := LoadWithOpts(path, LoadOpts{ + Event: map[string]any{"source": map[string]any{"system": "github"}}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "forge and overlays cannot coexist") +} diff --git a/internal/harness/lint.go b/internal/harness/lint.go index 912127cdfd..e84318386a 100644 --- a/internal/harness/lint.go +++ b/internal/harness/lint.go @@ -71,6 +71,14 @@ func (h *Harness) Lint() []Diagnostic { } } + if h.Forge != nil { + diags = append(diags, Diagnostic{ + Severity: SeverityWarning, + Field: "forge", + Message: "forge is deprecated; use overlays with CEL when expressions instead (see ADR 0088)", + }) + } + if strings.TrimSpace(h.Trigger) != "" { if err := ValidateTriggerExpression(h.Trigger); err != nil { diags = append(diags, Diagnostic{ diff --git a/internal/harness/lint_test.go b/internal/harness/lint_test.go index 04252ebb14..3719b6102f 100644 --- a/internal/harness/lint_test.go +++ b/internal/harness/lint_test.go @@ -90,6 +90,38 @@ func TestLint_EnvSandboxWithHostFilesNoOverlap(t *testing.T) { assert.Empty(t, diags) } +func TestLint_ForgeDeprecationWarning(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Forge: map[string]*ForgeConfig{ + "github": {PreScript: "scripts/gh.sh"}, + }, + } + diags := h.Lint() + var found bool + for _, d := range diags { + if d.Field == "forge" { + found = true + assert.Equal(t, SeverityWarning, d.Severity) + assert.Contains(t, d.Message, "deprecated") + assert.Contains(t, d.Message, "overlays") + } + } + assert.True(t, found, "expected forge deprecation warning") +} + +func TestLint_NoForgeNoDeprecationWarning(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + } + diags := h.Lint() + for _, d := range diags { + assert.NotEqual(t, "forge", d.Field, "should not have forge warning") + } +} + func TestDiagnostic_String(t *testing.T) { t.Run("warning", func(t *testing.T) { d := Diagnostic{Severity: SeverityWarning, Field: "role", Message: "msg"} From 8155464aa4ea5ca061bb20f9165ba2b380290b03 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 17 Aug 2026 12:22:07 +0000 Subject: [PATCH 02/17] fix(harness): add URL-base resolution for overlay entries and docs fixes Add overlay-iteration loops to resolveBaseScripts, resolveBaseResources, resolveBaseHostFiles, resolveBaseProfiles, and resolveBaseProviders, paralleling the existing forge-iteration loops. Without this, overlay entries inherited from URL-sourced base harnesses would have unresolved relative paths for scripts, skills, host_files, providers, and profiles. Also: annotate ADR 0045 with cross-reference to ADR 0088 (forge deprecation), clarify overlay merge semantics in user docs, and enhance validateOverlayForgeConfig doc comment to explain the ForgeConfig embedding relationship. Addresses review feedback on #6285 --- .../0045-forge-portable-harness-schema.md | 6 + docs/guides/user/bring-your-own-agent.md | 2 +- internal/harness/compose.go | 186 +++++++++++++++++- internal/harness/forge.go | 6 +- 4 files changed, 196 insertions(+), 4 deletions(-) diff --git a/docs/ADRs/0045-forge-portable-harness-schema.md b/docs/ADRs/0045-forge-portable-harness-schema.md index 430428f3d4..c9870cefcc 100644 --- a/docs/ADRs/0045-forge-portable-harness-schema.md +++ b/docs/ADRs/0045-forge-portable-harness-schema.md @@ -24,6 +24,11 @@ Superseded by [ADR-0088](0088-cel-guarded-overlays.md) (CEL-guarded overlays) > acceptance. For the current authoritative version, see > [Harness Field Reference](../contributing/harness-fields.md). +> **Note:** The `forge:` section introduced by this ADR is deprecated in favor +> of CEL-guarded `overlays:` — see [ADR 0088](0088-cel-guarded-overlays.md). +> The rest of this ADR (role, slug, base composition, merge rules) remains +> current. + ## Context ADR 0024 established the harness YAML as the self-contained execution unit @@ -766,3 +771,4 @@ forge-specific artifact. The harness and agent definition are portable. - [Issue #322](https://github.com/fullsend-ai/fullsend/issues/322): Platform-specific component identification - [Issue #1986](https://github.com/fullsend-ai/fullsend/issues/1986): Default agents should use the same delivery mechanism as custom agents - [ADR 0058](0058-agent-registration.md): Agent registration — re-adds `agents` config key with URL/path semantics (supersedes the role/name/slug schema removed in Phase 4) +- [ADR 0088](0088-cel-guarded-overlays.md): CEL-guarded overlays — deprecates the `forge:` section in favor of `overlays:` with CEL `when` expressions diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index 161c5e624c..37bb35e83d 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -188,7 +188,7 @@ Key patterns to note: - **`policy: policies/triage.yaml`** is a per-agent policy that includes filesystem, landlock, process, and network rules (via inline `network_policies`). This agent predates the provider-based pattern — new agents can use `providers:` instead (see [Minimum viable agent](#minimum-viable-agent)). - **`host_files`** copy credentials from the trusted runner into the sandbox. `expand: true` resolves `${VAR}` references before copying. - **`validation_loop.schema`** references the JSON schema file directly — the validation script checks agent output against it. -- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Here, the overlay matches GitHub events. Multiple overlays can match a single event (e.g. one for the source system, another for the event type) — they are applied in declaration order. +- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Here, the overlay matches GitHub events. Multiple overlays can match a single event — all matching entries are merged in declaration order (later scalars win, lists accumulate). - **`common/env/gcp-vertex.env`** is referenced by relative path because both files live in the same repo. If your agent lives in a different repo, reference it by URL (see [Remote references](#referencing-resources-local-vs-remote)) or copy it locally. ## Harness field reference diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 273a907260..6101731319 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -850,6 +850,70 @@ func resolveBaseScripts(ctx context.Context, base *Harness, baseURL string, allo } } + // Overlay-level scripts: same fetch treatment as forge-level scripts. + // Each overlay entry embeds a ForgeConfig whose script, policy, and + // validation_loop fields may contain relative paths from the base URL. + for i := range base.Overlays { + oc := &base.Overlays[i].ForgeConfig + overlayScripts := []struct { + name string + ptr *string + }{ + {fmt.Sprintf("overlays[%d].pre_script", i), &oc.PreScript}, + {fmt.Sprintf("overlays[%d].post_script", i), &oc.PostScript}, + } + for _, f := range overlayScripts { + if *f.ptr == "" || IsURL(*f.ptr) || isFullsendCachePath(*f.ptr, opts.WorkspaceRoot) { + continue + } + if err := validateBaseRelPath(f.name, *f.ptr); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseScriptOrDir(ctx, f.name, baseURLDir, *f.ptr, allowlist, opts) + if err != nil { + return nil, err + } + *f.ptr = cachePath + deps = append(deps, dep) + } + if oc.Policy != "" && !IsURL(oc.Policy) && !isFullsendCachePath(oc.Policy, opts.WorkspaceRoot) { + fieldName := fmt.Sprintf("overlays[%d].policy", i) + if err := validateBaseRelPath(fieldName, oc.Policy); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, fieldName, baseURLDir, oc.Policy, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + oc.Policy = cachePath + deps = append(deps, dep) + } + if oc.ValidationLoop != nil && oc.ValidationLoop.Script != "" && !IsURL(oc.ValidationLoop.Script) && !isFullsendCachePath(oc.ValidationLoop.Script, opts.WorkspaceRoot) { + fieldName := fmt.Sprintf("overlays[%d].validation_loop.script", i) + if err := validateBaseRelPath(fieldName, oc.ValidationLoop.Script); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseScriptOrDir(ctx, fieldName, baseURLDir, oc.ValidationLoop.Script, allowlist, opts) + if err != nil { + return nil, err + } + oc.ValidationLoop.Script = cachePath + deps = append(deps, dep) + } + if oc.ValidationLoop != nil && oc.ValidationLoop.Schema != "" && !IsURL(oc.ValidationLoop.Schema) && !isFullsendCachePath(oc.ValidationLoop.Schema, opts.WorkspaceRoot) { + fieldName := fmt.Sprintf("overlays[%d].validation_loop.schema", i) + if err := validateBaseRelPath(fieldName, oc.ValidationLoop.Schema); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, fieldName, baseURLDir, oc.ValidationLoop.Schema, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + oc.ValidationLoop.Schema = cachePath + deps = append(deps, dep) + } + } + // agent_input is a directory at runtime (uploaded recursively) and cannot // be fetched as a single file from a URL. Clear it so it doesn't resolve // against the child's local directory where it won't exist. @@ -973,6 +1037,42 @@ func resolveBaseResources(ctx context.Context, base *Harness, baseURL string, al } } + // Overlay-specific skills: same fetch treatment as forge-specific skills. + for i := range base.Overlays { + oc := &base.Overlays[i].ForgeConfig + for j, skill := range oc.Skills { + if skill.Source != "" && !IsURL(skill.Source) && !isFullsendCachePath(skill.Source, opts.WorkspaceRoot) { + fieldName := fmt.Sprintf("overlays[%d].skills[%d]", i, j) + if err := validateBaseRelPath(fieldName, skill.Source); err != nil { + return nil, err + } + dep, localDir, err := fetchBaseSkill(ctx, fieldName, baseURLDir, skill.Source, allowlist, opts) + if err != nil { + return nil, err + } + oc.Skills[j].Source = localDir + deps = append(deps, dep) + } + + for key, val := range skill.Overrides { + if val == nil || *val == "" || IsURL(*val) || isFullsendCachePath(*val, opts.WorkspaceRoot) { + continue + } + overrideField := fmt.Sprintf("overlays[%d].skills[%d].overrides[%s]", i, j, key) + if err := validateBaseRelPath(overrideField, *val); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, overrideField, baseURLDir, *val, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + resolved := cachePath + oc.Skills[j].Overrides[key] = &resolved + deps = append(deps, dep) + } + } + } + return deps, nil } @@ -1034,6 +1134,28 @@ func resolveBaseHostFiles(ctx context.Context, base *Harness, baseURL string, al } } + // Overlay-specific host_files: same fetch treatment as forge-specific + // host_files. Entries with ${VAR} expansion are left unchanged. + for i := range base.Overlays { + oc := &base.Overlays[i].ForgeConfig + for j := range oc.HostFiles { + src := oc.HostFiles[j].Src + if src == "" || strings.Contains(src, "${") || IsURL(src) || isFullsendCachePath(src, opts.WorkspaceRoot) { + continue + } + fieldName := fmt.Sprintf("overlays[%d].host_files[%d].src", i, j) + if err := validateBaseRelPath(fieldName, src); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, fieldName, baseURLDir, src, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + oc.HostFiles[j].Src = cachePath + deps = append(deps, dep) + } + } + return deps, nil } @@ -1048,7 +1170,14 @@ func resolveBaseProfiles(ctx context.Context, base *Harness, baseURL string, all if fc := base.Forge[opts.ForgePlatform]; fc != nil && fc.OpenShell != nil && len(fc.OpenShell.Profiles) > 0 { hasForge = true } - if !hasTopLevel && !hasForge { + hasOverlay := false + for _, oe := range base.Overlays { + if oe.OpenShell != nil && len(oe.OpenShell.Profiles) > 0 { + hasOverlay = true + break + } + } + if !hasTopLevel && !hasForge && !hasOverlay { return nil, nil } @@ -1097,6 +1226,29 @@ func resolveBaseProfiles(ctx context.Context, base *Harness, baseURL string, all } } + // Overlay-specific profiles: same fetch treatment as forge-specific profiles. + for i := range base.Overlays { + oc := &base.Overlays[i].ForgeConfig + if oc.OpenShell == nil { + continue + } + for j, p := range oc.OpenShell.Profiles { + if p == "" || IsURL(p) || isFullsendCachePath(p, opts.WorkspaceRoot) { + continue + } + fieldName := fmt.Sprintf("overlays[%d].openshell.profiles[%d]", i, j) + if err := validateBaseRelPath(fieldName, p); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, fieldName, baseURLDir, p, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + oc.OpenShell.Profiles[j] = cachePath + deps = append(deps, dep) + } + } + return deps, nil } @@ -1111,7 +1263,14 @@ func resolveBaseProviders(ctx context.Context, base *Harness, baseURL string, al if fc := base.Forge[opts.ForgePlatform]; fc != nil && len(fc.Providers) > 0 { hasForge = true } - if !hasTopLevel && !hasForge { + hasOverlay := false + for _, oe := range base.Overlays { + if len(oe.Providers) > 0 { + hasOverlay = true + break + } + } + if !hasTopLevel && !hasForge && !hasOverlay { return nil, nil } @@ -1164,6 +1323,29 @@ func resolveBaseProviders(ctx context.Context, base *Harness, baseURL string, al } } + // Overlay-specific providers: same fetch treatment as forge-specific providers. + for i := range base.Overlays { + oc := &base.Overlays[i].ForgeConfig + for j, p := range oc.Providers { + if p == "" || IsURL(p) || isFullsendCachePath(p, opts.WorkspaceRoot) { + continue + } + if !IsProviderPath(p) { + continue + } + fieldName := fmt.Sprintf("overlays[%d].providers[%d]", i, j) + if err := validateBaseRelPath(fieldName, p); err != nil { + return nil, err + } + dep, cachePath, err := fetchBaseFile(ctx, fieldName, baseURLDir, p, allowlist, opts, "resource", false) + if err != nil { + return nil, err + } + oc.Providers[j] = cachePath + deps = append(deps, dep) + } + } + return deps, nil } diff --git a/internal/harness/forge.go b/internal/harness/forge.go index c5c2871406..a7b9863067 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -134,7 +134,11 @@ func (h *Harness) validateForge() error { } // validateOverlayForgeConfig validates a ForgeConfig embedded in an overlay -// entry, applying the same checks as validateForge per entry. +// entry, applying the same checks as validateForge per entry. OverlayEntry +// embeds ForgeConfig via yaml:",inline" (see OverlayEntry), so overlay +// entries carry the same override fields as forge platform blocks. The +// "ForgeConfig" name is a legacy artifact from the forge feature being +// deprecated in favor of overlays (ADR 0088). func validateOverlayForgeConfig(idx int, fc *ForgeConfig) error { prefix := fmt.Sprintf("overlays[%d]", idx) if fc.Policy != "" && IsURL(fc.Policy) { From 258df3a7f7deb326ea3f7b3a23a367328c7f8e6b Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 17 Aug 2026 16:58:39 -0400 Subject: [PATCH 03/17] refactor(harness): adopt first-match-wins overlays and expanded CEL env Update overlay resolution to match revised ADR 0088: - Switch from merge-all to first-match-wins semantics: the first overlay entry whose `when` evaluates to true is merged; remaining entries are skipped. - Expand the overlay CEL environment with `runtime.forge` (effective forge platform) and `config` (per-repo config from config.yaml) alongside the existing `event` variable. - Add `Config map[string]any` to LoadOpts and ComposeOpts; wire ForgePlatform and Config through to ResolveOverlays. - Update docs to reflect first-match-wins and runtime.forge usage. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/contributing/harness-composition.md | 2 +- docs/guides/user/bring-your-own-agent.md | 32 ++++++-- internal/harness/compose.go | 8 +- internal/harness/compose_test.go | 17 +++-- internal/harness/forge.go | 22 +++--- internal/harness/forge_test.go | 96 +++++++++++++++++++++--- internal/harness/harness.go | 3 +- internal/harness/harness_test.go | 42 +++++++++++ internal/harness/trigger.go | 56 ++++++++++++++ 9 files changed, 240 insertions(+), 38 deletions(-) diff --git a/docs/contributing/harness-composition.md b/docs/contributing/harness-composition.md index 5f9728313f..cc09c4a5a0 100644 --- a/docs/contributing/harness-composition.md +++ b/docs/contributing/harness-composition.md @@ -43,7 +43,7 @@ and update the others as needed. | `validateForge` | `internal/harness/forge.go` | Validates `forge:` block keys and `ForgeConfig` field values | | `validateOverlays` | `internal/harness/forge.go` | Validates `overlays:` entries — CEL `when` expressions and `ForgeConfig` field values; enforces mutual exclusion with `forge:` | | `ResolveForge` | `internal/harness/forge.go` | Merges the selected forge platform's config into the harness and nils the forge map | -| `ResolveOverlays` | `internal/harness/forge.go` | Evaluates overlay `when` expressions against event data, merges matching entries in order, nils the overlays list | +| `ResolveOverlays` | `internal/harness/forge.go` | Evaluates overlay `when` expressions against event/runtime/config CEL environment; merges the first matching entry (first-match-wins) and nils the overlays list | ### How they correspond diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index 37bb35e83d..b7c7163a8d 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -171,7 +171,7 @@ validation_loop: timeout_minutes: 10 overlays: -- when: 'event.source.system == "github"' +- when: 'runtime.forge == "github"' pre_script: scripts/pre-triage.sh post_script: scripts/post-triage.sh env: @@ -188,7 +188,7 @@ Key patterns to note: - **`policy: policies/triage.yaml`** is a per-agent policy that includes filesystem, landlock, process, and network rules (via inline `network_policies`). This agent predates the provider-based pattern — new agents can use `providers:` instead (see [Minimum viable agent](#minimum-viable-agent)). - **`host_files`** copy credentials from the trusted runner into the sandbox. `expand: true` resolves `${VAR}` references before copying. - **`validation_loop.schema`** references the JSON schema file directly — the validation script checks agent output against it. -- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Here, the overlay matches GitHub events. Multiple overlays can match a single event — all matching entries are merged in declaration order (later scalars win, lists accumulate). +- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Resolution is first-match-wins: the first entry whose `when` evaluates to true is merged; remaining entries are skipped. The CEL environment exposes `event` (the triggering event), `runtime.forge` (the effective forge platform), and `config` (per-repo config from config.yaml). - **`common/env/gcp-vertex.env`** is referenced by relative path because both files live in the same repo. If your agent lives in a different repo, reference it by URL (see [Remote references](#referencing-resources-local-vs-remote)) or copy it locally. ## Harness field reference @@ -275,12 +275,19 @@ api_servers: # Host-side REST proxies exposed to sandbox env: # Env vars for the server process API_KEY: "${API_KEY}" -# ── Conditional overrides (CEL-guarded) ────────────────────── +# ── Conditional overrides (CEL-guarded, first-match-wins) ──── overlays: -- when: 'event.source.system == "github"' +- when: 'event.source.system == "jira" && runtime.forge == "github"' + pre_script: scripts/pre-jira-on-gh.sh + skills: [skills/jira-read] # Merged with top-level + env: + runner: + GH_TOKEN: "${GH_TOKEN}" + JIRA_TOKEN: "${JIRA_TOKEN}" +- when: 'runtime.forge == "github"' pre_script: scripts/pre-gh.sh post_script: scripts/post-gh.sh - skills: [skills/github-specific] # Concatenated with top-level + skills: [skills/github-specific] # Merged with top-level providers: [providers/github.yaml] # Concatenated with top-level openshell: profiles: [profiles/github.yaml] # Concatenated with top-level @@ -312,8 +319,12 @@ security: > expressions instead (see [ADR 0088](../../ADRs/0088-cel-guarded-overlays.md)). > The `forge` field still works but emits a deprecation warning at lint time. > Migration: each forge key becomes an overlay entry — e.g. `forge: github:` -> becomes `overlays: - when: 'event.source.system == "github"'`. -> `forge` and `overlays` cannot coexist in the same harness. +> becomes `overlays: - when: 'runtime.forge == "github"'`. Note the conditioning +> axis: `runtime.forge` reflects the effective forge platform (from `--forge` +> flag, `config.forge`, or CI env vars), while `event.source.system` identifies +> the event origin. These diverge for cross-system events (e.g. a JIRA issue +> triggering work on GitHub). `forge` and `overlays` cannot coexist in the +> same harness. > **Deprecated:** `runner_env` is deprecated. Use `env.runner` > instead. The `runner_env` field still works but emits a deprecation warning @@ -322,11 +333,16 @@ security: ### Field merge rules (for `base` and `overlays`) +Overlays use first-match-wins: exactly one overlay (or none) applies to any +given event. When an agent needs config from multiple concerns (e.g. +JIRA-specific scripts *and* GitHub-specific runner env), create a combined +entry. More-specific entries go first; broader fallbacks go last. + | Field type | Behavior | |-----------|----------| | Scalars (`model`, `pre_script`, `policy`, `image`, etc.) | Child wins if non-empty | | `skills` | Merged with deduplication by basename (child overrides base) | -| `providers`, `openshell.profiles` | Concatenated (base + child); also applies per-overlay | +| `providers`, `openshell.profiles` | Concatenated (base + child); also applies per matched overlay | | `plugins`, `api_servers` | Concatenated (base + child) | | `host_files` | Concatenated; child overrides by `dest` | | `env`, `runner_env` (deprecated) | Merged; child keys win | diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 6101731319..715939f173 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -80,6 +80,10 @@ type ComposeOpts struct { // If nil, ResolveOverlays is a no-op. Event map[string]any + // Config is the per-repo config (from config.yaml) exposed to overlay + // CEL when expressions as the config variable (ADR 0088). + Config map[string]any + // allowSelfAllowlist permits using the child harness's own AllowedRemoteResources // when OrgAllowlist is empty. This is for testing only; production callers should // always provide OrgAllowlist from config.yaml. Unexported to prevent misuse. @@ -161,7 +165,7 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, if err := child.ResolveForge(opts.ForgePlatform); err != nil { return nil, nil, fmt.Errorf("resolving forge config: %w", err) } - if err := child.ResolveOverlays(opts.Event); err != nil { + if err := child.ResolveOverlays(opts.Event, opts.ForgePlatform, opts.Config); err != nil { return nil, nil, fmt.Errorf("resolving overlays: %w", err) } if err := child.Validate(); err != nil { @@ -264,7 +268,7 @@ func LoadWithBase(ctx context.Context, path string, opts ComposeOpts) (*Harness, if err := child.ResolveForge(opts.ForgePlatform); err != nil { return nil, nil, fmt.Errorf("resolving forge config: %w", err) } - if err := child.ResolveOverlays(opts.Event); err != nil { + if err := child.ResolveOverlays(opts.Event, opts.ForgePlatform, opts.Config); err != nil { return nil, nil, fmt.Errorf("resolving overlays: %w", err) } if err := child.Validate(); err != nil { diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 65a9f762f1..c7630de84a 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -8329,6 +8329,10 @@ base: `+baseURL+` func TestLoadWithBase_OverlayConcatBothHaveOverlays(t *testing.T) { dir := t.TempDir() + // With first-match-wins, cross-concern scenarios need combined entries. + // Base overlays come first in the concatenated list; child appended after. + // The first matching entry wins — a child entry with a more-specific when + // shadows base entries. baseContent := ` agent: agents/test.md role: fix @@ -8341,21 +8345,22 @@ overlays: childContent := ` base: base.yaml overlays: -- when: 'event.entity.kind == "issue"' - post_script: scripts/child-issue.sh +- when: 'event.source.system == "github"' + pre_script: scripts/child-gh.sh + post_script: scripts/child-post.sh ` require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseContent), 0o644)) require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childContent), 0o644)) h, _, err := LoadWithBase(context.Background(), filepath.Join(dir, "child.yaml"), ComposeOpts{ WorkspaceRoot: dir, - Event: map[string]any{"source": map[string]any{"system": "github"}, "entity": map[string]any{"kind": "issue"}}, + Event: map[string]any{"source": map[string]any{"system": "github"}}, }) require.NoError(t, err) - // Base overlay matched → pre_script set + // Base overlay is first in concat order and matches → first-match-wins assert.Equal(t, "scripts/base-gh.sh", h.PreScript) - // Child overlay matched → post_script set - assert.Equal(t, "scripts/child-issue.sh", h.PostScript) + // Child overlay not reached because base entry matched first + assert.Empty(t, h.PostScript) } func TestLoadWithBase_OverlayConcatOnlyBaseHasOverlays(t *testing.T) { diff --git a/internal/harness/forge.go b/internal/harness/forge.go index a7b9863067..a71cc5014a 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -27,10 +27,12 @@ type ForgeConfig struct { } // OverlayEntry is a CEL-guarded conditional config block (ADR 0088). -// Each entry carries a CEL expression in When (evaluated against the event -// variable, same environment as trigger:) and the same override fields as -// ForgeConfig. Entries whose When evaluates to true are merged into the -// harness in declaration order. +// Each entry carries a CEL expression in When and the same override fields +// as ForgeConfig. The first entry whose When evaluates to true is merged +// into the harness; remaining entries are skipped (first-match-wins). +// The When expression is evaluated against the overlay CEL environment: +// event (normevent map), runtime.forge (platform string), and config +// (per-repo config map). type OverlayEntry struct { When string `yaml:"when"` ForgeConfig `yaml:",inline"` @@ -218,7 +220,7 @@ func (h *Harness) validateOverlays() error { if when == "" { return fmt.Errorf("overlays[%d].when is required", i) } - env, err := NewTriggerEnv() + env, err := NewOverlayEnv() if err != nil { return fmt.Errorf("overlays[%d]: creating CEL env: %w", i, err) } @@ -237,11 +239,12 @@ func (h *Harness) validateOverlays() error { return nil } -// ResolveOverlays evaluates each overlay's When expression against event -// data and merges matching entries into the harness in declaration order. +// ResolveOverlays evaluates each overlay's When expression against the +// overlay CEL environment (event, runtime.forge, config) and merges the +// first matching entry into the harness (first-match-wins, ADR 0088). // After resolution, h.Overlays is set to nil (consumed). When event is // nil or h.Overlays is empty, this is a no-op. -func (h *Harness) ResolveOverlays(event map[string]any) error { +func (h *Harness) ResolveOverlays(event map[string]any, forgePlatform string, config map[string]any) error { if len(h.Overlays) == 0 { h.Overlays = nil return nil @@ -251,13 +254,14 @@ func (h *Harness) ResolveOverlays(event map[string]any) error { return nil } for i, entry := range h.Overlays { - matched, err := EvaluateTrigger(entry.When, event) + matched, err := EvaluateOverlay(entry.When, event, forgePlatform, config) if err != nil { return fmt.Errorf("overlays[%d].when: %w", i, err) } if matched { fc := entry.ForgeConfig mergeForgeConfig(h, &fc) + break } } h.Overlays = nil diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 4cf3d4da2f..e5c8be6504 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1045,6 +1045,30 @@ func TestValidateOverlays_ValidCELAccepted(t *testing.T) { require.NoError(t, err) } +func TestValidateOverlays_RuntimeForgeAccepted(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `runtime.forge == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.NoError(t, err) +} + +func TestValidateOverlays_ConfigVariableAccepted(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `config.tracker == "jira" && runtime.forge == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/pre.sh"}}, + }, + } + err := h.validateOverlays() + require.NoError(t, err) +} + func TestValidateOverlays_InvalidScriptPathRejected(t *testing.T) { h := &Harness{ Agent: "agents/test.md", @@ -1093,13 +1117,13 @@ func TestResolveOverlays_SingleMatch(t *testing.T) { }, } event := map[string]any{"source": map[string]any{"system": "github"}} - err := h.ResolveOverlays(event) + err := h.ResolveOverlays(event, "", nil) require.NoError(t, err) assert.Equal(t, "scripts/gh.sh", h.PreScript) assert.Nil(t, h.Overlays) } -func TestResolveOverlays_MultipleMatchesScalarLastWins(t *testing.T) { +func TestResolveOverlays_FirstMatchWins(t *testing.T) { h := &Harness{ Agent: "agents/test.md", Role: "fix", @@ -1109,12 +1133,12 @@ func TestResolveOverlays_MultipleMatchesScalarLastWins(t *testing.T) { }, } event := map[string]any{"source": map[string]any{"system": "github"}} - err := h.ResolveOverlays(event) + err := h.ResolveOverlays(event, "", nil) require.NoError(t, err) - assert.Equal(t, "b.sh", h.PreScript) + assert.Equal(t, "a.sh", h.PreScript, "first-match-wins: first entry should be applied") } -func TestResolveOverlays_ListFieldsAccumulate(t *testing.T) { +func TestResolveOverlays_FirstMatchWinsSkipsLater(t *testing.T) { h := &Harness{ Agent: "agents/test.md", Role: "fix", @@ -1125,12 +1149,11 @@ func TestResolveOverlays_ListFieldsAccumulate(t *testing.T) { }, } event := map[string]any{"source": map[string]any{"system": "github"}} - err := h.ResolveOverlays(event) + err := h.ResolveOverlays(event, "", nil) require.NoError(t, err) - require.Len(t, h.Skills, 3) + require.Len(t, h.Skills, 2, "only first matching overlay should be applied") assert.Equal(t, "skills/base", h.Skills[0].Source) assert.Equal(t, "skills/gh", h.Skills[1].Source) - assert.Equal(t, "skills/extra", h.Skills[2].Source) } func TestResolveOverlays_NoMatchUnchanged(t *testing.T) { @@ -1143,7 +1166,7 @@ func TestResolveOverlays_NoMatchUnchanged(t *testing.T) { }, } event := map[string]any{"source": map[string]any{"system": "github"}} - err := h.ResolveOverlays(event) + err := h.ResolveOverlays(event, "", nil) require.NoError(t, err) assert.Equal(t, "scripts/common.sh", h.PreScript) assert.Nil(t, h.Overlays) @@ -1157,7 +1180,7 @@ func TestResolveOverlays_NilEventNoop(t *testing.T) { {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, }, } - err := h.ResolveOverlays(nil) + err := h.ResolveOverlays(nil, "", nil) require.NoError(t, err) assert.Nil(t, h.Overlays) } @@ -1168,7 +1191,58 @@ func TestResolveOverlays_EmptyOverlaysNoop(t *testing.T) { Role: "fix", PreScript: "scripts/common.sh", } - err := h.ResolveOverlays(map[string]any{"source": map[string]any{"system": "github"}}) + err := h.ResolveOverlays(map[string]any{"source": map[string]any{"system": "github"}}, "", nil) require.NoError(t, err) assert.Equal(t, "scripts/common.sh", h.PreScript) } + +func TestResolveOverlays_RuntimeForge(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `runtime.forge == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + {When: `runtime.forge == "gitlab"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gl.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "jira"}} + err := h.ResolveOverlays(event, "github", nil) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) +} + +func TestResolveOverlays_ConfigVariable(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `config.tracker == "jira"`, ForgeConfig: ForgeConfig{PreScript: "scripts/jira.sh"}}, + {When: `config.tracker == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "jira"}} + config := map[string]any{"tracker": "jira"} + err := h.ResolveOverlays(event, "github", config) + require.NoError(t, err) + assert.Equal(t, "scripts/jira.sh", h.PreScript) +} + +func TestResolveOverlays_CombinedWhenExpression(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + Overlays: []OverlayEntry{ + {When: `event.source.system == "jira" && runtime.forge == "github"`, ForgeConfig: ForgeConfig{ + PreScript: "scripts/jira-on-gh.sh", + Skills: []SkillEntry{{Source: "skills/jira-read"}}, + }}, + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + }, + } + event := map[string]any{"source": map[string]any{"system": "jira"}} + err := h.ResolveOverlays(event, "github", nil) + require.NoError(t, err) + assert.Equal(t, "scripts/jira-on-gh.sh", h.PreScript) + require.Len(t, h.Skills, 1) + assert.Equal(t, "skills/jira-read", h.Skills[0].Source) +} diff --git a/internal/harness/harness.go b/internal/harness/harness.go index c2e4c5c973..011cea2634 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -374,6 +374,7 @@ func Load(path string) (*Harness, error) { type LoadOpts struct { ForgePlatform string Event map[string]any // event data for CEL overlay resolution (ADR 0088) + Config map[string]any // per-repo config for CEL overlay resolution (ADR 0088) } // LoadWithOpts reads a harness YAML file and applies forge resolution before @@ -399,7 +400,7 @@ func LoadWithOpts(path string, opts LoadOpts) (*Harness, error) { return nil, fmt.Errorf("resolving forge config: %w", err) } - if err := h.ResolveOverlays(opts.Event); err != nil { + if err := h.ResolveOverlays(opts.Event, opts.ForgePlatform, opts.Config); err != nil { return nil, fmt.Errorf("resolving overlays: %w", err) } diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index c36f65935f..dd5a52006f 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -2334,3 +2334,45 @@ overlays: require.Error(t, err) assert.Contains(t, err.Error(), "forge and overlays cannot coexist") } + +func TestLoadWithOpts_OverlayWithRuntimeForge(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'runtime.forge == "github"' + pre_script: scripts/gh.sh +- when: 'runtime.forge == "gitlab"' + pre_script: scripts/gl.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + ForgePlatform: "github", + Event: map[string]any{"source": map[string]any{"system": "jira"}}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) +} + +func TestLoadWithOpts_OverlayWithConfig(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'config.tracker == "jira"' + pre_script: scripts/jira.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + Event: map[string]any{"source": map[string]any{"system": "jira"}}, + Config: map[string]any{"tracker": "jira"}, + }) + require.NoError(t, err) + assert.Equal(t, "scripts/jira.sh", h.PreScript) +} diff --git a/internal/harness/trigger.go b/internal/harness/trigger.go index e8e24bb89c..094b61af19 100644 --- a/internal/harness/trigger.go +++ b/internal/harness/trigger.go @@ -16,6 +16,18 @@ func NewTriggerEnv() (*cel.Env, error) { ) } +// NewOverlayEnv creates a CEL environment for overlay when expressions. +// It extends the trigger environment with runtime and config variables +// (ADR 0088). runtime.forge is the effective forge platform; config is +// the per-repo config from config.yaml. +func NewOverlayEnv() (*cel.Env, error) { + return cel.NewEnv( + cel.Variable("event", cel.DynType), + cel.Variable("runtime", cel.DynType), + cel.Variable("config", cel.DynType), + ) +} + // ValidateTriggerExpression compiles a harness trigger CEL expression. // Empty trigger is valid (manual fullsend run only). func ValidateTriggerExpression(expr string) error { @@ -70,3 +82,47 @@ func EvaluateTrigger(expr string, event map[string]any) (bool, error) { } return bool(b), nil } + +// EvaluateOverlay evaluates an overlay when expression against the overlay +// CEL environment: event data, runtime context (forge platform), and +// per-repo config (ADR 0088). +func EvaluateOverlay(expr string, event map[string]any, forgePlatform string, config map[string]any) (bool, error) { + expr = strings.TrimSpace(expr) + if expr == "" { + return false, nil + } + env, err := NewOverlayEnv() + if err != nil { + return false, err + } + ast, issues := env.Compile(expr) + if issues != nil && issues.Err() != nil { + return false, issues.Err() + } + prg, err := env.Program(ast) + if err != nil { + return false, err + } + if config == nil { + config = map[string]any{} + } + activation := map[string]any{ + "event": event, + "runtime": map[string]any{"forge": forgePlatform}, + "config": config, + } + out, _, err := prg.Eval(activation) + if err != nil { + return false, err + } + b, ok := out.(types.Bool) + if !ok { + if br, ok := out.(ref.Val); ok { + if b, ok := br.Value().(bool); ok { + return b, nil + } + } + return false, fmt.Errorf("overlay when result is not bool: %T", out) + } + return bool(b), nil +} From ee955e597423dd767e1b76f145f6d4cf2deda5ed Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 17:20:24 +0000 Subject: [PATCH 04/17] fix: address review feedback on PR #6285 - Fix misleading comment in mergeBaseIntoChild: overlay concatenation uses first-match-wins (base entries take precedence), not last-entry-wins as the comment incorrectly stated - Add openshell to list of overlay-applicable fields in user docs - Document base-first overlay precedence in user guide - Add forge deprecation cross-reference in docs/architecture.md - Add forge deprecation annotation in ADR 0055 Addresses review feedback on #6285 --- docs/ADRs/0055-unified-env-var-delivery.md | 6 ++++-- docs/guides/user/bring-your-own-agent.md | 4 +++- internal/harness/compose.go | 5 +++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/ADRs/0055-unified-env-var-delivery.md b/docs/ADRs/0055-unified-env-var-delivery.md index cfbcd7bbc3..afa9d0bf7e 100644 --- a/docs/ADRs/0055-unified-env-var-delivery.md +++ b/docs/ADRs/0055-unified-env-var-delivery.md @@ -78,8 +78,10 @@ env: environment, same as `runner_env` and `expand: true` host_files today. The `env:` field can appear at the top level and inside `forge.` -blocks, replacing `runner_env` at both levels -([ADR 0045](0045-forge-portable-harness-schema.md)). +blocks (or `overlays:` entries), replacing `runner_env` at both levels +([ADR 0045](0045-forge-portable-harness-schema.md)). `forge:` is deprecated +in favor of CEL-guarded `overlays:` — see +[ADR 0088](0088-cel-guarded-overlays.md). Go struct: diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index b7c7163a8d..e09aa1e454 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -188,7 +188,7 @@ Key patterns to note: - **`policy: policies/triage.yaml`** is a per-agent policy that includes filesystem, landlock, process, and network rules (via inline `network_policies`). This agent predates the provider-based pattern — new agents can use `providers:` instead (see [Minimum viable agent](#minimum-viable-agent)). - **`host_files`** copy credentials from the trusted runner into the sandbox. `expand: true` resolves `${VAR}` references before copying. - **`validation_loop.schema`** references the JSON schema file directly — the validation script checks agent output against it. -- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, host_files, and env vars. Resolution is first-match-wins: the first entry whose `when` evaluates to true is merged; remaining entries are skipped. The CEL environment exposes `event` (the triggering event), `runtime.forge` (the effective forge platform), and `config` (per-repo config from config.yaml). +- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, openshell, host_files, and env vars. Resolution is first-match-wins: the first entry whose `when` evaluates to true is merged; remaining entries are skipped. The CEL environment exposes `event` (the triggering event), `runtime.forge` (the effective forge platform), and `config` (per-repo config from config.yaml). - **`common/env/gcp-vertex.env`** is referenced by relative path because both files live in the same repo. If your agent lives in a different repo, reference it by URL (see [Remote references](#referencing-resources-local-vs-remote)) or copy it locally. ## Harness field reference @@ -419,6 +419,8 @@ timeout_minutes: 15 Base chains support up to 5 levels (`MaxBaseDepth` in `internal/harness/compose.go`). Circular references are detected and rejected. Resolution order: base chain → child overrides → overlay resolution. See [field merge rules](#field-merge-rules-for-base-and-overlays) for how each field type combines. +> **Overlay precedence with `base:`:** Overlays are concatenated base-first, child-appended — the same ordering as `plugins`, `providers`, and `api_servers`. Because `ResolveOverlays` uses first-match-wins, a base overlay whose `when` matches will take precedence over a child overlay with the same condition. This is consistent with the trusted-base model (base URLs require an org-level allowlist). + > **Note:** `allowed_remote_resources`, `allow_runtime_fetch`, and `max_runtime_fetches` are NOT inherited from base harnesses — the child must declare its own. This prevents a base harness from injecting arbitrary URL prefixes or enabling runtime fetching in the child. ## Configuring existing agents diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 715939f173..69a0b3c1e4 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -672,8 +672,9 @@ func mergeBaseIntoChild(base, child *Harness) { } // Overlays: concatenated (base first, child appended) — same as plugins, - // providers, api_servers. Declaration order matters: later entries override - // earlier ones for scalars, so child entries naturally take precedence. + // providers, api_servers. Declaration order matters: ResolveOverlays uses + // first-match-wins semantics, so base entries (placed first) take + // precedence over child entries with the same when condition. if base.Overlays != nil { merged := make([]OverlayEntry, 0, len(base.Overlays)+len(child.Overlays)) merged = append(merged, base.Overlays...) From 7cc342faef6a5c9330ec24cf31e47dd6057f9560 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:28:50 +0000 Subject: [PATCH 05/17] fix(harness): wire event and config into production overlay callers Add --event-file flag to fullsend run for normalized event JSON input and wire ComposeOpts.Event and ComposeOpts.Config into the run path so ResolveOverlays can evaluate overlay when expressions at runtime. - Load normalized event from --event-file, parse via normevent.ParseJSON, convert to map[string]any via ToMap(), pass to ComposeOpts.Event - Build config map from per-repo config reader (forge, tracker, runtime, roles fields) and pass to ComposeOpts.Config - Add configMapForOverlays helper with tests - Update all runAgent test callers for the new eventFile parameter Addresses review feedback on #6285 --- internal/cli/prescript_run_test.go | 12 ++-- internal/cli/run.go | 62 ++++++++++++++++++- internal/cli/run_test.go | 95 +++++++++++++++++++++--------- 3 files changed, 134 insertions(+), 35 deletions(-) diff --git a/internal/cli/prescript_run_test.go b/internal/cli/prescript_run_test.go index b132e0ad47..335eefe985 100644 --- a/internal/cli/prescript_run_test.go +++ b/internal/cli/prescript_run_test.go @@ -92,7 +92,7 @@ func TestRunAgent_PreScriptSkip_ReturnsBeforeSandboxCreation(t *testing.T) { `echo "reason=open PR exists" >> "${FULLSEND_PRESCRIPT_OUTPUT}"`+"\n") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{}) require.NoError(t, err) } @@ -109,7 +109,7 @@ func TestRunAgent_PreScriptNoSkip_ProceedsToSandboxAndRelaysFalse(t *testing.T) dir := newSkipHarnessDir(t, "true\n") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "creating sandbox") @@ -130,7 +130,7 @@ func TestRunAgent_NoPreScript_StillRelaysSkippedFalse(t *testing.T) { dir := newSkipHarnessDir(t, "") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "creating sandbox") @@ -151,7 +151,7 @@ func TestRunAgent_PreScriptSkip_RelaysSkippedTrue(t *testing.T) { `echo "reason=open PR exists" >> "${FULLSEND_PRESCRIPT_OUTPUT}"`+"\n") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - require.NoError(t, runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", + require.NoError(t, runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{})) data, err := os.ReadFile(out) @@ -169,7 +169,7 @@ func TestRunAgent_PreScriptRelayFailureIsHardError(t *testing.T) { dir := newSkipHarnessDir(t, `echo "skipped=true" >> "${FULLSEND_PRESCRIPT_OUTPUT}"`+"\n") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", rFlags, + err := runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{}) require.ErrorContains(t, err, "relaying pre-script outputs") } @@ -333,7 +333,7 @@ func TestRunAgent_PreScriptExit78_RelaysSkippedTrue(t *testing.T) { dir := newSkipHarnessDir(t, "echo \"Nothing to do\"\nexit 78\n") rFlags := resolveFlags{maxDepth: 10, maxResources: 50} - require.NoError(t, runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", + require.NoError(t, runAgent(context.Background(), "code", dir, "", t.TempDir(), "", nil, false, "", "", "", rFlags, statusOpts{}, ui.New(io.Discard), false, runOverrideFlags{})) data, err := os.ReadFile(out) diff --git a/internal/cli/run.go b/internal/cli/run.go index 1f609588c5..4f9a84b528 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -40,6 +40,7 @@ import ( "github.com/fullsend-ai/fullsend/internal/lock" "github.com/fullsend-ai/fullsend/internal/mintclient" "github.com/fullsend-ai/fullsend/internal/mintcore" + "github.com/fullsend-ai/fullsend/internal/normevent" "github.com/fullsend-ai/fullsend/internal/prescript" "github.com/fullsend-ai/fullsend/internal/resolve" agentruntime "github.com/fullsend-ai/fullsend/internal/runtime" @@ -270,6 +271,7 @@ func newRunCmd() *cobra.Command { var debugFilter string var keepSandbox bool var forgeFlag string + var eventFile string var rFlags resolveFlags var sOpts statusOpts var oFlags runOverrideFlags @@ -282,7 +284,7 @@ func newRunCmd() *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { agentName := args[0] printer := ui.New(os.Stdout) - return runAgent(cmd.Context(), agentName, fullsendDir, outputBase, targetRepo, fullsendBinary, envFiles, noPostScript, debugFilter, forgeFlag, rFlags, sOpts, printer, keepSandbox, oFlags) + return runAgent(cmd.Context(), agentName, fullsendDir, outputBase, targetRepo, fullsendBinary, envFiles, noPostScript, debugFilter, forgeFlag, eventFile, rFlags, sOpts, printer, keepSandbox, oFlags) }, } @@ -296,6 +298,7 @@ func newRunCmd() *cobra.Command { cmd.Flags().StringVar(&debugFilter, "debug", "", `enable agent runtime debug logging with optional category filter (e.g. "api,hooks")`) cmd.Flags().Lookup("debug").NoOptDefVal = "*" cmd.Flags().StringVar(&forgeFlag, "forge", "", `forge platform to use (e.g. "github", "gitlab"); auto-detected from CI env vars when omitted`) + cmd.Flags().StringVar(&eventFile, "event-file", "", "path to a normalized event JSON file for CEL overlay resolution (ADR 0088)") cmd.Flags().BoolVar(&rFlags.offline, "offline", false, "reject network fetches; only use cached remote resources") cmd.Flags().IntVar(&rFlags.maxDepth, "max-depth", resolve.DefaultMaxDepth, "maximum dependency depth for transitive resolution (0 disables)") cmd.Flags().IntVar(&rFlags.maxResources, "max-resources", resolve.DefaultMaxResources, "maximum total remote resources per harness") @@ -312,7 +315,7 @@ func newRunCmd() *cobra.Command { return cmd } -func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRepo, fullsendBinary string, envFiles []string, noPostScript bool, debug string, forgeFlag string, rFlags resolveFlags, sOpts statusOpts, printer *ui.Printer, keepSandbox bool, oFlags runOverrideFlags) (runErr error) { +func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRepo, fullsendBinary string, envFiles []string, noPostScript bool, debug string, forgeFlag string, eventFile string, rFlags resolveFlags, sOpts statusOpts, printer *ui.Printer, keepSandbox bool, oFlags runOverrideFlags) (runErr error) { printer.Banner(Version()) printer.Blank() printer.Header("Running agent: " + agentName) @@ -372,6 +375,26 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } } + // Load normalized event for CEL overlay resolution (ADR 0088). + // When --event-file is provided, the event is passed to ComposeOpts.Event + // so ResolveOverlays can evaluate overlay when expressions. + var eventMap map[string]any + if eventFile != "" { + eventData, readErr := os.ReadFile(eventFile) + if readErr != nil { + return fmt.Errorf("reading event file %s: %w", eventFile, readErr) + } + ev, parseErr := normevent.ParseJSON(eventData) + if parseErr != nil { + return fmt.Errorf("parsing event file %s: %w", eventFile, parseErr) + } + var mapErr error + eventMap, mapErr = ev.ToMap() + if mapErr != nil { + return fmt.Errorf("converting event to map: %w", mapErr) + } + } + composeOpts := harness.ComposeOpts{ WorkspaceRoot: absFullsendDir, FetchPolicy: policy, @@ -380,6 +403,8 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep OrgAllowlist: orgAllowlist, TreeFetcher: rFlags.treeFetcher, GitToken: composeGitToken, + Event: eventMap, + Config: configMapForOverlays(orgCfg), } // Resolve agent source: config agents take precedence, then agents repo @@ -4607,3 +4632,36 @@ func emitRunInfoNotice(w io.Writer, inCI bool, info statuscomment.RunInfo) { fmt.Fprintf(w, "::notice::%s\n", footer) } } + +// configMapForOverlays builds the config map[string]any exposed to overlay +// CEL when expressions as the "config" variable (ADR 0088). Extracts +// user-facing per-repo config fields from the config reader. Returns nil +// when cfg is nil (no config loaded). +func configMapForOverlays(cfg config.ConfigWriter) map[string]any { + if cfg == nil { + return nil + } + pr, ok := cfg.(config.PerRepoConfigReader) + if !ok { + return nil + } + m := map[string]any{} + if v := pr.ConfigForge(); v != "" { + m["forge"] = v + } + if v := pr.ConfigTracker(); v != "" { + m["tracker"] = v + } + if v := pr.ConfigRuntime(); v != "" { + m["runtime"] = v + } + if roles := pr.ConfigRoles(); len(roles) > 0 { + // Convert to []any for CEL evaluation compatibility. + anyRoles := make([]any, len(roles)) + for i, r := range roles { + anyRoles[i] = r + } + m["roles"] = anyRoles + } + return m +} diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 8506edbe04..ff67c90555 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -194,7 +194,7 @@ func TestRunAgent_HarnessLoadPipeline(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -224,7 +224,7 @@ func TestRunAgent_YMLFallback(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -237,7 +237,7 @@ func TestRunAgent_HarnessNotFound(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -269,7 +269,7 @@ func TestRunAgent_HarnessLoadWithOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -299,7 +299,7 @@ func TestRunAgent_PerRepoConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -562,7 +562,7 @@ func TestRunAgent_MalformedOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -589,7 +589,7 @@ func TestRunAgent_MalformedOrgConfigWithURLRefs(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -611,7 +611,7 @@ func TestRunAgent_URLRefsNoOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -653,7 +653,7 @@ func TestRunAgent_WithURLBase(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -709,7 +709,7 @@ openshell: rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) // The test will fail after the orchestration block (e.g. during // bootstrapCommon or pre-script setup), but it must NOT fail at // the gateway check or provider/profile steps. @@ -745,7 +745,7 @@ func TestRunAgent_URLBaseNoAllowlist(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in allowed_remote_resources") } @@ -774,7 +774,7 @@ func TestRunAgent_URLBaseMalformedOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -886,7 +886,7 @@ func TestRunAgent_ConfigAgentLocalPath(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "custom", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "custom", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -922,7 +922,7 @@ func TestRunAgent_ConfigAgentURL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "triage", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "triage", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -956,7 +956,7 @@ func TestRunAgent_ConfigAgentOverridesScaffold(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -978,7 +978,7 @@ func TestRunAgent_AgentNotInConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in config and agents-repo fallback unavailable") } @@ -998,7 +998,7 @@ func TestRunAgent_UnknownAgentName(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in config and agents-repo fallback unavailable") } @@ -3521,7 +3521,7 @@ func TestRunAgent_PreflightCheck_Passing(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) // Must pass the preflight guard and reach the openshell check. assert.Contains(t, err.Error(), "openshell") @@ -3536,7 +3536,7 @@ func TestRunAgent_PreflightCheck_Failing(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "preflight_check failed") } @@ -3550,7 +3550,7 @@ func TestRunAgent_PreflightCheck_NoCheckConfigured(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -3582,7 +3582,7 @@ func TestRunAgent_PreflightCheck_NilValidationLoop(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -3603,7 +3603,7 @@ func TestRunAgent_PreflightCheck_Timeout(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "timed out") } @@ -4353,7 +4353,7 @@ func TestRunAgent_ErrorOnMissingRole(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "invalid harness: role field is required") @@ -4998,7 +4998,7 @@ func TestRunAgent_FallsBackToFULLSEND_MINT_URL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") @@ -5041,7 +5041,7 @@ func TestRunAgent_WarnsWhenNoMintURL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, buf.String(), "skipping token minting") @@ -5083,7 +5083,7 @@ func TestRunAgent_MintTokenError(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "agent token minting failed") @@ -5134,7 +5134,7 @@ func TestRunAgent_StatusNotifierSetup(t *testing.T) { statusNum: 42, mintURL: "https://mint.example.com", } - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", rFlags, sOpts, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, sOpts, printer, false) // Will error downstream (openshell not available), but status notifier setup should succeed require.Error(t, err) @@ -5892,3 +5892,44 @@ func TestGenerateSandboxName_AgentSlug(t *testing.T) { }) } } + +func TestConfigMapForOverlays_NilConfig(t *testing.T) { + t.Parallel() + assert.Nil(t, configMapForOverlays(nil)) +} + +func TestConfigMapForOverlays_PerRepoConfig(t *testing.T) { + t.Parallel() + cfg := config.NewPerRepoConfig([]string{"triage", "code"}, "org/repo") + pr, ok := cfg.(config.PerRepoConfigReader) + require.True(t, ok) + // Set per-repo specific fields via the writer interface. + if w, ok := cfg.(config.PerRepoConfigWriter); ok { + w.SetRuntime("claude") + } + _ = pr // verify type assertion works + + m := configMapForOverlays(cfg) + require.NotNil(t, m) + assert.Equal(t, "claude", m["runtime"]) + roles, ok := m["roles"].([]any) + require.True(t, ok) + assert.Contains(t, roles, "triage") + assert.Contains(t, roles, "code") +} + +func TestConfigMapForOverlays_OrgConfig(t *testing.T) { + t.Parallel() + // Org configs don't implement PerRepoConfigReader, so the map + // should be nil (no per-repo fields to expose). + orgCfg := config.NewOrgConfig(nil, nil, nil, "", "") + m := configMapForOverlays(orgCfg) + assert.Nil(t, m) +} + +func TestRunCommand_HasEventFileFlag(t *testing.T) { + cmd := newRunCmd() + flag := cmd.Flags().Lookup("event-file") + require.NotNil(t, flag) + assert.Equal(t, "", flag.DefValue) +} From cb8efcf96107dc76a86a36044d573f997090047b Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 19 Aug 2026 14:47:04 -0400 Subject: [PATCH 06/17] fix(harness): wire Config in all callers and enable overlays without event Fix ResolveOverlays to evaluate overlays even when event is nil, allowing overlays conditioned only on runtime.forge or config to match in CLI paths (run, lock) that don't have event context. When event is nil, pass an empty map to CEL instead of short-circuiting. Wire ComposeOpts.Config in lock.go and enumerate.go so overlay when expressions can reference config.* fields. Also add documentation callout in harness-composition.md about overlay precedence exception (base-first, not child-overrides-base). Changes: - ResolveOverlays: use empty map when event is nil instead of early return - lock.go: wire Config via configMapForOverlays - enumerate.go: add buildConfigMap helper and wire Config - Update tests (TestResolveOverlays_NilEventNoop, TestLoadWithOpts_OverlayNoEvent) to use runtime.forge conditions instead of event-dependent conditions - Add overlay precedence note in docs/contributing/harness-composition.md Addresses review feedback on PR #6285. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- docs/contributing/harness-composition.md | 9 +++++++ internal/cli/lock.go | 1 + internal/harness/forge.go | 11 ++++++--- internal/harness/forge_test.go | 14 +++++++---- internal/harness/harness_test.go | 10 +++++--- internal/harnessdispatch/enumerate.go | 31 ++++++++++++++++++++++++ 6 files changed, 63 insertions(+), 13 deletions(-) diff --git a/docs/contributing/harness-composition.md b/docs/contributing/harness-composition.md index cc09c4a5a0..5b5ecf74fa 100644 --- a/docs/contributing/harness-composition.md +++ b/docs/contributing/harness-composition.md @@ -36,6 +36,15 @@ and update the others as needed. | `mergeHostFiles` | `internal/harness/compose.go` | Deduplicates host files by dest path (base + child) | | `mergeForgeBlocks` | `internal/harness/compose.go` | Merges `forge:` maps key-by-key across base and child | +> **Note — overlay precedence exception.** When `overlays` are concatenated +> during base composition, base entries are placed first. Because overlay +> resolution uses first-match-wins semantics (`ResolveOverlays` stops at the +> first matching `when` expression), base overlay entries take precedence over +> child overlay entries with the same condition. This is an intentional +> exception to the child-overrides-base convention used by scalar and map +> merges, and matches the concatenation behavior for `plugins`, `providers`, +> and `api_servers` lists. + ### Validation and resolution side | Function | File | Purpose | diff --git a/internal/cli/lock.go b/internal/cli/lock.go index f92b146e32..f8280aa327 100644 --- a/internal/cli/lock.go +++ b/internal/cli/lock.go @@ -229,6 +229,7 @@ func lockOneAgent(ctx context.Context, agentName, absFullsendDir, forgeFlag stri OrgAllowlist: orgAllowlist, TreeFetcher: rFlags.treeFetcher, GitToken: composeGitToken, + Config: configMapForOverlays(orgCfg), }) if loadErr != nil { printer.StepFail(fmt.Sprintf("Failed to load harness (forge: %s)", platform)) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index a71cc5014a..4359292e89 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -242,16 +242,19 @@ func (h *Harness) validateOverlays() error { // ResolveOverlays evaluates each overlay's When expression against the // overlay CEL environment (event, runtime.forge, config) and merges the // first matching entry into the harness (first-match-wins, ADR 0088). -// After resolution, h.Overlays is set to nil (consumed). When event is -// nil or h.Overlays is empty, this is a no-op. +// After resolution, h.Overlays is set to nil (consumed). When h.Overlays +// is empty, this is a no-op. When event is nil, an empty map is passed +// to CEL so overlays conditioned only on runtime.forge or config can still +// match (e.g., CLI run/lock flows that don't have an event context). func (h *Harness) ResolveOverlays(event map[string]any, forgePlatform string, config map[string]any) error { if len(h.Overlays) == 0 { h.Overlays = nil return nil } + // Use empty map if event is nil so overlays conditioned on runtime.forge + // or config can still evaluate and match (CLI paths without event context). if event == nil { - h.Overlays = nil - return nil + event = make(map[string]any) } for i, entry := range h.Overlays { matched, err := EvaluateOverlay(entry.When, event, forgePlatform, config) diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index e5c8be6504..1c01fc5893 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1174,15 +1174,19 @@ func TestResolveOverlays_NoMatchUnchanged(t *testing.T) { func TestResolveOverlays_NilEventNoop(t *testing.T) { h := &Harness{ - Agent: "agents/test.md", - Role: "fix", + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", Overlays: []OverlayEntry{ - {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + {When: `runtime.forge == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, }, } - err := h.ResolveOverlays(nil, "", nil) + // Nil event is converted to empty map; overlays conditioned on runtime.forge + // or config can still match (ADR 0088 — overlays work in CLI paths without event). + err := h.ResolveOverlays(nil, "github", nil) require.NoError(t, err) - assert.Nil(t, h.Overlays) + assert.Equal(t, "scripts/gh.sh", h.PreScript, "overlay should match on runtime.forge even when event is nil") + assert.Nil(t, h.Overlays, "overlays should be consumed after resolution") } func TestResolveOverlays_EmptyOverlaysNoop(t *testing.T) { diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index dd5a52006f..2ae3802fc7 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -2299,18 +2299,20 @@ func TestLoadWithOpts_OverlayNoEvent(t *testing.T) { content := ` agent: agents/test.md role: fix +pre_script: scripts/common.sh overlays: -- when: 'event.source.system == "github"' +- when: 'runtime.forge == "github"' pre_script: scripts/gh.sh ` dir := t.TempDir() path := filepath.Join(dir, "test.yaml") require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) - h, err := LoadWithOpts(path, LoadOpts{}) + h, err := LoadWithOpts(path, LoadOpts{ForgePlatform: "github"}) require.NoError(t, err) - // No event → overlays are a no-op and consumed - assert.Nil(t, h.Overlays) + // Overlays can match on runtime.forge even when event is nil (ADR 0088). + assert.Equal(t, "scripts/gh.sh", h.PreScript) + assert.Nil(t, h.Overlays, "overlays should be consumed after resolution") } func TestLoadWithOpts_OverlayAndForgeReject(t *testing.T) { diff --git a/internal/harnessdispatch/enumerate.go b/internal/harnessdispatch/enumerate.go index 56eeca3081..dcbe0030c8 100644 --- a/internal/harnessdispatch/enumerate.go +++ b/internal/harnessdispatch/enumerate.go @@ -46,6 +46,7 @@ func ListTriggeredHarnesses(ctx context.Context, configDir string, cfg config.Co WorkspaceRoot: filepath.Dir(configDir), OrgAllowlist: allowlist, FetchPolicy: policy, + Config: buildConfigMap(cfg), } var out []TriggeredHarness @@ -104,3 +105,33 @@ func MergedConfigAgents(configDir string) ([]config.AgentEntry, error) { } return cfg.AgentEntries(), nil } + +// buildConfigMap extracts user-facing per-repo config fields for overlay +// CEL evaluation (ADR 0088). Returns nil when cfg is nil. +func buildConfigMap(cfg config.ConfigReader) map[string]any { + if cfg == nil { + return nil + } + pr, ok := cfg.(config.PerRepoConfigReader) + if !ok { + return nil + } + m := map[string]any{} + if v := pr.ConfigForge(); v != "" { + m["forge"] = v + } + if v := pr.ConfigTracker(); v != "" { + m["tracker"] = v + } + if v := pr.ConfigRuntime(); v != "" { + m["runtime"] = v + } + if roles := pr.ConfigRoles(); len(roles) > 0 { + anyRoles := make([]any, len(roles)) + for i, r := range roles { + anyRoles[i] = r + } + m["roles"] = anyRoles + } + return m +} From d8472e9f9406bc45b785bb99f171bafdd493fdea Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:16:58 +0000 Subject: [PATCH 07/17] fix(harness): treat CEL eval errors as non-matching in ResolveOverlays Change ResolveOverlays to log-and-continue on CEL evaluation errors instead of aborting overlay resolution. This matches the MatchHarnesses pattern in harnessdispatch/enumerate.go and fixes the documented more-specific-first overlay pattern: when a specific overlay (e.g., event.source.system == "jira" && runtime.forge == "github") errors on key access because the event map is empty, the broader fallback overlay (e.g., runtime.forge == "github") is now evaluated instead of the entire resolution failing. Addresses review feedback on #6285 --- internal/harness/forge.go | 16 +++++++++- internal/harness/forge_test.go | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 4359292e89..41e37a11fa 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -2,6 +2,7 @@ package harness import ( "fmt" + "log" "sort" "strings" @@ -246,6 +247,13 @@ func (h *Harness) validateOverlays() error { // is empty, this is a no-op. When event is nil, an empty map is passed // to CEL so overlays conditioned only on runtime.forge or config can still // match (e.g., CLI run/lock flows that don't have an event context). +// +// CEL evaluation errors are treated as non-matching: the error is logged +// and evaluation continues to the next entry. This matches the +// MatchHarnesses pattern in harnessdispatch/enumerate.go and ensures that +// a more-specific overlay (e.g., event.source.system == "jira" && +// runtime.forge == "github") that fails on key access when event is empty +// does not prevent a broader fallback overlay from matching. func (h *Harness) ResolveOverlays(event map[string]any, forgePlatform string, config map[string]any) error { if len(h.Overlays) == 0 { h.Overlays = nil @@ -259,7 +267,13 @@ func (h *Harness) ResolveOverlays(event map[string]any, forgePlatform string, co for i, entry := range h.Overlays { matched, err := EvaluateOverlay(entry.When, event, forgePlatform, config) if err != nil { - return fmt.Errorf("overlays[%d].when: %w", i, err) + // Treat CEL evaluation errors as non-matching: log and + // continue to the next entry, matching the MatchHarnesses + // pattern in harnessdispatch/enumerate.go. This allows + // fallback overlays to match when a more-specific overlay + // errors (e.g., event key access on an empty event map). + log.Printf("harness: overlay[%d].when eval failed: %v", i, err) + continue } if matched { fc := entry.ForgeConfig diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 1c01fc5893..65a42c4e9e 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1250,3 +1250,56 @@ func TestResolveOverlays_CombinedWhenExpression(t *testing.T) { require.Len(t, h.Skills, 1) assert.Equal(t, "skills/jira-read", h.Skills[0].Source) } + +// TestResolveOverlays_CELErrorSkipsToFallback verifies that a CEL evaluation +// error in an earlier overlay (e.g., accessing event.source.system when event +// is empty) does not abort resolution — the error is logged as non-matching, +// and a broader fallback overlay can still match. This matches the +// MatchHarnesses pattern in harnessdispatch/enumerate.go and supports the +// more-specific-first pattern documented in bring-your-own-agent.md. +func TestResolveOverlays_CELErrorSkipsToFallback(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", + Overlays: []OverlayEntry{ + // More-specific overlay: references event.source.system which will + // error when event is empty (no such key). + {When: `event.source.system == "jira" && runtime.forge == "github"`, ForgeConfig: ForgeConfig{ + PreScript: "scripts/jira-on-gh.sh", + }}, + // Broader fallback: conditioned only on runtime.forge, always evaluable. + {When: `runtime.forge == "github"`, ForgeConfig: ForgeConfig{ + PreScript: "scripts/gh-fallback.sh", + }}, + }, + } + // Empty event: event.source.system access will error on the first overlay. + // The fallback overlay should still match. + err := h.ResolveOverlays(map[string]any{}, "github", nil) + require.NoError(t, err) + assert.Equal(t, "scripts/gh-fallback.sh", h.PreScript, + "fallback overlay should match when earlier overlay has a CEL eval error") + assert.Nil(t, h.Overlays, "overlays should be consumed after resolution") +} + +// TestResolveOverlays_CELErrorAllFail verifies that when all overlays fail +// with CEL evaluation errors, no overlay is applied and the harness retains +// its original values. +func TestResolveOverlays_CELErrorAllFail(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "fix", + PreScript: "scripts/common.sh", + Overlays: []OverlayEntry{ + {When: `event.source.system == "jira"`, ForgeConfig: ForgeConfig{PreScript: "scripts/jira.sh"}}, + {When: `event.source.system == "github"`, ForgeConfig: ForgeConfig{PreScript: "scripts/gh.sh"}}, + }, + } + // Empty event: both overlays will fail on event.source access. + err := h.ResolveOverlays(map[string]any{}, "", nil) + require.NoError(t, err) + assert.Equal(t, "scripts/common.sh", h.PreScript, + "harness should retain original values when all overlays fail") + assert.Nil(t, h.Overlays, "overlays should be consumed even when all fail") +} From a082658e08355a715e81068dd31323eae3810e8b Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:10:44 +0000 Subject: [PATCH 08/17] refactor(harness): extract shared BuildConfigMap to eliminate code duplication Extract configMapForOverlays (internal/cli/run.go) and buildConfigMap (internal/harnessdispatch/enumerate.go) into a single exported harness.BuildConfigMap function accepting config.ConfigReader. Both call sites now use the shared function, ensuring overlay CEL resolution sees the same config shape regardless of the call path. This also fixes the interface mismatch: configMapForOverlays accepted ConfigWriter but only read from it. BuildConfigMap correctly accepts ConfigReader (the narrowest sufficient interface). Tests moved from run_test.go to forge_test.go alongside the function. Addresses review feedback on #6285 --- internal/cli/lock.go | 2 +- internal/cli/run.go | 35 +----------------------- internal/cli/run_test.go | 34 ------------------------ internal/harness/forge.go | 38 +++++++++++++++++++++++++++ internal/harness/forge_test.go | 37 ++++++++++++++++++++++++++ internal/harnessdispatch/enumerate.go | 32 +--------------------- 6 files changed, 78 insertions(+), 100 deletions(-) diff --git a/internal/cli/lock.go b/internal/cli/lock.go index f8280aa327..2649bf9997 100644 --- a/internal/cli/lock.go +++ b/internal/cli/lock.go @@ -229,7 +229,7 @@ func lockOneAgent(ctx context.Context, agentName, absFullsendDir, forgeFlag stri OrgAllowlist: orgAllowlist, TreeFetcher: rFlags.treeFetcher, GitToken: composeGitToken, - Config: configMapForOverlays(orgCfg), + Config: harness.BuildConfigMap(orgCfg), }) if loadErr != nil { printer.StepFail(fmt.Sprintf("Failed to load harness (forge: %s)", platform)) diff --git a/internal/cli/run.go b/internal/cli/run.go index 4f9a84b528..17fa100fc2 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -404,7 +404,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep TreeFetcher: rFlags.treeFetcher, GitToken: composeGitToken, Event: eventMap, - Config: configMapForOverlays(orgCfg), + Config: harness.BuildConfigMap(orgCfg), } // Resolve agent source: config agents take precedence, then agents repo @@ -4632,36 +4632,3 @@ func emitRunInfoNotice(w io.Writer, inCI bool, info statuscomment.RunInfo) { fmt.Fprintf(w, "::notice::%s\n", footer) } } - -// configMapForOverlays builds the config map[string]any exposed to overlay -// CEL when expressions as the "config" variable (ADR 0088). Extracts -// user-facing per-repo config fields from the config reader. Returns nil -// when cfg is nil (no config loaded). -func configMapForOverlays(cfg config.ConfigWriter) map[string]any { - if cfg == nil { - return nil - } - pr, ok := cfg.(config.PerRepoConfigReader) - if !ok { - return nil - } - m := map[string]any{} - if v := pr.ConfigForge(); v != "" { - m["forge"] = v - } - if v := pr.ConfigTracker(); v != "" { - m["tracker"] = v - } - if v := pr.ConfigRuntime(); v != "" { - m["runtime"] = v - } - if roles := pr.ConfigRoles(); len(roles) > 0 { - // Convert to []any for CEL evaluation compatibility. - anyRoles := make([]any, len(roles)) - for i, r := range roles { - anyRoles[i] = r - } - m["roles"] = anyRoles - } - return m -} diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index ff67c90555..b00a3e6632 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -5893,40 +5893,6 @@ func TestGenerateSandboxName_AgentSlug(t *testing.T) { } } -func TestConfigMapForOverlays_NilConfig(t *testing.T) { - t.Parallel() - assert.Nil(t, configMapForOverlays(nil)) -} - -func TestConfigMapForOverlays_PerRepoConfig(t *testing.T) { - t.Parallel() - cfg := config.NewPerRepoConfig([]string{"triage", "code"}, "org/repo") - pr, ok := cfg.(config.PerRepoConfigReader) - require.True(t, ok) - // Set per-repo specific fields via the writer interface. - if w, ok := cfg.(config.PerRepoConfigWriter); ok { - w.SetRuntime("claude") - } - _ = pr // verify type assertion works - - m := configMapForOverlays(cfg) - require.NotNil(t, m) - assert.Equal(t, "claude", m["runtime"]) - roles, ok := m["roles"].([]any) - require.True(t, ok) - assert.Contains(t, roles, "triage") - assert.Contains(t, roles, "code") -} - -func TestConfigMapForOverlays_OrgConfig(t *testing.T) { - t.Parallel() - // Org configs don't implement PerRepoConfigReader, so the map - // should be nil (no per-repo fields to expose). - orgCfg := config.NewOrgConfig(nil, nil, nil, "", "") - m := configMapForOverlays(orgCfg) - assert.Nil(t, m) -} - func TestRunCommand_HasEventFileFlag(t *testing.T) { cmd := newRunCmd() flag := cmd.Flags().Lookup("event-file") diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 41e37a11fa..a98df0da8c 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -6,6 +6,7 @@ import ( "sort" "strings" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/google/cel-go/common/types" ) @@ -388,3 +389,40 @@ func forgeKeyList(m map[string]*ForgeConfig) string { } return strings.Join(keys, ", ") } + +// BuildConfigMap extracts user-facing per-repo config fields for overlay +// CEL evaluation (ADR 0088). The returned map is exposed to overlay when +// expressions as the "config" variable. Returns nil when cfg is nil or +// does not implement PerRepoConfigReader (e.g. org-mode configs). +// +// Both the CLI (run, lock) and harnessdispatch (enumerate) call sites use +// this single implementation to ensure overlay CEL resolution sees the +// same config shape regardless of the call path. +func BuildConfigMap(cfg config.ConfigReader) map[string]any { + if cfg == nil { + return nil + } + pr, ok := cfg.(config.PerRepoConfigReader) + if !ok { + return nil + } + m := map[string]any{} + if v := pr.ConfigForge(); v != "" { + m["forge"] = v + } + if v := pr.ConfigTracker(); v != "" { + m["tracker"] = v + } + if v := pr.ConfigRuntime(); v != "" { + m["runtime"] = v + } + if roles := pr.ConfigRoles(); len(roles) > 0 { + // Convert to []any for CEL evaluation compatibility. + anyRoles := make([]any, len(roles)) + for i, r := range roles { + anyRoles[i] = r + } + m["roles"] = anyRoles + } + return m +} diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 65a42c4e9e..996e44257f 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1303,3 +1304,39 @@ func TestResolveOverlays_CELErrorAllFail(t *testing.T) { "harness should retain original values when all overlays fail") assert.Nil(t, h.Overlays, "overlays should be consumed even when all fail") } + +// --- BuildConfigMap tests --- + +func TestBuildConfigMap_NilConfig(t *testing.T) { + t.Parallel() + assert.Nil(t, BuildConfigMap(nil)) +} + +func TestBuildConfigMap_PerRepoConfig(t *testing.T) { + t.Parallel() + cfg := config.NewPerRepoConfig([]string{"triage", "code"}, "org/repo") + pr, ok := cfg.(config.PerRepoConfigReader) + require.True(t, ok) + // Set per-repo specific fields via the writer interface. + if w, ok := cfg.(config.PerRepoConfigWriter); ok { + w.SetRuntime("claude") + } + _ = pr // verify type assertion works + + m := BuildConfigMap(cfg) + require.NotNil(t, m) + assert.Equal(t, "claude", m["runtime"]) + roles, ok := m["roles"].([]any) + require.True(t, ok) + assert.Contains(t, roles, "triage") + assert.Contains(t, roles, "code") +} + +func TestBuildConfigMap_OrgConfig(t *testing.T) { + t.Parallel() + // Org configs don't implement PerRepoConfigReader, so the map + // should be nil (no per-repo fields to expose). + orgCfg := config.NewOrgConfig(nil, nil, nil, "", "") + m := BuildConfigMap(orgCfg) + assert.Nil(t, m) +} diff --git a/internal/harnessdispatch/enumerate.go b/internal/harnessdispatch/enumerate.go index dcbe0030c8..1d5351d244 100644 --- a/internal/harnessdispatch/enumerate.go +++ b/internal/harnessdispatch/enumerate.go @@ -46,7 +46,7 @@ func ListTriggeredHarnesses(ctx context.Context, configDir string, cfg config.Co WorkspaceRoot: filepath.Dir(configDir), OrgAllowlist: allowlist, FetchPolicy: policy, - Config: buildConfigMap(cfg), + Config: harness.BuildConfigMap(cfg), } var out []TriggeredHarness @@ -105,33 +105,3 @@ func MergedConfigAgents(configDir string) ([]config.AgentEntry, error) { } return cfg.AgentEntries(), nil } - -// buildConfigMap extracts user-facing per-repo config fields for overlay -// CEL evaluation (ADR 0088). Returns nil when cfg is nil. -func buildConfigMap(cfg config.ConfigReader) map[string]any { - if cfg == nil { - return nil - } - pr, ok := cfg.(config.PerRepoConfigReader) - if !ok { - return nil - } - m := map[string]any{} - if v := pr.ConfigForge(); v != "" { - m["forge"] = v - } - if v := pr.ConfigTracker(); v != "" { - m["tracker"] = v - } - if v := pr.ConfigRuntime(); v != "" { - m["runtime"] = v - } - if roles := pr.ConfigRoles(); len(roles) > 0 { - anyRoles := make([]any, len(roles)) - for i, r := range roles { - anyRoles[i] = r - } - m["roles"] = anyRoles - } - return m -} From 031e6d06fa44c37b54865dbb6f2b97b3c63b623d Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 13:19:57 -0400 Subject: [PATCH 09/17] fix(harness): thread config.forge into runtime.forge precedence chain Per ADR 0088, runtime.forge should have the following precedence: 1. --forge flag 2. config.forge (from config.yaml) 3. CI environment variables (GITHUB_ACTIONS, GITLAB_CI) Previously, detectForgePlatform() only checked (1) and (3), skipping config.forge entirely. This commit threads the config through as a parameter and adds the config.forge check between flag and env. The call in runAgent() is moved to after orgCfg is loaded so that config.forge is available for consultation. The reconcilestatus.go call site passes nil since no config is available in that context. Added three test cases to verify the precedence chain: - TestDetectForgePlatform_ConfigForge: config.forge consulted when no flag/env - TestDetectForgePlatform_FlagOverridesConfig: flag takes precedence over config - TestDetectForgePlatform_ConfigOverridesEnv: config takes precedence over env Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/cli/reconcilestatus.go | 2 +- internal/cli/run.go | 30 +++++++++----- internal/cli/run_test.go | 70 +++++++++++++++++++++++++++++---- 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/internal/cli/reconcilestatus.go b/internal/cli/reconcilestatus.go index 5993c01070..780ce44813 100644 --- a/internal/cli/reconcilestatus.go +++ b/internal/cli/reconcilestatus.go @@ -60,7 +60,7 @@ finalized, this is a no-op.`, } owner, repoName := parts[0], parts[1] - forgePlatform, err := detectForgePlatform(forgeFlag) + forgePlatform, err := detectForgePlatform(forgeFlag, nil) if err != nil { return err } diff --git a/internal/cli/run.go b/internal/cli/run.go index 17fa100fc2..777cef3641 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -343,12 +343,6 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // 1. Resolve and load harness. harnessStart := time.Now() - forgePlatform, err := detectForgePlatform(forgeFlag) - if err != nil { - printer.StepFail("Invalid --forge flag") - return err - } - policy := fetch.DefaultPolicy policy.Offline = rFlags.offline @@ -359,6 +353,13 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // tryLoadOrgConfig but not surfaced as a distinct error here. orgConfigPath := filepath.Join(absFullsendDir, "config.yaml") orgCfg := tryLoadOrgConfig(orgConfigPath, printer) + + // Detect forge platform after config is loaded so config.forge can be consulted (ADR 0088). + forgePlatform, err := detectForgePlatform(forgeFlag, orgCfg) + if err != nil { + printer.StepFail("Invalid --forge flag") + return err + } // Fallback for absent config; EnsureDefaultAllowedRemoteResources // handles the omitted-field case when a config is present. orgAllowlist := config.DefaultAllowedRemoteResources() @@ -3783,16 +3784,27 @@ func sandboxArch() string { return runtime.GOARCH } -// detectForgePlatform determines the forge platform from the CLI flag or CI -// environment variables. Precedence: explicit flag > GITHUB_ACTIONS > GITLAB_CI. +// detectForgePlatform determines the forge platform from the CLI flag, config, +// or CI environment variables. Precedence (per ADR 0088): +// 1. explicit --forge flag +// 2. config.forge (from config.yaml) +// 3. CI environment variables (GITHUB_ACTIONS > GITLAB_CI) +// // Returns an error if the flag value is not a recognized forge key. -func detectForgePlatform(flag string) (string, error) { +func detectForgePlatform(flag string, cfg config.ConfigReader) (string, error) { if flag != "" { if !harness.ValidForgePlatform(flag) { return "", fmt.Errorf("--forge: %q is not a valid forge platform (valid: %s)", flag, harness.ForgeKeyList()) } return flag, nil } + if cfg != nil { + if pr, ok := cfg.(config.PerRepoConfigReader); ok { + if forge := pr.ConfigForge(); forge != "" { + return forge, nil + } + } + } if os.Getenv("GITHUB_ACTIONS") == "true" { return "github", nil } diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index b00a3e6632..f56389e615 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -3318,17 +3318,17 @@ func TestPRHeadSHAFromEventPath_NoInputs(t *testing.T) { // --- detectForgePlatform tests --- func TestDetectForgePlatform_ExplicitFlag(t *testing.T) { - p, err := detectForgePlatform("github") + p, err := detectForgePlatform("github", nil) require.NoError(t, err) assert.Equal(t, "github", p) - p, err = detectForgePlatform("gitlab") + p, err = detectForgePlatform("gitlab", nil) require.NoError(t, err) assert.Equal(t, "gitlab", p) } func TestDetectForgePlatform_InvalidFlag(t *testing.T) { - _, err := detectForgePlatform("bitbucket") + _, err := detectForgePlatform("bitbucket", nil) require.Error(t, err) assert.Contains(t, err.Error(), "not a valid forge platform") } @@ -3337,7 +3337,7 @@ func TestDetectForgePlatform_GitHubActions(t *testing.T) { t.Setenv("GITHUB_ACTIONS", "true") t.Setenv("GITLAB_CI", "") - p, err := detectForgePlatform("") + p, err := detectForgePlatform("", nil) require.NoError(t, err) assert.Equal(t, "github", p) } @@ -3346,7 +3346,7 @@ func TestDetectForgePlatform_GitLabCI(t *testing.T) { t.Setenv("GITHUB_ACTIONS", "") t.Setenv("GITLAB_CI", "true") - p, err := detectForgePlatform("") + p, err := detectForgePlatform("", nil) require.NoError(t, err) assert.Equal(t, "gitlab", p) } @@ -3355,7 +3355,7 @@ func TestDetectForgePlatform_NoEnv(t *testing.T) { t.Setenv("GITHUB_ACTIONS", "") t.Setenv("GITLAB_CI", "") - p, err := detectForgePlatform("") + p, err := detectForgePlatform("", nil) require.NoError(t, err) assert.Equal(t, "", p) } @@ -3363,7 +3363,7 @@ func TestDetectForgePlatform_NoEnv(t *testing.T) { func TestDetectForgePlatform_FlagOverridesEnv(t *testing.T) { t.Setenv("GITHUB_ACTIONS", "true") - p, err := detectForgePlatform("gitlab") + p, err := detectForgePlatform("gitlab", nil) require.NoError(t, err) assert.Equal(t, "gitlab", p) } @@ -3372,11 +3372,65 @@ func TestDetectForgePlatform_GitHubPrecedesGitLab(t *testing.T) { t.Setenv("GITHUB_ACTIONS", "true") t.Setenv("GITLAB_CI", "true") - p, err := detectForgePlatform("") + p, err := detectForgePlatform("", nil) require.NoError(t, err) assert.Equal(t, "github", p) } +func TestDetectForgePlatform_ConfigForge(t *testing.T) { + t.Setenv("GITHUB_ACTIONS", "") + t.Setenv("GITLAB_CI", "") + + yamlData := ` +version: "1" +forge: github +roles: + - triage +` + cfg, err := config.ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + + p, err := detectForgePlatform("", cfg) + require.NoError(t, err) + assert.Equal(t, "github", p) +} + +func TestDetectForgePlatform_FlagOverridesConfig(t *testing.T) { + t.Setenv("GITHUB_ACTIONS", "") + t.Setenv("GITLAB_CI", "") + + yamlData := ` +version: "1" +forge: github +roles: + - triage +` + cfg, err := config.ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + + p, err := detectForgePlatform("gitlab", cfg) + require.NoError(t, err) + assert.Equal(t, "gitlab", p) +} + +func TestDetectForgePlatform_ConfigOverridesEnv(t *testing.T) { + t.Setenv("GITHUB_ACTIONS", "true") + t.Setenv("GITLAB_CI", "") + + yamlData := ` +version: "1" +forge: gitlab +roles: + - triage +` + cfg, err := config.ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + + p, err := detectForgePlatform("", cfg) + require.NoError(t, err) + assert.Equal(t, "gitlab", p) +} + func TestRunCommand_HasForgeFlag(t *testing.T) { cmd := newRunCmd() flag := cmd.Flags().Lookup("forge") From af838e5d8e20634887eae5ac5699ce66194bb0ee Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 13:25:43 -0400 Subject: [PATCH 10/17] fix(lock): add overlays[N].* cases to resolveFromLock When a base harness has overlays with relative resource paths, compose.go records lock dependencies with paths like overlays[0].pre_script, overlays[0].skills[0], overlays[0].providers[0], etc. Previously, resolveFromLock only had cases for forge..* paths to avoid duplication. Overlay paths fell through to the default case and were incorrectly appended as skills. This commit adds overlays[N].* counterparts to every forge.* case: - In the mutation switch: handle overlays[N].skills[M], overlays[N].pre_script, overlays[N].post_script, overlays[N].policy, overlays[N].validation_loop.*, overlays[N].providers[M], and overlays[N].openshell.profiles[M] - In isTreeLockField: recognize overlays[N].skills[M] as a tree field - In isScriptLockField: recognize overlays[N].pre_script, overlays[N].post_script, and overlays[N].validation_loop.script as script fields - In provider/profile parsing: recognize overlays[N].providers[M] and overlays[N].openshell.profiles[M] for proper parsing Added comprehensive tests: - TestResolveFromLock_OverlayScopedSkillNoMutation: ensures overlay skills don't duplicate into h.Skills - TestResolveFromLock_OverlayScriptNoMutation: ensures overlay scripts don't become skills - TestResolveFromLock_OverlayProviderParsed: ensures overlay providers are parsed correctly - TestResolveFromLock_OverlayProfileParsed: ensures overlay profiles are parsed correctly All existing tests continue to pass. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/cli/lock.go | 35 +++++++- internal/cli/lock_test.go | 182 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+), 4 deletions(-) diff --git a/internal/cli/lock.go b/internal/cli/lock.go index 2649bf9997..63ec6b856d 100644 --- a/internal/cli/lock.go +++ b/internal/cli/lock.go @@ -826,7 +826,8 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot continue } if strings.HasPrefix(lockDep.Field, "openshell.profiles[") || - (strings.HasPrefix(lockDep.Field, "forge.") && strings.Contains(lockDep.Field, ".openshell.profiles[")) { + (strings.HasPrefix(lockDep.Field, "forge.") && strings.Contains(lockDep.Field, ".openshell.profiles[")) || + (strings.HasPrefix(lockDep.Field, "overlays[") && strings.Contains(lockDep.Field, ".openshell.profiles[")) { id, err := resolve.ParseProfileID(cachedContent) if err != nil { return resolve.ResolveResult{}, fmt.Errorf("cached profile %s: %w", lockDep.Field, err) @@ -841,7 +842,8 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot dep.LocalPath = namedPath profiles = append(profiles, resolve.ResolvedProfile{ID: id, LocalPath: localPath, FromURL: true}) } else if strings.HasPrefix(lockDep.Field, "providers[") || - (strings.HasPrefix(lockDep.Field, "forge.") && strings.Contains(lockDep.Field, ".providers[")) { + (strings.HasPrefix(lockDep.Field, "forge.") && strings.Contains(lockDep.Field, ".providers[")) || + (strings.HasPrefix(lockDep.Field, "overlays[") && strings.Contains(lockDep.Field, ".providers[")) { var def harness.ProviderDef if err := yaml.Unmarshal(cachedContent, &def); err != nil { return resolve.ResolveResult{}, fmt.Errorf("parsing cached provider %s: %w", lockDep.Field, err) @@ -931,6 +933,14 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot // Same as forge pre_script above. case strings.HasPrefix(m.field, "forge.") && strings.HasSuffix(m.field, ".validation_loop.script"): // Same as forge pre_script above. + case strings.HasPrefix(m.field, "overlays[") && strings.HasSuffix(m.field, ".pre_script"): + // Overlay scripts are resolved before overlay promotion; the field + // name is informational — the actual path was already set during + // LoadWithBase. This entry exists for cache verification. + case strings.HasPrefix(m.field, "overlays[") && strings.HasSuffix(m.field, ".post_script"): + // Same as overlay pre_script above. + case strings.HasPrefix(m.field, "overlays[") && strings.HasSuffix(m.field, ".validation_loop.script"): + // Same as overlay pre_script above. case m.field == "validation_loop.schema": if h.ValidationLoop != nil { h.ValidationLoop.Schema = m.localPath @@ -939,6 +949,10 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot // Same as forge pre_script above. case strings.HasPrefix(m.field, "forge.") && strings.HasSuffix(m.field, ".policy"): // Same as forge pre_script above. + case strings.HasPrefix(m.field, "overlays[") && strings.HasSuffix(m.field, ".validation_loop.schema"): + // Same as overlay pre_script above. + case strings.HasPrefix(m.field, "overlays[") && strings.HasSuffix(m.field, ".policy"): + // Same as overlay pre_script above. case strings.HasPrefix(m.field, "openshell.profiles["): // Profiles don't mutate harness fields — they're consumed via // the ResolvedProfile list built above. @@ -963,6 +977,14 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot // would duplicate the skill under the cache's internal tree name. case strings.HasPrefix(m.field, "forge.") && strings.Contains(m.field, ".providers["): case strings.HasPrefix(m.field, "forge.") && strings.Contains(m.field, ".openshell.profiles["): + case strings.HasPrefix(m.field, "overlays[") && strings.Contains(m.field, ".skills["): + // Overlay-scoped skills are resolved during LoadWithBase and merged + // into h.Skills by ResolveForge before resolveFromLock runs; the + // correctly named path is already in place. This entry exists for + // cache verification only — appending it via the default case + // would duplicate the skill under the cache's internal tree name. + case strings.HasPrefix(m.field, "overlays[") && strings.Contains(m.field, ".providers["): + case strings.HasPrefix(m.field, "overlays[") && strings.Contains(m.field, ".openshell.profiles["): case strings.Contains(m.field, ".overrides["): // Override file entries are resolved by ResolveHarness and cached // as individual files. Map the cache path back to the override @@ -1079,11 +1101,13 @@ func resolveFromLock(h *harness.Harness, entry *lock.HarnessLock, workspaceRoot // directory tree whose local basename must be derived from the recorded URL: // skills[N] and plugins[N] slots, plus forge-scoped skills // (forge..skills[N], see resolveBaseResources in -// internal/harness/compose.go). ForgeConfig has no plugins field. +// internal/harness/compose.go), plus overlay-scoped skills +// (overlays[N].skills[M]). ForgeConfig has no plugins field. func isTreeLockField(field string) bool { return strings.HasPrefix(field, "skills[") || strings.HasPrefix(field, "plugins[") || - (strings.HasPrefix(field, "forge.") && strings.Contains(field, ".skills[")) + (strings.HasPrefix(field, "forge.") && strings.Contains(field, ".skills[")) || + (strings.HasPrefix(field, "overlays[") && strings.Contains(field, ".skills[")) } // lockTreeDirName derives the local directory basename for a tree lock @@ -1121,6 +1145,9 @@ func isScriptLockField(field string) bool { case strings.HasPrefix(field, "forge.") && (strings.HasSuffix(field, ".pre_script") || strings.HasSuffix(field, ".post_script") || strings.HasSuffix(field, ".validation_loop.script")): return true + case strings.HasPrefix(field, "overlays[") && + (strings.HasSuffix(field, ".pre_script") || strings.HasSuffix(field, ".post_script") || strings.HasSuffix(field, ".validation_loop.script")): + return true default: return false } diff --git a/internal/cli/lock_test.go b/internal/cli/lock_test.go index eacbf7dc21..2ef91c12f4 100644 --- a/internal/cli/lock_test.go +++ b/internal/cli/lock_test.go @@ -2147,6 +2147,188 @@ func TestResolveFromLock_ForgeScopedSkillNoMutation(t *testing.T) { assert.Equal(t, mergedPath, h.Skills[0].Source) } +func TestResolveFromLock_OverlayScopedSkillNoMutation(t *testing.T) { + // Overlay-scoped skills are locked under overlays[N].skills[M] + // (see resolveBaseResources). Their paths were already merged into + // h.Skills by ResolveForge during LoadWithBase, so resolveFromLock must + // verify the cache entry but leave h.Skills alone — appending would + // duplicate the skill under the cache's internal tree name. + skillMD := []byte("---\nname: pr-review\n---\n") + skillFiles := map[string][]byte{"SKILL.md": skillMD} + treeHash := fetch.ComputeTreeHash(skillFiles) + + root := t.TempDir() + skillFileURL := "https://raw.githubusercontent.com/fullsend-ai/agents/abc123/skills/pr-review/SKILL.md" + _, err := fetch.CachePutDir(root, skillFileURL, skillFiles) + require.NoError(t, err) + treePath, _, err := fetch.CacheGetDir(root, treeHash) + require.NoError(t, err) + mergedPath, err := fetch.CacheNamedSymlink(treePath, "pr-review") + require.NoError(t, err) + require.True(t, filepath.IsAbs(mergedPath), + "merged path must live in the cache, not the test working directory") + + entry := &lock.HarnessLock{ + Dependencies: []lock.DependencyEntry{ + { + Field: "overlays[0].skills[0]", + URL: skillFileURL, + SHA256: treeHash, + Type: "directory", + Files: []lock.FileEntry{ + {Path: "SKILL.md", SHA256: fetch.ComputeSHA256(skillMD)}, + }, + }, + }, + } + + h := &harness.Harness{ + Agent: "agents/code.md", + Skills: []harness.SkillEntry{{Source: mergedPath}}, + AllowedRemoteResources: []string{"https://raw.githubusercontent.com/fullsend-ai/"}, + } + + printer := ui.New(os.Stdout) + lockResult, err := resolveFromLock(h, entry, root, printer) + require.NoError(t, err) + require.Len(t, lockResult.Deps, 1) + + require.Len(t, h.Skills, 1, "overlay-scoped skill lock entries must not append to h.Skills") + assert.Equal(t, mergedPath, h.Skills[0].Source) +} + +func TestResolveFromLock_OverlayScriptNoMutation(t *testing.T) { + // Overlay scripts (pre_script, post_script, validation_loop.script) are + // resolved during LoadWithBase and already set in the harness, so + // resolveFromLock must verify the cache entry but not mutate the harness. + script := []byte("#!/bin/bash\necho overlay\n") + scriptHash := fetch.ComputeSHA256(script) + + root := t.TempDir() + scriptURL := "https://raw.githubusercontent.com/fullsend-ai/agents/abc123/overlays/pre.sh" + require.NoError(t, fetch.CachePut(root, scriptURL, script)) + + cachePath, err := fetch.CachePath(root, scriptHash) + require.NoError(t, err) + cachePath = filepath.Join(cachePath, "content") + + entry := &lock.HarnessLock{ + Dependencies: []lock.DependencyEntry{ + { + Field: "overlays[0].pre_script", + URL: scriptURL, + SHA256: scriptHash, + Type: "file", + }, + }, + } + + h := &harness.Harness{ + Agent: "agents/code.md", + PreScript: cachePath, + AllowedRemoteResources: []string{"https://raw.githubusercontent.com/fullsend-ai/"}, + } + + printer := ui.New(os.Stdout) + lockResult, err := resolveFromLock(h, entry, root, printer) + require.NoError(t, err) + require.Len(t, lockResult.Deps, 1) + + // PreScript should remain unchanged — overlay scripts are already resolved + assert.Equal(t, cachePath, h.PreScript) + // Should not be appended to Skills + assert.Len(t, h.Skills, 0) +} + +func TestResolveFromLock_OverlayProviderParsed(t *testing.T) { + // Overlay providers must be parsed and added to ResolvedProvider list, + // not incorrectly appended as skills. + providerYAML := []byte("name: test\ntype: openai\n") + providerHash := fetch.ComputeSHA256(providerYAML) + + root := t.TempDir() + providerURL := "https://raw.githubusercontent.com/fullsend-ai/agents/abc123/overlays/provider.yaml" + require.NoError(t, fetch.CachePut(root, providerURL, providerYAML)) + + cachePath, err := fetch.CachePath(root, providerHash) + require.NoError(t, err) + cachePath = filepath.Join(cachePath, "content") + + entry := &lock.HarnessLock{ + Dependencies: []lock.DependencyEntry{ + { + Field: "overlays[0].providers[0]", + URL: providerURL, + SHA256: providerHash, + Type: "file", + }, + }, + } + + h := &harness.Harness{ + Agent: "agents/code.md", + Providers: []string{providerURL}, + AllowedRemoteResources: []string{"https://raw.githubusercontent.com/fullsend-ai/"}, + } + + printer := ui.New(os.Stdout) + lockResult, err := resolveFromLock(h, entry, root, printer) + require.NoError(t, err) + require.Len(t, lockResult.Deps, 1) + + // Should be in ResolvedProvider list + require.Len(t, lockResult.Providers, 1) + assert.Equal(t, "test", lockResult.Providers[0].Def.Name) + assert.Equal(t, "openai", lockResult.Providers[0].Def.Type) + assert.Equal(t, cachePath, lockResult.Providers[0].LocalPath) + + // Should not be appended to Skills + assert.Len(t, h.Skills, 0) +} + +func TestResolveFromLock_OverlayProfileParsed(t *testing.T) { + // Overlay profiles must be parsed and added to ResolvedProfile list, + // not incorrectly appended as skills. + profileYAML := []byte("id: test-profile\nshell: bash\n") + profileHash := fetch.ComputeSHA256(profileYAML) + + root := t.TempDir() + profileURL := "https://raw.githubusercontent.com/fullsend-ai/agents/abc123/overlays/profile.yaml" + require.NoError(t, fetch.CachePut(root, profileURL, profileYAML)) + + entry := &lock.HarnessLock{ + Dependencies: []lock.DependencyEntry{ + { + Field: "overlays[0].openshell.profiles[0]", + URL: profileURL, + SHA256: profileHash, + Type: "file", + }, + }, + } + + h := &harness.Harness{ + Agent: "agents/code.md", + OpenShell: &harness.OpenShellConfig{ + Profiles: []string{profileURL}, + }, + AllowedRemoteResources: []string{"https://raw.githubusercontent.com/fullsend-ai/"}, + } + + printer := ui.New(os.Stdout) + lockResult, err := resolveFromLock(h, entry, root, printer) + require.NoError(t, err) + require.Len(t, lockResult.Deps, 1) + + // Should be in ResolvedProfile list + require.Len(t, lockResult.Profiles, 1) + assert.Equal(t, "test-profile", lockResult.Profiles[0].ID) + assert.True(t, lockResult.Profiles[0].FromURL) + + // Should not be appended to Skills + assert.Len(t, h.Skills, 0) +} + func TestResolveFromLock_SkillRepoRootURLRejected(t *testing.T) { // A skills[N] lock entry whose URL is a forge repo root has no directory // segment to name the skill after; resolveFromLock must surface the From 7c579b363d77ebff14dccfe61be9b166ce4a648d Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 15:02:29 -0400 Subject: [PATCH 11/17] fix(harness): make forge deprecation warning reachable in CI Problem: The lint check for deprecated forge: field runs after ResolveForge has already cleared h.Forge = nil, so it never fires in CI (where forge platform is always set). Solution: Add a runtime-only hadForgeBeforeResolve field to the Harness struct (yaml:"-") that LoadWithOpts sets before calling ResolveForge. Lint() checks this field instead of h.Forge so the deprecation warning is emitted even when the forge platform is set. Fixes #6285 (Issue 3) Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/harness/harness.go | 8 ++++++++ internal/harness/lint.go | 2 +- internal/harness/lint_test.go | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 011cea2634..570c255061 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -349,6 +349,9 @@ type Harness struct { Forge map[string]*ForgeConfig `yaml:"forge,omitempty"` Overlays []OverlayEntry `yaml:"overlays,omitempty"` // CEL-guarded conditional config (ADR 0088) Trigger string `yaml:"trigger,omitempty"` // optional CEL boolean over normevent (ADR 0061) + + // Runtime-only fields (not serialized to YAML) + hadForgeBeforeResolve bool `yaml:"-"` // true if Forge was non-nil before ResolveForge; used by Lint() } // Load reads a harness YAML file from path, unmarshals it, and validates it. @@ -396,6 +399,11 @@ func LoadWithOpts(path string, opts LoadOpts) (*Harness, error) { return nil, fmt.Errorf("invalid harness: %w", err) } + // Capture whether forge was present before ResolveForge nils it out, + // so Lint() can emit the deprecation warning even when the forge + // platform is set (which is always the case in CI). + h.hadForgeBeforeResolve = h.Forge != nil + if err := h.ResolveForge(opts.ForgePlatform); err != nil { return nil, fmt.Errorf("resolving forge config: %w", err) } diff --git a/internal/harness/lint.go b/internal/harness/lint.go index e84318386a..1beffa845c 100644 --- a/internal/harness/lint.go +++ b/internal/harness/lint.go @@ -71,7 +71,7 @@ func (h *Harness) Lint() []Diagnostic { } } - if h.Forge != nil { + if h.hadForgeBeforeResolve { diags = append(diags, Diagnostic{ Severity: SeverityWarning, Field: "forge", diff --git a/internal/harness/lint_test.go b/internal/harness/lint_test.go index 3719b6102f..a76f20fe7f 100644 --- a/internal/harness/lint_test.go +++ b/internal/harness/lint_test.go @@ -97,6 +97,7 @@ func TestLint_ForgeDeprecationWarning(t *testing.T) { Forge: map[string]*ForgeConfig{ "github": {PreScript: "scripts/gh.sh"}, }, + hadForgeBeforeResolve: true, // simulate LoadWithOpts capturing this } diags := h.Lint() var found bool From b4f001abcf0eee8740ce15df26fee4d0552e7e42 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 15:02:42 -0400 Subject: [PATCH 12/17] fix(harness): remove config CEL variable whitelist Problem: BuildConfigMap only exposes 4 keys (forge, tracker, runtime, roles) but ADR 0088 says "full per-repo config". This was overly restrictive for overlay when expressions. Solution: Expand BuildConfigMap to expose all safe per-repo config fields via the PerRepoConfigReader interface. Sensitive fields (mint_url, inference provider details) remain excluded. Added comprehensive test coverage for the extended field set. Fields now exposed: - version, kill_switch (operational) - agents, allowed_remote_resources (policy) - create_issues, status_notifications (behavior config) Fixes #6285 (Issue 4) Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/harness/forge.go | 98 ++++++++++++++++++++++++++++++++-- internal/harness/forge_test.go | 52 ++++++++++++++++++ 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index a98df0da8c..350457b059 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -268,11 +268,17 @@ func (h *Harness) ResolveOverlays(event map[string]any, forgePlatform string, co for i, entry := range h.Overlays { matched, err := EvaluateOverlay(entry.When, event, forgePlatform, config) if err != nil { - // Treat CEL evaluation errors as non-matching: log and - // continue to the next entry, matching the MatchHarnesses - // pattern in harnessdispatch/enumerate.go. This allows - // fallback overlays to match when a more-specific overlay - // errors (e.g., event key access on an empty event map). + // Intentional exception to the harness package's return-errors + // convention: CEL evaluation errors are logged and treated as + // non-matching, allowing evaluation to continue to the next + // entry. This matches the MatchHarnesses pattern in + // harnessdispatch/enumerate.go and enables fallback overlays + // to match when a more-specific overlay errors (e.g., + // "event.source.system == 'jira'" fails with "no such key: + // source" when event is empty). Validation catches malformed + // CEL expressions at load time; runtime errors here are + // typically data-dependent (missing event keys) and should + // not prevent the harness from loading. log.Printf("harness: overlay[%d].when eval failed: %v", i, err) continue } @@ -395,6 +401,10 @@ func forgeKeyList(m map[string]*ForgeConfig) string { // expressions as the "config" variable. Returns nil when cfg is nil or // does not implement PerRepoConfigReader (e.g. org-mode configs). // +// All safe per-repo config fields are exposed. Sensitive fields (mint_url, +// inference provider details) are excluded. Per PR #6285 review feedback, +// the 4-key whitelist was expanded to expose the full non-sensitive config. +// // Both the CLI (run, lock) and harnessdispatch (enumerate) call sites use // this single implementation to ensure overlay CEL resolution sees the // same config shape regardless of the call path. @@ -407,6 +417,8 @@ func BuildConfigMap(cfg config.ConfigReader) map[string]any { return nil } m := map[string]any{} + + // Core platform fields if v := pr.ConfigForge(); v != "" { m["forge"] = v } @@ -424,5 +436,81 @@ func BuildConfigMap(cfg config.ConfigReader) map[string]any { } m["roles"] = anyRoles } + + // Operational fields + if v := pr.ConfigVersion(); v != "" { + m["version"] = v + } + if pr.IsKillSwitchActive() { + m["kill_switch"] = true + } + + // Agent entries (convert to CEL-compatible map slice) + if agents := pr.AgentEntries(); len(agents) > 0 { + anyAgents := make([]any, len(agents)) + for i, a := range agents { + agentMap := map[string]any{ + "source": a.Source, + } + if a.Name != "" { + agentMap["name"] = a.Name + } + if a.Enabled != nil { + agentMap["enabled"] = *a.Enabled + } + anyAgents[i] = agentMap + } + m["agents"] = anyAgents + } + + // Security policies + if arr := pr.AllowedResources(); len(arr) > 0 { + anyArr := make([]any, len(arr)) + for i, r := range arr { + anyArr[i] = r + } + m["allowed_remote_resources"] = anyArr + } + + // Issue creation config + if ci := pr.IssueCreationConfig(); ci != nil { + ciMap := map[string]any{} + if len(ci.AllowTargets.Orgs) > 0 { + anyOrgs := make([]any, len(ci.AllowTargets.Orgs)) + for i, o := range ci.AllowTargets.Orgs { + anyOrgs[i] = o + } + ciMap["allow_orgs"] = anyOrgs + } + if len(ci.AllowTargets.Repos) > 0 { + anyRepos := make([]any, len(ci.AllowTargets.Repos)) + for i, r := range ci.AllowTargets.Repos { + anyRepos[i] = r + } + ciMap["allow_repos"] = anyRepos + } + if len(ciMap) > 0 { + m["create_issues"] = ciMap + } + } + + // Status notifications config + if sn := pr.StatusNotifications(); sn != nil { + snMap := map[string]any{} + if sn.Comment.Start != "" { + snMap["start"] = sn.Comment.Start + } + if sn.Comment.Completion != "" { + snMap["completion"] = sn.Comment.Completion + } + if len(snMap) > 0 { + m["status_notifications"] = snMap + } + } + + // Note: mint_url and inference.* fields are intentionally excluded + // (contain credential URLs and GCP project identifiers that should not + // be exposed to harness CEL expressions). + return m } diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 996e44257f..361bee29ad 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1340,3 +1340,55 @@ func TestBuildConfigMap_OrgConfig(t *testing.T) { m := BuildConfigMap(orgCfg) assert.Nil(t, m) } + +func TestBuildConfigMap_AllFields(t *testing.T) { + t.Parallel() + // Test that BuildConfigMap exposes all non-sensitive per-repo config + // fields (PR #6285: removed 4-key whitelist). + cfg := config.NewPerRepoConfig([]string{"triage"}, "org/repo") + + // Set fields via writer interface + if w, ok := cfg.(config.PerRepoConfigWriter); ok { + w.SetRuntime("claude") + w.SetKillSwitch(true) + w.SetAgents([]config.AgentEntry{ + {Name: "my-agent", Source: "https://example.com/agent.yaml"}, + }) + w.SetAllowedRemoteResources([]string{"https://example.com/*"}) + } + + m := BuildConfigMap(cfg) + require.NotNil(t, m) + + // Core fields + assert.Equal(t, "claude", m["runtime"]) + roles, ok := m["roles"].([]any) + require.True(t, ok) + assert.Contains(t, roles, "triage") + + // Operational fields + assert.Equal(t, "1", m["version"]) + assert.Equal(t, true, m["kill_switch"]) + + // Agent entries + agents, ok := m["agents"].([]any) + require.True(t, ok) + require.Len(t, agents, 1) + agentMap, ok := agents[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "my-agent", agentMap["name"]) + assert.Equal(t, "https://example.com/agent.yaml", agentMap["source"]) + + // Security policies + arr, ok := m["allowed_remote_resources"].([]any) + require.True(t, ok) + assert.Contains(t, arr, "https://example.com/*") + + // Issue creation config (set by NewPerRepoConfig with targetRepo) + ci, ok := m["create_issues"].(map[string]any) + require.True(t, ok) + repos, ok := ci["allow_repos"].([]any) + require.True(t, ok) + assert.Contains(t, repos, "org/repo") + assert.Contains(t, repos, "fullsend-ai/fullsend") +} From 836724f702bfc44b7eadd744275d5695994090a0 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 15:02:57 -0400 Subject: [PATCH 13/17] docs(overlays): document has() guard and empty-event semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: Guide examples like "event.source.system == 'jira'" error with "no such key: source" when event is empty. has() exists but isn't documented. Several comments and ADR text are stale. Solution: - Document has(event.source) pattern in bring-your-own-agent.md and harness-composition.md - Update harness-fields.md to reflect current overlay resolution (nil event → empty map substitution, not no-op) - Fix stale comment in compose.go (Event field docs) - Add note to ADR 0088 pointing to harness-fields.md as living reference for current semantics Fixes #6285 (Issues 5, 6) Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- docs/ADRs/0088-cel-guarded-overlays.md | 7 ++++-- docs/contributing/harness-composition.md | 2 +- docs/contributing/harness-fields.md | 29 ++++++++++++------------ docs/guides/user/bring-your-own-agent.md | 2 +- internal/harness/compose.go | 4 +++- 5 files changed, 25 insertions(+), 19 deletions(-) diff --git a/docs/ADRs/0088-cel-guarded-overlays.md b/docs/ADRs/0088-cel-guarded-overlays.md index 56f4d8b846..46c792465a 100644 --- a/docs/ADRs/0088-cel-guarded-overlays.md +++ b/docs/ADRs/0088-cel-guarded-overlays.md @@ -126,6 +126,11 @@ warning when `forge:` is present, recommending migration to ### Resolution pipeline +> **Update (2026-08-21):** The empty-event semantics described below have +> evolved since this ADR was written. See +> [Harness Field Reference](../contributing/harness-fields.md) for the +> current behavior (nil event → empty map substitution). + `LoadWithOpts` and `LoadWithBase` gain `Event normevent.Event` and `Config map[string]any` fields in their options structs. The pipeline becomes: @@ -139,8 +144,6 @@ ResolveForge(platform) → ResolveOverlays(event, config) → Validate against the CEL environment (see below). The first entry whose `when` returns true is merged; remaining entries are skipped. Like `ResolveForge`, it nils out the field after resolution (consumed). -When `Event` is nil, `ResolveOverlays` is a no-op (no entries -match), paralleling `ResolveForge` when `ForgePlatform` is empty. ### CEL environment diff --git a/docs/contributing/harness-composition.md b/docs/contributing/harness-composition.md index 5b5ecf74fa..041d4c73fa 100644 --- a/docs/contributing/harness-composition.md +++ b/docs/contributing/harness-composition.md @@ -52,7 +52,7 @@ and update the others as needed. | `validateForge` | `internal/harness/forge.go` | Validates `forge:` block keys and `ForgeConfig` field values | | `validateOverlays` | `internal/harness/forge.go` | Validates `overlays:` entries — CEL `when` expressions and `ForgeConfig` field values; enforces mutual exclusion with `forge:` | | `ResolveForge` | `internal/harness/forge.go` | Merges the selected forge platform's config into the harness and nils the forge map | -| `ResolveOverlays` | `internal/harness/forge.go` | Evaluates overlay `when` expressions against event/runtime/config CEL environment; merges the first matching entry (first-match-wins) and nils the overlays list | +| `ResolveOverlays` | `internal/harness/forge.go` | Evaluates overlay `when` expressions against event/runtime/config CEL environment; merges the first matching entry (first-match-wins) and nils the overlays list. When event is nil (CLI flows without event context), an empty map is substituted so overlays conditioned on `runtime.forge` or `config` can still match. Use `has(event.source)` to guard event field access in `when` expressions. | ### How they correspond diff --git a/docs/contributing/harness-fields.md b/docs/contributing/harness-fields.md index 47198ecccb..48c5cf0045 100644 --- a/docs/contributing/harness-fields.md +++ b/docs/contributing/harness-fields.md @@ -115,28 +115,29 @@ The current forge resolution pipeline is: Unmarshal → validateForge → ResolveForge(platform) → Validate ``` -## Overlay resolution (planned — ADR-0088) +## Overlay resolution (ADR-0088) -> **Note:** This section describes planned behavior from -> [ADR-0088](../ADRs/0088-cel-guarded-overlays.md). The overlay feature -> has not been implemented yet. The current implementation uses `forge:` -> blocks only. +`overlays:` is the successor to deprecated `forge:` blocks. Each overlay +entry has a `when:` CEL expression and the same override fields as +`ForgeConfig`. The first entry whose `when` evaluates to true is merged; +remaining entries are skipped (first-match-wins). -`overlays:` is the planned successor to `forge:` blocks. Each overlay -entry will have a `when:` CEL expression and the same override fields as -`ForgeConfig`. The first entry whose `when` evaluates to true will be -merged; remaining entries will be skipped. - -### Planned resolution pipeline +### Resolution pipeline ``` Unmarshal → validateForge → validateOverlays → -ResolveForge(platform) → ResolveOverlays(event, config) → Validate +ResolveForge(platform) → ResolveOverlays(event, forgePlatform, config) → Validate ``` -### Planned CEL environment +When `event` is nil (CLI flows without event context like `fullsend run` +or `fullsend lock`), `ResolveOverlays` substitutes an empty map so +overlays conditioned on `runtime.forge` or `config` can still evaluate +and match. Overlays that reference `event` fields should use `has()` to +guard field access (e.g., `has(event.source) && event.source.system == "jira"`). + +### CEL environment -Overlay `when` expressions will be evaluated with: +Overlay `when` expressions are evaluated with: | Variable | Type | Source | |---|---|---| diff --git a/docs/guides/user/bring-your-own-agent.md b/docs/guides/user/bring-your-own-agent.md index e09aa1e454..9818436b5e 100644 --- a/docs/guides/user/bring-your-own-agent.md +++ b/docs/guides/user/bring-your-own-agent.md @@ -188,7 +188,7 @@ Key patterns to note: - **`policy: policies/triage.yaml`** is a per-agent policy that includes filesystem, landlock, process, and network rules (via inline `network_policies`). This agent predates the provider-based pattern — new agents can use `providers:` instead (see [Minimum viable agent](#minimum-viable-agent)). - **`host_files`** copy credentials from the trusted runner into the sandbox. `expand: true` resolves `${VAR}` references before copying. - **`validation_loop.schema`** references the JSON schema file directly — the validation script checks agent output against it. -- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, openshell, host_files, and env vars. Resolution is first-match-wins: the first entry whose `when` evaluates to true is merged; remaining entries are skipped. The CEL environment exposes `event` (the triggering event), `runtime.forge` (the effective forge platform), and `config` (per-repo config from config.yaml). +- **`overlays`** uses CEL `when` expressions to conditionally apply scripts, skills, providers, openshell, host_files, and env vars. Resolution is first-match-wins: the first entry whose `when` evaluates to true is merged; remaining entries are skipped. The CEL environment exposes `event` (the triggering event), `runtime.forge` (the effective forge platform), and `config` (per-repo config from config.yaml). When running without an event context (e.g., `fullsend run` or `fullsend lock`), `event` is an empty map — use `has(event.source)` to guard event field access: `has(event.source) && event.source.system == "jira"` instead of just `event.source.system == "jira"` to avoid "no such key" errors. - **`common/env/gcp-vertex.env`** is referenced by relative path because both files live in the same repo. If your agent lives in a different repo, reference it by URL (see [Remote references](#referencing-resources-local-vs-remote)) or copy it locally. ## Harness field reference diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 69a0b3c1e4..291f9f41e4 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -77,7 +77,9 @@ type ComposeOpts struct { SourceURL string // Event is the normalized event data for CEL overlay resolution (ADR 0088). - // If nil, ResolveOverlays is a no-op. + // When nil, ResolveOverlays substitutes an empty map so overlays conditioned + // on runtime.forge or config can still evaluate and match (CLI flows without + // event context). Event map[string]any // Config is the per-repo config (from config.yaml) exposed to overlay From 379c3dac1e629c4f4e747b627f575732277d8ef0 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 21 Aug 2026 15:06:07 -0400 Subject: [PATCH 14/17] refactor(harness): clarify naming and error message conventions in overlay validation Address low-priority review feedback from PR #6285: - Issue 8: Add comment explaining why validateOverlayForgeConfig references "ForgeConfig" in the function name rather than "OverlayEntry". The name is semantically accurate (validates ForgeConfig fields) and remains clear with the expanded documentation. - Issue 9: Add comment explaining why the forge/overlays mutual exclusion error includes remediation advice unlike other validation errors. This is appropriate as a one-time migration message guiding users from the deprecated forge feature to overlays (ADR 0088). Both changes add explanatory comments rather than altering behavior, keeping the code semantically accurate while documenting the design choices. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/harness/forge.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 350457b059..4f01364ce2 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -140,9 +140,13 @@ func (h *Harness) validateForge() error { // validateOverlayForgeConfig validates a ForgeConfig embedded in an overlay // entry, applying the same checks as validateForge per entry. OverlayEntry // embeds ForgeConfig via yaml:",inline" (see OverlayEntry), so overlay -// entries carry the same override fields as forge platform blocks. The -// "ForgeConfig" name is a legacy artifact from the forge feature being -// deprecated in favor of overlays (ADR 0088). +// entries carry the same override fields as forge platform blocks. +// +// Naming: The function references "ForgeConfig" rather than "OverlayEntry" +// because it validates the ForgeConfig fields embedded in each overlay entry. +// The ForgeConfig type name is a legacy artifact from the original forge +// feature (ADR 0088 deprecated forge in favor of overlays), but the name +// remains accurate: this function validates ForgeConfig fields. func validateOverlayForgeConfig(idx int, fc *ForgeConfig) error { prefix := fmt.Sprintf("overlays[%d]", idx) if fc.Policy != "" && IsURL(fc.Policy) { @@ -215,6 +219,11 @@ func (h *Harness) validateOverlays() error { return nil } if h.Forge != nil { + // This error includes remediation advice ("migrate forge entries to + // overlays") unlike other validation errors because it's a migration + // error guiding users from the deprecated forge feature to overlays + // (ADR 0088). The remediation is brief, actionable, and appropriate + // for a one-time migration scenario. return fmt.Errorf("forge and overlays cannot coexist in the same harness; migrate forge entries to overlays") } for i, entry := range h.Overlays { From 126ee0ca215429b0edc39625b2b3a2880f1d6e15 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:46:47 +0000 Subject: [PATCH 15/17] =?UTF-8?q?fix(harness):=20address=20review=20feedba?= =?UTF-8?q?ck=20=E2=80=94=20validate=20config.forge,=20update=20docs,=20im?= =?UTF-8?q?prove=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate config.forge in detectForgePlatform: typos in config.forge now return an error instead of silently shadowing CI env vars - Update harness-fields.md: remove "planned"/"not yet implemented" markers for overlays (now implemented) - Add tests for validateOverlayForgeConfig (31.6% → 97.4%) - Add tests for EvaluateOverlay (68% → 88%) - Add tests for overlay URL-base composition (scripts, policy, skills, providers, host_files, profiles, validation_loop) - Add tests for forge deprecation warning through LoadWithOpts - Add test for config.forge validation in detectForgePlatform Addresses review feedback on #6285 Co-Authored-By: Claude Opus 4.6 --- docs/contributing/harness-fields.md | 4 +- internal/cli/run.go | 3 + internal/cli/run_test.go | 20 ++ internal/harness/compose_test.go | 339 ++++++++++++++++++++++++++++ internal/harness/forge_test.go | 222 ++++++++++++++++++ internal/harness/harness_test.go | 80 +++++++ internal/harness/trigger_test.go | 81 +++++++ 7 files changed, 747 insertions(+), 2 deletions(-) diff --git a/docs/contributing/harness-fields.md b/docs/contributing/harness-fields.md index 48c5cf0045..5330175fa4 100644 --- a/docs/contributing/harness-fields.md +++ b/docs/contributing/harness-fields.md @@ -81,7 +81,7 @@ field type follows specific merge semantics. The same rules apply during | `api_servers` | Concatenated (base + child) | Absent (nil) = inherit | | `env` | Sub-maps (`runner`, `sandbox`) merged independently; forge/child keys win (ADR-0055) | Absent (nil) = inherit | | `security` | Child replaces base entirely (if non-nil) | Absent (nil) = inherit | -| `overlays` *(planned)* | Concatenated (base + child); first-match-wins at resolution (ADR-0088, not yet implemented) | Absent (nil) = inherit | +| `overlays` | Concatenated (base + child); first-match-wins at resolution (ADR-0088) | Absent (nil) = inherit | ## `ForgeConfig` struct @@ -148,7 +148,7 @@ Overlay `when` expressions are evaluated with: ### Mutual exclusion `forge:` and `overlays:` must not coexist in the same harness (post-merge). -`forge:` is deprecated; new harnesses should use `overlays:` once implemented. +`forge:` is deprecated; new harnesses should use `overlays:` instead. ## Related diff --git a/internal/cli/run.go b/internal/cli/run.go index 777cef3641..a37705556c 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -3801,6 +3801,9 @@ func detectForgePlatform(flag string, cfg config.ConfigReader) (string, error) { if cfg != nil { if pr, ok := cfg.(config.PerRepoConfigReader); ok { if forge := pr.ConfigForge(); forge != "" { + if !harness.ValidForgePlatform(forge) { + return "", fmt.Errorf("config.forge: %q is not a valid forge platform (valid: %s)", forge, harness.ForgeKeyList()) + } return forge, nil } } diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index f56389e615..fd56c1229c 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -3431,6 +3431,26 @@ roles: assert.Equal(t, "gitlab", p) } +func TestDetectForgePlatform_InvalidConfigForge(t *testing.T) { + t.Setenv("GITHUB_ACTIONS", "true") + t.Setenv("GITLAB_CI", "") + + yamlData := ` +version: "1" +forge: gihub +roles: + - triage +` + cfg, err := config.ParsePerRepoConfig([]byte(yamlData)) + require.NoError(t, err) + + _, err = detectForgePlatform("", cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "config.forge") + assert.Contains(t, err.Error(), "gihub") + assert.Contains(t, err.Error(), "not a valid forge platform") +} + func TestRunCommand_HasForgeFlag(t *testing.T) { cmd := newRunCmd() flag := cmd.Flags().Lookup("forge") diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index c7630de84a..57b4e85539 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -8437,3 +8437,342 @@ overlays: assert.Equal(t, "scripts/gh.sh", h.PreScript) assert.Nil(t, h.Overlays) } + +func TestLoadWithBase_URLBase_OverlayScriptsFetched(t *testing.T) { + overlayPre := []byte("#!/bin/bash\necho overlay-pre") + overlayPost := []byte("#!/bin/bash\necho overlay-post") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + pre_script: scripts/overlay-pre.sh + post_script: scripts/overlay-post.sh +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/scripts/overlay-pre.sh": overlayPre, + "/scripts/overlay-post.sh": overlayPost, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, scripts are promoted to top level + assert.True(t, filepath.IsAbs(h.PreScript)) + assert.True(t, filepath.IsAbs(h.PostScript)) + + preContent, err := os.ReadFile(h.PreScript) + require.NoError(t, err) + assert.Equal(t, overlayPre, preContent) + + postContent, err := os.ReadFile(h.PostScript) + require.NoError(t, err) + assert.Equal(t, overlayPost, postContent) + + // Should have deps for: base, overlay pre_script, overlay post_script, agent + var overlayDeps int + for _, d := range deps { + if strings.HasPrefix(d.Field, "overlays[") { + overlayDeps++ + } + } + assert.GreaterOrEqual(t, overlayDeps, 2, "expected at least 2 overlay script deps") +} + +func TestLoadWithBase_URLBase_OverlayPolicyFetched(t *testing.T) { + policyContent := []byte("# test policy") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + policy: policies/overlay-sandbox.yaml +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/policies/overlay-sandbox.yaml": policyContent, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, policy is promoted to top level + assert.True(t, filepath.IsAbs(h.Policy)) + + var foundPolicyDep bool + for _, d := range deps { + if strings.HasPrefix(d.Field, "overlays[") && strings.HasSuffix(d.Field, ".policy") { + foundPolicyDep = true + } + } + assert.True(t, foundPolicyDep, "expected overlay policy dep") +} + +func TestLoadWithBase_URLBase_OverlaySkillsFetchedFromBase(t *testing.T) { + skillContent := []byte("# Overlay skill\nThis is an overlay-specific skill.") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + skills: + - skills/overlay-skill +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/skills/overlay-skill/SKILL.md": skillContent, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +base: `+baseURL+` +allowed_remote_resources: + - `+server.URL+`/ +`) + + // The test server URL is not a raw.githubusercontent.com URL, so skill + // directory resolution errors out (same as TestLoadWithBase_URLBase_ForgeSkillsFetchedFromBase). + // This confirms resolveBaseResources now iterates overlay-level skills. + _, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a raw.githubusercontent.com URL") + assert.Contains(t, err.Error(), "overlays[0].skills[0]") +} + +func TestLoadWithBase_URLBase_OverlayProvidersFetched(t *testing.T) { + providerContent := []byte("name: test-provider\ntype: openai") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + providers: + - providers/overlay-provider.yaml +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/providers/overlay-provider.yaml": providerContent, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, providers are merged to top level + assert.GreaterOrEqual(t, len(h.Providers), 1, "expected at least 1 provider after overlay resolution") + + var foundProviderDep bool + for _, d := range deps { + if strings.HasPrefix(d.Field, "overlays[") && strings.Contains(d.Field, ".providers[") { + foundProviderDep = true + } + } + assert.True(t, foundProviderDep, "expected overlay provider dep") +} + +func TestLoadWithBase_URLBase_OverlayHostFilesFetched(t *testing.T) { + envContent := []byte("KEY=value") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + host_files: + - src: env/overlay.env + dest: /run/secrets/overlay.env +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/env/overlay.env": envContent, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, host files are merged to top level + assert.GreaterOrEqual(t, len(h.HostFiles), 1, "expected at least 1 host file after overlay resolution") + + var foundHostFileDep bool + for _, d := range deps { + if strings.HasPrefix(d.Field, "overlays[") && strings.Contains(d.Field, ".host_files[") { + foundHostFileDep = true + } + } + assert.True(t, foundHostFileDep, "expected overlay host file dep") +} + +func TestLoadWithBase_URLBase_OverlayProfilesFetched(t *testing.T) { + profileContent := []byte("id: test-profile\nshell: bash") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + openshell: + profiles: + - profiles/overlay-profile.yaml +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/profiles/overlay-profile.yaml": profileContent, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, profiles are merged to top level + assert.NotNil(t, h.OpenShell, "expected openshell config after overlay resolution") + if h.OpenShell != nil { + assert.GreaterOrEqual(t, len(h.OpenShell.Profiles), 1, "expected at least 1 profile after overlay resolution") + } +} + +func TestLoadWithBase_URLBase_OverlayValidationLoopFetched(t *testing.T) { + valScript := []byte("#!/bin/bash\necho validate") + + baseContent := []byte(` +agent: agents/triage.md +role: test +overlays: +- when: 'runtime.forge == "github"' + validation_loop: + script: scripts/overlay-validate.sh +`) + + server, policy := setupScriptTestServer(t, baseContent, map[string][]byte{ + "/scripts/overlay-validate.sh": valScript, + }) + + hash := computeHash(baseContent) + dir := t.TempDir() + cacheDir := filepath.Join(dir, "cache") + baseURL := server.URL + "/harness/triage.yaml#sha256=" + hash + + path := writeTestHarness(t, dir, "child.yaml", ` +agent: agents/child.md +role: test +base: `+baseURL+` +`) + + h, deps, err := LoadWithBase(context.Background(), path, ComposeOpts{ + WorkspaceRoot: cacheDir, + FetchPolicy: policy, + OrgAllowlist: []string{server.URL + "/"}, + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + // After overlay resolution, validation loop is promoted to top level + require.NotNil(t, h.ValidationLoop) + assert.True(t, filepath.IsAbs(h.ValidationLoop.Script)) + + var foundValDep bool + for _, d := range deps { + if strings.HasPrefix(d.Field, "overlays[") && strings.Contains(d.Field, "validation_loop") { + foundValDep = true + } + } + assert.True(t, foundValDep, "expected overlay validation_loop dep") +} diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 361bee29ad..f466aeed64 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -1392,3 +1392,225 @@ func TestBuildConfigMap_AllFields(t *testing.T) { assert.Contains(t, repos, "org/repo") assert.Contains(t, repos, "fullsend-ai/fullsend") } + +func TestBuildConfigMap_ForgeAndTracker(t *testing.T) { + t.Parallel() + yamlData := []byte(` +version: "1" +forge: github +tracker: jira +roles: + - triage +`) + cfg, err := config.ParsePerRepoConfig(yamlData) + require.NoError(t, err) + + m := BuildConfigMap(cfg) + require.NotNil(t, m) + assert.Equal(t, "github", m["forge"]) + assert.Equal(t, "jira", m["tracker"]) +} + +func TestBuildConfigMap_EmptyFieldsOmitted(t *testing.T) { + t.Parallel() + // Config with only roles (no forge, tracker) — empty fields + // should not appear in the map. Runtime defaults to "claude". + yamlData := []byte(` +version: "1" +roles: + - code +`) + cfg, err := config.ParsePerRepoConfig(yamlData) + require.NoError(t, err) + + m := BuildConfigMap(cfg) + require.NotNil(t, m) + _, hasForge := m["forge"] + assert.False(t, hasForge, "forge should not be in map when empty") + _, hasTracker := m["tracker"] + assert.False(t, hasTracker, "tracker should not be in map when empty") + // Runtime defaults to "claude" so it should be present + assert.Equal(t, "claude", m["runtime"]) +} + +// --- validateOverlayForgeConfig coverage --- + +func TestValidateOverlayForgeConfig_PolicyURLWithoutHash(t *testing.T) { + fc := &ForgeConfig{ + Policy: "https://example.com/policy.yaml", + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].policy URL must include #sha256=") +} + +func TestValidateOverlayForgeConfig_PolicyURLWithHash(t *testing.T) { + fc := &ForgeConfig{ + Policy: "https://example.com/policy.yaml#sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_PolicyLocalPath(t *testing.T) { + fc := &ForgeConfig{ + Policy: "policies/sandbox.yaml", + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_PostScriptURL(t *testing.T) { + fc := &ForgeConfig{ + PostScript: "https://example.com/post.sh", + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].post_script must be a local path, not a URL") +} + +func TestValidateOverlayForgeConfig_SkillURLWithoutHash(t *testing.T) { + fc := &ForgeConfig{ + Skills: []SkillEntry{{Source: "https://example.com/skill"}}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].skills[0] URL must include #sha256=") +} + +func TestValidateOverlayForgeConfig_SkillURLWithHash(t *testing.T) { + fc := &ForgeConfig{ + Skills: []SkillEntry{{Source: "https://example.com/skill#sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"}}, + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_ProviderURLWithoutHash(t *testing.T) { + fc := &ForgeConfig{ + Providers: []string{"https://example.com/provider.yaml"}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].providers[0] URL must include #sha256=") +} + +func TestValidateOverlayForgeConfig_ProviderURLWithHash(t *testing.T) { + fc := &ForgeConfig{ + Providers: []string{"https://example.com/provider.yaml#sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"}, + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_OpenShellProfileURLWithoutHash(t *testing.T) { + fc := &ForgeConfig{ + OpenShell: &OpenShellConfig{ + Profiles: []string{"https://example.com/profile.yaml"}, + }, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].openshell.profiles[0] URL must include #sha256=") +} + +func TestValidateOverlayForgeConfig_OpenShellProfileWithHash(t *testing.T) { + fc := &ForgeConfig{ + OpenShell: &OpenShellConfig{ + Profiles: []string{"https://example.com/profile.yaml#sha256=e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"}, + }, + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_HostFileMissingSrc(t *testing.T) { + fc := &ForgeConfig{ + HostFiles: []HostFile{{Src: "", Dest: "/run/secrets/token"}}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].host_files[0]: src is required") +} + +func TestValidateOverlayForgeConfig_HostFileMissingDest(t *testing.T) { + fc := &ForgeConfig{ + HostFiles: []HostFile{{Src: "env/token.env", Dest: ""}}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].host_files[0]: dest is required") +} + +func TestValidateOverlayForgeConfig_HostFileURLSrc(t *testing.T) { + fc := &ForgeConfig{ + HostFiles: []HostFile{{Src: "https://example.com/token.env", Dest: "/run/secrets/token"}}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].host_files[0].src must be a local path, not a URL") +} + +func TestValidateOverlayForgeConfig_HostFileValid(t *testing.T) { + fc := &ForgeConfig{ + HostFiles: []HostFile{{Src: "env/token.env", Dest: "/run/secrets/token"}}, + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_ValidationLoopMissingScript(t *testing.T) { + fc := &ForgeConfig{ + ValidationLoop: &ValidationLoop{Script: ""}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].validation_loop.script is required when validation_loop is set") +} + +func TestValidateOverlayForgeConfig_ValidationLoopScriptURL(t *testing.T) { + fc := &ForgeConfig{ + ValidationLoop: &ValidationLoop{Script: "https://example.com/validate.sh"}, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].validation_loop.script must be a local path, not a URL") +} + +func TestValidateOverlayForgeConfig_ValidationLoopSchemaURL(t *testing.T) { + fc := &ForgeConfig{ + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + Schema: "https://example.com/schema.json", + }, + } + err := validateOverlayForgeConfig(0, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[0].validation_loop.schema must be a local path, not a URL") +} + +func TestValidateOverlayForgeConfig_ValidationLoopValid(t *testing.T) { + fc := &ForgeConfig{ + ValidationLoop: &ValidationLoop{ + Script: "scripts/validate.sh", + Schema: "schemas/output.json", + }, + } + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_EmptyForgeConfig(t *testing.T) { + fc := &ForgeConfig{} + err := validateOverlayForgeConfig(0, fc) + require.NoError(t, err) +} + +func TestValidateOverlayForgeConfig_SecondIndex(t *testing.T) { + fc := &ForgeConfig{ + PreScript: "https://example.com/pre.sh", + } + err := validateOverlayForgeConfig(1, fc) + require.Error(t, err) + assert.Contains(t, err.Error(), "overlays[1].pre_script must be a local path, not a URL") +} diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index 2ae3802fc7..99e6efabad 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -2378,3 +2378,83 @@ overlays: require.NoError(t, err) assert.Equal(t, "scripts/jira.sh", h.PreScript) } + +func TestLoadWithOpts_ForgeDeprecationWarningAfterResolve(t *testing.T) { + // Verify that the forge deprecation warning is reachable even after + // ResolveForge nils out h.Forge (the hadForgeBeforeResolve flag). + content := ` +agent: agents/test.md +role: fix +forge: + github: + pre_script: scripts/gh.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + // forge should have been resolved (nilled) + assert.Nil(t, h.Forge) + assert.Equal(t, "scripts/gh.sh", h.PreScript) + + // Lint should still emit the deprecation warning + diags := h.Lint() + var found bool + for _, d := range diags { + if d.Field == "forge" && d.Severity == SeverityWarning { + found = true + assert.Contains(t, d.Message, "deprecated") + } + } + assert.True(t, found, "expected forge deprecation warning from Lint() after ResolveForge") +} + +func TestLoadWithOpts_NoForgeNoDeprecationWarning(t *testing.T) { + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'runtime.forge == "github"' + pre_script: scripts/gh.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + ForgePlatform: "github", + Event: map[string]any{}, + }) + require.NoError(t, err) + + diags := h.Lint() + for _, d := range diags { + assert.NotEqual(t, "forge", d.Field, "should not have forge deprecation warning for overlays-only harness") + } +} + +func TestLoadWithOpts_OverlayNilEvent(t *testing.T) { + // Overlays conditioned on runtime.forge should still match when + // event is nil (CLI run/lock flows without event context). + content := ` +agent: agents/test.md +role: fix +overlays: +- when: 'runtime.forge == "github"' + pre_script: scripts/gh.sh +` + dir := t.TempDir() + path := filepath.Join(dir, "test.yaml") + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + + h, err := LoadWithOpts(path, LoadOpts{ + ForgePlatform: "github", + // Event is nil — simulates CLI flow without --event-file + }) + require.NoError(t, err) + assert.Equal(t, "scripts/gh.sh", h.PreScript) +} diff --git a/internal/harness/trigger_test.go b/internal/harness/trigger_test.go index 856e739cb7..5404b26d1e 100644 --- a/internal/harness/trigger_test.go +++ b/internal/harness/trigger_test.go @@ -35,3 +35,84 @@ func TestEvaluateTrigger(t *testing.T) { require.NoError(t, err) assert.False(t, ok) } + +func TestEvaluateOverlay_RuntimeForge(t *testing.T) { + event := map[string]any{"source": map[string]any{"system": "github"}} + ok, err := EvaluateOverlay(`runtime.forge == "github"`, event, "github", nil) + require.NoError(t, err) + assert.True(t, ok) + + ok, err = EvaluateOverlay(`runtime.forge == "github"`, event, "gitlab", nil) + require.NoError(t, err) + assert.False(t, ok) +} + +func TestEvaluateOverlay_EventField(t *testing.T) { + event := map[string]any{"source": map[string]any{"system": "jira"}} + ok, err := EvaluateOverlay(`event.source.system == "jira"`, event, "github", nil) + require.NoError(t, err) + assert.True(t, ok) +} + +func TestEvaluateOverlay_ConfigVariable(t *testing.T) { + event := map[string]any{} + config := map[string]any{"tracker": "jira"} + ok, err := EvaluateOverlay(`config.tracker == "jira"`, event, "", config) + require.NoError(t, err) + assert.True(t, ok) +} + +func TestEvaluateOverlay_EmptyExpression(t *testing.T) { + ok, err := EvaluateOverlay("", nil, "", nil) + require.NoError(t, err) + assert.False(t, ok) +} + +func TestEvaluateOverlay_WhitespaceExpression(t *testing.T) { + ok, err := EvaluateOverlay(" ", nil, "", nil) + require.NoError(t, err) + assert.False(t, ok) +} + +func TestEvaluateOverlay_InvalidCEL(t *testing.T) { + _, err := EvaluateOverlay(`runtime.forge ==`, nil, "", nil) + require.Error(t, err) +} + +func TestEvaluateOverlay_NonBoolResult(t *testing.T) { + _, err := EvaluateOverlay(`"not a bool"`, nil, "", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "not bool") +} + +func TestEvaluateOverlay_NilConfig(t *testing.T) { + event := map[string]any{} + // nil config should be substituted with empty map + ok, err := EvaluateOverlay(`runtime.forge == "github"`, event, "github", nil) + require.NoError(t, err) + assert.True(t, ok) +} + +func TestEvaluateOverlay_HasGuard(t *testing.T) { + // has() guard should work for missing keys in empty events + event := map[string]any{} + ok, err := EvaluateOverlay(`has(event.source) && event.source.system == "jira"`, event, "", nil) + require.NoError(t, err) + assert.False(t, ok) +} + +func TestEvaluateOverlay_CombinedExpression(t *testing.T) { + event := map[string]any{"source": map[string]any{"system": "jira"}} + ok, err := EvaluateOverlay( + `event.source.system == "jira" && runtime.forge == "github"`, + event, "github", nil, + ) + require.NoError(t, err) + assert.True(t, ok) +} + +func TestNewOverlayEnv(t *testing.T) { + env, err := NewOverlayEnv() + require.NoError(t, err) + require.NotNil(t, env) +} From 0c20de3107f1477c5c454df95ee7596716769703 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 24 Aug 2026 14:46:26 -0400 Subject: [PATCH 16/17] fix(cli): correct runAgent call signatures in tests Remove extra empty string argument from runAgent test calls that was accidentally added during conflict resolution. The function signature has 3 string parameters after noPostScript (debug, forgeFlag, eventFile), not 4. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/cli/run_test.go | 52 ++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index fd56c1229c..45f6e039b6 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -194,7 +194,7 @@ func TestRunAgent_HarnessLoadPipeline(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -224,7 +224,7 @@ func TestRunAgent_YMLFallback(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -237,7 +237,7 @@ func TestRunAgent_HarnessNotFound(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -269,7 +269,7 @@ func TestRunAgent_HarnessLoadWithOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -299,7 +299,7 @@ func TestRunAgent_PerRepoConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -562,7 +562,7 @@ func TestRunAgent_MalformedOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -589,7 +589,7 @@ func TestRunAgent_MalformedOrgConfigWithURLRefs(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -611,7 +611,7 @@ func TestRunAgent_URLRefsNoOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -653,7 +653,7 @@ func TestRunAgent_WithURLBase(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -709,7 +709,7 @@ openshell: rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) // The test will fail after the orchestration block (e.g. during // bootstrapCommon or pre-script setup), but it must NOT fail at // the gateway check or provider/profile steps. @@ -745,7 +745,7 @@ func TestRunAgent_URLBaseNoAllowlist(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in allowed_remote_resources") } @@ -774,7 +774,7 @@ func TestRunAgent_URLBaseMalformedOrgConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "no config and agents-repo fallback unavailable") } @@ -886,7 +886,7 @@ func TestRunAgent_ConfigAgentLocalPath(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "custom", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "custom", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -922,7 +922,7 @@ func TestRunAgent_ConfigAgentURL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "triage", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "triage", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -956,7 +956,7 @@ func TestRunAgent_ConfigAgentOverridesScaffold(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -978,7 +978,7 @@ func TestRunAgent_AgentNotInConfig(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in config and agents-repo fallback unavailable") } @@ -998,7 +998,7 @@ func TestRunAgent_UnknownAgentName(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "nonexistent", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "not in config and agents-repo fallback unavailable") } @@ -3595,7 +3595,7 @@ func TestRunAgent_PreflightCheck_Passing(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) // Must pass the preflight guard and reach the openshell check. assert.Contains(t, err.Error(), "openshell") @@ -3610,7 +3610,7 @@ func TestRunAgent_PreflightCheck_Failing(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "preflight_check failed") } @@ -3624,7 +3624,7 @@ func TestRunAgent_PreflightCheck_NoCheckConfigured(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -3656,7 +3656,7 @@ func TestRunAgent_PreflightCheck_NilValidationLoop(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") } @@ -3677,7 +3677,7 @@ func TestRunAgent_PreflightCheck_Timeout(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(io.Discard) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "timed out") } @@ -4427,7 +4427,7 @@ func TestRunAgent_ErrorOnMissingRole(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "invalid harness: role field is required") @@ -5072,7 +5072,7 @@ func TestRunAgent_FallsBackToFULLSEND_MINT_URL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "openshell") @@ -5115,7 +5115,7 @@ func TestRunAgent_WarnsWhenNoMintURL(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, buf.String(), "skipping token minting") @@ -5157,7 +5157,7 @@ func TestRunAgent_MintTokenError(t *testing.T) { rFlags := resolveFlags{maxDepth: 10, maxResources: 50} printer := ui.New(&buf) repoDir := t.TempDir() - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, statusOpts{}, printer, false, runOverrideFlags{}) require.Error(t, err) assert.Contains(t, err.Error(), "agent token minting failed") From 63ae43a51add95f25ac47ca3beee46612b6017a5 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Mon, 24 Aug 2026 14:51:03 -0400 Subject: [PATCH 17/17] fix(cli): add missing runOverrideFlags arg to runAgent call Add missing runOverrideFlags{} argument to the status notifier test case that was missed by the earlier fix. Assisted-by: Claude Sonnet 4.5 Signed-off-by: Ralph Bean --- internal/cli/run_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 45f6e039b6..f7843e6274 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -5208,7 +5208,7 @@ func TestRunAgent_StatusNotifierSetup(t *testing.T) { statusNum: 42, mintURL: "https://mint.example.com", } - err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, sOpts, printer, false) + err := runAgent(context.Background(), "code", dir, "", repoDir, "", nil, false, "", "", "", rFlags, sOpts, printer, false, runOverrideFlags{}) // Will error downstream (openshell not available), but status notifier setup should succeed require.Error(t, err)