feat(harness): implement CEL-guarded overlays (ADR 0088) by ralphbean · Pull Request #6285 · fullsend-ai/fullsend

ralphbean · 2026-08-17T11:56:23Z

Summary

Implements ADR 0088 (CEL-guarded overlays in the harness schema).

Adds overlays: list field to Harness — each entry has a when: CEL expression and the same override fields as ForgeConfig
First-match-wins resolution: ResolveOverlays evaluates when expressions and merges the first matching entry using mergeForgeConfig semantics; remaining entries are skipped
CEL environment exposes event (normevent map), runtime.forge (effective forge platform), and config (per-repo config from config.yaml)
validateOverlays() compiles when expressions against the expanded overlay CEL environment and validates override fields; rejects forge: + overlays: coexistence
LoadWithOpts/LoadWithBase gain Config map[string]any and wire ForgePlatform + Config through to overlay resolution
mergeBaseIntoChild concatenates overlay lists (base first, child appended)
Lint() emits a deprecation warning when forge: is present
User docs (bring-your-own-agent.md) updated with first-match-wins semantics, runtime.forge examples, and cross-concern combined-entry pattern

Stacked on #6237 (ADR 0088)

Test plan

Unit tests for validateOverlays (empty when, non-bool CEL, valid CEL, runtime.forge CEL, config variable CEL, URL script, mutual exclusion)
Unit tests for ResolveOverlays — first-match-wins (first match applied, later matches skipped), runtime.forge conditioning, config variable, combined when expressions, no match, nil event
End-to-end tests for LoadWithOpts and LoadWithBase with overlay + event data, runtime.forge, and config
Base composition tests (both/only-base/only-child have overlays; first-match-wins with concatenated list)
Lint tests (forge deprecation warning present/absent)
All existing tests pass (internal/harness, internal/cli, internal/harnessdispatch)

🤖 Generated with Claude Code

fullsend-ai-review · 2026-08-17T11:57:25Z

🤖 Finished Review · ✅ Success · Started 11:57 AM UTC · Completed 12:13 PM UTC

Commit: 9457ce0 · View workflow run →

github-actions · 2026-08-17T11:58:16Z

Site preview

Preview: https://76520e74-site.fullsend-ai.workers.dev

Commit: 63ae43a51add95f25ac47ca3beee46612b6017a5

fullsend-ai-review · 2026-08-17T12:13:44Z

Review

Findings

Medium

[import-convention] internal/harness/forge.go:4 — The diff introduces log as a new import to internal/harness. No other file in the package uses log — the established convention is to return errors via the error return value. ResolveOverlays uses log.Printf for CEL evaluation errors, breaking the package's error-handling idiom. The comment explains this mirrors harnessdispatch/enumerate.go's MatchHarnesses pattern, but that is a different package with different conventions.
Remediation: Return the CEL evaluation error via the error return value, or collect evaluation errors into a structured list similar to Lint() diagnostics.
[missing-doc] docs/guides/dev/cli-internals.md:94 — The fullsend run flag tree does not include the new --event-file flag added by this PR.
Remediation: Add --event-file <path> entry to the run command flag tree after --forge.
[stale-doc] docs/guides/dev/cli-internals.md:368 — The sandbox lifecycle diagram shows ResolveForge(--forge / env) → Validate but the pipeline now includes ResolveOverlays after ResolveForge.
Remediation: Update to: ResolveForge → ResolveOverlays → Validate.
[missing-doc] docs/guides/user/running-agents-locally.md:212 — The flag table for fullsend run does not include the new --event-file flag.
Remediation: Add an --event-file row to the flag table.
[missing-doc] docs/cli/run.md:30 — The Flags table for fullsend run does not include the new --event-file flag.
Remediation: Add --event-file to the flag table.

Low

[logic-error] internal/harness/compose.go:161 — hadForgeBeforeResolve is set in LoadWithOpts but not in the LoadWithBase path. LoadWithBase calls ResolveForge in two places but neither sets hadForgeBeforeResolve = child.Forge != nil beforehand. The forge deprecation lint warning is silently skipped for harnesses loaded via LoadWithBase (the primary path for fullsend run and fullsend lock).
[error-handling] internal/cli/run.go:395 — The third error path in event file loading drops filename context. The first two paths include the filename (reading event file %s and parsing event file %s) but the third says only converting event to map: %w without the filename.
Remediation: Change to: fmt.Errorf("converting event file %s to map: %w", eventFile, mapErr).
[fail-open] internal/harness/forge.go:279 — ResolveOverlays treats CEL evaluation errors as non-matching (log and continue). If a security-critical overlay has a runtime CEL error (data-dependent, not caught by validation), it is silently skipped. This matches the MatchHarnesses pattern and is intentional, but runtime data-dependent errors could cause security-relevant overlays to be dropped in favor of a broader fallback.
[privilege-escalation] internal/cli/run.go:358 — The new detectForgePlatform precedence (flag > config.forge > CI env vars) allows per-repo config.yaml to override CI-detected forge platform. Since config.yaml is in the repository, a contributor with write access could set config.forge to influence which overlays match. The --forge flag still takes highest precedence, and the actor who controls config.yaml also controls the harness itself, limiting the practical risk.
[data-exposure] internal/harness/forge.go:420 — BuildConfigMap exposes per-repo config fields to CEL overlay when expressions. Sensitive fields (mint_url, inference provider details) are excluded. Exposed fields include allowed_remote_resources, agent entries, and issue creation config — operational metadata, not credentials.
[intent-coherence] internal/harnessdispatch/enumerate.go:49 — ListTriggeredHarnesses passes Config to ComposeOpts but does not pass Event. Overlays conditioned on event fields will fail CEL evaluation and be skipped during dispatch. This is intentional — at enumeration time, the specific event is not yet known, and trigger is a top-level field that overlays cannot modify.
[function-signature-growth] internal/cli/run.go:318 — runAgent now has 16 positional parameters after adding eventFile. The codebase already uses option structs (resolveFlags, statusOpts, runOverrideFlags) for grouped parameters.
[stale-doc] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars when omitted" but the PR adds config.forge as a middle precedence level (flag > config.forge > CI env vars).
Remediation: Update to include config.forge precedence.
[stale-doc] docs/cli/run.md:30 — Same stale --forge precedence description.
Remediation: Update to include config.forge precedence.
[stale-doc] docs/guides/dev/cli-internals.md:94 — Same stale --forge precedence description in the dev-facing CLI reference.
Remediation: Update to include config.forge precedence.

Previous run

Review

Findings

Medium

[import-convention] internal/harness/forge.go:5 — The diff introduces log as a new import to internal/harness. No other file in the package uses log — the established convention is to return errors via the error return value. ResolveOverlays uses log.Printf for CEL evaluation errors, breaking the package's error-handling idiom. The code comment acknowledges this as an "intentional exception" referencing the MatchHarnesses pattern in harnessdispatch/enumerate.go, but that is a different package with different conventions.
Remediation: Return CEL eval errors to the caller as a structured diagnostic (like Lint() returns []Diagnostic), or accept an io.Writer/logger parameter so the caller decides how to surface them.
[missing-cli-flag] docs/guides/dev/cli-internals.md:94 — The fullsend run flag tree does not include the new --event-file flag added by this PR.
Remediation: Add --event-file <path> entry to the run command flag tree after --forge.
[stale-pipeline-description] docs/guides/dev/cli-internals.md:368 — The sandbox lifecycle diagram shows ResolveForge(--forge / env) → Validate but the pipeline now includes ResolveOverlays after ResolveForge.
Remediation: Update to: ResolveForge → ResolveOverlays → Validate.
[missing-cli-flag] docs/guides/user/running-agents-locally.md:212 — The flag table for fullsend run does not include the new --event-file flag.
Remediation: Add an --event-file row to the flag table.
[missing-cli-flag] docs/cli/run.md:30 — The Flags table for fullsend run does not include the new --event-file flag.
Remediation: Add --event-file to the flag table.

Low

[intent-coherence] internal/harnessdispatch/enumerate.go:45 — ListTriggeredHarnesses passes Config to ComposeOpts but does not pass Event. Overlays conditioned on event fields will fail CEL evaluation and be skipped during dispatch. This appears intentional (the event is available later in MatchHarnesses and ResolveOverlays substitutes an empty map for nil event), but event-conditioned overlays never match at dispatch time.
Remediation: Confirm this is intentional and document the expectation.
[logic-error] internal/harness/compose.go:161 — hadForgeBeforeResolve is set in LoadWithOpts but not in the LoadWithBase path. LoadWithBase calls ResolveForge in two places (the no-base path and the with-base path) but neither sets hadForgeBeforeResolve = child.Forge != nil beforehand. This means the forge deprecation lint warning is silently skipped for harnesses loaded via LoadWithBase (the primary path for fullsend run and fullsend lock).
Remediation: Add child.hadForgeBeforeResolve = child.Forge != nil before each child.ResolveForge(opts.ForgePlatform) call in LoadWithBase.
[error-handling] internal/cli/run.go:395 — The third error path in event file loading drops filename context. The first two paths include the filename (reading event file %s and parsing event file %s) but the third says only converting event to map: %w without the filename.
Remediation: Change to: fmt.Errorf("converting event file %s to map: %w", eventFile, mapErr).
[edge-case] internal/harness/trigger.go:89 — EvaluateOverlay guards nil config (converts to empty map) but does not guard nil event. ResolveOverlays converts nil event before calling, so production code is safe, but the API asymmetry could trip future callers.
Remediation: Add if event == nil { event = map[string]any{} } for consistency with the nil config guard.
[fail-open] internal/harness/forge.go:279 — ResolveOverlays treats CEL evaluation errors as non-matching (log and continue). If a security-critical overlay has a runtime CEL error (data-dependent, not caught by validation), it is silently skipped. This matches the MatchHarnesses pattern and is intentional, but runtime data-dependent errors could cause security-relevant overlays to be dropped in favor of a broader fallback.
[privilege-escalation] internal/cli/run.go:358 — The new detectForgePlatform precedence (flag > config.forge > CI env vars) allows per-repo config.yaml to override CI-detected forge platform. Since config.yaml is in the repository, a contributor with write access could set config.forge to influence which overlays match. The --forge flag still takes highest precedence, and the actor who controls config.yaml also controls the harness itself, limiting the practical risk.
[data-exposure] internal/harness/forge.go:420 — BuildConfigMap exposes per-repo config fields to CEL overlay when expressions. Sensitive fields (mint_url, inference provider details) are excluded. Exposed fields include allowed_remote_resources, agent entries, and issue creation config — operational metadata, not credentials. See also: [scope-creep] finding at this location.
[scope-creep] internal/harness/forge.go:420 — BuildConfigMap exposes a broad surface of per-repo config fields beyond the original 4-key whitelist. ADR 0088 says "full per-repo config" and sensitive fields are excluded, but the breadth invites CEL expressions that couple to config structure. See also: [data-exposure] finding at this location.
[function-signature-growth] internal/cli/run.go:318 — runAgent now has 16 positional parameters after adding eventFile. The codebase already uses option structs (resolveFlags, statusOpts, runOverrideFlags) for grouped parameters. Every test call site (20+ occurrences) passes an extra empty string.
[stale-precedence-description] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars when omitted" but the PR adds config.forge as a middle precedence level (flag > config.forge > CI env vars).
Remediation: Update to include config.forge precedence.
[stale-precedence-description] docs/cli/run.md:30 — Same stale --forge precedence description.
Remediation: Update to include config.forge precedence.
[stale-precedence-description] docs/guides/dev/cli-internals.md:94 — Same stale --forge precedence description in the dev-facing CLI reference.
Remediation: Update to include config.forge precedence.
[stale-pipeline-description] docs/contributing/harness-fields.md:115 — The "Current resolution pipeline" section still shows the forge-only pipeline (Unmarshal → validateForge → ResolveForge(platform) → Validate) without validateOverlays or ResolveOverlays. The correct pipeline is shown later in the same file under "Overlay resolution", creating an internal inconsistency.
Remediation: Update the "Current resolution pipeline" to include overlay steps, or merge the two pipeline sections.

Previous run (2)

Review

Findings

Medium

[import-convention] internal/harness/forge.go:5 — The diff introduces log as a new import to internal/harness. No other file in the package uses log — the established convention is to return errors via the error return value. ResolveOverlays uses log.Printf for CEL evaluation errors, breaking the package's error-handling idiom. The code comment acknowledges this as an "intentional exception" referencing the MatchHarnesses pattern in harnessdispatch/enumerate.go, but that is a different package with different conventions.
Remediation: Return CEL eval errors to the caller as a structured diagnostic (like Lint() returns []Diagnostic), or accept an io.Writer/logger parameter so the caller decides how to surface them.
[missing-cli-flag] docs/guides/dev/cli-internals.md:94 — The fullsend run flag tree does not include the new --event-file flag added by this PR.
Remediation: Add --event-file <path> entry to the run command flag tree after --forge.
[stale-pipeline-description] docs/guides/dev/cli-internals.md:368 — The sandbox lifecycle diagram shows ResolveForge(--forge / env) → Validate but the pipeline now includes ResolveOverlays after ResolveForge.
Remediation: Update to: ResolveForge → ResolveOverlays → Validate.
[missing-cli-flag] docs/guides/user/running-agents-locally.md:210 — The flag table for fullsend run does not include the new --event-file flag.
Remediation: Add an --event-file row to the flag table.
[missing-cli-flag] docs/cli/run.md:30 — The Flags table for fullsend run does not include the new --event-file flag.
Remediation: Add --event-file to the flag table.

Low

[logic-error] internal/harness/compose.go:154 — hadForgeBeforeResolve is set in LoadWithOpts but not in the LoadWithBase path. LoadWithBase calls ResolveForge in two places (the no-base path and the with-base path) but neither sets hadForgeBeforeResolve = child.Forge != nil beforehand. This means the forge deprecation lint warning is silently skipped for harnesses loaded via LoadWithBase (the primary path for fullsend run and fullsend lock).
Remediation: Add child.hadForgeBeforeResolve = child.Forge != nil before each child.ResolveForge(opts.ForgePlatform) call in LoadWithBase.
[error-handling] internal/cli/run.go:395 — The third error path in event file loading drops filename context. The first two paths include the filename (reading event file %s and parsing event file %s) but the third says only converting event to map: %w without the filename.
Remediation: Change to: fmt.Errorf("converting event file %s to map: %w", eventFile, mapErr).
[edge-case] internal/harness/trigger.go:100 — EvaluateOverlay guards nil config (converts to empty map) but does not guard nil event. ResolveOverlays converts nil event before calling, so production code is safe, but the API asymmetry could trip future callers.
Remediation: Add if event == nil { event = map[string]any{} } for consistency with the nil config guard.
[fail-open] internal/harness/forge.go:189 — ResolveOverlays treats CEL evaluation errors as non-matching (log and continue). If a security-critical overlay has a runtime CEL error (data-dependent, not caught by validation), it is silently skipped. This matches the MatchHarnesses pattern and is intentional, but runtime data-dependent errors could cause security-relevant overlays to be dropped in favor of a broader fallback.
[privilege-escalation] internal/cli/run.go:353 — The new detectForgePlatform precedence (flag > config.forge > CI env vars) allows per-repo config.yaml to override CI-detected forge platform. Since config.yaml is in the repository, a contributor with write access could set config.forge to influence which overlays match. The --forge flag still takes highest precedence, and the actor who controls config.yaml also controls the harness itself, limiting the practical risk.
[function-signature-growth] internal/cli/run.go:315 — runAgent now has 15+ positional parameters after adding eventFile. The codebase already uses option structs (resolveFlags, statusOpts, runOverrideFlags) for grouped parameters. Every test call site (20+ occurrences) passes an extra empty string.
[stale-precedence-description] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars when omitted" but the PR adds config.forge as a middle precedence level (flag > config.forge > CI env vars).
Remediation: Update to include config.forge precedence.
[stale-precedence-description] docs/cli/run.md:30 — Same stale --forge precedence description.
Remediation: Update to include config.forge precedence.
[stale-precedence-description] docs/guides/dev/cli-internals.md:94 — Same stale --forge precedence description in the dev-facing CLI reference.
Remediation: Update to include config.forge precedence.

Previous run (3)

Review

Findings

Medium

[logic-error] internal/harness/compose.go:161 — hadForgeBeforeResolve is set in LoadWithOpts but not in the LoadWithBase path. LoadWithBase calls ResolveForge in two places (the no-base path and the with-base path) but neither sets hadForgeBeforeResolve = child.Forge != nil beforehand. After ResolveForge nils out h.Forge, Lint() checks hadForgeBeforeResolve to emit the forge deprecation warning. Since the field defaults to false, harnesses loaded via LoadWithBase that use the deprecated forge: field will silently skip the deprecation warning.
Remediation: Add child.hadForgeBeforeResolve = child.Forge != nil before each child.ResolveForge(opts.ForgePlatform) call in LoadWithBase.
[import-convention] internal/harness/forge.go:5 — The diff introduces the log package as a new import. No other file in internal/harness/ uses log — the established convention is to return errors rather than log-and-continue. ResolveOverlays uses log.Printf for CEL evaluation errors, which breaks the package's error-handling idiom and sends messages to stderr via the default logger rather than the UI/printer pattern used elsewhere.
Remediation: Either return errors wrapped with context (letting the caller decide whether to continue), or accept a logger/printer parameter. Consider slog instead of bare log.
[missing-cli-flag] docs/guides/dev/cli-internals.md:94 — The fullsend run flag tree does not include the new --event-file flag added by this PR.
Remediation: Add --event-file <path> entry to the run command flag tree after --forge.
[stale-pipeline-description] docs/guides/dev/cli-internals.md:367 — The sandbox lifecycle diagram shows ResolveForge(--forge / env) → Validate but the pipeline now includes ResolveOverlays after ResolveForge.
Remediation: Update to: ResolveForge → ResolveOverlays → Validate.
[missing-cli-flag] docs/guides/user/running-agents-locally.md:210 — The flag table for fullsend run does not include the new --event-file flag. Users running agents locally with overlay-based harnesses need this flag to pass event context for CEL overlay resolution.
Remediation: Add an --event-file row to the flag table.

Low

[edge-case] internal/cli/lock.go:683 — lockForgePlatforms only inspects h.Forge to discover platforms for iteration. For an overlays-only harness, ForgePlatform is empty during lock. Resources are still fetched for all overlay entries (so lock captures all dependencies), but if future changes make lock dependent on which overlay matched, this would break.
[data-exposure] internal/harness/forge.go — BuildConfigMap exposes allowed_remote_resources and other config fields to CEL overlay expressions. While sensitive fields (mint_url, inference) are excluded and CEL expressions can only return booleans (limiting exfiltration), the allowlist configuration reveals the org's security boundary for remote resource fetching.
Remediation: Consider restricting BuildConfigMap to only fields explicitly documented for CEL use (forge, tracker, runtime, roles).
[authorization-bypass] internal/harness/compose.go — Overlay base-first concatenation with first-match-wins means a child harness author expecting their overlay to override a base overlay with the same when condition will silently have their overlay ignored. This is documented as intentional (trusted-base model), but a lint warning for duplicate when expressions would prevent silent misconfiguration.
Remediation: Consider adding a lint diagnostic when a child overlay has the same when expression as a base overlay.
[fail-open] internal/harness/forge.go — ResolveOverlays treats CEL evaluation errors as non-matching (log and continue). If a security-critical overlay has a CEL error, it is silently skipped with only a log.Printf message. This is intentional (matching MatchHarnesses pattern) and CEL validation catches malformed expressions at load time, but runtime data-dependent errors could cause security-relevant overlays to be dropped.
[adr-drift] docs/ADRs/0088-cel-guarded-overlays.md:129 — The PR removes two original ADR lines about nil-event no-op semantics and adds an update note. While the update note is appropriate, removing original ADR text (rather than using strikethrough with an adjacent addendum) diverges from typical ADR immutability practices.
[function-signature-growth] internal/cli/run.go:284 — runAgent now has 15 positional parameters after this PR adds eventFile. The codebase already uses option structs (resolveFlags, statusOpts) for grouped parameters — the string flags are the odd group out. This pattern predates the PR; not blocking but worth consolidating in a follow-up.
[comment-convention] internal/harness/forge.go — ResolveOverlays has a ~22-line doc comment plus a ~10-line inline comment repeating the same CEL-error-as-non-matching rationale. The design rationale should appear once, not twice.
Remediation: Consolidate the CEL error explanation into either the doc comment or the inline comment.
[stale-precedence-description] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars when omitted" but the PR adds config.forge as a middle precedence level (flag > config.forge > CI env vars). The description is now incomplete.
Remediation: Update to include config.forge precedence.

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (4)

Review

Findings

High

[stale-doc] docs/contributing/harness-fields.md:84 — The merge and inheritance rules table still marks overlays as (planned) with "not yet implemented". This PR implements overlays (validation, CEL evaluation, resolution, tests), so the label directly contradicts the code.
Remediation: Remove the *(planned)* tag and "not yet implemented" qualifier from the overlays row.

Medium

[missing-state-capture] internal/harness/compose.go:161 — LoadWithBase does not set hadForgeBeforeResolve before calling ResolveForge, unlike LoadWithOpts (harness.go line ~388). Both the no-base path (line ~164) and the with-base path (line ~268) call ResolveForge which nils out h.Forge, but neither captures whether forge was present beforehand. This means Lint() will never emit the forge deprecation warning for harnesses loaded via LoadWithBase (the compose path used by fullsend lock and harnessdispatch).
Remediation: Add child.hadForgeBeforeResolve = child.Forge != nil before each child.ResolveForge(opts.ForgePlatform) call in LoadWithBase.
[missing-event-data] internal/harnessdispatch/enumerate.go:45 — ListTriggeredHarnesses passes Config to ComposeOpts but not Event or ForgePlatform. Overlays conditioned on runtime.forge or event fields will not match during dispatch enumeration. The actual run re-resolves with full context, but trigger expressions depending on overlay-merged fields could cause incorrect filtering.
Remediation: Thread the event and detected forge platform through to ComposeOpts in ListTriggeredHarnesses.
[logging-convention] internal/harness/forge.go:282 — ResolveOverlays uses log.Printf for CEL evaluation errors, introducing the stdlib log package into the harness package for the first time. The rest of the package returns errors to callers or uses the Diagnostic type from lint.go. Additionally, ResolveOverlays always returns nil — there is no code path that returns a non-nil error, making the error return value dead code. The design is intentional and well-tested, but it introduces an inconsistent error-reporting pattern.
Remediation: Consider returning CEL eval errors to the caller or collecting failures into a []Diagnostic, consistent with the package's existing patterns.
[stale-doc] docs/contributing/harness-fields.md:110 — The "Current resolution pipeline" section still shows the old pipeline (Unmarshal → validateForge → ResolveForge(platform) → Validate) without validateOverlays or ResolveOverlays. The correct pipeline is shown in the "Overlay resolution" section immediately below, creating a contradiction within the same document.
Remediation: Update the "Current resolution pipeline" to include overlay steps, or merge the two pipeline sections.
[scope-alignment] internal/harness/forge.go:404 — BuildConfigMap doc comment contains "Per PR feat(harness): implement CEL-guarded overlays (ADR 0088) #6285 review feedback, the 4-key whitelist was expanded" — a self-referential PR citation in production code that will be meaningless to future readers.
Remediation: Remove the self-referential sentence. The behavior is documented by ADR 0088 ("full per-repo config").

Low

[missing-validation] internal/cli/run.go:3411 — detectForgePlatform does not validate the config.forge value against ValidForgePlatform(). The flag path validates, but the config.forge path returns the raw string without checking. Impact is limited: config.yaml is author-controlled and ResolveForge validates downstream.
[doc-inconsistency] docs/guides/user/bring-your-own-agent.md:278 — The field reference examples use bare event.source.system == "jira" without a has() guard, while the same document at line 191 explicitly advises using has(event.source) to guard event field access. The examples contradict the guidance.
[stale-doc] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars" but now config.forge is also consulted (precedence: flag > config.forge > CI env). This staleness also affects the status notification flags table and the cli-internals.md flag comment.
[missing-doc] docs/guides/user/running-agents-locally.md:212 — The flag table for fullsend run does not include the new --event-file flag. Similarly absent from docs/guides/dev/cli-internals.md.
[stale-doc] docs/guides/dev/cli-internals.md:367 — The sandbox lifecycle diagram shows ResolveForge → Validate without ResolveOverlays between them.
[inconsistent-call-site] internal/cli/reconcilestatus.go:63 — detectForgePlatform is called with (forgeFlag, nil), meaning reconcile-status never consults config.forge. No comment explains this divergence from the run and lock call sites.

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (5)

Review

Findings

High

[stale-doc] docs/contributing/harness-fields.md:84,118 — The overlays row in the merge rules table says *(planned)* and not yet implemented (line 84), and the "Overlay resolution" section header says (planned — ADR-0088) with a note "The overlay feature has not been implemented yet" (lines 118–125). This PR implements overlay support, making these annotations actively misleading — readers will believe overlays are unavailable when they are functional.
Remediation: Remove *(planned)* and not yet implemented from the overlays row; remove (planned — ADR-0088) from the section heading; remove the not-yet-implemented note block; change future-tense verbs to present tense.

Medium

[missing-validation] internal/cli/run.go — detectForgePlatform validates --forge flag values against ValidForgePlatform but does not validate config.forge values from config.yaml. An invalid value (e.g., forge: bitbucket) would be silently accepted and propagated, bypassing the validation that the --forge flag path enforces.
Remediation: Add a ValidForgePlatform check for the config.forge value, consistent with the flag validation path.
[stale-doc] docs/guides/dev/cli-internals.md:367 — The Sandbox Lifecycle diagram shows ResolveForge(--forge / env) → Validate but this PR adds ResolveOverlays to the pipeline between ResolveForge and Validate.
Remediation: Update the diagram to show ResolveForge → ResolveOverlays → Validate.
[stale-doc] docs/guides/user/running-agents-locally.md:212 — The --forge flag description says "Auto-detected from CI env vars (GITHUB_ACTIONS, GITLAB_CI) when omitted" but this PR changes detectForgePlatform to also check config.forge from config.yaml between the CLI flag and CI env vars.
Remediation: Update the description to reflect the new precedence: CLI flag > config.forge > CI env vars.
[missing-doc] docs/guides/user/running-agents-locally.md — The PR adds a new --event-file flag to fullsend run (for CEL overlay resolution, ADR 0088), but it is not documented in the flag table where all other fullsend run flags are listed.
Remediation: Add --event-file to the flag table.
[behavioral-contract] internal/cli/run.go — detectForgePlatform precedence changed from flag > CI env vars to flag > config.forge > CI env vars. A repo with forge: gitlab in config.yaml running on GitHub Actions will now detect gitlab instead of github unless --forge github is explicitly passed. This is intentional per ADR 0088 and documented for reviewer awareness.

Low

[misleading-comment] internal/cli/lock.go:976 — Comment for overlay-scoped skills says "merged into h.Skills by ResolveForge" but overlay-scoped skills are merged by ResolveOverlays, not ResolveForge.
[naming-consistency] internal/harness/forge.go — Parameter name config in ResolveOverlays and EvaluateOverlay shadows the imported config package. If these functions later need to reference config.ConfigReader, they will fail to compile.
[missing-test] internal/harness/compose_test.go — No test for when a base harness uses forge: and a child uses overlays: (or vice versa). After mergeBaseIntoChild, both would be present and validateOverlays should reject the combination.
[logging-convention] internal/harness/forge.go — log.Printf in ResolveOverlays adds the log standard library import to forge.go. While this matches the harnessdispatch/enumerate.go pattern for CEL skip-and-continue, no other file in internal/harness/ uses the log package.
[function-signature-growth] internal/cli/run.go — runAgent now has 15 positional parameters after adding eventFile string. The codebase already groups related parameters into structs (resolveFlags, statusOpts).

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (6)

Review

Findings

High

[stale-doc] docs/contributing/harness-fields.md:84 — harness-fields.md marks overlays as "(planned)" and "not yet implemented" in multiple places (lines 84, 110–116, 118–123). This PR implements overlays, making this language factually incorrect. The "Current resolution pipeline" section is stale (missing validateOverlays and ResolveOverlays), and the "Planned resolution pipeline" is now the actual pipeline.
Remediation: Remove "(planned)" annotations, update the resolution pipeline diagram, and change future-tense language to present-tense throughout the overlay sections.
[missing-doc] docs/guides/dev/cli-internals.md:89 — The CLI command tree for fullsend run does not list the new --event-file flag added by this PR. This file is the authoritative CLI reference.
Remediation: Add --event-file <path> to the run command block after --forge.

Medium

[missing-doc] docs/guides/user/running-agents-locally.md:210 — The flag table for fullsend run documents --forge but not the new --event-file flag.
Remediation: Add a row for --event-file to the flag table.
[logging-convention] internal/harness/forge.go:259 — ResolveOverlays uses log.Printf for CEL evaluation errors, introducing stdlib logging to the harness package which previously had none. All other error paths in this package return errors or emit Diagnostic values via Lint().
Remediation: Consider returning CEL evaluation errors as a structured signal rather than logging directly, consistent with the rest of the harness package.
[error-handling-gap] internal/harness/forge.go — CEL eval errors in ResolveOverlays are logged and treated as non-matching (continue to next entry). A typo causing a runtime eval error (as opposed to a compile-time error caught by validateOverlays) would silently skip the overlay, potentially applying the wrong one. This is consistent with the MatchHarnesses pattern in harnessdispatch/enumerate.go but could surprise users in the first-match-wins context.

Low

[missing-doc] docs/guides/user/cel-triggers-reference.md:40 — Documents the trigger CEL environment (event only) without mentioning the overlay CEL environment (event + runtime.forge + config). A cross-reference would help discoverability.
[function-signature-style] internal/cli/run.go:277 — runAgent now has 15 positional parameters including two adjacent string parameters (forgeFlag, eventFile). Pre-existing design issue, but this PR adds to it.
[naming-convention] internal/harness/forge.go:140 — validateOverlayForgeConfig names a function that validates the ForgeConfig embedded within an OverlayEntry. The name is semantically accurate but references ForgeConfig in new code where OverlayEntry is the primary type.
[error-message-format] internal/harness/forge.go:201 — Validation error "forge and overlays cannot coexist...; migrate forge entries to overlays" includes remediation advice, unlike other validation errors in the package which only state what is wrong.
[performance] internal/harness/trigger.go — EvaluateOverlay creates a new CEL environment, compiles, and programs on every call. Consistent with the existing EvaluateTrigger pattern.
[scope-creep] internal/harness/forge.go — BuildConfigMap is a new exported function not specified in ADR 0088. Justified by deduplication across CLI and harnessdispatch call sites.

Info

[provenance-warning] Prior review context discarded: provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments.

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (7)

Review

Findings

High

[stale-planned-marker] docs/contributing/harness-fields.md:84 — The harness-fields.md living reference document still contains "planned" / "not yet implemented" markers for overlays in multiple places. Line 84: the merge rules table has overlays *(planned)* with "not yet implemented". Lines 109–113: the "Current resolution pipeline" still shows the old pipeline without validateOverlays/ResolveOverlays. Lines 118–128: the overlay resolution section heading says "Overlay resolution (planned — ADR-0088)" with a note stating "The overlay feature has not been implemented yet." ADR 0088 designates this file as the living reference to update when overlay behavior evolves — this PR implements overlays but leaves the reference document saying they are planned.
Remediation: Remove "(planned)" and "not yet implemented" annotations. Update the overlay resolution section to reflect implemented status. Update the resolution pipeline to include validateOverlays and ResolveOverlays.

Medium

[code-duplication] internal/cli/run.go:4217 — configMapForOverlays (run.go) and buildConfigMap (harnessdispatch/enumerate.go) are near-identical functions extracting per-repo config fields for overlay CEL evaluation. They accept different interface types (ConfigWriter vs ConfigReader) but perform identical logic. If one is updated without the other, overlay CEL resolution will see different config shapes depending on the call path. Additionally, configMapForOverlays accepts ConfigWriter but only reads — should accept ConfigReader per codebase convention.
Remediation: Extract a shared function accepting config.ConfigReader into a common package.
[missing-new-flag] docs/guides/dev/cli-internals.md:89 — The fullsend run command flag tree does not include the new --event-file flag added by this PR.
Remediation: Add --event-file to the run command flag tree.
[missing-new-flag] docs/guides/user/running-agents-locally.md:210 — The "Remote resource flags" table for fullsend run does not include the new --event-file flag.
Remediation: Add --event-file to the flag table.

Low

[missing-event-plumbing] internal/harnessdispatch/enumerate.go:45 — ListTriggeredHarnesses builds ComposeOpts with Config but without Event. Overlays conditioned on event.* will fail CEL evaluation and be skipped during dispatch. Currently safe because the dispatch path only reads h.Trigger, and ResolveOverlays explicitly handles nil event.
[logging-convention-deviation] internal/harness/forge.go:256 — ResolveOverlays introduces log.Printf into the internal/harness/ package, which currently uses error returns exclusively. This matches the MatchHarnesses pattern in harnessdispatch/ but is new for this package.
[CEL-env-construction] internal/harness/trigger.go:89 — EvaluateOverlay creates a new CEL environment and compiles the expression on every call. Consistent with the existing EvaluateTrigger pattern, but overlays iterate multiple entries making the repeated construction more relevant.
[function-signature-growth] internal/cli/run.go:280 — runAgent now has 15 positional parameters. eventFile is closely related to forgeFlag but is passed as a standalone string rather than being grouped into a struct.

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (8)

Review

Findings

Medium

[stale-doc] docs/guides/dev/cli-internals.md:359 — The harness loading pipeline description references ResolveForge → Validate but the actual pipeline is now ResolveForge → ResolveOverlays → Validate (confirmed in compose.go and harness.go changes). The file is not updated in this PR.
Remediation: Update the pipeline description to include the ResolveOverlays step.

Low

[code duplication] internal/cli/run.go / internal/harnessdispatch/enumerate.go — configMapForOverlays (run.go) and buildConfigMap (enumerate.go) are functionally identical, extracting per-repo config fields into map[string]any for CEL overlay evaluation. They differ only in parameter type (ConfigWriter vs ConfigReader, both type-asserting to PerRepoConfigReader).
[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event is wired in run.go (via --event-file) but not in enumerate.go (dispatch) or lock.go. Config is now wired in both paths. Consistent with iterative rollout — dispatch and lock callers will need Event wired before overlay-bearing harnesses are used in production.
[lock command does not iterate overlay platforms] internal/cli/lock.go — lockForgePlatforms discovers platforms via h.Forge keys. With overlays, h.Forge is nil, so lock runs once with an empty platform. Overlay-specific resources from URL bases are still pre-cached during resolveBase* functions. Config is now wired. Follow-up needed when overlay callers are wired.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit). Consistent with the existing EvaluateTrigger pattern. Expressions are defined by operators, not untrusted end-users, limiting practical risk.
[missing-doc] docs/guides/dev/cli-internals.md:89 — The CLI command tree for fullsend run omits the new --event-file flag.
[unreachable error return] internal/harness/forge.go — ResolveOverlays has return type error but can never return a non-nil error in the current implementation. All error paths use log.Printf + continue, and the function always returns nil. The error return type is harmless and provides forward compatibility.

Previous run (9)

Review

Findings

Medium

[CEL error propagation] internal/harness/forge.go:268 — ResolveOverlays returns the first CEL evaluation error immediately, aborting the loop. When a harness lists an event-conditioned overlay (e.g., event.source.system == "jira" && runtime.forge == "github") before a broader fallback (e.g., runtime.forge == "github"), and the harness is loaded without --event-file (event = empty map), the first overlay's expression errors on event.source (no such key), and the fallback overlay is never evaluated. This is inconsistent with MatchHarnesses in harnessdispatch/enumerate.go, which catches trigger eval errors and continues. The user guide (bring-your-own-agent.md) documents exactly this more-specific-first pattern.
Remediation: Catch CEL evaluation errors in the ResolveOverlays loop and treat them as non-matching (log and continue to next entry), matching the MatchHarnesses pattern.
[stale-doc] docs/guides/dev/cli-internals.md:359 — The harness loading pipeline description references ResolveForge → Validate but the actual pipeline is now ResolveForge → ResolveOverlays → Validate (confirmed in compose.go and harness.go changes). The file is not updated in this PR.
Remediation: Update the pipeline description to include the ResolveOverlays step.

Low

[code duplication] internal/cli/run.go / internal/harnessdispatch/enumerate.go — configMapForOverlays (run.go) and buildConfigMap (enumerate.go) are functionally identical, extracting per-repo config fields into map[string]any for CEL overlay evaluation. They differ only in parameter type (ConfigWriter vs ConfigReader, both type-asserting to PerRepoConfigReader).
[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event is wired in run.go (via --event-file) but not in enumerate.go (dispatch) or lock.go. Config is now wired in both paths. Consistent with iterative rollout — dispatch and lock callers will need Event wired before overlay-bearing harnesses are used in production.
[lock command does not iterate overlay platforms] internal/cli/lock.go:693 — lockForgePlatforms discovers platforms via h.Forge keys. With overlays, h.Forge is nil, so lock runs once with an empty platform. Overlay-specific resources from URL bases are still pre-cached during resolveBase* functions. Config is now wired. Follow-up needed when overlay callers are wired.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit). Consistent with the existing EvaluateTrigger pattern. Expressions are defined by operators, not untrusted end-users, limiting practical risk.
[missing-doc] docs/guides/dev/cli-internals.md:89 — The CLI command tree for fullsend run omits the new --event-file flag.

Next steps:

/fs-fix — agent addresses review findings automatically
/fs-fix <your instruction> — agent fixes with your specific guidance
Push commits directly — review re-runs automatically on push
/fs-fix-stop — disable automatic fix runs for this PR

Previous run (10)

Review

Findings

Medium

[function signature growth] internal/cli/run.go:275 — The runAgent function signature has grown to 15 positional parameters after adding the eventFile string parameter. The codebase already uses option structs for grouping related parameters (e.g. resolveFlags, statusOpts). The new eventFile parameter belongs in an options struct alongside forgeFlag to prevent further positional parameter sprawl. This is a pre-existing condition that the PR makes marginally worse by adding one more parameter.
Remediation: Group forgeFlag and eventFile (and future runtime-context flags) into an options struct.

Low

[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event, ComposeOpts.Config, LoadOpts.Event, and LoadOpts.Config are added but no production caller currently sets them. The --event-file flag provides CLI wiring, but CI callers haven't been updated. ResolveOverlays is a no-op when Event is nil, so any overlay-bearing harness deployed before callers are wired will have its overlays silently consumed without applying conditional config. Consistent with iterative rollout — should be wired before the feature is considered live.
[lock command does not iterate overlay platforms] internal/cli/lock.go:693 — lockForgePlatforms discovers forge platforms by inspecting h.Forge keys. When a harness uses overlays instead of forge, h.Forge is nil, so lock runs only once with an empty platform. Overlay-specific resources from URL bases are still pre-cached during resolveBase* functions, but the lock command has no overlay-aware iteration equivalent. Follow-up needed when overlay callers are wired.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit, cost tracking). This is consistent with the existing EvaluateTrigger pattern. The expressions are defined by harness authors (operators), not untrusted end-users, limiting practical risk. Consider adding cost limits as a hardening measure for both environments. See also: [defense-in-depth] finding at forge.go.
[defense-in-depth] internal/harness/forge.go:245 — ResolveOverlays silently drops all overlays when event is nil (no-op path). When a harness declares overlays and the operator does not supply --event-file, all conditional config is silently discarded. This is consistent with the existing ResolveForge pattern (which also no-ops when platform is empty) and is a deliberate design choice for incremental rollout. Harness authors who expect overlay config to always apply should be aware of this.
[error wrapping consistency] internal/cli/run.go:348 — Inconsistent error wrapping in event file loading: first two error paths include the eventFile filename (reading event file %s, parsing event file %s), but the third error path (converting event to map: %w) drops the filename context.
Remediation: Add event file path to third error: fmt.Errorf("converting event %s to map: %w", eventFile, mapErr).

Previous run (11)

Review

Findings

Low

[overlay-base composition precedence reversal] internal/harness/compose.go:676 — When overlays are concatenated during base composition, base entries are placed first. Because ResolveOverlays uses first-match-wins semantics, base overlay entries take precedence over child overlay entries with the same when condition. This is consistent with other concatenated list fields (plugins, providers, api_servers) and is documented in both code comments and the user guide, but it is an intentional exception to the child-overrides-base convention used by scalar and map merges. Consider adding a callout in the contributor guide's merge rules table noting that overlay precedence follows base-first ordering. See also: [resource-exhaustion] finding at trigger.go.
[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event, ComposeOpts.Config, LoadOpts.Event, and LoadOpts.Config are added but no production caller currently sets them. ResolveOverlays is a no-op when Event is nil, so any overlay-bearing harness deployed before callers are wired will have its overlays silently consumed without applying conditional config. Consistent with iterative rollout — should be wired before the feature is considered live.
[lock command does not iterate overlay platforms] internal/cli/lock.go:693 — lockForgePlatforms discovers forge platforms by inspecting h.Forge keys. When a harness uses overlays instead of forge, h.Forge is nil, so lock runs only once with an empty platform. Overlay-specific resources from URL bases are still pre-cached during resolveBase* functions, but the lock command has no overlay-aware iteration equivalent. Follow-up needed when overlay callers are wired.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit, cost tracking). This is consistent with the existing EvaluateTrigger pattern but the overlay environment exposes a wider surface with 3 input variables vs 1. Consider adding cost limits as a hardening measure for both environments.

Previous run (12)

Review

Findings

Medium

[misleading-comment] internal/harness/compose.go:676 — The comment on overlay concatenation in mergeBaseIntoChild states "Declaration order matters: later entries override earlier ones for scalars, so child entries naturally take precedence." This is incorrect: ResolveOverlays uses first-match-wins semantics (breaks on the first matching entry), and base entries are placed first in the concatenated list. Base entries that match will shadow child entries — the opposite of the comment's claim.
Remediation: Update the comment to accurately describe first-match-wins semantics with base-first ordering, e.g.: "Declaration order matters: the first matching entry wins, so base entries (placed first) take precedence over child entries with the same when condition." See also: [privilege-escalation] finding at this location.

Low

[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event and LoadOpts.Event are added but no production caller currently sets them. ResolveOverlays is a no-op when Event is nil, so any overlay-bearing harness deployed before callers are wired will have its overlays silently consumed without applying conditional config. Consistent with iterative rollout — should be wired before the feature is considered live.
[documentation-accuracy] docs/guides/user/bring-your-own-agent.md:188 — The updated prose lists fields that overlays can conditionally apply as "scripts, skills, providers, host_files, and env vars" but omits openshell (profiles). The YAML example below still shows openshell.profiles, and mergeForgeConfig (reused by ResolveOverlays) handles it. The omission could mislead users into thinking openshell is not supported in overlays.
Remediation: Add openshell to the list.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit, cost tracking). This is consistent with the existing EvaluateTrigger pattern but the overlay environment exposes a wider surface with 3 input variables vs 1. Consider adding cost limits as a hardening measure for both environments.
[privilege-escalation] internal/harness/compose.go — Base composition concatenates base overlays before child overlays. With first-match-wins semantics, base entries match before child entries, which diverges from the child-wins convention used for other fields. This is consistent with other concatenated list fields (plugins, providers, api_servers use the same base-first pattern) and the base author is trusted (base URLs require org-level allowlist). Consider documenting this precedence explicitly in the user guide. See also: [misleading-comment] finding at this location.
[stale-reference] docs/architecture.md:108 — The Agent Harness section describes forge: as current functionality without mentioning the ADR 0088 deprecation. A separate bullet later in the same section (lines 160-165) does note the deprecation, creating minor inconsistency within the document.
[stale-reference] docs/ADRs/0055-unified-env-var-delivery.md:80 — ADR 0055 references forge.<platform> blocks as current functionality without noting the deprecation in favor of overlay entries (ADR 0088).

Labels: PR modifies Go harness code and implements a new feature (CEL-guarded overlays)

Previous run (13)

Review

Findings

Medium

[misleading-comment] internal/harness/compose.go:672 — The comment on overlay concatenation in mergeBaseIntoChild states "Declaration order matters: later entries override earlier ones for scalars, so child entries naturally take precedence." This is incorrect: ResolveOverlays uses first-match-wins semantics (breaks on the first matching entry), and base entries are placed first in the concatenated list. Base entries that match will shadow child entries — the opposite of the comment's claim. The test TestLoadWithBase_OverlayConcatBothHaveOverlays confirms this: the base overlay wins and the child overlay is never reached. A developer relying on this comment to design a child harness that overrides a base overlay will get incorrect behavior.
Remediation: Update the comment to accurately describe first-match-wins semantics with base-first ordering, or reverse the concatenation order to match the child-wins convention used for other fields.

Low

[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event and LoadOpts.Event are added but no production caller currently sets them. ResolveOverlays is a no-op when Event is nil, so any overlay-bearing harness deployed before callers are wired will have its overlays silently consumed without applying conditional config. Consistent with iterative rollout — should be wired before the feature is considered live.
[resource-exhaustion] internal/harness/trigger.go — NewOverlayEnv() CEL environment is created without resource limits (cel.CostLimit, cost tracking). This is consistent with the existing EvaluateTrigger pattern but the overlay environment exposes a wider surface with 3 input variables vs 1. Consider adding cost limits as a hardening measure for both environments.
[privilege-escalation] internal/harness/compose.go — Base composition concatenates base overlays before child overlays. With first-match-wins semantics, base entries match before child entries, which diverges from the child-wins convention used for other fields. Consider documenting this as intentional or reversing the ordering to match the child-wins convention. See also: [misleading-comment] finding at this location.
[stale-reference] docs/architecture.md:108 — The Agent Harness section describes forge: as current functionality without mentioning the ADR 0088 deprecation. Since forge: remains functional (with a deprecation lint warning), this is not urgently misleading but should be updated.
[stale-reference] docs/ADRs/0055-unified-env-var-delivery.md:80 — ADR 0055 references forge.<platform> blocks as current functionality without noting the deprecation in favor of overlay entries (ADR 0088).

Labels: PR modifies Go harness code and implements a new feature

Previous run (14)

Review

Findings

Low

[no production caller wires Event] internal/harness/compose.go:79 — ComposeOpts.Event and LoadOpts.Event are added but no production caller currently sets them. ResolveOverlays is a no-op when Event is nil, so any overlay-bearing harness deployed before callers are wired will have its overlays silently consumed without applying conditional config. Consistent with the draft status and iterative rollout — should be wired before the feature is considered live.
[broken-reference] docs/ADRs/0045-forge-portable-harness-schema.md:23 — References to ADR 0088 (0088-cel-guarded-overlays.md) point to a file that does not exist on the current branch. Confirmed present on the stacked base branch (docs/tracker-forge-harness-config-split-adr) — this will resolve when the PR stack merges in order.
[api-shape-consistency] internal/harness/forge.go — validateOverlayForgeConfig is a standalone function rather than a method receiver. This is the correct design: it validates a single ForgeConfig value (not the Harness), paralleling the existing mergeForgeConfig standalone function. The doc comment explains the embedding relationship. No action needed.

fullsend-ai-coder · 2026-08-17T12:14:53Z

🤖 Finished Fix · ✅ Success · Started 12:14 PM UTC · Completed 12:25 PM UTC

Commit: 9457ce0 · View workflow run →

fullsend-ai-coder · 2026-08-17T12:25:17Z

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed 4 of 6 review findings with code and doc fixes. The high-severity finding (missing URL-base resolution for overlay entries) was the most significant — added overlay-iteration loops to all 5 resolveBase* functions. Disagreed with 2 low-severity findings: production caller wiring is acknowledged as acceptable follow-up work, and ADR 0088 reference resolves when the PR stack merges.

Fixed (4):

missing URL-base resolution for overlay entries (internal/harness/compose.go): Added overlay-iteration loops to resolveBaseScripts, resolveBaseResources, resolveBaseHostFiles, resolveBaseProfiles, and resolveBaseProviders, paralleling the existing forge-iteration loops. Each function now resolves relative paths in overlay ForgeConfig entries (pre_script, post_script, policy, validation_loop, skills, host_files, providers, openshell.profiles) when inherited from URL-sourced base harnesses. Also updated the early-exit checks in resolveBaseProfiles and resolveBaseProviders to account for overlay entries.
stale-doc ADR 0045 (docs/ADRs/0045-forge-portable-harness-schema.md): Added cross-reference annotation to ADR 0088 in the Status section and a reference entry. Status kept as Accepted (not Superseded) because ADR 0045 covers role, slug, base composition, and merge rules which remain current — only the forge section is deprecated by ADR 0088.
documentation-scope-alignment (docs/guides/user/bring-your-own-agent.md): Clarified overlay merge wording: changed 'Multiple overlays can match a single event (e.g. one for the source system, another for the event type)' to 'Multiple overlays can match a single event — all matching entries are merged in declaration order (later scalars win, lists accumulate)' to avoid implying overlays must condition on orthogonal dimensions.
naming-convention validateOverlayForgeConfig (internal/harness/forge.go): Enhanced the doc comment on validateOverlayForgeConfig to explain that OverlayEntry embeds ForgeConfig via yaml:",inline" and that the ForgeConfig name is a legacy artifact from the forge feature being deprecated in favor of overlays (ADR 0088).

Disagreed (2):

no production caller wires Event: The reviewer acknowledged this is acceptable for an infrastructure PR. Wiring Event from normalized events into ComposeOpts.Event in production callers (run.go, dispatch path) is a follow-up task, not in scope for this PR which establishes the overlay infrastructure.
broken-reference to ADR 0088: The reviewer confirmed that ADR 0088 is present on the stacked base branch (docs/tracker-forge-harness-config-split-adr) and the links will resolve when the PR stack merges in order. No fix needed.

Tests: passed

Next steps:

/fs-review — request a re-review of the changes
/fs-fix <your instruction> — run another fix pass with specific guidance
Push commits directly — review re-runs automatically on push
_{Updated by fullsend fix agent}

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

fullsend-ai-review · 2026-08-17T12:26:38Z

🤖 Finished Review · ✅ Success · Started 12:26 PM UTC · Completed 12:42 PM UTC

Commit: 4007729 · View workflow run →

Superseded by updated review

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

fullsend-ai-review · 2026-08-17T20:41:23Z

🤖 Review · ⚠️ Cancelled · Started 8:41 PM UTC · Ended 8:59 PM UTC

Commit: 6ffc734 · View workflow run →

codecov · 2026-08-17T20:45:11Z

Codecov Report

❌ Patch coverage is 69.67930% with 104 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	51.44%	44 Missing and 23 partials ⚠️
internal/harness/forge.go	84.78%	15 Missing and 6 partials ⚠️
internal/cli/lock.go	64.70%	6 Missing ⚠️
internal/harness/trigger.go	82.85%	3 Missing and 3 partials ⚠️
internal/harness/harness.go	42.85%	2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

fullsend-ai-review · 2026-08-17T21:00:18Z

🤖 Review · ⚠️ Cancelled · Started 9:00 PM UTC · Ended 9:02 PM UTC

Commit: 050f6fb · View workflow run →

qodo-code-review · 2026-08-17T21:03:01Z

PR Summary by Qodo

Implement CEL-guarded harness overlays (ADR 0088) with first-match resolution

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add CEL-guarded overlays blocks to conditionally override harness configuration.
• Resolve the first matching overlay using event, runtime.forge, and config CEL variables.
• Deprecate forge, extend base/URL resolution for overlay fields, and add tests/docs.

Diagram

graph TD
  A[/"Harness YAML"/] --> B["LoadWithOpts / LoadWithBase"] --> C["validateForge + validateOverlays"] --> D["ResolveForge"] --> E["ResolveOverlays (first match)"] --> F["Effective Harness"]
  G[("event + config")] --> E
  H{{"Overlay CEL env"}} --> E
  subgraph Legend
    direction LR
    _in[/Input/] ~~~ _step["Process"] ~~~ _data[(Data)] ~~~ _env{{Env}}
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Merge-all-matching overlays (declaration order)

➕ Supports layering multiple concerns without combined entries
➕ Closer to typical "all conditions apply" rule systems
➖ Harder to reason about outcomes; later overlays can unintentionally override earlier ones
➖ More expensive evaluation/merge; higher risk of subtle precedence bugs

2. Keep `forge` map and add CEL conditions per platform entry

➕ Minimizes schema churn; easier migration for existing harnesses
➕ Preserves platform-centric structure
➖ Doesn’t generalize beyond forge platform; mixes concerns (platform vs event/config)
➖ Complicates validation and resolution semantics across two conditional mechanisms

3. Per-field conditional overrides instead of overlay blocks

➕ Fine-grained control; avoids large overlay entries for small changes
➖ Significantly more schema/implementation complexity
➖ Harder to validate, document, and compose across base harness chains

Recommendation: The PR’s first-match-wins overlay blocks are the best fit for ADR 0088: deterministic, easy-to-explain precedence, and a clean migration path from forge while enabling conditions over event/runtime/config. The main tradeoff (needing combined entries for multi-concern cases) is acknowledged and documented; that explicitness is preferable to implicit multi-merge precedence.

Files changed (12) +1008 / -41

Enhancement (5) +444 / -7

compose.goWire overlay resolution into composition and base URL/resource handling +217/-5

Wire overlay resolution into composition and base URL/resource handling

• Extends 'ComposeOpts' with 'Event' and 'Config' and runs 'validateOverlays' + 'ResolveOverlays' alongside existing forge steps in 'LoadWithBase'. Concatenates overlays during base composition (base first, child appended) and expands base URL/path resolution to cover overlay-embedded script, policy, validation loop, skills, host_files, providers, and openshell profiles.

internal/harness/compose.go

forge.goIntroduce OverlayEntry, validate overlays, and resolve first matching overlay +147/-0

Introduce OverlayEntry, validate overlays, and resolve first matching overlay

• Adds 'OverlayEntry' (CEL 'when' + inline 'ForgeConfig') and implements 'validateOverlays' to compile CEL expressions, enforce boolean results, validate embedded fields, and reject 'forge'+'overlays' coexistence. Implements 'ResolveOverlays' to evaluate overlays against event/runtime/config and merge only the first match, then consume the overlays list.

internal/harness/forge.go

harness.goAdd 'overlays' to schema and extend load options for overlay inputs +16/-2

Add 'overlays' to schema and extend load options for overlay inputs
• Adds 'Overlays []OverlayEntry' to the harness struct and extends 'LoadOpts' with 'Event' and 'Config'. Updates 'LoadWithOpts' and 'Validate' to run overlay validation and resolution in the load pipeline.
internal/harness/harness.go

lint.goEmit deprecation warning when 'forge' is present +8/-0

Emit deprecation warning when 'forge' is present
• Adds a lint-time warning diagnostic for the 'forge' field directing users to migrate to overlays per ADR 0088.
internal/harness/lint.go

trigger.goAdd overlay CEL environment and evaluation helper +56/-0

Add overlay CEL environment and evaluation helper

• Introduces 'NewOverlayEnv' providing 'event', 'runtime', and 'config' CEL variables and adds 'EvaluateOverlay' to compile/evaluate overlay expressions with proper activation data and boolean result handling.

internal/harness/trigger.go

Tests (4) +490 / -0

compose_test.goAdd base composition tests for overlay concatenation and resolution +112/-0

Add base composition tests for overlay concatenation and resolution
• Adds tests that validate overlay list concatenation behavior across base/child harnesses and confirm first-match-wins resolution after composition. Ensures overlays are consumed (nilled) after resolution.
internal/harness/compose_test.go

forge_test.goAdd unit tests for overlay validation and first-match resolution +242/-0

Add unit tests for overlay validation and first-match resolution

• Adds coverage for required/invalid 'when', non-bool CEL rejection, acceptance of 'runtime.forge' and 'config' variables, embedded field validation (e.g., URL scripts rejected), mutual exclusion with 'forge', and first-match-wins behavior including skip of later matches.

internal/harness/forge_test.go

harness_test.goAdd end-to-end tests for LoadWithOpts overlay behavior +104/-0

Add end-to-end tests for LoadWithOpts overlay behavior

• Validates overlay resolution during 'LoadWithOpts' (including runtime forge and config-variable conditions), confirms overlays are consumed, and ensures 'forge'+'overlays' is rejected. Covers the no-event behavior where overlays become a no-op but are still consumed.

internal/harness/harness_test.go

lint_test.goTest forge deprecation lint warning presence/absence +32/-0

Test forge deprecation lint warning presence/absence
• Adds tests ensuring a 'forge' deprecation warning is emitted when forge config exists, and not emitted when forge is absent.
internal/harness/lint_test.go

Documentation (3) +74 / -34

0045-forge-portable-harness-schema.mdDocument 'forge' deprecation and reference ADR 0088 overlays +6/-0

Document 'forge' deprecation and reference ADR 0088 overlays
• Adds a note that the ADR 0045 'forge:' section is now deprecated in favor of CEL-guarded 'overlays:'. Links ADR 0088 in the references while keeping the rest of the merge/composition rules current.
docs/ADRs/0045-forge-portable-harness-schema.md

harness-composition.mdUpdate composition guide to include overlay validation/resolution +12/-1

Update composition guide to include overlay validation/resolution
• Extends the function map and workflow explanation to include 'validateOverlays' and 'ResolveOverlays', and clarifies 'mergeForgeConfig' now applies to overlays too. Adds links to ADR 0088 for contributor context.
docs/contributing/harness-composition.md

bring-your-own-agent.mdReplace forge examples with overlays and explain first-match-wins +56/-33

Replace forge examples with overlays and explain first-match-wins

• Updates user-facing examples from 'forge:' to 'overlays:' and documents the CEL variables ('event', 'runtime.forge', 'config'). Adds migration guidance, deprecation note for 'forge', and clarifies base-chain resolution order and overlay merge semantics.

docs/guides/user/bring-your-own-agent.md

fullsend-ai-review · 2026-08-17T21:03:12Z

🤖 Review · ❌ Terminated · Started 9:03 PM UTC · Ended 9:23 PM UTC

Commit: 050f6fb · View workflow run →

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

- 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

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

…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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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

…plication 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

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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.<platform>.* 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

…erlay 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

… docs, improve coverage - 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 <noreply@anthropic.com>

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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

Add missing runOverrideFlags{} argument to the status notifier test case that was missed by the earlier fix. Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

fullsend-ai-review · 2026-08-24T19:56:44Z

🤖 Finished Review · ✅ Success · Started 7:56 PM UTC · Completed 8:14 PM UTC

Commit: 63ae43a · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $11.11

fullsend-ai-review

See the review comment for full details.

fullsend-ai-retro · 2026-08-24T20:53:11Z

🤖 Finished Retro · ✅ Success · Started 8:53 PM UTC · Completed 9:07 PM UTC

Commit: 63ae43a · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.58

fullsend-ai-retro · 2026-08-24T21:07:27Z

Retro: PR #6285 — CEL-guarded overlays (ADR 0088)

Analyzed the full lifecycle of this human-authored feature PR (ralphbean, Aug 17–24). The PR added overlays: to the harness schema with first-match-wins CEL evaluation. 23 files changed, 2,468 additions, 17 commits, 6 successful fix-agent iterations, and 14 review-agent iterations.

Review quality

Human reviewer (waynesun09) dramatically outperformed the review agent. A single human pass found 6 comments (100% actionable, 2 HIGH-severity correctness bugs) that the review agent missed across 8+ review passes:

HIGH: config.forge never consulted in detectForgePlatform — the ADR specified flag > config.forge > CI env precedence, but the implementation skipped the middle tier.
HIGH: resolveFromLock had no overlays[N].* cases — locked overlay resources would be misassigned as bogus skills.

Both bugs were ADR-implementation mismatches that required reading the spec and verifying the code matched. The review agent never performed this analysis despite the PR title explicitly referencing ADR 0088.

The review agent's severity calibration was also off: it rated "no production caller wires Event" as LOW for 7 consecutive runs, despite it meaning the PR's core feature was a no-op in production. The author had to manually escalate it.

Review agent strengths: Reliably caught documentation staleness (planned-vs-implemented markers, missing CLI flags, stale pipeline diagrams) and code duplication. These mechanical checks are where the bot adds consistent value.

Rework and token cost

14 review iterations with ~44% repetitive findings. Findings like function-signature-growth (8 runs), resource-exhaustion (6 runs), and import-convention (8 runs) were re-raised repeatedly without code changes to the relevant areas. The import-convention finding persisted even after the fix agent added an explicit "intentional exception" comment.

Fix agent

Performed well — 6/8 iterations succeeded, correctly implementing fixes including non-trivial refactors (extracting BuildConfigMap, adding overlay cases to resolveFromLock). Two failures were infrastructure issues (gitleaks false positives during rebase, tracked in #6259).

Evidence supporting existing issues

Cross-iteration finding dedup (agents#721, agents#685, Review agent should deduplicate findings across iterations on the same PR #1013, Review agent should deduplicate findings across re-review iterations on the same PR #2959): 44% of findings were repeated 4–8 times across iterations. The sticky comment history mechanism exists but doesn't suppress unchanged findings.
ADR compliance checking (Review agent should cross-reference ADRs cited in PR descriptions to validate implementation compliance #3007): The 2 highest-severity bugs were ADR-code mismatches the review agent never detected. This PR is a strong case for implementing Review agent should cross-reference ADRs cited in PR descriptions to validate implementation compliance #3007.
Severity calibration for unreachable capabilities (agents#777, agents#412): "No production caller wires Event" rated LOW for 7 runs when the feature was effectively unreachable.
Diminishing-value findings after approval (Review agent posts diminishing-value findings after approval #3025): 3 review runs on merge day (Aug 24) after human approval on Aug 21, producing mostly repeated findings.

Autonomy readiness

The review agent is not ready for autonomous approval on large feature PRs implementing ADRs/specifications. It reliably handles documentation and style checks but misses semantic correctness bugs requiring spec-compliance analysis. Human review remains essential for this class of change.

Proposals filed

Correctness sub-agent should verify exhaustive handling of new struct fields (in fullsend-ai/agents)

ralphbean added the fullsend-fix Enables automatic bot-triggered fix runs on human-authored PRs label Aug 17, 2026

github-actions Bot deployed to site-preview August 17, 2026 11:58 View deployment

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added component/harness Agent harness, config, and skills loading component/docs User-facing documentation feature Feature-category issue awaiting human prioritization labels Aug 17, 2026

github-actions Bot deployed to site-preview August 17, 2026 12:27 View deployment

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Aug 17, 2026

ralphbean force-pushed the cel-guraded-overlays-implementation branch from 4007729 to 6ffc734 Compare August 17, 2026 20:39

github-actions Bot deployed to site-preview August 17, 2026 20:42 View deployment

github-actions Bot deployed to site-preview August 17, 2026 21:00 View deployment

ralphbean marked this pull request as ready for review August 17, 2026 21:01

ralphbean requested a review from a team as a code owner August 17, 2026 21:01

ralphbean and others added 17 commits August 24, 2026 15:54

fix(cli): add missing runOverrideFlags arg to runAgent call

63ae43a

Add missing runOverrideFlags{} argument to the status notifier test case that was missed by the earlier fix. Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>

fullsend-ai-review Bot reviewed Aug 24, 2026

View reviewed changes

fullsend-ai-retro Bot mentioned this pull request Aug 24, 2026

Correctness sub-agent should verify exhaustive handling of new struct fields fullsend-ai/agents#996

Open

fullsend-ai-coder Bot mentioned this pull request Aug 25, 2026

feat(#1013): port triage harness from forge to overlays fullsend-ai/agents#1024

Merged

3 tasks

Uh oh!

Conversation

ralphbean commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Site preview

Uh oh!

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

Uh oh!

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

High

Medium

Low

Review

Findings

High

Medium

Low

Review

Findings

High

Medium

Low

Info

Review

Findings

High

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

Low

Uh oh!

ralphbean commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

github-actions Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

codecov Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 17, 2026 •

edited

Loading

fullsend-ai-review Bot commented Aug 24, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Aug 24, 2026 •

edited

Loading