feat(harness): resolve declarative resources from URL-referenced bases

[ggallen]

Summary

• When a harness inherits from a URL base via base:, declarative resources (agent, policy, skills) that are relative paths in the base now get fetched and cached locally during composition
• Previously only scripts were resolved by resolveBaseScripts (PR-2525); declarative resources were left as relative paths, which would fail ValidateFilesExist because the files live in the remote repo, not locally
• This closes the "breakage gap" identified in the ADR-0045 migration analysis — URL-based harness composition now works end-to-end

Changes

• Renamed fetchBaseScript → fetchBaseFile with depType/executable params to avoid code duplication
• Renamed validateBaseScriptPath → validateBaseRelPath (shared validation)
• Renamed auditScriptFetch → auditBaseFetch with configurable fetch type
• Added resolveBaseResources() to fetch agent, policy, and skills from URL bases
• Skills use CachePutDir to create cached directory trees containing SKILL.md
• All fetches go through the same allowlist, integrity, cache, URL index, and audit pipeline
• Updated existing tests for new dependency counts and resource deps
• Added new tests: agent+policy fetch, skill directory caching, offline cache hit, allowlist enforcement, path validation, audit log entries

Test plan

• All existing internal/harness tests pass with updated assertions
• All existing internal/cli tests pass (lock_test.go, run_test.go)
• Full internal/... test suite passes
• Coverage > 85% for changed code (package: 87%, resolveBaseResources: 96.3%)
• New tests cover: agent+policy fetch, skill directory caching, offline cache hit, path validation, allowlist rejection, audit logging

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Resolve URL-base declarative resources during harness composition
✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Description

• Fetch and cache agent/policy/skills inherited from URL base: when paths are relative.
• Reuse the existing allowlist, URL-index, cache, integrity, and audit pipeline for all base
 fetches.
• Expand harness/CLI tests to cover resource fetching, offline cache hits, and allowlist
 enforcement.

Diagram

graph TD
  L["LoadWithBase"] --> C["loadBaseChain"] --> S["resolveBaseScripts"] --> F["fetchBaseFile/Skill"] --> A{{"Allowlist"}} --> K[("Cache + URL index")] --> U["Audit log"]
  C --> R["resolveBaseResources"] --> F

  subgraph Legend
    direction LR
    _p["Process"] ~~~ _d{{"Decision"}} ~~~ _db[("Cache/Index")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Defer inherited resource fetching until after merge

• ➕ Avoids fetching resources that the child overrides
• ➕ Keeps pre-merge base processing smaller
• ➖ Harder to preserve the same dependency/audit semantics as scripts
• ➖ Requires extra bookkeeping to know which fields originated from a URL base

2. Fetch an archive/tree of the base repo instead of per-file fetches

• ➕ Fewer HTTP requests; simpler skill directory materialization
• ➕ Better consistency if multiple resources share a versioned snapshot
• ➖ Larger downloads and cache footprint
• ➖ More complex cache invalidation and allowlist enforcement (repo vs file URLs)

3. Treat URL base as a virtual filesystem layer (no local rewrite)

• ➕ No path rewriting; validation could operate against remote layer
• ➕ Potentially less local state
• ➖ Breaks existing expectations that ValidateFilesExist operates locally
• ➖ Offline mode becomes significantly more complex

Recommendation: The PR’s approach (fetch + cache + rewrite to local paths) is the best fit because it preserves existing local-file validation semantics, reuses the established allowlist/cache/audit pipeline, and supports offline mode. While fetching after merge could reduce unnecessary downloads, it complicates provenance tracking and dependency accounting; the current consistent pre-merge fetch strategy matches the existing script behavior.

Files changed (4) +698 / -101

Enhancement (1) +199 / -51

compose.goFetch and cache URL-base agent/policy/skills during composition +199/-51

Fetch and cache URL-base agent/policy/skills during composition

• Adds resolveBaseResources() to fetch declarative resources inherited from URL bases and rewrite them to local cache paths. Refactors script fetching into fetchBaseFile() with shared validation (validateBaseRelPath) and generalized auditing (auditBaseFetch), and adds skill directory caching via CachePutDir with offline cache support.

internal/harness/compose.go

Tests (3) +499 / -50

lock_test.goLockfile tests account for URL-base resource dependencies +9/-5

Lockfile tests account for URL-base resource dependencies

• Extends the URL-base lock tests to serve agent and skill content from the test server. Updates assertions to expect additional dependencies (base + declarative resources) in the lock entries.

internal/cli/lock_test.go

run_test.goRun tests provide URL-base agent resource fixture +2/-1

Run tests provide URL-base agent resource fixture

• Updates the URL-base run test server fixtures to include an agent file so resource resolution succeeds during composition.

internal/cli/run_test.go

compose_test.goExpand harness composition tests for resource resolution and security rules +488/-44

Expand harness composition tests for resource resolution and security rules

• Updates existing URL-base composition tests to include new resource dependencies and to serve default resource content for script-focused tests. Adds new coverage for agent+policy fetch, skill directory caching, offline cache hits, allowlist enforcement, path validation hardening, and audit log entries for resource fetches.

internal/harness/compose_test.go

[github-actions]

Site preview

Preview: https://1569ae12-site.fullsend-ai.workers.dev

Commit: 287b105c42d1e1f4fcae44513adcbb3d50a2123e

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:51 PM UTC · Completed 1:05 PM UTC
Commit: 805a901 · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 86.25954% with 18 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	86.25%	10 Missing and 8 partials ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Skill type breaks lock 🐞 Bug ≡ Correctness

Description

fetchBaseSkill caches a directory tree but returns Dependency.Type="skill"; downstream lock-file
logic only treats Type=="directory" as a directory and otherwise uses file-cache APIs, which will
fail for skill tree hashes and break lock/offline runs.

Code

internal/harness/compose.go[R830-838]

+	return Dependency{
+		Field:     field,
+		URL:       skillFileURL,
+		LocalPath: treePath,
+		SHA256:    treeHash,
+		FetchedAt: fetchedAt,
+		CacheHit:  false,
+		Type:      "skill",
+	}, treePath, nil

Relevance

⭐⭐⭐ High

PR #2139 lock logic uses Type=="directory" for CacheGetDir; Type="skill" will follow file-cache
path.

PR-#2139

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The skill fetch path caches a directory (CachePutDir + CacheGetDir) but labels the dependency as
"skill"; the lock resolver branches exclusively on Type == "directory" to decide whether to call
CacheGetDir vs CacheGet, so a "skill" entry will be treated as a file and fail cache
verification/resolution.

internal/harness/compose.go[763-838]
internal/resolve/resolve.go[22-31]
internal/cli/lock.go[193-235]
internal/cli/lock.go[567-608]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchBaseSkill` returns `Dependency.Type = "skill"` while the cached artifact is a directory tree (via `CachePutDir`). The lock pipeline and `resolveFromLock` only use directory-cache handling when `Type == "directory"`, so skill deps recorded from base composition are treated as file deps and can’t be resolved from cache.

### Issue Context
- Lock writing/reading expects dependency types to follow the `resolve.Dependency` contract: `"file"` or `"directory"`.
- Base-composition deps are converted into `resolve.Dependency` and persisted into `lock.yaml`.

### Fix Focus Areas
- internal/harness/compose.go[763-839]
 - Change both returns in `fetchBaseSkill` to set `Type: "directory"`.
 - (Optional but recommended) Consider aligning other base-composition file deps to `Type: "file"` (scripts/resources) to keep lock schema consistent.
- internal/harness/compose_test.go[472-488] and other updated assertions
 - Update tests that assert `Type == "skill"` to assert `Type == "directory"` (and adjust any dependency-type expectations accordingly).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Base skills cache incomplete 🐞 Bug ≡ Correctness

Description

fetchBaseSkill caches only SKILL.md into the local skill directory tree, so any other files in the
skill directory (e.g., scripts/ used by some skills) will be missing locally and in the sandbox at
runtime.

Code

internal/harness/compose.go[R807-809]

+	files := map[string][]byte{"SKILL.md": content}
+	treeHash, err := fetch.CachePutDir(opts.WorkspaceRoot, skillFileURL, files)
+	if err != nil {

Relevance

⭐⭐⭐ High

PR #2139 models skills as full directory trees (scripts/assets); caching only SKILL.md conflicts
with that standard.

PR-#2139

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation only caches SKILL.md into the directory tree, but the runtime uploads skill
directories recursively and some skills reference scripts within their directory; those referenced
files will not be present in the cached tree.

internal/harness/compose.go[763-813]
internal/runtime/claude.go[54-63]
skills/merge-queue/SKILL.md[2-16]
internal/resolve/resolve.go[268-269]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchBaseSkill` constructs a cached skill directory containing only `SKILL.md`. In this codebase, skills can include companion files (e.g., `scripts/`) and the runtime uploads the entire skill directory into the sandbox; caching only `SKILL.md` means those companion files won’t exist when the skill runs.

### Issue Context
- Runtime bootstrapping uploads each skill path as a directory.
- Existing skills in-repo demonstrate reliance on companion files under the skill directory (e.g., `skills/merge-queue/scripts/*`).

### Fix Focus Areas
- internal/harness/compose.go[763-839]
 - Update `fetchBaseSkill` to materialize the full skill directory contents (not only `SKILL.md`) into the cached tree.
 - If full directory listing is not possible for arbitrary HTTP bases, consider tightening semantics: either require URL-based bases to reference skills as forge URLs (so `resolve` can fetch full trees), or explicitly reject skill directories that require companion files when inherited via URL base.
- internal/runtime/claude.go[54-63]
 - Confirm expected contract: skill paths are directories uploaded recursively.
- skills/merge-queue/SKILL.md[2-16]
 - Use as a concrete example of a skill expecting `scripts/*` to exist alongside `SKILL.md`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Medium

• [incomplete resource resolution] internal/harness/compose.go — fetchBaseSkill fetches only SKILL.md from the base URL and caches it as a single-file directory via CachePutDir. Skills in this codebase can contain multiple files (SKILL.md, scripts/, sub-agents). When a multi-file skill is inherited through a URL base, the tree hash will be computed from only SKILL.md, which will differ from a hash computed from all files. There is no test covering multi-file skills inherited from URL bases, and no clear error message explaining this limitation.

• [undocumented-identifiers] docs/plans/universal-harness-access.md:393 — The FetchAuditEntry.FetchType documentation lists only "static" and "runtime" as valid values, but the codebase already uses "base_script" and this PR adds "base_resource" and "base_skill" via the dynamic "base_" + fetchType construction. The plan document's enumeration of valid FetchType values is incomplete.

• [missing-authorization] N/A — This PR adds 899 lines across 5 files with no linked issue. The change plausibly addresses the open question in ADR-0045 (relative path resolution, moving from Option (c) to Option (b)), but non-trivial feature PRs should have explicit authorization via a linked issue so that scope and intent are auditable.

Previous run (2)

Review

Findings

Medium

• [missing-authorization] N/A — This PR adds 698 lines across 4 files with no linked issue. The change plausibly addresses an open question documented in ADR-0045 (relative path resolution in URL-referenced bases, moving from Option (c) to Option (b)), but non-trivial changes should have explicit authorization via a linked issue so that scope and intent are auditable.

• [design-deviation] internal/harness/compose.go — The removed comment stated that non-script relative paths resolve against the child's directory (ADR-0045 Option (c) for Phase 1). The new resolveBaseResources implementation changes this to resolve declarative resources relative to the base URL's path prefix (Option (b)). ADR-0045 explicitly documented this as an open question with three options, so this is a natural evolution — but the ADR's Open Questions section should be updated to record the decision.

• [Missing Integrity Verification] internal/harness/compose.go — fetchBaseFile (agent/policy) and fetchBaseSkill (skills) do not verify per-resource content integrity hashes. The parent base harness URL requires #sha256=... integrity pinning, but individual resource files fetched from relative paths within that base are not integrity-checked. This is consistent with the pre-existing fetchBaseScript behavior and is not a regression, but the gap now applies to a broader surface (agent, policy, skills in addition to scripts). Consider extending the lockfile mechanism to record per-resource SHA256 hashes.

---

Labels: PR modifies harness base composition in internal/harness/ with a new feature for declarative resource resolution.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:23 PM UTC · Completed 1:38 PM UTC
Commit: 226bc94 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 1:58 PM UTC
Commit: 287b105 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:32 PM UTC · Completed 2:37 PM UTC
Commit: 287b105 · View workflow run →

[ralphbean]

I think this needs a change before we can merge. See inline comments.

[ralphbean]

[important] fetchBaseSkill only fetches SKILL.md, which means skills with companion files (sub-agents, scripts, meta-prompts) silently lose those files when inherited through a URL base.

For context, the scaffold's skills/pr-review/ has 9 files:

skills/pr-review/
  SKILL.md
  meta-prompt.md
  sub-agents/
    challenger.md
    correctness.md
    cross-repo-contracts.md
    docs-currency.md
    intent-coherence.md
    security.md
    style-conventions.md

Today this works because the dispatch workflow layers the scaffold into the workspace (the "Prepare workspace" step in reusable-review.yml copies the full directory tree). But if someone points a URL base at a harness referencing multi-file skills without that scaffold layering, the skill appears to resolve but the agent won't have the companion files at runtime.

The function comment says "skills with companion files must use forge-format URLs resolved by ResolveHarness instead" — but nothing warns the user when this limitation kicks in. Could we either fetch companion files (maybe by convention, or via a manifest), or at minimum emit a warning when a skill directory is cached with only SKILL.md?

[ralphbean]

[minor] (non-blocking) The count went from 1 to 3 but only deps[0] is asserted on. The compose_test.go tests check Field and Type for each new dep — would it make sense to do the same here so the integration test verifies the right deps, not just the right count?

[ralphbean]

[minor] (non-blocking) This catch-all serves # test resource for any /agents/, /policies/, or */SKILL.md path. If resolveBaseResources ever constructs a wrong URL (e.g., garbled prefix), script-focused tests would still pass because the server happily serves content for any matching prefix. The dedicated resource tests use precise setups, so it's not a problem today — just something to keep an eye on.

[fullsend-ai-retro]

Retro: PR #2690 — resolve declarative resources from URL-referenced bases

Timeline: Human PR by ggallen adding resolveBaseResources to fetch/cache agent, policy, and skill files from URL-referenced base harnesses. Three fullsend review agent passes ran (commits 805a901, 226bc94, 287b105). Qodo found a concrete bug (Type: "skill" should be Type: "directory") that the fullsend review agent missed despite flagging the general area. The author fixed the bug before merge. Human reviewer (rh-hemartin) approved.

Review quality gap: The fullsend review agent identified "incomplete resource resolution" as a medium finding — noting tree hashes would differ — but failed to trace the Type field through to the lock resolver's switch logic in lock.go, where only Type == "directory" triggers CacheGetDir. Qodo performed this cross-file data-flow trace and identified the exact breakage. This is a specific instance of the broader pattern in #1525 (cross-file impact analysis), but the novel sub-pattern — new string-typed enum variants not handled by downstream consumers — is not specifically covered by any existing open issue.

Existing coverage considered: Issues #1525 (cross-file analysis), #1453 (escalate medium correctness findings), #1476 (request changes for correctness bugs), and #1444 (external tool catches what fullsend misses) are related but none specifically address enum-variant exhaustiveness checking. One proposal filed below.

Proposals filed

• Review agent: detect unhandled new string-enum variants in downstream consumers (in fullsend-ai/fullsend)