feat(harness): add allow_runtime_fetch and max_runtime_fetches (ADR-0038 Phase 4, PR 3)

[ggallen]

Summary

Final PR for ADR-0038 (Universal Harness Access). Adds harness schema fields and CLI wiring to gate the runtime fetch service behind explicit opt-in.

• Add allow_runtime_fetch (bool) and max_runtime_fetches (int) fields to the Harness struct
• Validate: allow_runtime_fetch requires allowed_remote_resources; max_runtime_fetches must be non-negative and requires allow_runtime_fetch
• Gate fetch service startup on h.AllowRuntimeFetch instead of len(h.AllowedRemoteResources) > 0
• Pass h.MaxRuntimeFetches to ServiceConfig instead of hardcoded default

Test plan

• New validation tests: reject allow_runtime_fetch without allowed_remote_resources, reject negative max_runtime_fetches, reject max_runtime_fetches without allow_runtime_fetch
• YAML round-trip tests: fields load correctly when present and default to zero values when omitted
• Updated setupFetchService tests to use AllowRuntimeFetch: true
• make go-test — all tests pass
• make lint — clean
• go vet — clean
• Coverage: Validate() at 95.6%, setupFetchService() at 90.0%

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: f449b94be8a1260fdeb87388a1a465795753d26d

[codecov]

Codecov Report

❌ Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/run.go	58.33%	5 Missing ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:26 PM UTC · Completed 8:38 PM UTC
Commit: af2e7ab · View workflow run →

[fullsend-ai-review]

Looks good to me

Info

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags but does not cross-reference harness-level fields allow_runtime_fetch and max_runtime_fetches. The feature is documented in customizing-agents.md and building-custom-agents.md (updated in this PR), but a cross-reference from the CLI flags section would help users discover it.

• [permission-expansion] internal/harness/harness.go:221 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil→10) are correctly opt-in and fail-closed. Both excluded from base-to-child merge in compose.go, preventing privilege escalation.

• [fail-closed] internal/cli/run.go:1112 — shouldStartFetchService paths are all fail-closed. Default return is (false, ""). Deprecation path preserves backward compatibility without expanding permissions.

• [fail-closed] internal/harness/harness.go:718 — EffectiveMaxRuntimeFetches() returns defaultMaxRuntimeFetches (10) when nil. No fail-open path exists.

• [permission-reduction] internal/harness/compose.go:417 — AllowRuntimeFetch and MaxRuntimeFetches excluded from base-to-child merge, preventing privilege escalation via base harness injection.

• [backward-compatibility] internal/harness/harness.go — New optional YAML fields with omitempty tags preserve backward compatibility. Existing harness files continue to parse successfully.

Previous run

Review

Findings

Low

• [edge-case] internal/cli/run.go:1082 — In shouldStartFetchService, when HasURLSkills() is true but AllowRuntimeFetch is false, the function returns (true, "") with no deprecation warning. URL skills silently start the fetch service without explicit opt-in, unlike the AllowedRemoteResources-only path which warns. This is intentional (URL skills inherently need fetching) but the asymmetry is worth noting.

• [scope-creep] internal/cli/run.go:573 — The PR adds backward-compatibility logic for legacy harnesses that use AllowedRemoteResources without AllowRuntimeFetch. This compatibility layer was not specified in the Phase 4 plan, though it is standard engineering practice for behavioral changes.

• [deprecation-warnings] internal/cli/run.go:1102 — The deprecation warning message in shouldStartFetchService is a multi-line string embedded in business logic. Consider extracting to a const for maintainability.

• [naming-conventions] internal/cli/run.go:1102 — Function shouldStartFetchService return signature (start bool, deprecationWarning string) puts the boolean first. Subjective style preference — the bool-first order is arguably clearer since it is the primary decision.

• [naming-conventions] internal/harness/harness.go — The constant defaultMaxRuntimeFetches (unexported) duplicates fetchsvc.DefaultMaxFetches (exported). Standard Go visibility convention; mitigated by TestEffectiveMaxRuntimeFetches_MatchesFetchsvcDefault.

• [validation-ordering] internal/harness/harness.go — New runtime fetch validations are inline in Validate() while related validations are extracted into helpers (validateSecurity, validateForge). The inline checks are only ~10 lines, comparable to other inline checks in the method.

• [duplicated-constant] internal/harness/harness.go:715 — The constant defaultMaxRuntimeFetches = 10 duplicates fetchsvc.DefaultMaxFetches. Mitigated by the new test TestEffectiveMaxRuntimeFetches_MatchesFetchsvcDefault which catches drift.

Info

• [permission-expansion] internal/harness/harness.go:221 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil→10) are correctly opt-in and fail-closed. Both excluded from base-to-child merge in compose.go.

• [fail-closed] internal/cli/run.go:1113 — shouldStartFetchService paths are all fail-closed. Default return is (false, ""). Deprecation path ensures no breaking change.

• [fail-closed] internal/harness/harness.go:724 — EffectiveMaxRuntimeFetches() returns defaultMaxRuntimeFetches (10) when nil. No fail-open.

• [permission-reduction] internal/harness/compose.go:417 — AllowRuntimeFetch and MaxRuntimeFetches excluded from base-to-child merge, preventing privilege escalation. Positive security change.

• [backward-compatibility] internal/harness/harness.go — New optional YAML fields with omitempty tags preserve backward compatibility. Existing harness files continue to parse successfully.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags but does not cross-reference harness-level fields allow_runtime_fetch and max_runtime_fetches.

Previous run (2)

Review

Findings

Medium

• [documentation-code-inconsistency] docs/ADRs/0024-harness-definitions.md:443 — The example YAML block sets allow_runtime_fetch: false alongside max_runtime_fetches: 10. The validation logic in harness.go rejects this combination with the error max_runtime_fetches requires allow_runtime_fetch to be true. Users who copy this example verbatim will get a validation error. Either set allow_runtime_fetch: true in the example (with allowed_remote_resources), or remove max_runtime_fetches from this example.

• [scope-creep] internal/cli/run.go — The PR adds a backward-compatibility path for legacy harnesses that use AllowedRemoteResources without AllowRuntimeFetch (deprecation warning). This compatibility layer was not specified in the Phase 4 plan. While preserving backward compatibility is the correct engineering decision (removing it would break existing harnesses), the rationale should be documented in the PR description or as a comment referencing the prior review feedback that motivated it.

Low

• [duplicated-constant] internal/harness/harness.go:715 — The constant defaultMaxRuntimeFetches = 10 duplicates fetchsvc.DefaultMaxFetches (also 10), with only a comment enforcing the coupling. If either value is changed independently, EffectiveMaxRuntimeFetches() will silently disagree with the rate limiter's fallback default. The test TestEffectiveMaxRuntimeFetches_MatchesFetchsvcDefault provides a runtime check but not a compile-time guarantee.

• [missing-authorization] — No linked GitHub issue for this non-trivial change. Authorization is established via the Phase 4 plan document (docs/plans/universal-harness-access-phase4.md line 61: "PR 3: Harness schema and CLI integration"). Linking an issue would improve traceability.

• [incomplete-implementation] docs/plans/universal-harness-access.md — The PR updates status from "Not implemented" to "Implemented in Phase 4". If PRs 1 and 2 are already merged, this is correct; if not, consider "In progress" until all Phase 4 PRs land.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags for remote resource resolution but does not mention the harness-level fields allow_runtime_fetch and max_runtime_fetches. A cross-reference to customizing-agents.md would help users discover the harness-level configuration.

• [edge-case] internal/cli/run.go:1103 — In shouldStartFetchService, when HasURLSkills() is true but AllowRuntimeFetch is false, the function returns (true, "") with no deprecation warning. This is correct behavior — URL skills need the fetch service for static resolution regardless of the runtime fetch opt-in flag.

• [error-handling] internal/cli/run.go:1119 — In setupFetchService, the forge-token resolution condition includes h.AllowRuntimeFetch as an additional disjunct alongside HasURLSkills() and len(AllowedRemoteResources) > 0. Since Validate() guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, the AllowRuntimeFetch check is logically redundant. Harmless defense-in-depth.

Info

• [permission-expansion] internal/harness/harness.go:221 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly opt-in and fail-closed. AllowRuntimeFetch requires at least one AllowedRemoteResources entry. MaxRuntimeFetches requires AllowRuntimeFetch and is bounded to 1–1000. Both fields are excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance.

• [permission-reduction] internal/cli/run.go:1113 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy backward-compatibility path (AllowedRemoteResources without AllowRuntimeFetch) preserves existing behavior with a deprecation warning. No fail-open regression. Positive security change.

• [fail-open] internal/harness/harness.go:724 — EffectiveMaxRuntimeFetches() returns defaultMaxRuntimeFetches (10) when MaxRuntimeFetches is nil, consistent with fetchsvc.DefaultMaxFetches. No fail-open: a nil/absent value does not bypass the rate limit.

Previous run (3)

Looks good to me

Low

• [duplicated-constant] internal/harness/harness.go:715 — The constant defaultMaxRuntimeFetches = 10 duplicates fetchsvc.DefaultMaxFetches (also 10), with only a comment enforcing the coupling. If either value is changed independently, EffectiveMaxRuntimeFetches() will silently disagree with the rate limiter's fallback default. Consider importing fetchsvc.DefaultMaxFetches directly or adding a compile-time assertion.

• [edge-case] internal/cli/run.go:1103 — In shouldStartFetchService, when HasURLSkills() is true but AllowRuntimeFetch is false, the function returns (true, "") with no deprecation warning. This is correct behavior — URL skills need the fetch service for static resolution regardless of the runtime fetch opt-in flag.

• [error-handling] internal/cli/run.go:1119 — In setupFetchService, the forge-token resolution condition includes h.AllowRuntimeFetch as an additional disjunct alongside HasURLSkills() and len(AllowedRemoteResources) > 0. Since Validate() guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, the AllowRuntimeFetch check is logically redundant. Harmless defense-in-depth.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags for remote resource resolution but does not mention the harness-level fields allow_runtime_fetch and max_runtime_fetches. A cross-reference to customizing-agents.md would help users discover the harness-level configuration.

Info

• [permission-expansion] internal/harness/harness.go:221 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly opt-in and fail-closed. AllowRuntimeFetch requires at least one AllowedRemoteResources entry. MaxRuntimeFetches requires AllowRuntimeFetch and is bounded to 1–1000. Both fields are excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance.

• [permission-reduction] internal/cli/run.go:1113 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy backward-compatibility path (AllowedRemoteResources without AllowRuntimeFetch) preserves existing behavior with a deprecation warning. No fail-open regression. Positive security change.

• [fail-open] internal/harness/harness.go:724 — EffectiveMaxRuntimeFetches() returns defaultMaxRuntimeFetches (10) when MaxRuntimeFetches is nil. No fail-open: a nil value does not bypass the rate limit.

Previous run (4)

Looks good to me

Low

• [edge-case] internal/cli/run.go:1103 — In shouldStartFetchService, when HasURLSkills() is true but AllowRuntimeFetch is false, the function returns (true, "") with no deprecation warning. This is correct behavior — URL skills need the fetch service for static resolution regardless of the runtime fetch opt-in flag.

• [error-handling] internal/cli/run.go:1119 — In setupFetchService, the forge-token resolution condition includes h.AllowRuntimeFetch as an additional disjunct alongside HasURLSkills() and len(AllowedRemoteResources) > 0. Since Validate() guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, the AllowRuntimeFetch check is logically redundant. Harmless defense-in-depth.

• [logic-error] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil, relying on NewRateLimiter's fallback to convert 0 to DefaultMaxFetches (10). The method name suggests it should return the resolved effective value, not a sentinel. Currently safe — there is exactly one consumption path through ServiceConfig.MaxFetches → NewRateLimiter, and validation rejects explicit 0 — but a future caller that does not pass through NewRateLimiter would observe 0 instead of 10.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags for remote resource resolution but does not mention the harness-level fields allow_runtime_fetch and max_runtime_fetches. A cross-reference to customizing-agents.md would help users discover the harness-level configuration.

Info

• [fail-open] internal/harness/harness.go — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil, which maps to DefaultMaxFetches (10) fallback in NewRateLimiter. No fail-open: a nil/zero value does not bypass the rate limit.

• [permission-expansion] internal/harness/harness.go — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly opt-in and fail-closed. AllowRuntimeFetch requires at least one AllowedRemoteResources entry. MaxRuntimeFetches requires AllowRuntimeFetch and is bounded to 1–1000. Both fields are excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance.

• [permission-reduction] internal/cli/run.go:573 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy backward-compatibility path (AllowedRemoteResources without AllowRuntimeFetch) preserves existing behavior with a deprecation warning. No fail-open regression. Positive security change.

Previous run (5)

Looks good to me

Low

• [edge-case] internal/cli/run.go:1103 — In shouldStartFetchService, when HasURLSkills() is true but AllowRuntimeFetch is false, the function returns (true, "") with no deprecation warning. The fetch service starts and EffectiveMaxRuntimeFetches() returns 0, which NewRateLimiter converts to DefaultMaxFetches (10). This is correct behavior — URL skills need the fetch service for static resolution — and is unchanged from the prior code path.

• [error-handling] internal/cli/run.go:1119 — In setupFetchService, the forge-token resolution condition includes h.AllowRuntimeFetch as an additional disjunct alongside HasURLSkills() and len(AllowedRemoteResources) > 0. Since Validate() guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, the AllowRuntimeFetch check is logically redundant. Harmless defense-in-depth.

• [validation-error-message] internal/harness/harness.go — The validation error for max_runtime_fetches uses parenthetical guidance (omit the field to use the default of 10) which is a new pattern compared to terse existing errors in Validate(). The guidance is helpful but introduces a minor inconsistency.

• [inline-comment-style] internal/harness/harness.go:221 — AllowRuntimeFetch and MaxRuntimeFetches field comments use em-dash style (— opt-in to...) instead of the parenthetical style ((opt-in)) used by other boolean flags in the struct.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags for remote resource resolution but does not mention the harness-level fields allow_runtime_fetch and max_runtime_fetches. A cross-reference to customizing-agents.md would help users discover the harness-level configuration.

Info

• [fail-open] internal/harness/harness.go:718 — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil, relying on NewRateLimiter's documented fallback to DefaultMaxFetches (10). Validation rejects explicit 0; nil is the only path to this return value. No fail-open risk.

• [permission-expansion] internal/harness/harness.go:221 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly opt-in and fail-closed. AllowRuntimeFetch requires at least one AllowedRemoteResources entry. MaxRuntimeFetches requires AllowRuntimeFetch and is bounded to 1–1000. Both fields are excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance.

• [permission-reduction] internal/cli/run.go:1108 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy backward-compatibility path (AllowedRemoteResources without AllowRuntimeFetch) preserves existing behavior with a deprecation warning. No fail-open regression. Positive security change.

Previous run (6)

Review

Findings

Low

• [edge-case] internal/cli/run.go:1099 — In shouldStartFetchService, when HasURLSkills() is true, the function returns (true, "") with no deprecation warning even if AllowRuntimeFetch is false. This is correct — URL skills require the fetch service for static resolution — but the harness will also use EffectiveMaxRuntimeFetches() (returns 0 → default 10). Pre-existing behavior unchanged by this PR.

• [error-handling] internal/cli/run.go:1108 — In setupFetchService, the forge-token resolution condition adds h.AllowRuntimeFetch as a disjunct alongside h.HasURLSkills() and len(h.AllowedRemoteResources) > 0. Since validation guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, the AllowRuntimeFetch check is redundant today. Harmless defense-in-depth.

• [missing-feature-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags for remote resource resolution but does not mention the harness-level fields allow_runtime_fetch and max_runtime_fetches. The feature is documented in customizing-agents.md (updated in this PR), but a cross-reference from the CLI flags section would help users discover it.

Info

• [fail-open] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil, relying on NewRateLimiter's documented fallback to DefaultMaxFetches (10). The godoc and both consumer call sites document this sentinel-value convention consistently. Validation now correctly rejects explicit 0 (<= 0), so nil is the only path to this return value.

• [permission-expansion] internal/harness/harness.go:220 — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly opt-in and fail-closed. AllowRuntimeFetch requires at least one AllowedRemoteResources entry. MaxRuntimeFetches requires AllowRuntimeFetch and is bounded to 1–1000. Both fields are excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance.

• [permission-reduction] internal/cli/run.go:1099 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy backward-compatibility path (AllowedRemoteResources without AllowRuntimeFetch) preserves existing behavior with a deprecation warning. No fail-open regression. Positive security change.

• [error-message-consistency] internal/harness/harness.go — New validation errors use 'requires' phrasing for inter-field dependencies (e.g., allow_runtime_fetch requires at least one entry). Existing errors use 'is required' for missing fields and 'must be' for value constraints. The 'requires' phrasing is a reasonable fit for the new semantic category of inter-field dependencies.

• [incomplete-context] docs/architecture.md:81 — The 'Agent Harness' section's 'Decided' subsection lists ADR-0022 and ADR-0045 but not ADR-0038 (universal harness access).

• [missing-authorization] — No linked GitHub issue. Authorization is established via plan doc (docs/plans/universal-harness-access-phase4.md). Linking an issue would improve traceability.

Previous run (7)

Review

Findings

Medium

• [fail-open] internal/harness/harness.go — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil (unset) or when explicitly set to 0. The downstream NewRateLimiter(max) treats max <= 0 as DefaultMaxFetches (10). A harness author who explicitly sets max_runtime_fetches: 0 (intending to disable all runtime fetches) will silently get 10 fetches allowed. Validation permits 0 as a valid value (0–1000 range). The ADR-0024 example documents this behavior ("When omitted or 0, uses the default (10)"), and allow_runtime_fetch: false is the correct way to disable fetching, but the UX is surprising.
  Remediation: Either (a) change the validation lower bound to 1 (reject 0), or (b) clarify in the error message and docs that 0 is a synonym for the default, not for "disable."

• [missing-feature-documentation] docs/guides/user/building-custom-agents.md:112 — The "Step 2: Define the harness" section provides a complete harness.yaml template but does not mention allow_runtime_fetch or max_runtime_fetches. Users building custom agents with runtime skill fetching won't find guidance here.

Low

• [edge-case] internal/cli/run.go:1105 — In shouldStartFetchService, when HasURLSkills() is true, the function returns (true, "") with no deprecation warning even if AllowRuntimeFetch is false. This is correct — URL skills require the fetch service for static resolution — but the harness will also use EffectiveMaxRuntimeFetches() (returns 0 → default 10). Pre-existing behavior unchanged by this PR.

• [error-handling] internal/cli/run.go:1108 — In setupFetchService, the forge-token resolution condition includes h.AllowRuntimeFetch as a disjunct alongside h.HasURLSkills() and len(h.AllowedRemoteResources) > 0. Validation guarantees AllowRuntimeFetch implies len(AllowedRemoteResources) > 0, so the AllowRuntimeFetch check is redundant today. Harmless defense-in-depth.

• [error-message-consistency] internal/harness/harness.go — New validation errors use 'requires' phrasing (e.g., allow_runtime_fetch requires at least one entry) while existing validation errors consistently use 'is required' for field requirements and 'must be/must' for constraints.

• [stale-reference] docs/guides/dev/cli-internals.md:318 — Lines 318–319 mention FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN with "(if active)" but don't explain that the condition is allow_runtime_fetch: true.

Info

• [permission-expansion] internal/harness/harness.go — New harness fields AllowRuntimeFetch (bool, default false) and MaxRuntimeFetches (*int, default nil) are correctly excluded from base-to-child merge in compose.go, preventing privilege escalation via base harness inheritance. AllowRuntimeFetch requires at least one allowed_remote_resources entry. Well-structured fail-closed permission controls.

• [permission-reduction] internal/cli/run.go — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy path (AllowedRemoteResources without AllowRuntimeFetch) is preserved with a deprecation warning. Positive security change.

• [incomplete-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags but doesn't mention the harness-level fields allow_runtime_fetch and max_runtime_fetches that control whether the fetch service is started.

Previous run (8)

Review

Findings

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). NewRateLimiter(0) falls back to DefaultMaxFetches (10). A user writing max_runtime_fetches: 0 passes validation but gets 10 at runtime. Behavior is documented in the godoc and ADR-0024 example (When omitted or 0, uses the default (10)).

• [edge-case] internal/cli/run.go:1105 — In shouldStartFetchService, when HasURLSkills() is true, the function returns (true, "") with no deprecation warning even if AllowRuntimeFetch is false. This is correct — URL skills require the fetch service for static resolution — but the harness will also use EffectiveMaxRuntimeFetches() (returns 0 → default 10). Pre-existing behavior unchanged by this PR.

• [error-message-consistency] internal/harness/harness.go:355 — New validation errors use 'requires' phrasing while some existing errors use 'is required' or 'must' patterns. Minor inconsistency; no single dominant pattern in existing code.

Info

• [permission-expansion] internal/harness/harness.go:221 — New harness field AllowRuntimeFetch (default: false) gates the runtime fetch service behind explicit opt-in. Validation correctly requires at least one allowed_remote_resources entry when AllowRuntimeFetch is true, and MaxRuntimeFetches is bounded to [0, 1000]. The merge function in compose.go omits both fields by relying on Go zero-value semantics. Well-structured fail-closed permission control.

• [permission-reduction] internal/cli/run.go:1110 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy path (AllowedRemoteResources without AllowRuntimeFetch) is preserved with a deprecation warning. Positive security change.

• [incomplete-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags but doesn't mention the harness-level fields allow_runtime_fetch and max_runtime_fetches that control whether the fetch service is started.

Previous run (9)

Review

Findings

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). NewRateLimiter(0) falls back to DefaultMaxFetches (10). A user writing max_runtime_fetches: 0 passes validation but gets 10 at runtime. Behavior is documented in the godoc and ADR-0024 example (When omitted or 0, uses the default (10)).

• [edge-case] internal/cli/run.go:1105 — In shouldStartFetchService, when HasURLSkills() is true, the function returns (true, "") with no deprecation warning even if AllowRuntimeFetch is false. This is correct — URL skills require the fetch service for static resolution — but the harness will also use EffectiveMaxRuntimeFetches() (returns 0 → default 10). Pre-existing behavior unchanged by this PR.

• [permission-expansion] internal/harness/harness.go:221 — max_runtime_fetches has no upper bound; validation only checks >= 0. The harness author is a trusted operator and prompt injection cannot modify YAML, so this is a nice-to-have guardrail rather than a security control.

• [missing-documentation] docs/guides/user/customizing-agents.md:9 — The 'Harness YAML Structure' section does not document the new allow_runtime_fetch or max_runtime_fetches fields. Users reading this guide will not learn about runtime fetch configuration options.

• [error-message-consistency] internal/harness/harness.go — New validation errors use 'requires' phrasing while some existing errors use 'is required' or 'must' patterns. Minor inconsistency; no single dominant pattern in existing code.

• [conditional-logic-pattern] internal/cli/run.go:1102 — The conditionals in setupFetchService and shouldStartFetchService share similar sub-conditions but serve different purposes (service startup vs forge token resolution). Not duplicated logic, but could benefit from a cross-referencing comment.

• [function-organization] internal/harness/harness.go — EffectiveMaxRuntimeFetches returns 0 for nil, relying on callers to apply the default. The doc comment correctly explains this contract. This is idiomatic Go for optional config values.

Info

• [permission-expansion] internal/harness/harness.go:220 — New harness field AllowRuntimeFetch (default: false) gates the runtime fetch service behind explicit opt-in. Validation correctly requires at least one allowed_remote_resources entry. The merge function in compose.go intentionally omits these fields, preventing base harnesses from escalating child privileges. Well-structured fail-closed permission control.

• [permission-reduction] internal/cli/run.go:571 — shouldStartFetchService replaces the inline condition with explicit opt-in via AllowRuntimeFetch. The legacy path (AllowedRemoteResources without AllowRuntimeFetch) is preserved with a deprecation warning. Positive security change.

• [incomplete-documentation] docs/guides/user/running-agents-locally.md:188 — The 'Remote resource flags' section documents CLI flags but doesn't mention the harness-level fields allow_runtime_fetch and max_runtime_fetches that control whether the fetch service is started.

Previous run (10)

Review

Findings

Medium

• [consumer-completeness] internal/harness/compose.go:370 — The mergeBaseIntoChild function handles all existing scalar and struct fields but does not include merge logic for the new AllowRuntimeFetch (bool) or MaxRuntimeFetches (*int) fields. If a base harness sets allow_runtime_fetch: true and the child omits it, the child will default to false rather than inheriting the base's setting.
  Remediation: Add explicit merge logic for these fields in mergeBaseIntoChild(), or add a comment explaining why they are intentionally not inherited (analogous to the AllowedRemoteResources rationale).

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation but gets DefaultMaxFetches (10) at runtime via NewRateLimiter(0) fallback. Behavior is intentional and documented.

• [missing-documentation] docs/guides/user/customizing-agents.md:9 — The 'Harness YAML Structure' section does not document the new allow_runtime_fetch or max_runtime_fetches fields. Users reading this guide will not learn about runtime fetch configuration options.

• [error-message-consistency] internal/harness/harness.go:357 — New validation errors use 'requires' phrasing while some existing errors use 'is required' or 'must' patterns. Minor inconsistency; no single dominant pattern in existing code.

• [conditional-logic-pattern] internal/cli/run.go:1120 — The conditional in setupFetchService and shouldStartFetchService share similar sub-conditions but serve different purposes (service startup vs forge token resolution). Not duplicated logic, but could benefit from a cross-referencing comment.

• [function-organization] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches returns 0 for nil, relying on callers to apply the default. The doc comment correctly explains this contract.

Previous run (11)

Review

Findings

Medium

• [consumer-completeness] internal/harness/compose.go:370 — The mergeBaseIntoChild function handles all existing scalar and struct fields but does not include merge logic for the new AllowRuntimeFetch (bool) or MaxRuntimeFetches (*int) fields. If a base harness sets allow_runtime_fetch: true and the child omits it, the child will default to false rather than inheriting the base's setting.
  Remediation: Add explicit merge logic for these fields in mergeBaseIntoChild(), or add a comment explaining why they are intentionally not inherited (analogous to the AllowedRemoteResources rationale).

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation but gets DefaultMaxFetches (10) at runtime via NewRateLimiter(0) fallback. Behavior is intentional and documented.

• [missing-documentation] docs/guides/user/customizing-agents.md:9 — The 'Harness YAML Structure' section does not document the new allow_runtime_fetch or max_runtime_fetches fields. Users reading this guide will not learn about runtime fetch configuration options.

• [error-message-consistency] internal/harness/harness.go:357 — New validation errors use 'requires' phrasing while some existing errors use 'is required' or 'must' patterns. Minor inconsistency; no single dominant pattern in existing code.

• [conditional-logic-pattern] internal/cli/run.go:1120 — The conditional in setupFetchService and shouldStartFetchService share similar sub-conditions but serve different purposes (service startup vs forge token resolution). Not duplicated logic, but could benefit from a cross-referencing comment.

• [function-organization] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches returns 0 for nil, relying on callers to apply the default. The doc comment correctly explains this contract.

Previous run (12)

Review

Findings

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation but gets DefaultMaxFetches (10) at runtime via NewRateLimiter(0) fallback. Behavior is intentional and documented in both the field comment and ADR-0024 example (When omitted or 0, uses the default (10)), and allow_runtime_fetch: false is the correct way to disable fetching.

• [test-adequacy] internal/cli/run_test.go:1209 — No test exercises the path where h.EffectiveMaxRuntimeFetches() produces a non-default positive value (e.g., 50) and verifies that value propagates to ServiceConfig.MaxFetches. All setupFetchService tests hardcode MaxFetches: 10.

• [missing-documentation] docs/guides/user/customizing-agents.md:10 — The 'Harness YAML Structure' section does not mention allow_runtime_fetch or max_runtime_fetches. The building-custom-agents.md harness example (line 112) similarly omits these fields. Users referencing these guides will not discover the runtime fetch capability.

Info

• [authorization-gate] internal/cli/run.go:1099 — shouldStartFetchService is fail-closed for new harnesses: AllowRuntimeFetch defaults to false. Backward compatibility preserved for existing AllowedRemoteResources-only harnesses via a deprecation warning. EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [authorization-gate] internal/harness/harness.go:354 — Validation correctly enforces that allow_runtime_fetch requires at least one allowed_remote_resources entry, preventing the fetch service from being enabled without URL-prefix constraints. max_runtime_fetches is gated behind allow_runtime_fetch and must be non-negative.

• [documentation-completeness] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches doc comment says callers should treat 0 as 'use default' but does not state what the default is (10, defined as DefaultMaxFetches in internal/fetchsvc/ratelimit.go).

Previous run (13)

Review

Findings

High

• [API-contract-violation] docs/ADRs/0024-harness-definitions.md — The diff adds allowed_remote_resources to the canonical harness YAML example as a list of objects with type and url_prefix fields (e.g., - type: skill\n url_prefix: https://...). However, the Go implementation at internal/harness/harness.go:220 defines AllowedRemoteResources []string — a flat list of URL prefix strings. A harness authored according to this documentation example would fail YAML unmarshaling at runtime because Go's yaml.Unmarshal cannot decode a YAML mapping into a Go string.
  Remediation: Change the ADR-0024 example to use a flat string list matching the implementation: allowed_remote_resources:\n - https://example.com/skills/\n - https://example.com/policies/

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation but gets DefaultMaxFetches (10) at runtime via NewRateLimiter(0) fallback. Behavior is intentional and documented, and the call site in run.go relies on the documented 0-means-default convention in ServiceConfig.MaxFetches.

• [test-adequacy] internal/cli/run_test.go:1209 — No test exercises the path where h.EffectiveMaxRuntimeFetches() produces a non-default value and that value reaches ServiceConfig.MaxFetches. All setupFetchService tests hardcode MaxFetches: 10.

• [comment-style] internal/harness/harness.go:221 — Inline comments for AllowRuntimeFetch and MaxRuntimeFetches fields use different phrasing than related fields like ServiceConfig.MaxFetches which documents the same 0-means-default contract.

• [validation-error-format] internal/harness/harness.go:357 — Validation error messages for allow_runtime_fetch/max_runtime_fetches use different phrasing patterns compared to established validation patterns elsewhere in the file.

• [missing-documentation] docs/guides/user/customizing-agents.md:10 — The 'Harness YAML Structure' section documents harness fields including optional security configuration but does not mention allowed_remote_resources, allow_runtime_fetch, or max_runtime_fetches.

• [missing-documentation] docs/guides/user/building-custom-agents.md:112 — The 'Define the harness' section shows a complete harness example but does not include the runtime fetch fields.

Info

• [authorization-gate] internal/cli/run.go — shouldStartFetchService is fail-closed for new harnesses: AllowRuntimeFetch defaults to false. Backward compatibility preserved for existing AllowedRemoteResources with a deprecation warning. EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [documentation-completeness] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches doc comment says callers should treat 0 as 'use default' but does not state what the default is (10, defined as DefaultMaxFetches in internal/fetchsvc/ratelimit.go).

Previous run (14)

Review

Findings

Medium

• [missing_documentation] docs/ADRs/0024-harness-definitions.md — ADR-0024 documents the harness YAML schema (lines 309–427) but does not include the new Phase 4 fields allow_runtime_fetch and max_runtime_fetches. Users referencing the canonical harness schema documentation will not discover these runtime fetch control fields.
  Remediation: Add allow_runtime_fetch and max_runtime_fetches field documentation to the harness schema section in ADR-0024, after the allowed_remote_resources entry.

Low

• [logic-error] internal/cli/run.go:581 — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil, relying on NewRateLimiter(0) to fall back to DefaultMaxFetches (10). The 0-means-default convention is documented in ServiceConfig.MaxFetches, but having EffectiveMaxRuntimeFetches return the actual default when nil would make the caller self-documenting.

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation (0 ≥ 0) but gets DefaultMaxFetches (10) at runtime. The doc comment documents this intent, but the UX is surprising.

• [test-adequacy] internal/cli/run_test.go:1209 — No test exercises the end-to-end path where h.EffectiveMaxRuntimeFetches() produces a non-default value and that value reaches ServiceConfig.MaxFetches. All setupFetchService tests hardcode MaxFetches: 10.

• [error-message-style] internal/cli/run.go:1111 — Deprecation warning message spans multiple lines with concatenation. Minor style inconsistency with other messages in the file.

• [comment-style] internal/harness/harness.go:221 — Inline comment for AllowRuntimeFetch field is terser than similar opt-in boolean fields elsewhere in the codebase.

• [comment-style] internal/harness/harness.go:222 — Inline comment for MaxRuntimeFetches uses slightly different phrasing than ServiceConfig.MaxFetches which documents the same 0-means-default contract.

• [validation-error-format] internal/harness/harness.go:357 — Validation error message style differs from established patterns that prefix with field descriptor and use 'must be' or 'when' clauses.

• [validation-error-format] internal/harness/harness.go:360 — Validation error 'max_runtime_fetches requires allow_runtime_fetch to be true' uses 'requires' phrasing vs. the 'when' clause pattern used by similar conditional validations.

Info

• [authorization-gate] internal/cli/run.go:1102 — shouldStartFetchService is fail-closed for new harnesses: AllowRuntimeFetch defaults to false. Backward compatibility preserved for existing AllowedRemoteResources with a deprecation warning.

• [authorization-gate] internal/harness/harness.go:356 — Validation correctly enforces that allow_runtime_fetch requires at least one allowed_remote_resources entry. max_runtime_fetches is gated behind allow_runtime_fetch and must be non-negative. No fail-open path identified.

• [fail-open-analysis] internal/cli/run.go:581 — EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [permission-scope-change] internal/harness/harness.go:221 — New harness fields allow_runtime_fetch (bool) and max_runtime_fetches (*int) expand the harness permission surface for controlling the runtime fetch service. Both are properly gated.

• [trajectory-alignment] internal/cli/run.go:567 — Implementation correctly follows Phase 4 PR 3 scope as documented in docs/plans/universal-harness-access-phase4.md.

• [documentation-completeness] internal/harness/harness.go:713 — EffectiveMaxRuntimeFetches doc comment says callers should treat 0 as 'use default' but does not state what the default is (10).

Previous run (15)

Review

Findings

Medium

• [missing_documentation] README.md — README.md lists Phase 1 and Phase 2 implementation plan documents for Universal Harness Access (ADR-0038) but does not list Phase 3 (universal-harness-access-phase3.md) and Phase 4 (universal-harness-access-phase4.md) documents, which both exist in docs/plans/. Users looking at the README's documentation index will not discover the Phase 3 (lock files) and Phase 4 (runtime dependency loading) implementation plans.
  Remediation: Add references to the Phase 3 and Phase 4 implementation plan documents in the README.md plans section, following the same format as the existing Phase 1 and Phase 2 entries.

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 passes validation (0 ≥ 0) but gets DefaultMaxFetches (10) at runtime via NewRateLimiter's fallback, rather than zero fetches. The doc comment documents this intent, and allow_runtime_fetch: false is the correct way to disable fetching.

• [semantic-incoherence] internal/harness/harness.go:714 — EffectiveMaxRuntimeFetches() returns 0 when MaxRuntimeFetches is nil. The 0-means-default contract is consistent across all layers (NewRateLimiter treats max ≤ 0 as DefaultMaxFetches (10), and ServiceConfig documents this). However, returning a sentinel 0 rather than the actual default is a minor readability concern.

• [validation-consistency] internal/harness/harness.go:354 — New validation for allow_runtime_fetch and max_runtime_fetches is placed after ValidateResourceTypes(), while other field-interdependency checks appear inline before the delegated validation calls. Minor ordering inconsistency.

• [documentation-style] internal/cli/run.go:1103 — Function comment for shouldStartFetchService is adequate for its purpose. The current Go doc comment format is consistent with nearby functions like setupFetchService.

Info

• [authorization-gate] internal/cli/run.go:1102 — shouldStartFetchService is fail-closed for new harnesses: AllowRuntimeFetch defaults to false. Backward compatibility preserved for existing AllowedRemoteResources with a deprecation warning — correctly distinguishes URL skills (Phase 1–3, no warning) from legacy implicit opt-in (warning).

• [authorization-gate] internal/harness/harness.go:356 — Validation correctly enforces that allow_runtime_fetch requires at least one allowed_remote_resources entry, preventing the fetch service from being enabled without URL-prefix constraints. max_runtime_fetches is gated behind allow_runtime_fetch and must be non-negative. No fail-open path identified.

• [fail-open-analysis] internal/cli/run.go:581 — EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [trajectory-alignment] internal/cli/run.go:567 — Change correctly implements Phase 4 PR 3 scope. Backward-compat and pointer type concerns from prior review are resolved.

• [implementation-alignment] docs/plans/universal-harness-access.md:343 — Plan document status updated to reflect Phase 4 implementation. Prior stale-doc finding resolved.

Previous run (16)

Review

Findings

Medium

• [behavioral_change] docs/plans/universal-harness-access.md:343 — The design document still reads "Not implemented in initial design. Tracked in a future issue" for runtime dependency loading, but this PR implements the Phase 4 harness schema portion (allow_runtime_fetch, max_runtime_fetches). The doc is now stale.
  Remediation: Update line 343 to reflect that Phase 4 implementation is in progress, e.g. "Phase 4 (in progress): harness schema fields (allow_runtime_fetch, max_runtime_fetches) and CLI wiring added. See docs/plans/universal-harness-access-phase4.md."

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 gets DefaultMaxFetches (10) rather than zero fetches. The inline comment documents this, and allow_runtime_fetch: false is the correct way to disable fetching. Minor documentation-only improvement opportunity.

• [edge-case] internal/harness/harness.go:360 — Validation check order: AllowRuntimeFetch: false, MaxRuntimeFetches: -1 reports "requires allow_runtime_fetch" rather than "must be non-negative". The prerequisite check fires first, which is logically correct but produces a mildly misleading error message.

• [missing-authorization] — No linked issue for this non-trivial change. The PR title references ADR-0038 Phase 4 PR 3, and docs/plans/universal-harness-access-phase4.md explicitly authorizes this scope (lines 61–64), so authorization is established via plan doc. Linking an issue would improve traceability.

Info

• [fail-open-analysis] internal/cli/run.go:581 — EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [authorization-gate] internal/cli/run.go:1102 — shouldStartFetchService is fail-closed for new harnesses (AllowRuntimeFetch defaults to false). Backward compatibility preserved for existing AllowedRemoteResources with a deprecation warning — correctly distinguishes URL skills (Phase 1–3, no warning) from legacy implicit opt-in (warning).

• [trajectory-alignment] internal/cli/run.go:567 — Change correctly implements the Phase 4 PR 3 scope. The prior review's high finding (backward-compat break) is resolved: shouldStartFetchService preserves the legacy path with a deprecation warning. The prior review's medium finding (MaxRuntimeFetches *int) is resolved: the field now uses a pointer type.

Previous run (17)

Review

Findings

Medium

• [behavioral_change] docs/plans/universal-harness-access.md:343 — The design document still reads "Not implemented in initial design. Tracked in a future issue" for runtime dependency loading, but this PR implements the Phase 4 harness schema portion (allow_runtime_fetch, max_runtime_fetches). The doc is now stale.
  Remediation: Update line 343 to reflect that Phase 4 implementation is in progress, e.g. "Phase 4 (in progress): harness schema fields (allow_runtime_fetch, max_runtime_fetches) and CLI wiring added. See docs/plans/universal-harness-access-phase4.md."

Low

• [edge-case] internal/harness/harness.go:716 — EffectiveMaxRuntimeFetches returns 0 for both nil (omitted) and explicit *int(0). A user writing max_runtime_fetches: 0 gets DefaultMaxFetches (10) rather than zero fetches. The inline comment documents this, and allow_runtime_fetch: false is the correct way to disable fetching. Minor documentation-only improvement opportunity.

• [edge-case] internal/harness/harness.go:360 — Validation check order: AllowRuntimeFetch: false, MaxRuntimeFetches: -1 reports "requires allow_runtime_fetch" rather than "must be non-negative". The prerequisite check fires first, which is logically correct but produces a mildly misleading error message.

• [missing-authorization] — No linked issue for this non-trivial change. The PR title references ADR-0038 Phase 4 PR 3, and docs/plans/universal-harness-access-phase4.md explicitly authorizes this scope (lines 61–64), so authorization is established via plan doc. Linking an issue would improve traceability.

Info

• [fail-open-analysis] internal/cli/run.go:581 — EffectiveMaxRuntimeFetches() returns 0 when omitted; NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [authorization-gate] internal/cli/run.go:1102 — shouldStartFetchService is fail-closed for new harnesses (AllowRuntimeFetch defaults to false). Backward compatibility preserved for existing AllowedRemoteResources with a deprecation warning — correctly distinguishes URL skills (Phase 1–3, no warning) from legacy implicit opt-in (warning).

• [trajectory-alignment] internal/cli/run.go:567 — Change correctly implements the Phase 4 PR 3 scope. The prior review's high finding (backward-compat break) is resolved: shouldStartFetchService preserves the legacy path with a deprecation warning. The prior review's medium finding (MaxRuntimeFetches *int) is resolved: the field now uses a pointer type.

Previous run (18)

Review

Findings

High

• [logic-error] internal/cli/run.go:568 — The fetch-service gate condition changed from len(h.AllowedRemoteResources) > 0 to h.AllowRuntimeFetch, but Validate() does not require AllowRuntimeFetch: true when AllowedRemoteResources is populated. Existing harness configs that declare allowed_remote_resources without also setting allow_runtime_fetch: true will silently lose runtime fetch capability with no warning logged.
  Remediation: Either (a) emit a warning when len(h.AllowedRemoteResources) > 0 && !h.AllowRuntimeFetch so existing users know they need to add the new flag, or (b) keep AllowedRemoteResources as an implicit opt-in so the old behavior is preserved and the new flag is purely additive.

Medium

• [edge-case] internal/harness/harness.go:357 — MaxRuntimeFetches = 0 (the Go zero-value when the YAML field is omitted) passes validation and silently maps to DefaultMaxFetches (10) inside NewRateLimiter. A user who explicitly writes max_runtime_fetches: 0 in YAML will get 10 fetches — not zero — with no error or warning. Explicit-zero is indistinguishable from omitted.
  Remediation: Use a *int (pointer) type for MaxRuntimeFetches so omitted (nil) and explicit-zero (0) are distinguishable. Alternatively, document that 0 means "use default".

Low

• [test-inadequate] internal/harness/harness_test.go:1296 — No test covers the case where AllowRuntimeFetch: true and MaxRuntimeFetches: 0 (the default) with AllowedRemoteResources populated. The NewRateLimiter(0) fallback is tested in ratelimit_test.go, but no validation-level test confirms this path.

• [edge-case] internal/harness/harness.go:363 — Validation check order: AllowRuntimeFetch: false, MaxRuntimeFetches: -1 reports "must be non-negative" rather than the arguably more helpful "requires allow_runtime_fetch". Minor UX issue.

• [missing-default-documentation] internal/harness/harness.go:221 — AllowRuntimeFetch and MaxRuntimeFetches fields lack inline documentation of their default values. The non-obvious MaxRuntimeFetches=0 → DefaultMaxFetches fallback especially warrants a comment.

• [permission-scope] internal/harness/harness.go:221 — New harness schema fields allow_runtime_fetch and max_runtime_fetches expand the permission surface. Validation correctly requires allowed_remote_resources when allow_runtime_fetch is true, preventing fail-open. No upper bound on max_runtime_fetches, but harness is authored by trusted operator.

Info

• [fail-open-analysis] internal/cli/run.go:581 — Changing MaxFetches from fetchsvc.DefaultMaxFetches to h.MaxRuntimeFetches is safe: when omitted, Go zero-initializes to 0, and NewRateLimiter(0) falls back to DefaultMaxFetches (10). No fail-open path exists.

• [authorization-gate] internal/cli/run.go:573 — The fetch service activation condition changed to be strictly tighter: previously any harness with allowed_remote_resources entries would start the service; now it requires explicit allow_runtime_fetch: true.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 8:46 PM UTC · Ended 8:54 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:57 PM UTC · Completed 9:11 PM UTC
Commit: 9d21519 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:17 PM UTC · Completed 9:31 PM UTC
Commit: 59de906 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:36 PM UTC · Completed 9:50 PM UTC
Commit: f6fe791 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:57 PM UTC · Ended 9:58 PM UTC
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:02 PM UTC · Completed 10:18 PM UTC
Commit: b58cbcd · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:45 PM UTC · Completed 6:04 PM UTC
Commit: 9f8c554 · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 6:20 PM UTC · Completed 6:27 PM UTC
Commit: b7e2b11 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:28 PM UTC · Completed 8:42 PM UTC
Commit: 331cad2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:52 PM UTC · Completed 9:09 PM UTC
Commit: f449b94 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 9:20 PM UTC · Completed 9:25 PM UTC
Commit: f449b94 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2270 — feat(harness): add allow_runtime_fetch and max_runtime_fetches

Overall assessment: Review quality was high, but run volume was excessive.

Timeline

• 2026-06-12 20:22 — Human author (ggallen) opens PR with 12 files changed (+309/-15)
• 2026-06-12 20:38 — First review: CHANGES_REQUESTED with a high-severity finding (backward-compat break in fetch service gate) and a medium finding (use *int pointer for zero-value semantics). Both were real, actionable issues.
• 2026-06-12 20:41–21:59 — Author fixes all findings across several commits
• 2026-06-12 22:18 — Second review: CHANGES_REQUESTED with only low-severity items (style, test coverage)
• 2026-06-13 02:49 — Third review: APPROVED. All issues resolved.
• 2026-06-13–2026-06-15 — Author pushes 14 more commits (tests, docs, rebases). Bot runs 19 more times (9 additional approvals, 3 cancelled, 1 failure).
• 2026-06-15 21:14 — PR merged

What went well

• Review caught a real bug. The first review identified that changing the fetch-service gate from len(h.AllowedRemoteResources) > 0 to h.AllowRuntimeFetch would silently break existing configs. This was a genuine backward-compatibility issue that the author fixed with a deprecation-warning fallback.
• Severity calibration was accurate. High for the breaking change, medium for the pointer-type design issue, low for style nits.
• Author iteration was productive. The human quickly addressed all findings.

What could go better

• 22 review runs for a single PR is excessive. After the first approval (June 13, 02:49 UTC), the bot ran 19 more times across 2.5 days. Most of these produced no new findings — just redundant approvals.
• Estimated waste: ~17 of the 22 runs added no value (the 2 CHANGES_REQUESTED and ~3 early reviews were useful). At ~12 minutes per run, that's ~3.5 hours of compute and significant token spend.

Proposals skipped (covered by existing issues)

All improvement opportunities identified are already tracked by open issues:

• Debouncing rapid pushes: Debounce review dispatch on rapid synchronize events #1014, Deduplicate review runs on rapid successive pushes #1418, Deduplicate review runs when PR is rebased multiple times in quick succession #1422
• SHA-based dedup / skip after approval: Skip review dispatch when HEAD SHA was already reviewed and approved #963, Cancel-and-skip redundant re-reviews on force-pushed PRs with no prior findings #1372
• Pass prior findings to re-reviews: Pass prior review findings to follow-up reviews for targeted verification #1552, Review comment should state whether findings changed since prior review #1155
• Concurrency groups: Add concurrency group to dispatch-review job in fullsend.yaml #981
• Reduce redundant iterations: Review agent accumulates redundant internal iterations on clean diffs #1370, Reduce unnecessary re-review runs on large PRs #1390
• Cancel stale-head reviews: Reduce wasted review runs from stale-head races #1331, Review agent should discard findings from superseded SHAs during mid-review pushes #2116

The existing issue backlog comprehensively covers the review-cost problem observed here. No new proposals are warranted — implementing any combination of #963, #1014, and #1552 would substantially reduce the waste pattern seen on this PR.