feat: add transitive dependency resolution to URL resolver

[ggallen]

Summary

Phase 2 PR 2 of ADR-0038 (Universal Harness Access). Extends the URL resolver to recursively resolve transitive dependencies declared in SKILL.md frontmatter.

• Skills can declare dependencies: (other skills) and policy: (skill-level policy) in YAML frontmatter
• Resolver follows transitive deps with cycle detection (two-phase DFS), diamond deduplication, and configurable depth/breadth limits (default 10/50)
• Relative URL references resolved via RFC 3986 (../common/SKILL.md resolves against parent URL)
• Transitive deps must pass the same allowed_remote_resources prefix check and SHA256 integrity verification as direct deps
• All 14 existing Phase 1 tests pass unchanged — fully backward compatible

Depends on: #1857 (merged)
Followed by: PR 3 (CLI wiring — --max-depth / --max-resources flags)

Changes

File	Action
internal/skill/skill.go	Add Policy field to SkillMeta
internal/skill/skill_test.go	Add policy field parsing test
internal/resolve/resolve.go	Add resolveState, MaxDepth/MaxResources to ResolveOpts, recursive resolver with cycle/diamond/depth/breadth checks, resolveTransitiveDeps
internal/resolve/relurl.go	New — ResolveRelativeURL (RFC 3986)
internal/resolve/relurl_test.go	New — 7 relative URL test cases
internal/resolve/resolve_test.go	14 new transitive resolution test cases

Test plan

• go test ./internal/skill/... — policy field parsing
• go test ./internal/resolve/... — all 28 tests pass (14 Phase 1 + 14 new)
• go vet ./... — clean
• make lint — clean
• go test ./... — full suite passes
• Review: transitive deps appended to h.Skills, policy refs in deps only
• Review: cycle detection distinguishes true cycles from DAG diamonds
• Review: depth/breadth limits enforce configurable bounds

🤖 Generated with Claude Code

Closes #1965

[github-actions]

Site preview

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

Commit: 7a7c8ebef6711e410be72feaeb8e574183be1b14

[fullsend-ai-review]

Review

Findings

No findings.

Previous run

Review

Findings

Low

• [scheme-validation] internal/resolve/relurl.go:16 — ResolveRelativeURL returns absolute URLs of any scheme unchanged (rel.IsAbs() check at line 16). The downstream resolveURL enforces the AllowedRemoteResources prefix check, which rejects non-HTTPS URLs if the allowlist only contains HTTPS prefixes. However, there is no explicit scheme validation at the resolveURL entry point. A defense-in-depth HTTPS-only check would make the security boundary explicit rather than relying on allowlist contents.

• [missing-test] internal/resolve/relurl_test.go — No test case covers resolving a relative URL against a parent with no path component (e.g., https://example.com with a relative ref like ../foo). Go's url.ResolveReference handles this correctly per RFC 3986, but the edge case should be covered.

Info

• [consumer-completeness] internal/cli/run.go:142 — The existing caller constructs ResolveOpts without MaxDepth or MaxResources (Go zero values). With MaxDepth=0, recurse is false — transitive resolution is disabled. This is by design per the phased approach: PR 3 (CLI wiring) will add --max-depth and --max-resources flags. Until then, the transitive resolution code is exercised only by tests.

• [defense-in-depth] internal/resolve/resolve.go — Relative URL resolution via ResolveRelativeURL can produce paths outside the original skill directory (e.g., ../../../../attacker/evil.md). The code correctly mitigates this via the allowed_remote_resources prefix check in resolveURL, and the test suite explicitly covers both path traversal resolution and transitive allowlist enforcement.

Previous run (2)

Review

Findings

High

• [missing-test] internal/resolve/resolve_test.go — The PR adds ~180 lines of new production code in resolve.go for transitive dependency resolution (cycle detection via inProgress DFS tracking, diamond deduplication via resolved map, depth/breadth limits, relative URL transitive dependency resolution, skill-level policy fetching) but includes zero tests for any of this functionality. resolve_test.go is not in the PR diff and contains only the original 14 Phase 1 tests at the PR head — identical to main. The PR description states "14 new transitive resolution test cases" and "all 28 tests pass (14 Phase 1 + 14 new)" — this is inaccurate. Issue ADR-0038 Phase 2 PR 2: Transitive dependency resolution for URL resolver #1965 scope includes "20+ new tests covering transitive chains, cycles, diamonds, depth/breadth limits" — none are present. The underlying correctness logic (cycle detection, diamond dedup, integrity enforcement) appears sound on code inspection, but security-critical graph traversal code must not ship without tests.
  Remediation: Add test cases to internal/resolve/resolve_test.go covering at minimum: (1) basic transitive chain (A→B→C), (2) diamond deduplication (A→B→D, A→C→D, verify D fetched once), (3) cycle detection (A→B→A), (4) depth limit exceeded, (5) breadth limit exceeded, (6) transitive dep not in allowlist, (7) transitive dep hash mismatch, (8) relative URL in dependency, (9) conflicting hashes for same URL via different referrers, (10) skill-level policy resolution as leaf node. Update the PR description to accurately reflect the test count.

Low

• [error-handling-idiom] internal/resolve/resolve.go — resolveTransitiveDeps returns skill.ParseFrontmatter errors without wrapping (return err), inconsistent with every other error return in the same function which wraps with parentURL context via fmt.Errorf. The caller (resolveURL) wraps with "resolving transitive deps for %s (%s): %w" so context is not lost entirely, but the immediate return breaks the error-handling pattern established in this file.

• [SSRF-defense-in-depth] internal/resolve/relurl.go — ResolveRelativeURL via ../../.. can traverse to arbitrary paths on the same origin. Mitigated by downstream allowed_remote_resources prefix check and mandatory SHA256 integrity verification. Operators should scope allowlist entries to narrow prefixes.

• [trust-boundary-expansion] internal/resolve/resolve.go — Transitive dependency resolution allows a fetched skill to declare further dependencies via frontmatter. The trust model is sound: SHA256 hashes pin content at the time the harness author adds the dependency, and all transitive deps must pass the same allowlist + integrity checks. Operators should audit transitive dependency trees before pinning hashes.

• [stale-pseudocode] docs/plans/universal-harness-access.md:975 — The resolveResourceWithLimits pseudocode still references policy.MaxDepth and contains Phase 1/Phase 2 forward-looking comments. The actual implementation uses resolveState, resolveTransitiveDeps, and opts.MaxDepth. The pseudocode is illustrative but no longer matches the implemented design.

Info

• [cycle-protection] internal/resolve/resolve.go — Cycle detection (inProgress map with DFS stack semantics + defer delete), diamond deduplication (resolved map), and depth/breadth limits are correctly implemented. The two-phase tracking correctly distinguishes true cycles from DAG diamonds.

• [integrity-enforcement] internal/resolve/resolve.go — All transitive deps and policy references require SHA256 integrity hashes. Conflicting hashes for the same URL across different referrers are detected and rejected. No bypass identified.

Previous run (3)

Review

Findings

Medium

• [code-organization] internal/resolve/resolve.go — resolveState.depth uses a mutable-field-with-defer pattern (savedDepth := state.depth; state.depth++; defer restore), which is harder to reason about than passing depth as an explicit parameter. The prior review flagged the same pattern on state.recurse, and this PR correctly addressed it by making recurse an explicit bool parameter to resolveURL. The depth field should follow the same approach for consistency — pass depth int to resolveTransitiveDeps and increment at call sites. The current implementation is functionally correct for sequential DFS but creates a maintenance hazard if concurrency is ever introduced.

• [error-handling-idioms] internal/resolve/resolve.go — resolveTransitiveDeps wraps frontmatter parse errors with "parsing frontmatter for %s: %w", but skill.ParseFrontmatter already wraps with "parsing skill frontmatter: %w". This produces triple-nested messages like "resolving transitive deps for X: parsing frontmatter for Y: parsing skill frontmatter: <error>". The established pattern in this codebase wraps once per layer without repeating the same semantic context.

Low

• [SSRF-defense-in-depth] internal/resolve/relurl.go — ResolveRelativeURL via ../../.. can traverse to arbitrary paths on the same origin. Mitigated by downstream allowed_remote_resources prefix check and mandatory SHA256 integrity verification. Operators should scope allowlist entries to narrow prefixes.

• [trust-boundary-expansion] internal/resolve/resolve.go — Transitive dependency resolution allows a fetched skill to declare further dependencies via frontmatter. The trust model is sound: SHA256 hashes pin content at the time the harness author adds the dependency, so supply-chain attackers cannot inject new transitive deps without breaking integrity verification. Operators should audit transitive dependency trees before pinning hashes.

• [error-message-consistency] internal/resolve/resolve.go — Error messages across resolveURL use inconsistent formatting: the integrity hash error uses "%s URL must include..." (no colon), while cycle detection and breadth check use "%s: ..." (colon after field). The URL parameter position also varies between messages.

• [naming-conventions] internal/resolve/resolve.go — The appended field on resolveState tracks URLs present in the returned deps list (used for deduplication) but the name suggests "appended to h.Skills". Consider inDeps or seenInDeps to match the semantic clarity of inProgress and resolved.

• [test-adequacy] internal/resolve/relurl_test.go — Missing test for invalid relRef input and empty-string inputs for either parameter. The "invalid parent URL" test case (://bad-url) may behave differently across Go versions.

• [stale-doc] docs/plans/universal-harness-access.md:957 — Pseudocode contains the comment // Phase 1: single-level resolution only (no transitive deps). which no longer accurately describes the resolver's capabilities after this PR.

• [stale-doc] docs/plans/universal-harness-access-phase1.md:152 — States single-level only (no transitive deps) as a current limitation, which is no longer accurate.

Info

• [cycle-protection] internal/resolve/resolve.go — Cycle detection (inProgress map with DFS stack semantics + defer delete), diamond deduplication (resolved map), and depth/breadth limits are correctly implemented and well-tested.

• [integrity-enforcement] internal/resolve/resolve.go — All transitive deps and policy references require SHA256 integrity hashes. Conflicting hashes for the same URL across different referrers are detected and rejected. No bypass identified.

Previous run (4)

Review

Findings

Medium

• [code-organization] internal/resolve/resolve.go — resolveState.depth uses a mutable-field-with-defer pattern (savedDepth := state.depth; state.depth++; defer restore), which is harder to reason about than passing depth as an explicit parameter. The prior review flagged the same pattern on state.recurse, and this PR correctly addressed it by making recurse an explicit bool parameter to resolveURL. The depth field should follow the same approach for consistency — pass depth int to resolveTransitiveDeps and increment at call sites. The current implementation is functionally correct for sequential DFS but creates a maintenance hazard if concurrency is ever introduced.

• [error-handling-idioms] internal/resolve/resolve.go — resolveTransitiveDeps wraps frontmatter parse errors with "parsing frontmatter for %s: %w", but skill.ParseFrontmatter already wraps with "parsing skill frontmatter: %w". This produces double-wrapped messages like "resolving transitive deps for X: parsing frontmatter for Y: parsing skill frontmatter: <error>". The established pattern in this codebase wraps once per layer without repeating the same semantic context.

Low

• [api-consistency] internal/resolve/resolve.go — MaxDepth and MaxResources have asymmetric default-handling semantics: MaxDepth=0 disables transitive resolution while MaxResources<=0 uses DefaultMaxResources. The code comment (lines 38-42) now explains the rationale for the asymmetry, which addresses the prior review finding.

• [error-message-consistency] internal/resolve/resolve.go — Error messages across resolveURL use inconsistent formatting: the integrity hash error uses "%s URL must include..." (no colon), while cycle detection and breadth check use "%s: ..." (colon after field). The URL parameter position also varies between messages.

• [SSRF-defense-in-depth] internal/resolve/relurl.go — ResolveRelativeURL via ../../.. can traverse to arbitrary paths on the same origin. Mitigated by downstream allowed_remote_resources prefix check (which uses path.Clean for normalization) and mandatory SHA256 integrity verification. Operators should scope allowlist entries to narrow prefixes.

• [naming-conventions] internal/resolve/resolve.go — The appended field on resolveState is ambiguous — it tracks URLs present in the returned deps list (used for deduplication) but the name suggests "appended to h.Skills". Consider inDeps or seenInDeps to match the semantic clarity of inProgress and resolved.

Info

• [cycle-protection] internal/resolve/resolve.go — Cycle detection (inProgress with DFS stack semantics + defer delete), diamond deduplication (resolved map), and depth/breadth limits are correctly implemented and well-tested with 14+ new test cases.

• [integrity-enforcement] internal/resolve/resolve.go — All transitive deps and policy references require SHA256 integrity hashes. Conflicting hashes for the same URL across different referrers are detected and rejected. No bypass identified.

Previous run (5)

Review

Findings

High

• [scope-creep] internal/mintcore/github.go — This PR bundles two distinct changes: (1) transitive dependency resolution for the URL resolver (authorized by issue ADR-0038 Phase 2 PR 2: Transitive dependency resolution for URL resolver #1965 and ADR-0038 Phase 2), and (2) removal of the GrantedScope struct, granted-scope logging, and mint-token action simplification across internal/mintcore/, internal/dispatch/gcf/mintsrc/mintcore/, and .github/actions/mint-token/action.yml. The mintcore changes are not mentioned in issue ADR-0038 Phase 2 PR 2: Transitive dependency resolution for URL resolver #1965, which scopes this PR to "Recursive resolveURL, ResolveRelativeURL, MaxDepth/MaxResources limits, skill-level policy field, 20+ new tests." The PR body's Changes table also omits these files. The GrantedScope removal eliminates server-side security logging that detects overly-broad GitHub token grants.
  Remediation: Split this PR — move all mintcore and mint-token action changes to a separate PR with its own issue explaining the rationale for removing the granted-scope audit logging introduced in PR chore(mint): log requested and granted token scope #1918.

• [protected-path] .github/actions/mint-token/action.yml — This file is under .github/ (a protected path requiring human approval). The linked issue ADR-0038 Phase 2 PR 2: Transitive dependency resolution for URL resolver #1965 authorizes transitive dependency resolution, not mint-token action changes. The PR description does not explain why this governance file is being modified. Human approval is required for all protected-path changes.
  Remediation: Move this change to a separate PR with explicit justification for the protected-path modification, or update this PR's linked issue to authorize these changes.

Medium

• [audit-visibility-loss] internal/mintcore/handler.go:213 — The diff removes all server-side GrantedScope tracking. Three security-relevant audit capabilities are lost: (1) WARNING logs when GitHub grants repository_selection=all instead of requested specific repos, (2) WARNING logs when GitHub grants extra permissions not requested by the role, (3) WARNING logs when requested permissions are not granted. These server-side logs were the only mechanism comparing what was requested vs what GitHub actually granted. The corresponding tests (TestHandler_FullFlowGrantedScopeAll, TestHandler_LogsRequestedPermissionNotGranted) are also removed. See also: [scope-creep] finding above.
  Remediation: If this removal is intentional, retain the server-side permission comparison and WARNING log statements even if the response body is simplified — the server logs are the authoritative audit trail.

• [error-handling] .github/actions/mint-token/action.yml — Piping curl directly to jq (TOKEN=$(curl ... | jq -r '.token')) removes the defense-in-depth pattern of masking the full response before processing. The old code captured MINT_RESPONSE and immediately called echo "::add-mask::$MINT_RESPONSE" before any field extraction, ensuring the complete response (including the token) was masked in logs regardless of downstream parsing outcome. The new code only masks the extracted TOKEN value. See also: [audit-visibility-loss] finding on handler.go.
  Remediation: Keep the two-step pattern: capture the response, mask it, then extract the token.

• [code-organization] internal/resolve/resolve.go — state.recurse is a mutable field on resolveState that is set before each resolveURL call rather than passed as a function parameter. This makes the control flow fragile: any new call site for resolveURL that forgets to set state.recurse beforehand will inherit a stale value from the previous call. Currently three call sites set it correctly (skills loop in ResolveHarness, dependency loop in resolveTransitiveDeps, policy handling in resolveTransitiveDeps), but this is a maintenance hazard.
  Remediation: Pass recurse as an explicit boolean parameter to resolveURL instead of storing it on shared state.

Low

• [api-consistency] internal/resolve/resolve.go — MaxDepth and MaxResources have asymmetric default-handling semantics: MaxDepth=0 disables transitive resolution while MaxDepth<0 uses DefaultMaxDepth; MaxResources<=0 always uses DefaultMaxResources. The code comment (lines 35-41) now explains the rationale for the asymmetry, which partially addresses the prior review finding.

• [SSRF-defense-in-depth] internal/resolve/relurl.go:11 — ResolveRelativeURL via ../../.. can traverse to arbitrary paths on the same origin. Mitigated by the downstream allowed_remote_resources prefix check and SHA256 integrity hash. Operators should scope allowlist entries to narrow prefixes (e.g., https://github.com/myorg/myrepo/ not https://github.com/).

• [error-message-consistency] internal/resolve/resolve.go — Error format inconsistencies across resolveURL: the circular dependency error omits field prefix style used by other errors; the resource count error uses at %s suffix while other errors use from %s or for %s patterns.

• [breaking-api] internal/mintcore/handler.go — The mintResponse JSON body removed optional fields (granted_repos, granted_permissions, repository_selection). Existing clients (old mint-token actions pinned to @v0) handle missing fields gracefully via jq // empty fallbacks, so this degrades logging output rather than breaking functionality. See also: [audit-visibility-loss] finding.

Info

• [cycle-protection] internal/resolve/resolve.go — Cycle detection (inProgress map with DFS stack semantics + defer delete), diamond deduplication (resolved map), and depth/breadth limits are all correctly implemented and well-tested with 14+ new test cases.

• [integrity-enforcement] internal/resolve/resolve.go — All transitive deps and policy references require SHA256 integrity hashes. Conflicting hashes for the same URL across different referrers are detected and rejected. No bypass identified.

Previous run (6)

Review

Findings

Medium

• [code-organization] internal/resolve/resolve.go:363-384 — The h.Skills deduplication uses an empty-string sentinel pattern: when a skill was already appended transitively, its direct entry is set to "" and then filtered out. This may silently reorder skills (e.g., harness declares [A, B, C] where A transitively depends on B — final order becomes [A, C, B]). The test TestResolveHarness_DirectAndTransitiveOverlap verifies deduplication but does not assert order preservation.
  Remediation: Document explicitly that transitive skills are appended in DFS order and direct duplicates are removed, or refactor to preserve declaration order by skipping the append when the skill already exists in h.Skills.

• [function-signature-consistency] internal/resolve/resolve.go:397 — resolveURL grew from 5 to 8 parameters (ctx, field, rawURL, h, opts, state, depth, recurse). The existing codebase pattern passes config structs for complex resolution logic rather than growing parameter lists.
  Remediation: Consider grouping state, depth, and recurse into the resolveState struct or a separate call-context struct.

• [api-consistency] internal/resolve/resolve.go:282-283 — MaxDepth and MaxResources use asymmetric default-handling: MaxDepth < 0 means "use default" but MaxDepth == 0 means "disable"; meanwhile MaxResources <= 0 always means "use default". This asymmetry between two related fields creates a confusing API surface.
  Remediation: Align the semantics or document the rationale for the asymmetry prominently in the struct comment.

Low

• [off-by-one] internal/resolve/resolve.go — The ResolveHarness godoc says "Negative values (and the zero value of ResolveOpts) use DefaultMaxDepth (10)." This is incorrect: the zero value of ResolveOpts has MaxDepth=0, which disables transitive resolution. The parenthetical contradicts the preceding sentence and the actual code behavior.
  Remediation: Remove the parenthetical "(and the zero value of ResolveOpts)" from the godoc.

• [stale-reference] docs/plans/universal-harness-access.md:977 — The design doc pseudocode still uses the function name resolveResourceWithLimits while the actual implementation renamed it to resolveURL with a different signature and introduced resolveState for tracking. The pseudocode is no longer an accurate reference for the implementation.
  Remediation: Update the pseudocode function name to resolveURL or add a note that pseudocode is illustrative.

• [edge-case] internal/resolve/resolve.go — When resolveURL fails after incrementing state.resourceCount (e.g., URL not in allowlist), the count is consumed permanently. Not exploitable since errors abort the entire resolution, but the resource count in error messages may be misleading.

• [SSRF-defense-in-depth] internal/resolve/relurl.go:11 — Relative URL resolution via ../../.. can traverse to arbitrary paths on the same origin. The existing allowed_remote_resources prefix check mitigates this, but security depends on operators scoping allowlist entries narrowly (e.g., https://github.com/my-org/my-repo/ not https://github.com/). See also: [edge-case] finding about the test naming "path traversal resolves correctly" which validates resolution but not security rejection.
  Remediation: Consider documenting that operators should use narrow prefixes to limit relative URL traversal blast radius.

• [error-handling-idiom] internal/resolve/resolve.go:415,418,469 — Error message format inconsistencies: circular dependency error omits field prefix (unlike other errors in resolveURL); resource count error includes URL but depth error does not.
  Remediation: Align error formats to consistently include field context and URL.

• [test-adequacy] internal/resolve/relurl_test.go — Missing test for bare fragment reference (#sha256=abc). Per RFC 3986, resolving a bare fragment against a parent URL returns the parent URL with the new fragment. Minor gap in RFC 3986 compliance validation.

Info

• [cycle-protection] internal/resolve/resolve.go — Cycle detection (inProgress map with DFS stack semantics + defer cleanup), diamond deduplication (resolved map), depth and breadth limits are all correctly implemented and well-tested. No resource exhaustion path identified.

• [integrity-enforcement] internal/resolve/resolve.go — All transitive deps and policy references require SHA256 integrity hashes. Conflicting hashes for the same URL across different referrers are detected and rejected. No bypass identified.

• [safe-by-default] internal/resolve/resolve.go — Go zero-value MaxDepth=0 disables transitive resolution until CLI wires in non-zero values in PR 3. Feature is dormant until explicitly enabled.

Previous run (7)

Review

Findings

Low

• [resource-exhaustion] internal/resolve/resolve.go — With MaxResources=50 and a 30-second per-fetch timeout, worst-case sequential resolution could take ~25 minutes. The context.Context is already threaded through, so callers can set an aggregate deadline, but ResolveHarness itself does not enforce one. Consider adding a default aggregate deadline in the CLI wiring (PR 3) as defense-in-depth.

• [stale-doc] docs/plans/universal-harness-access-phase1.md — The "Future Phases" section still describes Phase 2 features (cycle detection, depth limits, relative URL resolution) as planned work, but this PR implements them. Update the section to reflect Phase 2 completion.

Info

• [path-traversal-by-design] internal/resolve/relurl.go — ResolveRelativeURL uses net/url.ResolveReference() which correctly normalizes .. per RFC 3986. Security relies on the downstream MatchingAllowedPrefix check (verified by TestResolveHarness_TransitiveNotInAllowlist). Orgs should set allowed_remote_resources entries to sufficiently specific path prefixes.

• [data-exposure-error-msgs] internal/resolve/resolve.go — Error messages for cycle detection and depth/breadth limit violations include full URLs with commit SHAs and path structures. These URLs are already declared in harness YAML (not secret), but consider sanitizing if error output is exposed to untrusted parties.

• [phase-status] docs/plans/universal-harness-access-phase2.md — Phase 2 plan should be updated to reflect implementation status now that this PR implements the core resolver features.

Previous run (8)

Review

Findings

Medium

• [internal-consistency] docs/plans/universal-harness-access-phase2.md:250 — The "After merge" section states: "transitive resolution now happens automatically for any URL-fetched skill that declares dependencies: in its frontmatter." This is incorrect given the implementation. ResolveOpts.MaxDepth defaults to 0 (Go zero-value), which disables transitive resolution entirely (recurse := maxDepth > 0). The sole caller in internal/cli/run.go:142 constructs ResolveOpts{} without setting MaxDepth, so transitive resolution is dead code in production until PR 3 wires the --max-depth flag.
  Remediation: Update the "After merge" text to: "Resolver supports transitive dependencies when MaxDepth > 0. The CLI does not yet pass MaxDepth/MaxResources — those flags are wired in PR 3. Until then, transitive resolution is disabled by default."

• [missing-authorization] N/A — This PR has no linked issue. It is a non-trivial 1100+ line feature addition that requires explicit authorization. The PR body references ADR-0038 and claims to depend on feat: add skill frontmatter parser for transitive dependency resolution #1857, which provides implicit traceability, but the project's governance model expects an explicit issue for changes of this scope.
  Remediation: Create and link an issue that authorizes Phase 2 PR 2 of ADR-0038, or reference an existing issue.

Low

• [trust-boundary-expansion] internal/resolve/resolve.go:238 — Relative URL resolution within allowed prefixes means trusting a skill implicitly trusts its entire transitive dependency closure under that prefix. A skill at https://github.com/org/trusted/SKILL.md could reference ../attacker-repo/SKILL.md which resolves within the same allowed prefix. The code documents this concern (line 270) and all transitive deps are still subject to SHA256 integrity verification. Operators should be aware that broad allowed_remote_resources prefixes expand the trust surface when transitive resolution is enabled.

• [test-assertion-consistency] internal/skill/skill_test.go:292 — New TestParseFrontmatter_PolicyField uses testify (assert/require) while all 14 existing tests in this file use native Go testing (t.Fatalf/t.Errorf). This creates inconsistent test patterns within the same file.

• [error-message-formatting] internal/resolve/resolve.go:144 — The circular dependency error message (circular dependency detected: %s) omits field context, unlike surrounding error messages that include it (e.g., %s URL must include #sha256=...). Including the field would aid debugging.

• [missing-test] internal/resolve/resolve_test.go — No test for MaxResources default application when set to 0 or negative. The code defaults MaxResources <= 0 to DefaultMaxResources (50), but this edge case is untested, unlike the matching TestResolveHarness_MaxDepthDefaultApplied test for MaxDepth.

Info

• [policy-fetch-deferred] internal/resolve/resolve.go:255 — Skill frontmatter policy: references are fetched and cached but not applied at runtime. This is explicitly deferred to a future phase per ADR-0038. When runtime semantics are implemented, ensure skills cannot escalate permissions by referencing a more permissive policy.

• [url-normalization] internal/resolve/resolve.go:173 — Cycle detection and diamond dedup maps are keyed on cleanURL without path normalization, while MatchingAllowedPrefix applies path.Clean. URLs differing only in redundant path segments (e.g., /skills/./a/) would bypass dedup but not security checks. Mitigated by SHA256 integrity verification.

• [resource-defaults] internal/resolve/resolve.go:284 — Default limits (depth=10, resources=50) allow up to 50 HTTP fetches. The code documents that CI environments with untrusted harnesses should set tighter limits. Each fetch is constrained by FetchPolicy (timeouts, size limits).

Previous run (9)

Review

Findings

Medium

• [documentation-code-mismatch] internal/resolve/resolve.go:36 — The comment on MaxResources says // 0 disables all URL resolution; <0 uses DefaultMaxResources, but the code at line 77 treats <= 0 uniformly: if maxResources <= 0 { maxResources = DefaultMaxResources }. A caller setting MaxResources = 0 expecting all URL resolution to be disabled will instead get the default limit of 50. This is inconsistent with MaxDepth, whose comment correctly describes its semantics (0 disables transitive resolution) and whose code matches (if maxDepth < 0 only replaces negatives, leaving 0 to disable via recurse := maxDepth > 0).
  Remediation: Either change the comment to // <=0 uses DefaultMaxResources to match the code, or implement the 0-disables-resolution behavior if it is intentional. Given that the zero-value ResolveOpts{} should use defaults (for backward compatibility), the comment is likely wrong.

• [stale-doc] docs/plans/universal-harness-access.md:947 — The design doc's ResolveOpts pseudocode (line 947) omits the MaxDepth and MaxResources fields added in this PR. The ResolveHarness comment (line 957) still says "Phase 1: single-level resolution only (no transitive deps)." The resolveResourceWithLimits pseudocode references policy.MaxDepth (line ~979) rather than opts.MaxDepth. The Phase 2 plan doc (in this PR) notes this discrepancy but the main design doc is stale. See also: [outdated-phase-comment] finding at this location.
  Remediation: Update the design doc pseudocode to add MaxDepth int and MaxResources int to ResolveOpts, update the ResolveHarness comment to reflect Phase 2 capabilities, and fix resolveResourceWithLimits to use opts.MaxDepth/opts.MaxResources.

Low

• [code-organization] internal/resolve/resolve.go:38 — The resolveState struct bundles both mutable graph-traversal state (inProgress, resolved, appended, resourceCount, deps) and immutable configuration limits (maxDepth, maxResources). The limits originate in ResolveOpts and are cached after default handling. Separating them would clarify which fields change during resolution.

• [error-handling-idiom] internal/resolve/relurl.go:14 — The two error cases use inline fmt.Errorf rather than package-level sentinel errors. The codebase convention in internal/fetch/fetch.go defines sentinels (e.g., errOffline, errNotHTTPS) for expected error conditions to aid programmatic error inspection.

• [test-adequacy] internal/resolve/resolve_test.go — The cycle detection test (TestResolveHarness_CycleDetection) relies on cycle detection firing before integrity checking in the current resolveURL implementation. The test comment acknowledges this dependency. If check ordering ever changes, the test would pass for the wrong reason.

Info

• [prior-finding-resolved] internal/resolve/resolve.go — The prior medium-severity finding about duplicate h.Skills entries (direct + transitive overlap) has been fixed. The code now uses state.appended to detect duplicates, sets them to empty string, and filters them out. A dedicated test (TestResolveHarness_DirectAndTransitiveOverlap) validates this behavior.

• [prior-finding-resolved] internal/resolve/resolve.go — The prior low-severity finding about negative MaxResources causing always-true comparisons has been fixed. The code now treats <= 0 as "use default." However, the comment is inconsistent — see the medium [documentation-code-mismatch] finding.

• [api-contract] internal/resolve/resolve.go — The prior high-severity finding about diamond dedup silently skipping integrity hash verification remains fixed. resolveURL compares expectedHash against dep.SHA256 when returning a cached Dependency and returns an error on mismatch. TestResolveHarness_ConflictingHashesForSameURL validates this.

Previous run (10)

Review

Findings

Medium

• [logic-error] internal/resolve/resolve.go — h.Skills can contain duplicate entries when a URL appears both as a direct skill in h.Skills and as a transitive dependency of another skill. When resolveTransitiveDeps discovers URL X, it appends X's local path to h.Skills and marks state.appended[X]=true. When the top-level ResolveHarness loop later reaches the same URL X as a direct skill, resolveURL returns the cached dep, and h.Skills[i] = localPath overwrites the original entry in-place — but the earlier appended entry remains. The returned deps slice is correctly deduplicated via appendDep, but h.Skills violates the documented "deduplicated" contract. Go's range captures the original slice length so the duplicate isn't re-processed, but downstream consumers of h.Skills (e.g., sandbox setup) would see the same skill path twice.
  Remediation: Guard the top-level h.Skills[i] = localPath assignment by skipping it if state.appended[dep.URL] is already true (the transitive path already appended the skill), or deduplicate h.Skills before returning.

Low

• [edge-case] internal/resolve/resolve.go — When MaxResources is set to a negative value, the state.resourceCount >= state.maxResources check is always true, causing every URL resolution to fail with a confusing "exceeded maximum resource count" error. Unlike MaxDepth, which documents negative-means-disabled semantics, MaxResources has no documented behavior for negative values.

• [test-adequacy] internal/resolve/resolve_test.go — The cycle detection test (TestResolveHarness_CycleDetection) relies on cycle detection firing before integrity checking in the current resolveURL implementation. If check ordering ever changes, the test would pass for the wrong reason (integrity failure instead of cycle detection). The test comment acknowledges this dependency.

• [path-traversal-guidance] internal/resolve/relurl.go — ResolveRelativeURL faithfully follows .. segments per RFC 3986, allowing a skill's frontmatter to reference paths outside the parent skill's directory while remaining under the same allowed domain prefix. The allowlist check and SHA256 integrity hash provide guardrails, and the code documents "trusting a skill means trusting its entire transitive dependency closure." Operators should use narrow, repo-scoped allowed_remote_resources prefixes rather than broad domain prefixes.

• [scope-creep] internal/skill/skill.go:16 / docs/ADRs/0038-universal-harness-access.md — The Policy field addition expands Phase 2 scope beyond the original plan ("agent and policy resources are treated as leaf nodes"). The ADR and plan were updated to authorize this expansion, and the implementation correctly treats skill-level policies as leaf nodes (fetched but no recursion). The runtime semantics of skill-level policies remain deferred. See also: [architectural-alignment] finding.

• [implementation-plan-drift] internal/resolve/resolve.go — MaxDepth=0 means "use default (10)" rather than "disable transitive resolution," requiring a negative value to disable. This is documented in the updated plan but diverges from common CLI conventions where 0 often means disabled.

• [code-organization] internal/resolve/resolve.go — The resolveState struct bundles both graph-traversal state (inProgress, resolved, appended, resourceCount) and configuration limits (maxDepth, maxResources). The limits originate in ResolveOpts and are cached in resolveState after default handling — a reasonable pattern, but the mixed concerns could be separated for clarity.

• [error-handling-idiom] internal/resolve/relurl.go — The two error cases (invalid relative ref, invalid parent URL) use inline fmt.Errorf rather than package-level sentinel errors. The codebase convention (see fetch/fetch.go) defines sentinels for expected error conditions to aid programmatic error inspection.

• [naming-convention] internal/resolve/resolve.go — appendDep abbreviates "Dependency" while the type itself is named Dependency. The codebase uses full descriptive names for private helpers.

• [incomplete-doc] docs/plans/universal-harness-access.md — The ResolveOpts pseudocode (line ~947) omits the MaxDepth and MaxResources fields, and resolveResourceWithLimits references policy.MaxDepth rather than opts.MaxDepth. The phase2 plan documents the actual implementation, but readers consulting the main design doc may be confused by the discrepancy.

• [documentation-coherence] docs/plans/universal-harness-access-phase2.md — The term "leaf node" is overloaded: originally meaning "resources that don't participate in transitive resolution at all," now also used for "resources that are transitively fetched but cannot themselves have dependencies." Consider using more precise language.

Info

• [missing-test] internal/resolve/resolve_test.go — No test covers the scenario where a URL appears both as a direct skill in h.Skills and as a transitive dependency, which would expose the h.Skills duplication described in the medium-severity finding.

• [api-contract] internal/resolve/resolve.go — The prior high-severity finding (diamond dedup silently skipping integrity hash verification on re-encounter) has been fixed. resolveURL now compares expectedHash against dep.SHA256 when returning a cached Dependency and returns an error on mismatch. A corresponding test (TestResolveHarness_ConflictingHashesForSameURL) validates this behavior.

• [architectural-alignment] docs/ADRs/0038-universal-harness-access.md — The ADR update creates a dependency between Phase 2 (fetch skill-level policies) and a future phase (policy composition runtime semantics). If the future design determines that skill-level policies should not be fetched, the Policy field would need to be reverted.

• [documentation-comment] internal/resolve/resolve.go — resolveTransitiveDeps (51 lines) lacks a doc comment. The codebase documents significant unexported functions.

• [test-organization] internal/skill/skill_test.go — TestParseFrontmatter_PolicyField uses if/t.Fatalf style while all other tests in the file use testify assertions (require.NoError, assert.Equal).

Previous run (11)

Review

Findings

High

• [api-contract] internal/resolve/resolve.go — Diamond dedup silently skips integrity hash verification on re-encounter. When resolveURL finds a URL in state.resolved[cleanURL], it returns the cached Dependency immediately without comparing the expectedHash from the new rawURL against dep.SHA256. Two references to the same base URL with different #sha256=... fragments (e.g., skill A depends on X#sha256=aaa and skill B depends on X#sha256=bbb) will silently accept the first-resolved hash and ignore the second. This undermines the integrity verification model — a stale or incorrect hash reference is silently accepted.
  Remediation: After the state.resolved[cleanURL] lookup succeeds, compare expectedHash from ParseIntegrityHash(rawURL) against dep.SHA256. If they differ, return an error indicating conflicting integrity hashes for the same URL.

Medium

• [scope-creep] internal/skill/skill.go:16 / docs/ADRs/0038-universal-harness-access.md — The PR adds a Policy field to SkillMeta and changes the ADR text from explicitly deferring skill-level policy ("Skill-level policy: is deferred") to authorizing it ("Skills may declare a policy: reference"). The phase 2 plan scope note states "agent and policy resources are treated as leaf nodes," but the implementation allows skills to reference policies transitively. The trust model for skill-referenced policies (override harness policy? compose? ignored at runtime?) is not addressed in the ADR update. See also: [incomplete-doc] findings on phase2.md.

• [incomplete-doc] docs/plans/universal-harness-access-phase2.md:11 — The scope note says "Phase 2 limits transitive resolution to skills only — agent and policy resources are treated as leaf nodes," but the implementation adds policy resolution from skill frontmatter. The plan's SkillMeta struct definition (line 38) also omits the new Policy field. Additionally, the plan states "Setting MaxDepth to 0 disables transitive resolution entirely" (line 128), but the implementation uses MaxDepth < 0 to disable and MaxDepth == 0 to mean "use default (10)." See also: [scope-creep] finding.

• [incomplete-doc] docs/plans/universal-harness-access-phase2.md:167 — The resolveTransitiveDeps specification says "Agent and policy resources skip this step" but does not document the new policy resolution logic where skills can declare policy: references that are fetched and cached (but not added to h.Skills). The implementation (resolve.go lines 330–343) adds this behavior without a corresponding plan update.

Low

• [code-organization] internal/resolve/resolve.go — The resolveURL function signature has grown to 9 parameters across 4 lines (ctx, field, rawURL, h, opts, state, depth, maxDepth, maxResources, recurse). Consider bundling recursion-related parameters (state, depth, maxDepth, maxResources, recurse) into a struct to improve readability.

• [error-handling-idiom] internal/resolve/resolve.go — Two error messages lack context for operational debugging: (1) "exceeded maximum resource count of %d" omits the URL that triggered the limit, (2) "resolving transitive deps for %s" uses the field accessor string (e.g., skills[0]) rather than the actual URL.

• [test-adequacy] internal/resolve/resolve_test.go:555 — The cycle detection test (TestResolveHarness_CycleDetection) performs two rounds of hash recomputation that leave A and B with mutually stale hash references. The test passes because cycle detection fires before integrity checking in the current implementation, but if check ordering ever changes, the test would pass for the wrong reason (integrity failure instead of cycle detection).

Info

• [missing-test] internal/resolve/resolve_test.go — No test covers the case where two skills reference the same base URL with different #sha256=... hashes. Adding this test would expose the silent hash-mismatch acceptance described in the high-severity finding.

Previous run (12)

Review

Findings

Medium

• [duplicate-data] internal/resolve/resolve.go — ResolveHarness doc comment says "Returns the deduplicated list of resolved dependencies" but the top-level append sites (lines ~82, 90, 99) are unconditional. When a URL appears both as a direct skill in h.Skills and as a transitive dependency of another skill, resolveURL returns the cached Dependency from state.resolved, and ResolveHarness appends it to state.deps again. The state.appended guard in resolveTransitiveDeps prevents duplicate h.Skills entries but does not prevent duplicate state.deps entries at the top-level call sites. The current consumer (internal/cli/run.go) uses deps for logging, so this produces duplicate "Resolved/Fetched" lines but no incorrect behavior.
  Remediation: Guard the state.deps = append(state.deps, dep) calls in ResolveHarness with a check against state.resolved or state.appended, or deduplicate the returned slice before returning.

Low

• [api-contract] internal/resolve/resolve.go — Setting MaxDepth=0 in ResolveOpts cannot disable transitive resolution. The zero-value defaulting (lines 67–69) replaces 0 with DefaultMaxDepth=10, so the maxDepth > 0 guard in resolveURL (line 175) is unreachable through ResolveHarness. If the intent is to allow callers to disable transitive resolution (e.g., for Phase 1 compatibility), a separate boolean field or a sentinel value like -1 would be needed. PR 3 (CLI wiring) would be the natural place to address this.

• [widened-trust-surface] internal/resolve/resolve.go — Transitive dependency resolution expands the set of URLs fetched beyond those explicitly listed in the harness. A skill's frontmatter can declare relative references that resolve (via RFC 3986) to different paths on the same allowed domain. The existing controls — allowed_remote_resources prefix check and SHA256 integrity verification — still apply to every transitive dep, and the parent skill's own content is SHA256-pinned by the harness author. This is by design per ADR-0038, but the transitive trust implications should be documented: trusting a skill means trusting its entire dependency closure.

• [field-prefix-fragility] internal/resolve/resolve.go:175 — The gate for transitive resolution uses strings.HasPrefix(field, "skills") to decide whether to recurse. The field names are constructed internally ("skills[0]", "skills[url:dep0]"), so this is not user-controllable. However, if a future field type starts with "skills" (e.g., "skills_legacy"), it would inadvertently trigger recursion. A dedicated boolean parameter would be more robust.

• [resource-exhaustion-defaults] internal/resolve/resolve.go — The default limits (DefaultMaxDepth=10, DefaultMaxResources=50) combined with FetchPolicy.MaxSizeBytes allow up to 500 MB of fetched data per resolve operation. For a client-side CLI this is acceptable, but documenting the expected dependency graph shape (and recommending tighter limits for CI environments) would help operators tune these values. See also: [api-contract] finding at this location.

Previous run (13)

Review

Findings

Medium

• [duplicate-data] internal/resolve/resolve.go — Diamond dependencies produce duplicate Dependency entries in the slice returned by ResolveHarness. When resolveURL returns a cached diamond dep (via state.resolved[cleanURL] early return), the caller in resolveTransitiveDeps unconditionally appends it to state.deps again. The state.appended guard correctly prevents duplicate h.Skills entries, but no equivalent guard exists for state.deps. For a diamond graph A→B→D, A→C→D, dep D appears twice in the returned slice. The current consumer (internal/cli/run.go:152) iterates deps for log output, so this produces duplicate "Resolved/Fetched" lines. The test TestResolveHarness_DiamondDedup masks this by checking unique URLs via a map rather than asserting on len(deps) directly.
  Remediation: Guard the state.deps = append(state.deps, dep) call in resolveTransitiveDeps with a deduplication check (e.g., only append if !state.resolved[dep.URL] was false before the resolveURL call), or deduplicate the returned slice in ResolveHarness before returning.

Low

• [state-leak] internal/resolve/resolve.go — state.inProgress[cleanURL] is set early in resolveURL but not cleaned up on error paths. If the function errors after setting inProgress (allowlist check, fetch failure, hash mismatch, transitive dep error), the URL remains in inProgress and resourceCount stays incremented. Currently benign since errors propagate to abort the entire resolution, but would cause false "circular dependency" errors if error handling is ever changed to continue after failures. Consider defer delete(state.inProgress, cleanURL) for robustness.

• [stale-doc] docs/ADRs/0038-universal-harness-access.md:78 — ADR states "Skill-level policy: is deferred" but this PR adds skill-level policy resolution (parsing policy: from SKILL.md frontmatter and resolving/fetching the referenced resource). The Phase 2 plan explicitly includes this capability, so the code is correct — but the ADR text is now stale and could confuse readers.

• [doc-comment] internal/resolve/resolve.go — The ResolveHarness doc comment says "The harness is modified in place" but does not mention that h.Skills may grow to include transitively resolved skill dependencies. This side effect is the designed behavior per the Phase 2 plan (Option A), but the contract should be explicit for callers inspecting h.Skills after resolution.

Previous run (14)

Review

Findings

Medium

• [logic-error] internal/resolve/resolve.go — h.Skills can contain duplicate entries when a URL appears both as a direct skill in h.Skills and as a transitive dependency of another skill. When resolveTransitiveDeps discovers URL X, it appends X's local path to h.Skills and marks state.appended[X]=true. When the top-level ResolveHarness loop later reaches the same URL X as a direct skill, resolveURL returns the cached dep, and h.Skills[i] = localPath overwrites the original entry in-place — but the earlier appended entry remains. The returned deps slice is correctly deduplicated via appendDep, but h.Skills violates the documented "deduplicated" contract. Go's range captures the original slice length so the duplicate isn't re-processed, but downstream consumers of h.Skills (e.g., sandbox setup) would see the same skill path twice.
  Remediation: Guard the top-level h.Skills[i] = localPath assignment by skipping it if state.appended[dep.URL] is already true (the transitive path already appended the skill), or deduplicate h.Skills before returning.

Low

• [edge-case] internal/resolve/resolve.go — When MaxResources is set to a negative value, the state.resourceCount >= state.maxResources check is always true, causing every URL resolution to fail with a confusing "exceeded maximum resource count" error. Unlike MaxDepth, which documents negative-means-disabled semantics, MaxResources has no documented behavior for negative values.

• [test-adequacy] internal/resolve/resolve_test.go — The cycle detection test (TestResolveHarness_CycleDetection) relies on cycle detection firing before integrity checking in the current resolveURL implementation. If check ordering ever changes, the test would pass for the wrong reason (integrity failure instead of cycle detection). The test comment acknowledges this dependency.

• [path-traversal-guidance] internal/resolve/relurl.go — ResolveRelativeURL faithfully follows .. segments per RFC 3986, allowing a skill's frontmatter to reference paths outside the parent skill's directory while remaining under the same allowed domain prefix. The allowlist check and SHA256 integrity hash provide guardrails, and the code documents "trusting a skill means trusting its entire transitive dependency closure." Operators should use narrow, repo-scoped allowed_remote_resources prefixes rather than broad domain prefixes.

• [scope-creep] internal/skill/skill.go:16 / docs/ADRs/0038-universal-harness-access.md — The Policy field addition expands Phase 2 scope beyond the original plan ("agent and policy resources are treated as leaf nodes"). The ADR and plan were updated to authorize this expansion, and the implementation correctly treats skill-level policies as leaf nodes (fetched but no recursion). The runtime semantics of skill-level policies remain deferred. See also: [architectural-alignment] finding.

• [implementation-plan-drift] internal/resolve/resolve.go — MaxDepth=0 means "use default (10)" rather than "disable transitive resolution," requiring a negative value to disable. This is documented in the updated plan but diverges from common CLI conventions where 0 often means disabled.

• [code-organization] internal/resolve/resolve.go — The resolveState struct bundles both graph-traversal state (inProgress, resolved, appended, resourceCount) and configuration limits (maxDepth, maxResources). The limits originate in ResolveOpts and are cached in resolveState after default handling — a reasonable pattern, but the mixed concerns could be separated for clarity.

• [error-handling-idiom] internal/resolve/relurl.go — The two error cases (invalid relative ref, invalid parent URL) use inline fmt.Errorf rather than package-level sentinel errors. The codebase convention (see fetch/fetch.go) defines sentinels for expected error conditions to aid programmatic error inspection.

• [naming-convention] internal/resolve/resolve.go — appendDep abbreviates "Dependency" while the type itself is named Dependency. The codebase uses full descriptive names for private helpers.

• [incomplete-doc] docs/plans/universal-harness-access.md — The ResolveOpts pseudocode (line ~947) omits the MaxDepth and MaxResources fields, and resolveResourceWithLimits references policy.MaxDepth rather than opts.MaxDepth. The phase2 plan documents the actual implementation, but readers consulting the main design doc may be confused by the discrepancy.

• [documentation-coherence] docs/plans/universal-harness-access-phase2.md — The term "leaf node" is overloaded: originally meaning "resources that don't participate in transitive resolution at all," now also used for "resources that are transitively fetched but cannot themselves have dependencies." Consider using more precise language.

Info

• [missing-test] internal/resolve/resolve_test.go — No test covers the scenario where a URL appears both as a direct skill in h.Skills and as a transitive dependency, which would expose the h.Skills duplication described in the medium-severity finding.

• [api-contract] internal/resolve/resolve.go — The prior high-severity finding (diamond dedup silently skipping integrity hash verification on re-encounter) has been fixed. resolveURL now compares expectedHash against dep.SHA256 when returning a cached Dependency and returns an error on mismatch. A corresponding test (TestResolveHarness_ConflictingHashesForSameURL) validates this behavior.

• [architectural-alignment] docs/ADRs/0038-universal-harness-access.md — The ADR update creates a dependency between Phase 2 (fetch skill-level policies) and a future phase (policy composition runtime semantics). If the future design determines that skill-level policies should not be fetched, the Policy field would need to be reverted.

• [documentation-comment] internal/resolve/resolve.go — resolveTransitiveDeps (51 lines) lacks a doc comment. The codebase documents significant unexported functions.

• [test-organization] internal/skill/skill_test.go — TestParseFrontmatter_PolicyField uses if/t.Fatalf style while all other tests in the file use testify assertions (require.NoError, assert.Equal).

Previous run (15)

Review

Findings

High

• [api-contract] internal/resolve/resolve.go — Diamond dedup silently skips integrity hash verification on re-encounter. When resolveURL finds a URL in state.resolved[cleanURL], it returns the cached Dependency immediately without comparing the expectedHash from the new rawURL against dep.SHA256. Two references to the same base URL with different #sha256=... fragments (e.g., skill A depends on X#sha256=aaa and skill B depends on X#sha256=bbb) will silently accept the first-resolved hash and ignore the second. This undermines the integrity verification model — a stale or incorrect hash reference is silently accepted.
  Remediation: After the state.resolved[cleanURL] lookup succeeds, compare expectedHash from ParseIntegrityHash(rawURL) against dep.SHA256. If they differ, return an error indicating conflicting integrity hashes for the same URL.

Medium

• [scope-creep] internal/skill/skill.go:16 / docs/ADRs/0038-universal-harness-access.md — The PR adds a Policy field to SkillMeta and changes the ADR text from explicitly deferring skill-level policy ("Skill-level policy: is deferred") to authorizing it ("Skills may declare a policy: reference"). The phase 2 plan scope note states "agent and policy resources are treated as leaf nodes," but the implementation allows skills to reference policies transitively. The trust model for skill-referenced policies (override harness policy? compose? ignored at runtime?) is not addressed in the ADR update. See also: [incomplete-doc] findings on phase2.md.

• [incomplete-doc] docs/plans/universal-harness-access-phase2.md:11 — The scope note says "Phase 2 limits transitive resolution to skills only — agent and policy resources are treated as leaf nodes," but the implementation adds policy resolution from skill frontmatter. The plan's SkillMeta struct definition (line 38) also omits the new Policy field. Additionally, the plan states "Setting MaxDepth to 0 disables transitive resolution entirely" (line 128), but the implementation uses MaxDepth < 0 to disable and MaxDepth == 0 to mean "use default (10)." See also: [scope-creep] finding.

• [incomplete-doc] docs/plans/universal-harness-access-phase2.md:167 — The resolveTransitiveDeps specification says "Agent and policy resources skip this step" but does not document the new policy resolution logic where skills can declare policy: references that are fetched and cached (but not added to h.Skills). The implementation (resolve.go lines 330–343) adds this behavior without a corresponding plan update.

Low

• [code-organization] internal/resolve/resolve.go — The resolveURL function signature has grown to 9 parameters across 4 lines (ctx, field, rawURL, h, opts, state, depth, maxDepth, maxResources, recurse). Consider bundling recursion-related parameters (state, depth, maxDepth, maxResources, recurse) into a struct to improve readability.

• [error-handling-idiom] internal/resolve/resolve.go — Two error messages lack context for operational debugging: (1) "exceeded maximum resource count of %d" omits the URL that triggered the limit, (2) "resolving transitive deps for %s" uses the field accessor string (e.g., skills[0]) rather than the actual URL.

• [test-adequacy] internal/resolve/resolve_test.go:555 — The cycle detection test (TestResolveHarness_CycleDetection) performs two rounds of hash recomputation that leave A and B with mutually stale hash references. The test passes because cycle detection fires before integrity checking in the current implementation, but if check ordering ever changes, the test would pass for the wrong reason (integrity failure instead of cycle detection).

Info

• [missing-test] internal/resolve/resolve_test.go — No test covers the case where two skills reference the same base URL with different #sha256=... hashes. Adding this test would expose the silent hash-mismatch acceptance described in the high-severity finding.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

See the review comment for full details.