feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2)

[ggallen]

Summary

• Adds fullsend fetch-skill <url> subcommand for agents to fetch skills at runtime from inside the sandbox, communicating with the runner-side fetch service over HTTP with bearer token auth
• Adds fetch server lifecycle management (startFetchService) with dynamic TCP port allocation, per-run UUID bearer token (timing-safe comparison), and graceful shutdown
• Adds SandboxUploader adapter wrapping sandbox.UploadDir to implement the fetchsvc.Uploader interface
• Wires the fetch service into runAgent() and injects FULLSEND_FETCH_URL / FULLSEND_FETCH_TOKEN env vars during sandbox bootstrap
• Includes carryover fixes from feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1) #2173 review: CachePutDir return value correctness, audit log error propagation, MaxBytesError → 413

Context

This is Phase 4, PR 2 of ADR-0038 (Universal Harness Access). PR 1 (#2173) delivered the runner-side fetchsvc package. This PR completes the client side so agents can invoke fullsend fetch-skill inside the sandbox. PR 3 will add allow_runtime_fetch / max_runtime_fetches harness schema fields to gate the feature.

Supersedes #2221 (closed due to branch rename).

Design pivot: Unix socket → HTTP

The original design in docs/plans/universal-harness-access.md specified a Unix socket for sandbox-to-runner communication. During Phase 4 implementation, this was changed to HTTP over TCP because OpenShell sandboxes are containers with no shared filesystem for Unix sockets. This aligns with ADR-0046, which establishes TCP through the L7 proxy as the standard transport for container-to-host services. Plan docs have been updated to reflect this change.

Key decisions

• Subcommand, not separate binary — reuses the fullsend binary already uploaded to /sandbox/workspace/bin/fullsend, avoiding cross-compilation and GoReleaser changes
• HTTP over TCP, not Unix socket — OpenShell sandboxes are containers with no shared filesystem; TCP through the L7 proxy is the established transport (ADR-0046)
• Bearer token auth — per ADR-0046, a per-run random token delivered via env var; timing-safe comparison prevents timing attacks
• Fetch service gated by AllowedRemoteResources — the service only starts when the harness declares remote resources, minimizing attack surface for harnesses that don't use runtime fetching. PR 3 will add dedicated allow_runtime_fetch for finer-grained control.

Test plan

• 16 unit tests for fetch-skill subcommand (success, error codes, missing env, auth propagation, request body, connection refused, registration, arg validation)
• 7 unit tests for fetch server (lifecycle, fetch endpoint, auth rejection, token validation, token generation)
• 6 unit tests for setupFetchService (forge client passthrough, token resolution, no-remote-resources, token failure warning)
• 2 unit tests for bootstrapEnv (includes/skips fetch env vars)
• go test ./... — all packages pass
• go vet clean
• make lint passes
• fullsend fetch-skill --help displays correct usage

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://898ac605-site.fullsend-ai.workers.dev

Commit: aeb0f1deeab9ec1c5717cf0d1296005ad8edb02b

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:58 PM UTC · Completed 1:14 PM UTC
Commit: 6f269db · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:905 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents. The code comment acknowledges this as a planned phased delivery gap, and the blast radius is limited by allowed_remote_resources prefix enforcement and rate limits.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [error handling] internal/fetchsvc/service.go:216 — The committed = true assignment was moved before the audit log write. The fetch is counted against the rate limit even when audit evidence is missing. This is correct fail-safe behavior (prevents bypassing rate limits by causing audit write failures), and the diff adds explicit error logging via log.Printf for audit write failures.

• [missing-authorization] — No linked GitHub issue authorizing this non-trivial work (15 files, 1000+ changed lines). Authorization is inferable from ADR-0038 and the Phase 4 plan which explicitly lists PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223.

Info

• [test-inadequate] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token with constant-time comparison.

• [secret-exposure] internal/cli/run.go:1136 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius.

• [injection] internal/cli/run.go:1132 — Prior finding resolved: FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN now use the same single-quote escaping pattern as FULLSEND_OUTPUT_FILE.

Previous run

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:905 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents. The code comment acknowledges this as a planned phased delivery gap, and the blast radius is limited by allowed_remote_resources prefix enforcement and rate limits.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [error handling] internal/fetchsvc/service.go:216 — The committed = true assignment was moved before the audit log write. The fetch is counted against the rate limit even when audit evidence is missing. This is correct fail-safe behavior (prevents bypassing rate limits by causing audit write failures), and the diff adds explicit error logging via log.Printf for audit write failures.

Info

• [test-inadequate] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [edge-case] internal/cli/run.go:982 — bootstrapEnv uses a variadic fetchEnv ...fetchServiceEnv parameter and only checks fetchEnv[0]. Safe by construction since the only call site passes at most one value, but the variadic signature implies multiple values are supported.

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token with constant-time comparison.

• [secret-exposure] internal/cli/run.go:1136 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius.

• [injection] internal/cli/run.go:1132 — Prior finding resolved: FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN now use the same single-quote escaping pattern as FULLSEND_OUTPUT_FILE.

Previous run (2)

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:905 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents. The code comment acknowledges this as a planned phased delivery gap, and the blast radius is limited by allowed_remote_resources prefix enforcement and rate limits.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [error handling] internal/fetchsvc/service.go:216 — The committed = true assignment was moved before the audit log write. The fetch is counted against the rate limit even when audit evidence is missing. This is correct fail-safe behavior (prevents bypassing rate limits by causing audit write failures), and the diff adds explicit error logging via log.Printf for audit write failures.

• [scope-creep] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR makes unrelated changes to the pr-review skill template, removing references to "Looks good to me" and changing the no-findings behavior. These changes are not mentioned in the PR description and do not relate to the runtime skill fetch feature.
  Remediation: Remove the pr-review skill changes from this PR and submit them separately, or document them in the PR description if intentional.

Info

• [missing-authorization] — No linked GitHub issue authorizing this non-trivial work (16 files, 600+ new lines). Authorization is inferable from ADR-0038 and prior PR feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1) #2173, and the Phase 4 plan explicitly lists PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223.

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token with constant-time comparison.

• [secret-exposure] internal/cli/run.go:1136 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius. Single-quote escaping now matches the pattern used for FULLSEND_OUTPUT_FILE.

• [test adequacy] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [internal consistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:700 — The new "No findings." wording is ambiguous about whether the ## Review header should still be emitted when there are no findings.

• [phased-delivery] internal/cli/run.go:913 — Fetch service uses hardcoded DefaultMaxFetches (10). Configurable max_runtime_fetches harness field deferred to PR 3.

• [injection] internal/cli/run.go:1132 — Prior finding resolved: FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN now use the same single-quote escaping pattern as FULLSEND_OUTPUT_FILE.

Previous run (3)

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:540 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [missing-authorization] — No linked GitHub issue authorizing this non-trivial work (16 files, 600+ new lines). Authorization is inferable from ADR-0038 and prior PR feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1) #2173, but a formal tracking issue is missing.
  Remediation: Link this PR to a tracking issue for ADR-0038 Phase 4 implementation.

• [error handling] internal/cli/run.go:979 — When resolveToken() fails in setupFetchService, the error is logged via log.Printf rather than the structured printer.StepWarn used elsewhere in runAgent.
  Remediation: Pass the printer into setupFetchService and use printer.StepWarn, or return a warning for the caller to log.

• [injection] internal/cli/run.go:1133 — FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN written into sandbox .env with single-quote wrapping but without the escape-for-single-quotes pattern (strings.ReplaceAll) used for FULLSEND_OUTPUT_FILE. Safe by construction (hex token, ip:port addr) but defense-in-depth gap.
  Remediation: Apply the same single-quote escaping pattern used for FULLSEND_OUTPUT_FILE.

• [scope-alignment] docs/plans/universal-harness-access-phase4.md:48 — The implementation pivoted from Unix socket to HTTP with bearer token per ADR-0046. The plan docs were updated, but the PR description does not explicitly call out this design pivot.

Info

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token.

• [secret-exposure] internal/cli/run.go:1134 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius.

• [test adequacy] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [phased-delivery] internal/cli/run.go:920 — Fetch service uses hardcoded DefaultMaxFetches (10). Configurable max_runtime_fetches harness field deferred to PR 3.

• [naming-consistency] internal/cli/fetchskill.go — CLI subcommand uses fetch-skill (hyphen) while the planned harness field is allow_runtime_fetch (underscore), following respective conventions.

Previous run (4)

Review

Findings

Medium

• [error handling] internal/fetchsvc/service.go:224 — The audit log write was changed from fire-and-forget to error-checked, returning a 500 on failure. However, at this point the skill has already been fetched, cached, and uploaded to the sandbox. Returning an error (a) causes the rate limiter slot to be released via the deferred Release() (since committed stays false), and (b) tells the client the operation failed even though all side effects succeeded. On retry, the skill would be uploaded again. If the audit log write is transiently broken, every fetch attempt will fail despite succeeding.
  Remediation: Either keep the audit log write as best-effort with a logged warning, or set committed = true before the audit check so the rate limiter reflects reality.

• [authorization] internal/cli/run.go:546 — The fetch service is started unconditionally for every agent run, regardless of whether the harness declares allow_runtime_fetch: true. Until PR 3 ships, every agent run exposes an authenticated HTTP service that can fetch and upload skill directories into the sandbox, even for harnesses that do not opt in. A prompt-injected agent could invoke fullsend fetch-skill to pull skills from any URL matching allowed_remote_resources. See also: [incomplete-phase-delivery] finding at this location.
  Remediation: Gate startFetchService behind h.AllowRuntimeFetch now, or at minimum skip injecting FULLSEND_FETCH_URL/FULLSEND_FETCH_TOKEN into the sandbox .env when the harness does not opt in.

• [incomplete-phase-delivery] internal/cli/run.go:546 — The fetch service starts unconditionally in runAgent. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Acknowledged as deliberate interim state. See also: [authorization] finding at this location.

• [stale-architecture-description] docs/plans/universal-harness-access.md:332 — Describes the runtime fetch API as using Unix sockets for communication. PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223 implemented HTTP communication with bearer token authentication (per ADR-0046), not Unix sockets.
  Remediation: Update to reflect HTTP-based communication with FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN environment variables.

• [stale-binary-name] docs/plans/universal-harness-access.md:326 — References fullsend-fetch-skill as a standalone command. PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223 implemented it as fullsend fetch-skill subcommand.

• [stale-binary-name] docs/plans/universal-harness-access.md:1425 — References implementing fullsend-fetch-skill binary. Implemented as a subcommand instead.

• [stale-binary-name] docs/plans/universal-harness-access-phase1.md:212 — References the old fullsend-fetch-skill binary name and Unix socket transport.

• [stale-binary-name] docs/plans/universal-harness-access-phase2.md:451 — References the old fullsend-fetch-skill binary name and Unix socket transport.

Low

• [error handling] internal/cli/run.go:544 — When resolveToken() fails in the fetch service setup block, the error is silently swallowed. The fetch service starts with runtimeForgeClient = nil, meaning any runtime fetch for an uncached skill will fail with a generic 500 error. No warning is logged.
  Remediation: Log a warning via printer.StepWarn when resolveToken() fails.

• [injection] internal/cli/run.go:1126 — FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN are written into the sandbox .env with single-quote wrapping but without the escape-for-single-quotes pattern (strings.ReplaceAll) used for FULLSEND_OUTPUT_FILE two lines earlier. Safe by construction (hex token, ip:port addr), but a defense-in-depth gap.

• [network-exposure] internal/cli/fetchserver.go:33 — The fetch service binds to 0.0.0.0:0 (all interfaces). Mitigated by per-run ephemeral 256-bit token and dynamic port. Required by ADR-0046 for current sandbox networking model.

• [secret-exposure] internal/cli/run.go:1127 — The bearer token is written to the sandbox .env file in plaintext. Token is ephemeral (per-run) and sandbox is isolated, limiting blast radius.

Info

• [test adequacy] internal/cli/fetchskill_test.go — The prior timeout test was removed but no replacement was added. fetchSkillTimeout (120s) is a package-level constant not injectable in tests.

• [error-formatting-consistency] internal/cli/fetchskill.go — Prior finding resolved: the code now follows the established pattern of returning errors via RunE without writing to stderr directly.

Previous run (5)

Review

Findings

Medium

• [error handling] internal/cli/fetchserver.go:46 — The shutdown function captures the parent ctx (which is context.Background() from runAgent) for server.Shutdown(ctx). Since context.Background() is never cancelled and has no deadline, server.Shutdown will block indefinitely if any in-flight request hangs.
  Remediation: Create a timeout-bounded context inside the shutdown function: ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second); defer cancel(); server.Shutdown(ctx)

• [test adequacy] internal/cli/fetchskill_test.go:183 — TestRunFetchSkill_Timeout does not actually test timeout behavior. The test server sleeps for 200ms while fetchSkillTimeout is 120s. The saved origTimeout is never used to reduce the timeout. The test verifies a 200ms request succeeds, not that the client correctly times out.
  Remediation: Make fetchSkillTimeout injectable (via function parameter or package-level override in tests) and set it shorter than the server sleep, or rename the test to reflect what it actually verifies.

• [error handling] internal/cli/fetchskill.go:77 — If json.NewDecoder(resp.Body).Decode fails (e.g., non-JSON error from a reverse proxy), the HTTP status code is lost. The caller sees "failed to decode response" with no indication whether it was a 403, 502, etc.
  Remediation: Include the status code: fmt.Fprintf(stderr, "failed to decode response (HTTP %d): %v\n", resp.StatusCode, err)

• [error-formatting-consistency] internal/cli/fetchskill.go:80 — Error construction pattern is inconsistent with the rest of the CLI. The codebase favors return fmt.Errorf(...) for command errors, letting cobra handle display. runFetchSkill writes to stderr AND returns errors, causing duplication in contexts where cobra also prints the error.
  Remediation: Remove the fmt.Fprintln(stderr, ...) calls and rely on returned errors, matching the established pattern in postcomment.go, postreview.go, etc.

• [incomplete-phase-delivery] internal/cli/run.go — The fetch service starts unconditionally in runAgent but the harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). This means every agent run starts the fetch service whether or not the harness intends to allow runtime fetches. Consider adding a comment near the initialization clarifying this is an intentional interim state, or gating behind a simple nil check on the config fields.

• [missing-cli-documentation] docs/guides/dev/cli-internals.md:8 — The CLI Command Tree section does not document the new fetch-skill subcommand.
  Remediation: Add fetch-skill to the CLI command tree.

• [missing-environment-variable-documentation] docs/guides/dev/cli-internals.md:312 — The bootstrapEnv() documentation section does not include the new FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN environment variables.
  Remediation: Add both variables to the bootstrapEnv() documentation.

Low

• [injection] internal/cli/run.go:1118 — FULLSEND_FETCH_TOKEN is injected into the sandbox .env file without shell quoting. The token is a 64-char hex string (safe by construction), but the pattern diverges from the single-quote-with-escape approach used for FULLSEND_OUTPUT_FILE. Defense-in-depth: apply the same quoting pattern.

• [http-server-configuration] internal/cli/fetchserver.go:37 — The http.Server has no ReadTimeout, WriteTimeout, or IdleTimeout set. Other servers in the codebase (e.g., appsetup.go) configure timeouts. Consider adding standard timeouts for defense-in-depth.

• [naming-consistency] internal/cli/fetchskill.go — ADR-0038 Phase 4 plan refers to a "fullsend-fetch-skill binary" but the implementation is a fetch-skill subcommand of the existing fullsend binary. Consider updating the plan to match the implementation.

• [missing configuration] internal/cli/run.go:549 — MaxFetches and SkillDestDir are not explicitly set in ServiceConfig, relying on zero-value defaults. The defaults are reasonable but explicit values would improve readability.

• [race condition] internal/cli/fetchserver.go:40 — The goroutine uses fmt.Printf for server errors while the main goroutine uses the printer, creating potential for interleaved output. Consider using the printer or a logger.

• [network-exposure] internal/cli/fetchserver.go:33 — The server binds to 0.0.0.0:0 (all interfaces). Per ADR-0046 this is required for the current sandbox networking model, but worth noting for future hardening when loopback becomes available.

• [secret-exposure] internal/cli/run.go:1118 — The bearer token is written to the sandbox .env file in plaintext. Impact is limited since the token is ephemeral (per-run) and scoped to the fetch service lifetime.

Previous run

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:905 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents. The code comment acknowledges this as a planned phased delivery gap, and the blast radius is limited by allowed_remote_resources prefix enforcement and rate limits.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [error handling] internal/fetchsvc/service.go:216 — The committed = true assignment was moved before the audit log write. The fetch is counted against the rate limit even when audit evidence is missing. This is correct fail-safe behavior (prevents bypassing rate limits by causing audit write failures), and the diff adds explicit error logging via log.Printf for audit write failures.

Info

• [test-inadequate] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [edge-case] internal/cli/run.go:982 — bootstrapEnv uses a variadic fetchEnv ...fetchServiceEnv parameter and only checks fetchEnv[0]. Safe by construction since the only call site passes at most one value, but the variadic signature implies multiple values are supported.

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token with constant-time comparison.

• [secret-exposure] internal/cli/run.go:1136 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius.

• [injection] internal/cli/run.go:1132 — Prior finding resolved: FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN now use the same single-quote escaping pattern as FULLSEND_OUTPUT_FILE.

Previous run (2)

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:905 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents. The code comment acknowledges this as a planned phased delivery gap, and the blast radius is limited by allowed_remote_resources prefix enforcement and rate limits.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [error handling] internal/fetchsvc/service.go:216 — The committed = true assignment was moved before the audit log write. The fetch is counted against the rate limit even when audit evidence is missing. This is correct fail-safe behavior (prevents bypassing rate limits by causing audit write failures), and the diff adds explicit error logging via log.Printf for audit write failures.

• [scope-creep] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR makes unrelated changes to the pr-review skill template, removing references to "Looks good to me" and changing the no-findings behavior. These changes are not mentioned in the PR description and do not relate to the runtime skill fetch feature.
  Remediation: Remove the pr-review skill changes from this PR and submit them separately, or document them in the PR description if intentional.

Info

• [missing-authorization] — No linked GitHub issue authorizing this non-trivial work (16 files, 600+ new lines). Authorization is inferable from ADR-0038 and prior PR feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1) #2173, and the Phase 4 plan explicitly lists PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223.

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token with constant-time comparison.

• [secret-exposure] internal/cli/run.go:1136 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius. Single-quote escaping now matches the pattern used for FULLSEND_OUTPUT_FILE.

• [test adequacy] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [internal consistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:700 — The new "No findings." wording is ambiguous about whether the ## Review header should still be emitted when there are no findings.

• [phased-delivery] internal/cli/run.go:913 — Fetch service uses hardcoded DefaultMaxFetches (10). Configurable max_runtime_fetches harness field deferred to PR 3.

• [injection] internal/cli/run.go:1132 — Prior finding resolved: FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN now use the same single-quote escaping pattern as FULLSEND_OUTPUT_FILE.

Previous run (3)

Review

Findings

Medium

• [incomplete-phase-delivery] internal/cli/run.go:540 — The fetch service starts when h.HasURLSkills() || len(h.AllowedRemoteResources) > 0. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Any harness declaring allowed_remote_resources for build-time-only use will also expose the runtime fetch service to in-sandbox agents.
  Remediation: Gate fetch service startup behind the allow_runtime_fetch harness field in PR 3 as planned.

Low

• [missing-authorization] — No linked GitHub issue authorizing this non-trivial work (16 files, 600+ new lines). Authorization is inferable from ADR-0038 and prior PR feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1) #2173, but a formal tracking issue is missing.
  Remediation: Link this PR to a tracking issue for ADR-0038 Phase 4 implementation.

• [error handling] internal/cli/run.go:979 — When resolveToken() fails in setupFetchService, the error is logged via log.Printf rather than the structured printer.StepWarn used elsewhere in runAgent.
  Remediation: Pass the printer into setupFetchService and use printer.StepWarn, or return a warning for the caller to log.

• [injection] internal/cli/run.go:1133 — FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN written into sandbox .env with single-quote wrapping but without the escape-for-single-quotes pattern (strings.ReplaceAll) used for FULLSEND_OUTPUT_FILE. Safe by construction (hex token, ip:port addr) but defense-in-depth gap.
  Remediation: Apply the same single-quote escaping pattern used for FULLSEND_OUTPUT_FILE.

• [scope-alignment] docs/plans/universal-harness-access-phase4.md:48 — The implementation pivoted from Unix socket to HTTP with bearer token per ADR-0046. The plan docs were updated, but the PR description does not explicitly call out this design pivot.

Info

• [network-exposure] internal/cli/fetchserver.go:43 — Fetch service binds to 0.0.0.0:0 (all interfaces). Required by ADR-0046 for container-to-host connectivity; mitigated by per-run ephemeral 256-bit token.

• [secret-exposure] internal/cli/run.go:1134 — Bearer token written to sandbox .env in plaintext. Ephemeral per-run token with isolated sandbox limits blast radius.

• [test adequacy] internal/cli/fetchskill_test.go — fetchSkillTimeout (120s) is a package-level constant not injectable in tests. No test validates timeout behavior.

• [phased-delivery] internal/cli/run.go:920 — Fetch service uses hardcoded DefaultMaxFetches (10). Configurable max_runtime_fetches harness field deferred to PR 3.

• [naming-consistency] internal/cli/fetchskill.go — CLI subcommand uses fetch-skill (hyphen) while the planned harness field is allow_runtime_fetch (underscore), following respective conventions.

Previous run (4)

Review

Findings

Medium

• [error handling] internal/fetchsvc/service.go:224 — The audit log write was changed from fire-and-forget to error-checked, returning a 500 on failure. However, at this point the skill has already been fetched, cached, and uploaded to the sandbox. Returning an error (a) causes the rate limiter slot to be released via the deferred Release() (since committed stays false), and (b) tells the client the operation failed even though all side effects succeeded. On retry, the skill would be uploaded again. If the audit log write is transiently broken, every fetch attempt will fail despite succeeding.
  Remediation: Either keep the audit log write as best-effort with a logged warning, or set committed = true before the audit check so the rate limiter reflects reality.

• [authorization] internal/cli/run.go:546 — The fetch service is started unconditionally for every agent run, regardless of whether the harness declares allow_runtime_fetch: true. Until PR 3 ships, every agent run exposes an authenticated HTTP service that can fetch and upload skill directories into the sandbox, even for harnesses that do not opt in. A prompt-injected agent could invoke fullsend fetch-skill to pull skills from any URL matching allowed_remote_resources. See also: [incomplete-phase-delivery] finding at this location.
  Remediation: Gate startFetchService behind h.AllowRuntimeFetch now, or at minimum skip injecting FULLSEND_FETCH_URL/FULLSEND_FETCH_TOKEN into the sandbox .env when the harness does not opt in.

• [incomplete-phase-delivery] internal/cli/run.go:546 — The fetch service starts unconditionally in runAgent. The harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). Acknowledged as deliberate interim state. See also: [authorization] finding at this location.

• [stale-architecture-description] docs/plans/universal-harness-access.md:332 — Describes the runtime fetch API as using Unix sockets for communication. PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223 implemented HTTP communication with bearer token authentication (per ADR-0046), not Unix sockets.
  Remediation: Update to reflect HTTP-based communication with FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN environment variables.

• [stale-binary-name] docs/plans/universal-harness-access.md:326 — References fullsend-fetch-skill as a standalone command. PR feat(fetchsvc): in-sandbox fetch-skill subcommand (Phase 4, PR 2) #2223 implemented it as fullsend fetch-skill subcommand.

• [stale-binary-name] docs/plans/universal-harness-access.md:1425 — References implementing fullsend-fetch-skill binary. Implemented as a subcommand instead.

• [stale-binary-name] docs/plans/universal-harness-access-phase1.md:212 — References the old fullsend-fetch-skill binary name and Unix socket transport.

• [stale-binary-name] docs/plans/universal-harness-access-phase2.md:451 — References the old fullsend-fetch-skill binary name and Unix socket transport.

Low

• [error handling] internal/cli/run.go:544 — When resolveToken() fails in the fetch service setup block, the error is silently swallowed. The fetch service starts with runtimeForgeClient = nil, meaning any runtime fetch for an uncached skill will fail with a generic 500 error. No warning is logged.
  Remediation: Log a warning via printer.StepWarn when resolveToken() fails.

• [injection] internal/cli/run.go:1126 — FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN are written into the sandbox .env with single-quote wrapping but without the escape-for-single-quotes pattern (strings.ReplaceAll) used for FULLSEND_OUTPUT_FILE two lines earlier. Safe by construction (hex token, ip:port addr), but a defense-in-depth gap.

• [network-exposure] internal/cli/fetchserver.go:33 — The fetch service binds to 0.0.0.0:0 (all interfaces). Mitigated by per-run ephemeral 256-bit token and dynamic port. Required by ADR-0046 for current sandbox networking model.

• [secret-exposure] internal/cli/run.go:1127 — The bearer token is written to the sandbox .env file in plaintext. Token is ephemeral (per-run) and sandbox is isolated, limiting blast radius.

Info

• [test adequacy] internal/cli/fetchskill_test.go — The prior timeout test was removed but no replacement was added. fetchSkillTimeout (120s) is a package-level constant not injectable in tests.

• [error-formatting-consistency] internal/cli/fetchskill.go — Prior finding resolved: the code now follows the established pattern of returning errors via RunE without writing to stderr directly.

Previous run (5)

Review

Findings

Medium

• [error handling] internal/cli/fetchserver.go:46 — The shutdown function captures the parent ctx (which is context.Background() from runAgent) for server.Shutdown(ctx). Since context.Background() is never cancelled and has no deadline, server.Shutdown will block indefinitely if any in-flight request hangs.
  Remediation: Create a timeout-bounded context inside the shutdown function: ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second); defer cancel(); server.Shutdown(ctx)

• [test adequacy] internal/cli/fetchskill_test.go:183 — TestRunFetchSkill_Timeout does not actually test timeout behavior. The test server sleeps for 200ms while fetchSkillTimeout is 120s. The saved origTimeout is never used to reduce the timeout. The test verifies a 200ms request succeeds, not that the client correctly times out.
  Remediation: Make fetchSkillTimeout injectable (via function parameter or package-level override in tests) and set it shorter than the server sleep, or rename the test to reflect what it actually verifies.

• [error handling] internal/cli/fetchskill.go:77 — If json.NewDecoder(resp.Body).Decode fails (e.g., non-JSON error from a reverse proxy), the HTTP status code is lost. The caller sees "failed to decode response" with no indication whether it was a 403, 502, etc.
  Remediation: Include the status code: fmt.Fprintf(stderr, "failed to decode response (HTTP %d): %v\n", resp.StatusCode, err)

• [error-formatting-consistency] internal/cli/fetchskill.go:80 — Error construction pattern is inconsistent with the rest of the CLI. The codebase favors return fmt.Errorf(...) for command errors, letting cobra handle display. runFetchSkill writes to stderr AND returns errors, causing duplication in contexts where cobra also prints the error.
  Remediation: Remove the fmt.Fprintln(stderr, ...) calls and rely on returned errors, matching the established pattern in postcomment.go, postreview.go, etc.

• [incomplete-phase-delivery] internal/cli/run.go — The fetch service starts unconditionally in runAgent but the harness schema does not yet include allow_runtime_fetch or max_runtime_fetches to gate the feature (deferred to PR 3). This means every agent run starts the fetch service whether or not the harness intends to allow runtime fetches. Consider adding a comment near the initialization clarifying this is an intentional interim state, or gating behind a simple nil check on the config fields.

• [missing-cli-documentation] docs/guides/dev/cli-internals.md:8 — The CLI Command Tree section does not document the new fetch-skill subcommand.
  Remediation: Add fetch-skill to the CLI command tree.

• [missing-environment-variable-documentation] docs/guides/dev/cli-internals.md:312 — The bootstrapEnv() documentation section does not include the new FULLSEND_FETCH_URL and FULLSEND_FETCH_TOKEN environment variables.
  Remediation: Add both variables to the bootstrapEnv() documentation.

Low

• [injection] internal/cli/run.go:1118 — FULLSEND_FETCH_TOKEN is injected into the sandbox .env file without shell quoting. The token is a 64-char hex string (safe by construction), but the pattern diverges from the single-quote-with-escape approach used for FULLSEND_OUTPUT_FILE. Defense-in-depth: apply the same quoting pattern.

• [http-server-configuration] internal/cli/fetchserver.go:37 — The http.Server has no ReadTimeout, WriteTimeout, or IdleTimeout set. Other servers in the codebase (e.g., appsetup.go) configure timeouts. Consider adding standard timeouts for defense-in-depth.

• [naming-consistency] internal/cli/fetchskill.go — ADR-0038 Phase 4 plan refers to a "fullsend-fetch-skill binary" but the implementation is a fetch-skill subcommand of the existing fullsend binary. Consider updating the plan to match the implementation.

• [missing configuration] internal/cli/run.go:549 — MaxFetches and SkillDestDir are not explicitly set in ServiceConfig, relying on zero-value defaults. The defaults are reasonable but explicit values would improve readability.

• [race condition] internal/cli/fetchserver.go:40 — The goroutine uses fmt.Printf for server errors while the main goroutine uses the printer, creating potential for interleaved output. Consider using the printer or a logger.

• [network-exposure] internal/cli/fetchserver.go:33 — The server binds to 0.0.0.0:0 (all interfaces). Per ADR-0046 this is required for the current sandbox networking model, but worth noting for future hardening when loopback becomes available.

• [secret-exposure] internal/cli/run.go:1118 — The bearer token is written to the sandbox .env file in plaintext. Impact is limited since the token is ephemeral (per-run) and scoped to the fetch service lifetime.

[codecov]

Codecov Report

❌ Patch coverage is 76.02740% with 35 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/run.go	42.10%	21 Missing and 1 partial ⚠️
internal/cli/fetchserver.go	82.60%	4 Missing and 4 partials ⚠️
internal/cli/fetchskill.go	95.83%	1 Missing and 1 partial ⚠️
internal/fetchsvc/uploader.go	0.00%	2 Missing ⚠️
internal/fetchsvc/service.go	90.90%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:41 PM UTC · Completed 1:55 PM UTC
Commit: d0508ad · View workflow run →

[fullsend-ai-review]

🤖 Agent run interrupted (process terminated)
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Agent run interrupted (process terminated)
Commit: 4e21a60 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:25 PM UTC · Completed 2:38 PM UTC
Commit: 0815fc2 · View workflow run →

[ralphbean]

LGTM. One minor note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:27 PM UTC · Completed 4:44 PM UTC
Commit: 0476f7f · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:30 PM UTC · Completed 5:44 PM UTC
Commit: 13f7948 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:49 PM UTC · Completed 7:07 PM UTC
Commit: aeb0f1d · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:09 PM UTC · Completed 8:16 PM UTC
Commit: aeb0f1d · View workflow run →

[fullsend-ai-retro]

Retro: PR #2223 — feat(fetchsvc): in-sandbox fetch-skill subcommand

Overall assessment: The workflow completed successfully but with significant token waste from redundant review runs. Review quality was good but missed one finding that a human caught.

Timeline

PR #2223 was a human-authored PR (ggallen) adding the in-sandbox fetch-skill subcommand as Phase 4, PR 2 of ADR-0038. It changed 15 files with +1012/-26 lines. The PR was created at 12:54 UTC and merged at 20:04 UTC (~7 hours).

Key findings

1. 8 review agent runs, 2 interrupted — The review agent ran on 6 different SHAs as the author iteratively pushed changes. Two runs on commit 4e21a60 were both interrupted (process terminated), suggesting a concurrency/dedup gap. Each run reviewed a 1000+ line diff, resulting in high cumulative token cost.

2. Missed unrelated change — The review agent did not flag that changes to internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md were unrelated to the fetchsvc feature. Human reviewer ralphbean caught this and suggested it belong in its own commit. The author reverted it.

3. Good finding calibration — The review agent's findings were relevant: a medium finding about the phased delivery gap (fetch service gating), low findings about error handling and authorization, and info findings about test coverage and network exposure. All were well-contextualized.

Proposals

No new proposals filed — all identified improvement opportunities are already tracked by existing open issues:

• Review deduplication/debouncing: Debounce review dispatch on rapid synchronize events #1014, Add concurrency group to dispatch-review job in fullsend.yaml #981, Deduplicate review runs when PR is rebased multiple times in quick succession #1422, Deduplicate review runs on rapid successive pushes #1418, Reduce wasted review runs from stale-head races #1331, Cancel-and-skip redundant re-reviews on force-pushed PRs with no prior findings #1372, Reduce unnecessary re-review runs on large PRs #1390
• Unrelated change detection: Escalate scope-tier-mismatch to CHANGES_REQUESTED when out-of-scope changes dominate #2170 (scope-tier-mismatch escalation), Review agent: detect and flag scope expansion after prior approval #925 (scope expansion detection)
• Prior findings carryover: Pass prior review findings to follow-up reviews for targeted verification #1552 (pass prior findings to follow-up reviews), Review comment should state whether findings changed since prior review #1155 (state whether findings changed)

Prioritizing #981 (concurrency groups) and #1014 (debounce) would have the highest impact on this class of workflow — they would have prevented the two interrupted runs and reduced the 8 total runs to ~3-4.