feat: add content-addressed cache for remote resources

[ggallen]

Summary

• Adds content-addressed cache to internal/fetch/ for storing remote resources by SHA256 hash
• CachePut uses atomic writes (temp file + os.Rename) with restrictive permissions (0o700 dirs, 0o600 files)
• CacheGet re-verifies SHA256 integrity on every read to detect corruption
• Partial cache entries (crash during write) are treated as misses
• Part of ADR-0038 Phase 1 implementation (PR 3 in the implementation plan)

Test plan

• go test ./internal/fetch/ — all 16 tests pass
• go vet ./internal/fetch/ — clean
• Round-trip, cache miss, partial entry, integrity failure, dedup, path format tests

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://750d8332-site.fullsend-ai.workers.dev

Commit: 045aa12c0c4dd87a2061fce9b36525daa6032b73

[fullsend-ai-review]

Review

Findings

Info

• [documentation-currency] docs/plans/universal-harness-access-implementation.md:69 — Plan documents CachePath(workspaceRoot, hash) string but the implementation returns (string, error) due to hash validation. The signature change is an improvement (defense-in-depth against path traversal) but makes the plan doc stale.
  Remediation: Update the plan doc signature to CachePath(workspaceRoot, hash) (string, error).

Previous run

Review

Findings

Info

• [documentation-currency] docs/plans/universal-harness-access-implementation.md:69 — Plan documents CachePath(workspaceRoot, hash) string but the implementation returns (string, error) due to hash validation. The signature change is an improvement but makes the plan doc stale.
  Remediation: Update the plan doc signature to CachePath(workspaceRoot, hash) (string, error).

Previous run (2)

Review

Findings

Low

• [correctness] internal/fetch/cache.go:159 — atomicWrite does not clean up the temp file if os.Rename fails. All earlier error paths call os.Remove(tmpName), but a rename failure (e.g., cross-device rename, permissions) leaves an orphaned temp file in the cache directory.
  Remediation: Add defer-based cleanup or an explicit os.Remove(tmpName) on rename failure, consistent with the other error paths.

• [correctness] internal/fetch/cache_test.go — The implementation plan lists "concurrent writes" as a planned test case, but only sequential dedup is tested (TestCacheSameContentDedup). The atomic-rename approach is likely safe, but the planned concurrency test is missing.
  Remediation: Add a test that runs multiple CachePut goroutines with the same content and verifies no corruption.

Info

• [documentation-currency] docs/plans/universal-harness-access-implementation.md:69 — Plan documents CachePath(workspaceRoot, hash) string but the implementation returns (string, error) due to hash validation. The signature change is an improvement but makes the plan doc stale.
  Remediation: Update the plan doc signature to CachePath(workspaceRoot, hash) (string, error).

Previous run (3)

Review

Findings

Medium

• [correctness] internal/fetch/cache.go:87 — CacheGet verifies content integrity against entry.SHA256 (the stored metadata hash) instead of the hash parameter (the caller-requested hash). In the content-addressed model, the caller's hash IS the expected content identity. If an attacker with cache-directory write access replaces both content and metadata.json with a different file (whose hash is internally consistent), the integrity check passes and CacheGet returns the substituted content for the wrong address. Defense-in-depth fix: verify got != hash instead of got != entry.SHA256, and optionally also check entry.SHA256 == hash to detect metadata corruption independently.
  Remediation: Change line 87 from if got := ComputeSHA256(content); got != entry.SHA256 { to if got := ComputeSHA256(content); got != hash { and consider adding a separate check that entry.SHA256 == hash.

Low

• [correctness] internal/fetch/cache.go:28 — .fullsend-cache/ is not in .gitignore. When the cache is used inside a git workspace (which is the expected case — workspaceRoot is the repo root), cache files will appear as untracked files in git status and could be accidentally committed.
  Remediation: Add .fullsend-cache/ to .gitignore.

Previous run (4)

Review

Findings

Medium

• [platform-security] internal/fetch/cache.go:24 — CachePath passes the hash parameter directly to filepath.Join without validating it contains only hex characters. filepath.Join resolves .. components, so a caller passing user-controlled input like "../../etc" as the hash could escape the cache directory. Currently there are no callers, and the planned caller (the resource resolver) will pass ComputeSHA256 output which is always hex-safe — but as an exported function, defense-in-depth warrants validating the hash format at the API boundary.
  Remediation: Add a hex-only check at the top of CachePath (or in both CacheGet/CachePut): if !regexp.MustCompile("^[0-9a-f]{64}$").MatchString(hash) { return "", fmt.Errorf("invalid hash: must be 64 hex characters") }. Alternatively, a simpler check using encoding/hex to decode the hash and verify length.

Low

• [correctness] internal/fetch/cache.go:107 — atomicWrite does not call tmp.Sync() before tmp.Close() and os.Rename(). On power loss, the OS may have buffered the write, and the renamed file could contain incomplete data. For a cache this degrades gracefully (integrity re-verification in CacheGet catches corruption, producing a cache miss), so this is not a bug — but production atomic-write patterns typically include an fsync for durability.

• [correctness] internal/fetch/cache.go:75 — CachePut silently overwrites metadata when the same content is stored from a different URL. The last URL wins, losing provenance of earlier source URLs. This is tested and intentional, but worth noting since the ADR's audit logging (PR 4) may want to record all source URLs for a given content hash rather than relying on cache metadata alone.

[ggallen]

All three findings addressed in d227f57:

• Medium [platform-security]: CachePath now returns (string, error) and validates the hash via validateHash(), which requires exactly 64 lowercase hex characters. Added errInvalidHash sentinel error. Path traversal inputs like ../../etc/passwd, short hashes, and uppercase hex are all rejected. Test coverage added in TestCachePathValidation.

• Low [correctness] — fsync: atomicWrite now calls tmp.Sync() before tmp.Close() to ensure data is flushed to disk before the rename.

• Low [correctness] — URL overwrite: CachePut godoc now documents this as intentional design — identical content shares a single cache entry, last URL wins in metadata, and provenance of all source URLs is tracked via fetch audit logging (not cache metadata).

[ggallen]

Both findings addressed in 20e179b:

• Medium [correctness]: CacheGet now verifies the content hash against the caller's hash parameter (the content address) instead of entry.SHA256. This prevents substitution attacks where an attacker with cache-directory write access replaces both content and metadata with a different but internally-consistent file. A separate check also verifies entry.SHA256 == hash to detect metadata corruption independently. New test TestCacheMetadataCorruption covers this scenario.

• Low [correctness]: .fullsend-cache/ gitignore entry is handled by PR chore: add .fullsend-cache to gitignore #1553 (already open). No change needed in this PR.

[waynesun09]

Review Squad (7 agents) — 2 verified MEDIUM findings after deduplication and verification against source. See inline comments for details.

[waynesun09]

LGTM. All review findings addressed — TOCTOU eliminated, symlink protection added with validateCachePath, strict hash validation, fsync on atomic writes, and good test coverage including symlink and concurrency cases.

[ggallen]

LGTM. All review findings addressed — TOCTOU eliminated, symlink protection added with validateCachePath, strict hash validation, fsync on atomic writes, and good test coverage including symlink and concurrency cases.

Can you approve?

[waynesun09]

LGTM. All review findings addressed — TOCTOU eliminated, symlink protection added with validateCachePath, strict hash validation, fsync on atomic writes, and good test coverage including symlink and concurrency cases.

Can you approve?

it's already approved, check top right on the green check icon

[ggallen]

LGTM. All review findings addressed — TOCTOU eliminated, symlink protection added with validateCachePath, strict hash validation, fsync on atomic writes, and good test coverage including symlink and concurrency cases.

Can you approve?

it's already approved, check top right on the green check icon

Well, you have to refresh the page to see it. That stinks.

Thanks.