feat(fetchsvc): runner-side runtime fetch service (Phase 4, PR 1)

[ggallen]

Summary

• Adds internal/fetchsvc/ package implementing the runner-side fetch service for ADR-0038 Phase 4 (runtime dependency loading)
• Handles runtime skill fetch requests: URL validation, allowed_remote_resources allowlist enforcement, per-run rate limiting, forge API fetch, tree hash integrity verification, content-addressed caching, sandbox upload, and JSONL audit logging with fetch_type: "runtime"
• Transport-agnostic design: exposes HandleFetch(ctx, req) method and http.Handler; the actual transport (Unix socket, HTTP, exec-based) will be wired in Phase 4 PR 3

Context

Phase 4 of ADR-0038 adds runtime dependency loading — agents can discover and fetch additional skills mid-execution. This is split into 3 PRs:

1. This PR: Runner-side fetch service (core logic, rate limiting, tests)
2. PR 2: In-sandbox fullsend-fetch-skill binary
3. PR 3: Harness schema fields (allow_runtime_fetch, max_runtime_fetches) and transport wiring

This PR introduces no new callers — it's a standalone package with no risk to existing behavior.

Test plan

• 22 unit tests covering all code paths: cache hits, forge fetches, allowlist enforcement, missing/mismatched hashes, rate limiting, non-forge URL rejection, offline mode, HTTP handler status codes (200/400/403/429/500), upload verification, audit log content
• go test ./... — all existing tests pass (no regressions)
• go vet ./internal/fetchsvc/ — clean
• gofmt — clean
• make lint — all hooks pass

🤖 Generated with Claude Code

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[github-actions]

Site preview

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

Commit: df3228995c63345ce0590ca980aaa068204c883a

[fullsend-ai-review]

🤖 Review · Started 2:38 PM UTC
Commit: eeb3b92 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 82.35294% with 27 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/fetchsvc/service.go	80.91%	13 Missing and 12 partials ⚠️
internal/fetchsvc/ratelimit.go	90.90%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Review · Started 2:43 PM UTC
Commit: de33a4d · View workflow run →

[ggallen]

/ok-to-test

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:52 PM UTC · Completed 3:04 PM UTC
Commit: fb03fbe · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:57 PM UTC · Completed 3:04 PM UTC
Commit: fb03fbe · View workflow run →

[ggallen]

/ok-to-test

[fullsend-ai-review]

Review

Findings

Low

• [error-handling-gap] internal/fetchsvc/service.go:189 — Audit log write failures silently discarded via _ = fetch.AppendFetchAudit(...). Inconsistent with internal/resolve/resolve.go (lines 238-248, 372-382) which wraps the same call in error handling and returns fmt.Errorf("writing fetch audit log: %w", err).
  Remediation: Either propagate the error to match the resolve.go pattern, or document why runtime fetches intentionally tolerate audit log failures.

• [error-handling] internal/fetchsvc/service.go:211 — When json.NewDecoder(r.Body).Decode(&req) fails due to http.MaxBytesReader exceeding the 1 MB limit, the error is reported as generic 400 rather than 413 (Request Entity Too Large). Go's MaxBytesReader returns a *http.MaxBytesError which can be detected with errors.As.
  Remediation: Detect MaxBytesError and return 413.

• [naming-coherence] internal/fetchsvc/ — Package name fetchsvc uses a svc suffix not found in any of the 26 existing internal packages (all use plain nouns). This diverges from the established convention. See also: [architectural-placement] at this location.
  Remediation: Consider renaming to align with conventions, e.g., internal/runtimefetch or integrating into internal/fetch/.

• [missing-documentation] docs/plans/universal-harness-access-phase4.md:47 — Phase 4 implementation plan describes PR 1 at a high level but does not reference the concrete package name (internal/fetchsvc/) or key types now that implementation exists.
  Remediation: Update the plan to reference the implementation package and key types.

Info

• [fail-open-evaluation] internal/fetchsvc/service.go — All gates fail-closed: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), missing integrity hash rejected, non-forge URLs rejected, nil forge client rejected, offline mode prevents network fetches.

• [input-validation] internal/fetchsvc/service.go — All user-controlled inputs verified: req.URL validated via IsURL (HTTPS-only), ParseIntegrityHash (64-char hex), MatchingAllowedPrefix, ParseForgeURL (supported forge host only). Request body bounded by MaxBytesReader at 1 MB.

• [integrity-verification] internal/fetchsvc/service.go:155 — Tree hash integrity check works correctly. ComputeTreeHash compared against expected hash; cache hits keyed by the same hash.

• [edge-case] internal/fetchsvc/service.go:148 — ListDirectoryContents with recursive: true has no upper bound on entries at the fetchsvc level. The underlying forge client enforces depth/call limits, and the integrity hash pins the exact file set.

• [logic-error] internal/fetchsvc/service.go:172 — After CachePutDir returns the hash, the code discards it and calls CachePath(workspaceRoot, expectedHash) to reconstruct the tree path. Not a bug (integrity check guarantees hash equality), but redundant.

• [scope-boundary-enforcement] internal/fetchsvc/ — The PR correctly implements only the scope defined in Phase 4 PR 1. No premature sandbox integration logic or harness schema changes.

Previous run

Review

Findings

Low

• [error-handling-gap] internal/fetchsvc/service.go:189 — Audit log write failures silently discarded via _ = fetch.AppendFetchAudit(...). Inconsistent with internal/resolve/resolve.go (lines 238 and 372) which wraps the same call in error handling and returns the error to the caller.
  Remediation: Change _ = fetch.AppendFetchAudit(...) to check and handle the error, matching the pattern in resolve.go.

• [error-handling] internal/fetchsvc/service.go:211 — When json.NewDecoder(r.Body).Decode(&req) fails due to http.MaxBytesReader exceeding the 1 MB limit, the error is reported as generic 400 rather than 413 (Request Entity Too Large). Go's MaxBytesReader returns a *http.MaxBytesError which can be detected with errors.As.
  Remediation: Detect MaxBytesError and return 413.

Info

• [fail-open-evaluation] internal/fetchsvc/service.go — All gates fail-closed: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), missing integrity hash rejected, non-forge URLs rejected, nil forge client rejected, offline mode prevents network fetches.

• [input-validation] internal/fetchsvc/service.go — All user-controlled inputs verified: req.URL validated via IsURL (HTTPS-only), ParseIntegrityHash (64-char hex), MatchingAllowedPrefix, ParseForgeURL (supported forge host only). Request body bounded by MaxBytesReader at 1 MB.

• [integrity-verification] internal/fetchsvc/service.go:155 — Tree hash integrity check works correctly. ComputeTreeHash compared against expected hash; cache hits keyed by the same hash.

• [edge-case] internal/fetchsvc/service.go:148 — ListDirectoryContents with recursive: true has no upper bound on entries or aggregate download size. The integrity hash pins the exact file set, limiting the attack surface to scenarios where the harness author pre-approved both the URL prefix and the specific tree hash.

• [logic-error] internal/fetchsvc/service.go:172 — After CachePutDir returns the hash, the code discards it and calls CachePath(workspaceRoot, expectedHash) to reconstruct the tree path. Not a bug (integrity check guarantees hash equality), but redundant.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses svc suffix, diverging from the plain-noun convention used by 26+ existing internal packages.

• [scope-boundary-enforcement] internal/fetchsvc/ — The PR correctly implements only the scope defined in Phase 4 PR 1. No premature sandbox integration logic or harness schema changes.

Previous run (2)

Review

Findings

Low

• [logic-error] internal/fetchsvc/service.go:151 — The if forgeInfo.Path == "" branch inside the fetch loop is unreachable because the guard at line 119 already rejects empty paths. Dead code that could mislead a future maintainer into thinking this branch handles the empty-path case.
  Remediation: Remove the dead branch or add a comment explaining it is defense-in-depth.

• [error-handling-gap] internal/fetchsvc/service.go:189 — Audit log write failures silently discarded via _ = fetch.AppendFetchAudit(...). Inconsistent with internal/resolve/resolve.go (lines 238–248, 372–382) which wraps the same call in error handling and returns fmt.Errorf("writing fetch audit log: %w", err). See also: [error-handling] finding at line 211.
  Remediation: Either log the error or return it, matching the pattern in resolve.go.

• [error-handling] internal/fetchsvc/service.go:211 — When json.NewDecoder(r.Body).Decode(&req) fails due to http.MaxBytesReader exceeding the 1 MB limit, the error is reported as generic 400 rather than 413 (Request Entity Too Large). Go's MaxBytesReader returns a *http.MaxBytesError which can be detected with errors.As. See also: [error-handling-gap] finding at line 189.
  Remediation: Detect MaxBytesReader size limit errors and return 413.

• [edge-case] internal/fetchsvc/service.go:148 — ListDirectoryContents with recursive: true has no upper bound on the number of files fetched or the aggregate size downloaded via GetFileContentAtRef. A repository directory with many files could cause excessive memory usage and network traffic. Exploitation requires the URL to be in the allowlist and have a valid integrity hash, which limits the attack surface.
  Remediation: Add an upper bound on the number of files or total size fetched.

Info

• [fail-open-evaluation] internal/fetchsvc/service.go — All gates fail-closed: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), missing integrity hash rejected, non-forge URLs rejected, nil forge client rejected, offline mode prevents network fetches.

• [input-validation] internal/fetchsvc/service.go — All user-controlled inputs verified: req.URL validated via IsURL (HTTPS-only), ParseIntegrityHash (64-char hex), MatchingAllowedPrefix, ParseForgeURL (supported forge host only). Request body bounded by MaxBytesReader at 1 MB.

• [integrity-verification] internal/fetchsvc/service.go:155 — Tree hash integrity check works correctly. ComputeTreeHash compared against expected hash; cache hits keyed by the same hash.

• [edge-case] internal/fetchsvc/ratelimit.go:25 — The overflow guard if max > math.MaxInt32 is dead code on 32-bit platforms. Benign; the default value of 10 is well within range.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses svc suffix, diverging from the plain-noun convention used by 26+ existing internal packages. The suffix disambiguates from internal/fetch/.

• [documentation-format] internal/fetchsvc/service.go:58 — ServiceConfig.SkillDestDir has no inline annotation about its default value (/sandbox/claude-config/skills), unlike MaxFetches which documents its default.

• [code-organization] internal/fetchsvc/service.go:23 — fetchError type correctly eliminates string-based error classification. Consider exposing sentinel errors for errors.Is() matching as the API matures.

• [naming-consistency] internal/fetchsvc/service.go — The type implements both HandleFetch (direct call) and ServeHTTP (HTTP handler). Existing codebase names HTTP handlers Handler (internal/mintcore/handler.go), but Service is reasonable as the broader abstraction.

• [phase-coherence] internal/fetchsvc/ — The PR correctly implements only the scope defined in Phase 4 PR 1. No premature sandbox integration logic or harness schema changes.

Previous run (3)

Review

Findings

Medium

• [logic-error] internal/fetchsvc/service.go:162 — When forgeInfo.Path is empty (root-level repo URL like https://github.com/org/repo/tree/abc123), ListDirectoryContents is called with an empty path and recursive: true. This could return the entire repository tree, causing the service to download every file in the repository. There is no guard against an empty path.
  Remediation: Validate that forgeInfo.Path is non-empty before proceeding with the fetch. A root-level repository URL is unlikely to be a valid skill directory.

Low

• [missing-authorization] internal/fetchsvc/ — No linked GitHub issue for this non-trivial PR (4000+ lines, PR 1 of 3 in a phased rollout). The work is clearly authorized by ADR-0038, but a tracking issue would improve traceability across the multi-PR rollout.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses svc suffix, inconsistent with 26+ existing internal packages that all use plain nouns (fetch, forge, harness, resolve, sandbox, etc.).

• [error-handling-gap] internal/fetchsvc/service.go:219 — Audit log write failures silently discarded via _ = fetch.AppendFetchAudit(...). Inconsistent with resolve.go (lines 238, 372) which wraps the same call in error handling. See also: [error-handling] finding at line 244.

• [error-handling] internal/fetchsvc/service.go:244 — When json.NewDecoder(r.Body).Decode(&req) fails due to http.MaxBytesReader exceeding the 1 MB limit, the error is reported as generic 400 rather than 413 (Request Entity Too Large). See also: [error-handling-gap] finding at line 219.

Info

• [fail-open-evaluation] internal/fetchsvc/service.go — All gates fail-closed: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), missing integrity hash rejected, non-forge URLs rejected, nil forge client rejected, offline mode prevents network fetches.

• [input-validation] internal/fetchsvc/service.go — All user-controlled inputs verified: req.URL validated via IsURL (HTTPS-only), ParseIntegrityHash (64-char hex), MatchingAllowedPrefix, ParseForgeURL (supported forge host only). Request body bounded by MaxBytesReader at 1 MB.

• [edge-case] internal/fetchsvc/ratelimit.go:25 — The overflow guard if max > math.MaxInt32 is dead code on 32-bit platforms. Benign; the default value of 10 is well within range.

• [documentation-format] internal/fetchsvc/service.go:58 — ServiceConfig.SkillDestDir has no annotation about its default value (/sandbox/claude-config/skills), unlike MaxFetches which documents its default inline.

• [code-organization] internal/fetchsvc/service.go:23 — fetchError type correctly eliminates string-based error classification. Consider exposing sentinel errors for errors.Is() matching as the API matures.

• [phase-coherence] internal/fetchsvc/ — The PR correctly implements only the scope defined in Phase 4 PR 1. Does not prematurely implement Unix socket listener, in-sandbox binary, or harness schema extensions (deferred to PRs 2 and 3).

Previous run (4)

Review

Findings

Low

• [error-handling] internal/fetchsvc/service.go:244 — In ServeHTTP, when json.NewDecoder(r.Body).Decode(&req) fails due to http.MaxBytesReader exceeding the 1 MB limit, the error is reported as generic "invalid request body" (400) rather than 413 (Request Entity Too Large). The caller cannot distinguish a malformed JSON body from an oversized one.

• [error-handling-gap] internal/fetchsvc/service.go:219 — Audit log write failures silently discarded via _ = fetch.AppendFetchAudit(...). Inconsistent with the pattern in resolve.go (lines 246–247, 380–381) which wraps the same call in error handling and returns fmt.Errorf("writing fetch audit log: %w", err). If audit logging is a compliance requirement, this inconsistency creates a gap where runtime fetch audit failures go undetected.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses svc suffix, inconsistent with 26+ existing internal packages using plain nouns (fetch, forge, harness, resolve, sandbox, etc.). No other internal package uses a svc or service suffix.

Info

• [code-organization] internal/fetchsvc/service.go:23 — fetchError type correctly eliminates string-based error classification for production status mapping in ServeHTTP. Tests use strings.Contains(err.Error(), ...) for HandleFetch error verification, which is standard Go test practice. Consider exposing sentinel errors for errors.Is() matching as the API matures.

• [documentation-format] internal/fetchsvc/service.go:56 — ServiceConfig.SkillDestDir has no annotation about its default value (/sandbox/claude-config/skills, set in New() at line 79), unlike MaxFetches which documents its default inline.

Previous run (5)

Review

Findings

High

• [logic-error] .github/workflows/reusable-dispatch.yml:127 — Removing tr -d '\r' reverts a known bugfix from PR fix(dispatch): strip CRLF \r from comment body before command matching #2168 for issue Slash commands silently fail when comment body has multiple lines (CRLF \r not stripped) #2137. GitHub comment bodies can contain CRLF line endings, and without the \r strip, awk '{print $1}' returns /fullsend\r instead of /fullsend, causing the case match and [[ "${COMMAND}" == "/fullsend" ]] comparison to silently fail. The same regression applies to the SECOND_WORD extraction at line 155, where retro\r would not match retro. This breaks all slash command routing for comments submitted from Windows clients or certain API integrations.
  Remediation: Restore tr -d '\r' in both COMMAND and SECOND_WORD extraction pipelines.

• [logic-error] internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml:82 — Same tr -d '\r' removal as above. This is the scaffold template that generates per-repo dispatch workflows, so the regression will propagate to all newly scaffolded repositories. Both the COMMAND extraction (line 82) and the SECOND_WORD extraction (line 110) are affected.
  Remediation: Restore tr -d '\r' in both pipelines in the scaffold template.

• [protected-path] .github/workflows/reusable-dispatch.yml — This PR modifies a file under .github/, which is a protected path requiring human approval. The PR has no linked issue and does not explain why the dispatch workflow is being modified alongside the fetchsvc feature. Human review is required for all protected-path changes.

Medium

• [incorrect-change] .gitlint:9 — The comment change from COMMITS.md to CONTRIBUTING.md is factually incorrect. COMMITS.md (lines 16–27) contains the canonical list of conventional commit types (feat, fix, refactor, docs, test, chore, ci, perf, build) that .gitlint validates. CONTRIBUTING.md merely references COMMITS.md at line 7: "See COMMITS.md for the full specification." The original comment accurately described where the types are documented.
  Remediation: Revert the .gitlint comment to reference COMMITS.md, which is the source of truth for the commit types list.

Low

• [error-handling-gap] internal/fetchsvc/service.go:218 — Audit log write failures are silently discarded via _ = fetch.AppendFetchAudit(...). Unlike the existing pattern in resolve.go (lines 238–248), which wraps the same call in error handling and returns fmt.Errorf("writing fetch audit log: %w", err), the fetchsvc code swallows the error. If audit logging is a compliance requirement, this inconsistency should be deliberate.

• [scope-alignment] .github/workflows/reusable-dispatch.yml — PR bundles three unrelated changes: (1) new fetchsvc package for ADR-0038 Phase 4, (2) removal of tr -d '\r' from dispatch workflows (which reverts a bugfix), (3) .gitlint comment change. Bundling unrelated changes makes the PR harder to review and bisect.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses the svc suffix, which is inconsistent with the 25+ existing internal packages that all use plain descriptive nouns (fetch, forge, harness, mint, resolve, sandbox, etc.).

• [code-organization] internal/fetchsvc/service.go:262 — The fetchError doc comment states it "eliminat[es] string-based error classification for status mapping," but test code extensively uses strings.Contains(err.Error(), ...) for error verification. Consider exposing sentinel errors for errors.Is() matching.

• [documentation-format] internal/fetchsvc/service.go:290 — ServiceConfig doc comment is minimal. The SkillDestDir field has no annotation about its default value (/sandbox/claude-config/skills), unlike MaxFetches which documents its default inline.

• [edge-case] internal/fetchsvc/ratelimit.go:17 — NewRateLimiter now includes a max > math.MaxInt32 guard before the int32 conversion, resolving the prior finding about silent truncation.

Info

• [fail-closed-analysis] internal/fetchsvc/service.go:100 — Fail-closed verification passes. All inputs fail closed: empty URL, non-HTTPS, missing hash, empty allowlist, unsupported forge, rate limiter default (10, not unlimited), offline mode, nil forge client, tree hash integrity.

• [rate-limit-placement] internal/fetchsvc/service.go:115 — Rate-limit slot is released on all failure paths via a deferred function guarded by a committed flag. Slot is only permanently consumed on success.

• [input-validation] internal/fetchsvc/service.go:232 — HTTP handler applies MaxBytesReader (1 MB), enforces POST-only, and uses typed fetchError for status mapping with fallback to 500 for unexpected errors.

• [information-disclosure] internal/fetchsvc/service.go:115 — Error messages are generic and do not leak internal configuration values. Allowlist rejection includes the URL but not the allowlist entries themselves.

• [authorization] internal/fetchsvc/service.go — No linked issue for this PR. The work is clearly authorized by ADR-0038 Phase 4, but a tracking issue would improve traceability for this multi-PR feature rollout.

Previous run (6)

Review

Findings

Low

• [missing-authorization] internal/fetchsvc/ — No linked issue for this PR. This is a non-trivial change (~1000 lines) implementing Phase 4 of ADR-0038. The work is clearly authorized by ADR-0038 and the PR body describes a coherent 3-PR plan, but a tracking issue would improve traceability.

• [edge-case] internal/fetchsvc/ratelimit.go:17 — NewRateLimiter accepts int but stores as int32. On 64-bit systems, values exceeding math.MaxInt32 would silently truncate. Unlikely in practice (default is 10), but the narrowing conversion is unchecked.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses the svc suffix, which is not used by any of the 26 existing internal/ packages (all use plain nouns: fetch, forge, harness, mint, etc.). Minor naming inconsistency; trivial to rename later if a convention is established.

Info

• [fail-closed-analysis] internal/fetchsvc/service.go:100 — Fail-closed verification passes. All inputs fail closed: empty URL, non-HTTPS, missing hash, empty allowlist, unsupported forge, rate limiter default (10, not unlimited), offline mode, nil forge client, tree hash integrity.

• [rate-limit-placement] internal/fetchsvc/service.go:115 — Rate-limit slot is released on all failure paths via a deferred function guarded by a committed flag. Slot is only permanently consumed on success.

• [information-disclosure] internal/fetchsvc/service.go:115 — Error messages are generic and do not leak internal configuration values. Rate-limit error returns generic message; integrity error returns generic message.

• [audit-log-suppression] internal/fetchsvc/service.go:218 — Audit log write failures are silently ignored (_ = fetch.AppendFetchAudit(...)). Acceptable in agent sandbox context since the fetch is already authorized at this point.

• [input-validation] internal/fetchsvc/service.go:232 — HTTP handler applies MaxBytesReader (1 MB), enforces POST-only, and uses typed fetchError for status mapping with fallback to 500 for unexpected errors.

Previous run (7)

Review

Findings

Medium

• [edge-case] internal/fetchsvc/service.go:147 — When forgeClient.ListDirectoryContents returns entries that are all non-file types (e.g., only directories), the files map is empty. ComputeTreeHash on an empty map produces a hash that won't match expectedHash, resulting in the misleading error "integrity check failed". A clearer error for len(files)==0 ("skill directory contains no files") would aid debugging.
  Remediation: Add an explicit check for len(files)==0 after the loop at ~line 155 and return a descriptive error before computing the tree hash.

Low

• [counter-underflow] internal/fetchsvc/ratelimit.go:37 — Release() unconditionally decrements the counter via atomic.Add(-1) with no floor check. If called without a prior successful Allow(), the counter goes negative, allowing more than max concurrent fetches. The current call site in service.go uses a committed guard that prevents double-release, but the public API has no protection against misuse. See also: [rate-limiter-bypass] info finding at this location.

• [test-adequacy] internal/fetchsvc/ratelimit_test.go — No direct test coverage for Release(). The test file does not verify that releasing a slot allows a subsequent Allow() to succeed, or that double-releasing can produce counter underflow. TestHandleFetch_RateLimitRollbackOnFailure in service_test.go exercises Release indirectly, but the unit-level gap remains.

• [error-handling] internal/fetchsvc/service.go:258 — The writeJSON helper uses json.Marshal + w.Write, which is correct and avoids the partial-write issue from the prior review. No action needed.

• [architectural-alignment] internal/fetchsvc/ — Package name fetchsvc uses the svc suffix, which is not used by any of the 26 existing internal/ packages (all use plain nouns: fetch, forge, harness, mint, etc.). Minor naming inconsistency; trivial to rename later if a convention is established.

Info

• [rate-limit-placement] internal/fetchsvc/service.go:115 — Prior finding resolved. The rate-limit slot consumed by Allow() is now released on all failure paths via a deferred function guarded by a committed flag. The slot is only permanently consumed on the success path.

• [fail-closed-analysis] internal/fetchsvc/service.go:100 — Fail-closed verification passes. All inputs fail closed: empty URL, non-HTTPS, missing hash, empty allowlist, unsupported forge, rate limiter default (10, not unlimited), offline mode, nil forge client, tree hash integrity.

• [information-disclosure] internal/fetchsvc/service.go:115 — Prior finding resolved. Error messages no longer expose the configured max-fetches value or hash values. Rate-limit error returns generic "runtime fetch rate limit exceeded"; integrity error returns generic "integrity check failed".

• [audit-log-suppression] internal/fetchsvc/service.go:184 — Audit log write failures are silently ignored (_ = fetch.AppendFetchAudit(...)). Acceptable in agent sandbox context since the fetch is already authorized at this point.

Previous run (8)

Review

Findings

Medium

• [rate-limit-placement] internal/fetchsvc/service.go:115 — Rate-limit slot is consumed via s.limiter.Allow() before cache lookup, forge network fetch, integrity verification, cache write, and sandbox upload. If any subsequent step fails (transient network error, integrity mismatch, disk full, upload failure), the slot is permanently consumed with no rollback. Over the course of a run, transient failures could exhaust the per-run budget (default 10) while delivering zero successful fetches.
  Remediation: Either move s.limiter.Allow() to just before the success return (after upload and audit succeed), or add a decrement/release method on RateLimiter and call it on every error path after Allow().

Low

• [information-disclosure] internal/fetchsvc/service.go:115 — The rate-limit error exposes the configured max-fetches value, and the integrity mismatch error exposes both expected and actual SHA-256 hashes. The caller is an in-sandbox agent process so the blast radius is limited, but these details could aid exploitation if the sandbox is compromised.

• [error-handling] internal/fetchsvc/service.go:246 — In writeJSON, if json.NewEncoder(w).Encode(v) fails after w.WriteHeader(status) has been called, the fallback fmt.Fprintf appends raw text to a partially-written JSON body, producing malformed JSON for the client. Consider marshaling to a []byte buffer before writing the header.

Info

• [fail-closed-analysis] internal/fetchsvc/service.go:100 — Fail-closed verification passes: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), empty URL and missing integrity hash are rejected, ParseForgeURL rejects unrecognized forge hosts, offline mode prevents network fetches.

• [scope-coherence] internal/fetchsvc/ — Package name fetchsvc does not align with existing naming patterns (no other internal/ packages use svc or service suffixes). A team convention discussion may be warranted before this pattern proliferates.

• [dependency-coherence] internal/fetchsvc/service.go — The service reuses existing types from internal/harness, internal/fetch, and internal/forge. Reuse is architecturally sound per ADR-0038.

Previous run (9)

Review

Findings

Medium

• [rate-limit-placement] internal/fetchsvc/service.go:105 — Rate limit slot is consumed via s.limiter.Allow() before forge URL parsing, cache lookup, network fetch, integrity verification, upload, and audit log write. If any subsequent step fails, the slot is permanently consumed with no rollback. With a default limit of 10, transient failures (e.g., forge API timeouts) can exhaust the per-run fetch budget without any successful fetches, locking the agent out of further skill loading for the remainder of the run.
  Remediation: Move s.limiter.Allow() to just before the success return (after upload and audit), or add a decrement/rollback on error paths after the Allow() call.

• [missing-body-limit] internal/fetchsvc/service.go:220 — ServeHTTP passes r.Body directly to json.NewDecoder without limiting the request body size. An attacker with access to the endpoint can send an arbitrarily large request body, causing excessive memory allocation. Defense-in-depth applies even though the endpoint is a Unix socket for in-sandbox agents.
  Remediation: Wrap r.Body with http.MaxBytesReader before decoding: r.Body = http.MaxBytesReader(w, r.Body, maxRequestBytes) where maxRequestBytes is a reasonable limit (e.g., 1 MB).

Low

• [missing-authorization] No linked GitHub issue. The work is clearly authorized by ADR-0038 and the Phase 4 plan, but lacks traceability to a specific tracking issue.

• [scope-coherence] internal/fetchsvc/service.go — Package name fetchsvc does not align with existing naming patterns. No other internal/ package uses a svc suffix. The name disambiguates from internal/fetch, but alternatives like internal/fetch/runtime/ or internal/runtimefetch/ would align better with existing conventions.

• [mutex-scope] internal/fetchsvc/service.go:91 — HandleFetch holds s.mu for its entire execution including network I/O (forge API calls). This serializes all fetch requests for the duration of a network round-trip. The atomic CAS-based RateLimiter is never exercised under contention because the mutex serializes all callers before they reach Allow().

• [information-disclosure] internal/fetchsvc/service.go:115 — Multiple error responses include internal details such as cache paths and upstream error messages returned as JSON to the in-sandbox caller. If the sandbox is compromised, host-side paths could aid exploitation.

• [error-handling-pattern] internal/fetchsvc/service.go — HandleFetch returns application errors in FetchResponse.Error instead of using Go's error return pattern. This conflates business logic with transport concerns and makes classifyError (string matching on error messages) fragile. See also: [http-status-classification] finding at this location.

• [http-status-classification] internal/fetchsvc/service.go:238 — classifyError uses strings.Contains on error messages to determine HTTP status codes. If error message text changes without updating classifyError, status code mapping silently breaks. See also: [error-handling-pattern] finding at this location.

• [api-shape-consistency] internal/fetchsvc/service.go:236 — writeJSON helper ignores json.Encode error with //nolint:errcheck. While recovery is limited after headers are written, explicit handling would match patterns in internal/mintcore/handler.go.

• [package-documentation] internal/fetchsvc/service.go — Package documentation is missing. Other internal packages have substantive // Package ... doc comments explaining the package's role and key types.

Info

• [fail-closed-analysis] internal/fetchsvc/service.go:100 — Fail-closed verification passes: empty allowlist rejects all requests, rate limiter defaults to 10 (not unlimited), no wildcard handling found.

• [dependency-coherence] internal/fetchsvc/service.go — The service imports existing types from internal/harness (IsURL, ParseIntegrityHash, MatchingAllowedPrefix). The harness schema extensions (allow_runtime_fetch, max_runtime_fetches) are deferred to PR 3.

[fullsend-ai-retro]

Retro: PR #2173 — feat(fetchsvc): runner-side runtime fetch service

This is a human-authored PR by ggallen adding a new internal/fetchsvc/ package (885 lines across 4 files). The PR was force-pushed twice and then closed/reopened, creating a cascade of wasted agent compute:

Pattern	Waste	Details
Force push cancellations	~13 min agent time	2 review runs cancelled after 4.5 and 8.5 min respectively
Duplicate review runs on same SHA	3 concurrent runs	Runs 27355485271, 27355818104, 27355898627 all reviewing commit fb03fbe
Premature retro dispatch	1 retro run	Retro triggered by close event, ran despite PR being reopened 1:44 later

All major patterns are already covered by existing open issues. No new proposals are warranted:

• Debounce/dedup on 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, Deduplicate review dispatches for the same HEAD SHA #1452
• Concurrency groups / cancel-in-progress: Add concurrency group to dispatch-review job in fullsend.yaml #981, Enable cancel-in-progress for review dispatches on the same PR #1357
• SHA-level dedup: Deduplicate review dispatches for the same HEAD SHA #1452
• Skip retro on PRs with no agent results: Skip retro dispatch for unmerged PRs with no agent interaction #1411, Skip retro dispatch on PRs with no agent involvement #939
• Skip retro on close-without-merge with successor: Skip retro dispatch for closed-without-merge PRs with immediate successor #2176
• Cancel-in-progress already implemented (Cancel in-flight review runs when PR HEAD advances #887, Cancel in-progress reviews when PR is updated #1365 closed) but the close/reopen cycle creates a new PR event context that bypasses it

The one subtle gap — retro triggered by close event proceeding even after the PR is reopened (because the 60s debounce completes before the reopen) — is substantially covered by #1411 (skip retro when no completed agent interaction). If #1411 were implemented, this retro run would have been skipped since no review had posted results yet.

Code quality note: The PR itself looks well-structured with good security properties (integrity verification, allowlist enforcement). Codecov reports 81% patch coverage with ~25 uncovered error-handling paths in service.go. No blocking issues observed in the code.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:21 PM UTC · Completed 3:33 PM UTC
Commit: 8b6ab45 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:02 PM UTC · Completed 7:14 PM UTC
Commit: 1536a18 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:26 PM UTC · Completed 7:37 PM UTC
Commit: 5e96514 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:35 PM UTC · Completed 8:47 PM UTC
Commit: df32289 · View workflow run →

[ralphbean]

LGTM. One minor note inline.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:47 AM UTC · Completed 11:59 AM UTC
Commit: df32289 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2173 — feat(fetchsvc): runner-side runtime fetch service

Human-authored PR by ggallen adding internal/fetchsvc/ (1002 lines, 4 files) for ADR-0038 Phase 4. The PR went through 10 review runs over ~6 hours before merging.

What went well

• Review agent caught real bugs: The CRLF regression from accidentally included dispatch workflow files (high severity) was correctly flagged and led to their removal. The empty-path guard issue (medium) was also a genuine bug fix.
• Security posture validated: Every review confirmed fail-closed behavior, proper input validation, and integrity verification.
• Human reviewers approved efficiently: ralphbean left one thoughtful inline comment about rate-limit ordering; rh-hemartin approved after review.

What could go better

• Repeated findings across 10 runs: The audit log error handling finding was raised in all 10 review runs despite the author dismissing it with justification ("best-effort in sandbox context") four separate times. The MaxBytesReader 400→413 finding was similarly repeated ~7 times.
• Token waste: Runs 5–10 mostly re-flagged findings already addressed or explicitly dismissed, representing ~60% of total review compute with minimal incremental value.
• Author eventually capitulated: The author changed the audit log handling and MaxBytesReader behavior in the final commit, possibly to stop the repeated flagging rather than genuine conviction — a subtle form of review pressure that may not always produce the right outcome.

Existing issue coverage

All patterns observed are covered by existing open issues. No new proposals warranted.

Pattern	Existing Issues
Re-raising dismissed findings	#1672, #1583
Duplicate findings across iterations	#1013, #1285, #1500
Debounce rapid pushes	#1014, #1422, #1418
New low-severity findings surfacing on approved passes	#1367, #1582
Cancel-in-progress for reviews	#1357, #981

A previous retro (run at 15:04 UTC, before most review iterations completed) reached similar conclusions. This retro confirms those findings with the full interaction history.