fix(harness): fetch full skill directory from URL base via ForgeClient

[waynesun09]

Summary

• Fixes bug(harness): v0.22.0 URL-base skill resolution drops companion files, breaks review agent for all users #2705 — URL-base skill resolution in v0.22.0 drops companion files (sub-agents/, scripts/, meta-prompts/), breaking the review agent for all per-org installations
• Adds ParseRawContentURL() to parse raw.githubusercontent.com URLs into forge info (owner/repo/ref/path)
• Adds ForgeClient field to ComposeOpts so fetchBaseSkill() can use ListDirectoryContents + GetFileContentAtRef to fetch the full skill directory tree
• Requires ForgeClient (set GITHUB_TOKEN) or fails fast with a clear error directing users to run fullsend lock for offline pre-caching
• Adds empty-field validation to ParseRawContentURL
• Fixes ForgeClient injection in lock.go to match the run.go pattern
• Wires ForgeClient into ComposeOpts in both run.go and lock.go CLI callers

Impact

Only the pr-review skill has companion files today (meta-prompt.md, sub-agents/). Other skills (code-review, code-implementation, fix-review, issue-labels, retro-analysis) are SKILL.md-only and are unaffected by this bug or this fix.

Test plan

• ParseRawContentURL() — 8 unit tests covering valid URLs, edge cases, wrong host, missing segments
• fetchBaseSkill() with FakeClient — full-directory fetch, API error, missing SKILL.md, no ForgeClient error, parse error, audit error
• Cache hit path — offline cache hit test preserved
• CLI tests — lock and run tests updated to match new fail-fast behavior
• go build ./... — compiles cleanly
• go test ./internal/forge/... ./internal/harness/... ./internal/cli/... — all pass

[qodo-code-review]

PR Summary by Qodo

Fix URL-base skill composition to fetch full directory via ForgeClient
🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Description

• Fix URL-base skill composition to include companion files, not just SKILL.md
• Add raw.githubusercontent.com URL parsing to drive Forge-based directory fetch
• Wire ForgeClient through CLI compose callers with safe SKILL.md-only fallback

Diagram

graph TD
  A["CLI (run/lock)"] --> B["harness.LoadWithBase"] --> C["fetchBaseSkill"] --> D{ForgeClient set?}
  D -->|yes| E["ParseRawContentURL"] --> F["ForgeClient: list+get"] --> G["CachePutDir + URL index"]
  D -->|no / error| H["FetchURL SKILL.md only"] --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Download repo archive (zip/tar) for the base skill path

• ➕ Fewer API calls than per-file GetFileContentAtRef
• ➕ Naturally preserves directory structure and companion files
• ➖ More bytes transferred than needed when only one skill directory is required
• ➖ More complex extraction/security considerations (archive handling)

2. Require forge-style base URLs (github.com) instead of raw URLs

• ➕ Avoids special-case parsing of raw.githubusercontent.com URLs
• ➕ Keeps base resolution aligned with existing forge URL parsing
• ➖ Breaking change for existing per-org configs using raw URL bases
• ➖ Still needs a directory fetch mechanism for multi-file skills

Recommendation: The PR’s approach (parse raw URLs and use ForgeClient Contents API for recursive directory fetch, with SKILL.md-only fallback) is the best compatibility/performance tradeoff: it fixes multi-file skills without forcing config changes, and preserves backward behavior when auth/client is unavailable.

Files changed (6) +378 / -4

Enhancement (1) +55 / -0

url.goAdd ParseRawContentURL for raw.githubusercontent.com URL bases +55/-0

Add ParseRawContentURL for raw.githubusercontent.com URL bases

• Introduces ParseRawContentURL to extract owner/repo/ref/path from raw.githubusercontent.com URLs, stripping fragments and validating scheme/host. Returns ForgeURLInfo suitable for subsequent forge API calls.

internal/forge/url.go

Bug fix (3) +128 / -4

lock.goPass best-effort ForgeClient into base composition for lock +9/-0

Pass best-effort ForgeClient into base composition for lock

• Constructs a GitHub ForgeClient when a token is available and passes it into harness.ComposeOpts. Keeps behavior safe by allowing nil clients, which triggers SKILL.md-only fallback.

internal/cli/lock.go

run.goPass ForgeClient into base composition for run (flag or token) +11/-0

Pass ForgeClient into base composition for run (flag or token)

• Threads a ForgeClient into harness.ComposeOpts, preferring an injected client from flags and falling back to creating one from the resolved token. Enables full-directory skill fetches during agent execution.

internal/cli/run.go

compose.goFetch full skill directory via ForgeClient with SKILL.md fallback +108/-4

Fetch full skill directory via ForgeClient with SKILL.md fallback

• Extends ComposeOpts with ForgeClient and updates fetchBaseSkill to prefer a ForgeClient-backed directory fetch for URL bases. Implements fetchBaseSkillDir using ListDirectoryContents + GetFileContentAtRef, retains caching/indexing behavior, and only emits the 'SKILL.md-only' warning when the cached tree is actually SKILL.md-only.

internal/harness/compose.go

Tests (2) +195 / -0

url_test.goUnit test ParseRawContentURL across valid and invalid cases +86/-0

Unit test ParseRawContentURL across valid and invalid cases

• Adds table-driven coverage for parsing SHA/tag/branch refs, empty paths, fragment stripping, and validation errors for wrong host/scheme/short paths.

internal/forge/url_test.go

compose_test.goTest full-directory fetch, fallback behavior, and missing SKILL.md +109/-0

Test full-directory fetch, fallback behavior, and missing SKILL.md

• Adds ForgeClient-driven tests verifying companion files are cached, that directory-fetch errors fall back to SKILL.md-only retrieval, and that directories lacking SKILL.md fail as expected.

internal/harness/compose_test.go

[github-actions]

Site preview

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

Commit: dca90b1342449533fc8da41b852e3f55f4d8ef33

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 9:52 PM UTC · Ended 9:53 PM UTC
Commit: 7f5ce3c · View workflow run →

[codecov]

Codecov Report

❌ Patch coverage is 80.19802% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/forge/url.go	77.14%	4 Missing and 4 partials ⚠️
internal/cli/run.go	33.33%	2 Missing and 2 partials ⚠️
internal/harness/compose.go	90.90%	1 Missing and 3 partials ⚠️
internal/cli/lock.go	66.66%	1 Missing and 1 partial ⚠️
internal/fetch/cache.go	80.00%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (1) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 bug(harness): v0.22.0 URL-base skill resolution drops companion files, breaks review agent for all users

✅ Compliance rules (platform): 51 rules

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

1. Allowlist path-scope bypass 🐞 Bug ⛨ Security

Description

fetchBaseSkill only validates allowed_remote_resources against the SKILL.md URL, but when
ForgeClient is set it downloads every file in the skill directory without checking that each
companion path is allowed. If an allowlist intentionally whitelists only the SKILL.md URL (or other
narrow prefixes), ForgeClient implicitly expands what can be fetched beyond that policy intent.

Code

internal/harness/compose.go[R783-791]

	allowedBy := matchingAllowedPrefix(skillFileURL, allowlist)
	if allowedBy == "" {
		return Dependency{}, "", fmt.Errorf("base %s: URL %q is not in allowed_remote_resources", field, skillFileURL)
	}

+	// Check cache first (keyed by SKILL.md URL for backward compat).
	hash, indexHit := urlIndexLookup(opts.WorkspaceRoot, skillFileURL)
	if indexHit {
		treeHash, ok := urlIndexLookup(opts.WorkspaceRoot, "skill:"+skillFileURL)

Relevance

⭐⭐⭐ High

Team enforces strict allowed_remote_resources prefixing for URL fetches; directory fetch bypass
likely unacceptable (see #1555, #2525).

PR-#1555
PR-#2525
PR-#2690

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code enforces the allowlist only on the SKILL.md URL, then uses ForgeClient to fetch all files
in the directory without further allowlist checks; allowlist prefix matching is the intended
guardrail for remote resource access.

internal/harness/compose.go[779-905]
internal/harness/harness.go[793-813]

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()` checks `allowed_remote_resources` only for `.../SKILL.md`, but the ForgeClient path fetches additional sibling/nested files without any allowlist validation. This changes the effective policy from “only this URL/prefix is allowed” to “any file in this directory is allowed” when ForgeClient is available.

## Issue Context
- `allowed_remote_resources` is enforced via prefix matching (`MatchingAllowedPrefixInList`).
- The ForgeClient directory path should respect the same allowlist semantics as the URL fetch path.

## Fix Focus Areas
- internal/harness/compose.go[783-905]

## Suggested fix approach
1. Before doing a ForgeClient directory fetch, ensure the allowlist authorizes directory-wide access (e.g., require that `matchingAllowedPrefix(skillDirURL+"/", allowlist)` is non-empty and use that as `allowedBy`).
2. Additionally (or instead), for each `e.Path` fetched, synthesize the corresponding raw URL (`skillDirURL + "/" + e.Path`) and require `matchingAllowedPrefix(...)` to succeed before fetching/caching that file.
3. If a file is not allowed, either:
  - fail with a clear error stating which path is blocked, or
  - skip the file and attach a warning (but be careful: skipping companion files reintroduces the original bug for multi-file skills).

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

---

2. Silent ForgeClient fallback 🐞 Bug ◔ Observability

Description

When fetchBaseSkillDir returns an error, fetchBaseSkill discards the error and silently falls back
to SKILL.md-only fetching. This hides the underlying directory-fetch failure mode
(auth/rate-limit/API issues) and makes it hard to diagnose why companion files are still missing.

Code

internal/harness/compose.go[R824-833]

+	if opts.ForgeClient != nil {
+		dep, treePath, err := fetchBaseSkillDir(ctx, field, skillDirURL, skillFileURL, skillPath, allowedBy, opts)
+		if err == nil {
+			return dep, treePath, nil
+		}
+		// Fall through to SKILL.md-only fetch on parse/API errors so the
+		// agent can still run with degraded skill content.
+	}
+
+	warn := fmt.Sprintf("skill %q was fetched from a URL base with only SKILL.md; companion files (sub-agents, scripts, meta-prompts) may be missing — use a forge-format URL for multi-file skills", skillPath)

Relevance

⭐⭐ Medium

Team often improves diagnosability, but no clear precedent on preserving ForgeClient error during
fallback.

PR-#2697
PR-#2444

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The ForgeClient attempt explicitly falls through on any error without returning/recording it, so
callers cannot tell whether the multi-file path was attempted and why it failed.

internal/harness/compose.go[821-833]

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()` attempts a ForgeClient full-directory fetch, but if it fails it falls through to SKILL.md-only fetch without surfacing the original error. Users only see the generic “only SKILL.md” warning, which loses the actionable reason why the full-directory path failed.

## Issue Context
This fallback can reintroduce the original broken behavior (missing companion files) under conditions like rate limiting or insufficient token permissions, but currently provides no diagnostic breadcrumb.

## Fix Focus Areas
- internal/harness/compose.go[821-872]

## Suggested fix approach
- Capture the error from `fetchBaseSkillDir` and include it in the returned `Dependency.Warning` when falling back (e.g., append `"; full-directory fetch failed: <err>"`).
- Alternatively/additionally, emit a structured warning log at the call site (where a logger/printer exists), but ensure it is visible in both `run` and `lock` flows.

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

---

3. N+1 file content calls 🐞 Bug ➹ Performance

Description

fetchBaseSkillDir lists the directory and then calls GetFileContentAtRef once per file entry, which
can result in hundreds/thousands of API calls for large skill directories. This can significantly
slow fullsend lock/run and increase the likelihood of rate limiting.

Code

internal/harness/compose.go[R884-905]

+	entries, err := opts.ForgeClient.ListDirectoryContents(ctx, forgeInfo.Owner, forgeInfo.Repo, forgeInfo.Path, forgeInfo.Ref, true)
+	if err != nil {
+		return Dependency{}, "", fmt.Errorf("base %s: listing skill directory %s: %w", field, skillPath, err)
+	}
+
+	files := make(map[string][]byte)
+	for _, e := range entries {
+		if e.Type != "file" {
+			continue
+		}
+		var fullPath string
+		if forgeInfo.Path == "" {
+			fullPath = e.Path
+		} else {
+			fullPath = forgeInfo.Path + "/" + e.Path
+		}
+		content, fErr := opts.ForgeClient.GetFileContentAtRef(ctx, forgeInfo.Owner, forgeInfo.Repo, fullPath, forgeInfo.Ref)
+		if fErr != nil {
+			return Dependency{}, "", fmt.Errorf("base %s: fetching file %s for skill %s: %w", field, e.Path, skillPath, fErr)
+		}
+		files[e.Path] = content
+	}

Relevance

⭐⭐ Medium

Some history of addressing GitHub API/rate-limit inefficiencies, but no direct precedent for
batching directory content fetches.

PR-#1487
PR-#1040

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation loops over every returned file entry and performs a separate API call to fetch
its content; the GitHub client code shows directory listings can include up to 1000 files, implying
a large number of follow-on requests.

internal/harness/compose.go[875-906]
internal/forge/github/github.go[1043-1063]
internal/forge/github/github.go[1065-1145]

Agent prompt

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

## Issue description
The full-directory skill fetch performs an API request per file (`GetFileContentAtRef` inside a loop). For larger directories this amplifies latency and rate-limit pressure.

## Issue Context
- GitHub `ListDirectoryContents` caps listings at up to `maxDirFiles` (1000), which implies the follow-on fetch loop can also reach that scale.

## Fix Focus Areas
- internal/harness/compose.go[884-905]
- internal/forge/github/github.go[1043-1063]
- internal/forge/github/github.go[1065-1145]

## Suggested fix approach
Choose one:
1. Add a conservative max file count/total-bytes guard for skill directory fetch (e.g., fail with a clear error or fall back once exceeding a threshold).
2. Reduce API calls by implementing a batch fetch mechanism in `forge.Client` (e.g., Git Trees API to fetch blob SHAs + batch blob retrieval, or an archive/zipball download + extraction).
3. If keeping per-file fetch, fetch with bounded concurrency (errgroup + semaphore) to reduce wall-clock time, and add explicit rate-limit/backoff handling at the skill fetch layer.

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

---

4. fetchBaseSkill() allows SKILL.md-only 📎 Requirement gap ≡ Correctness

Description

fetchBaseSkill() intentionally falls back to a SKILL.md-only fetch (with just a warning) when
ForgeClient is nil or when directory fetch via ForgeClient fails, which allows multi-file skills
to run missing required companion files. This violates the requirement to fail fast with a clear
error (and to ensure companion files are included) for URL-based multi-file skill resolution.

Code

internal/harness/compose.go[R821-834]

+	// When ForgeClient is available, fetch the full skill directory via the
+	// GitHub Contents API. This retrieves companion files (sub-agents/,
+	// scripts/, meta-prompts/) that the SKILL.md-only path misses.
+	if opts.ForgeClient != nil {
+		dep, treePath, err := fetchBaseSkillDir(ctx, field, skillDirURL, skillFileURL, skillPath, allowedBy, opts)
+		if err == nil {
+			return dep, treePath, nil
+		}
+		// Fall through to SKILL.md-only fetch on parse/API errors so the
+		// agent can still run with degraded skill content.
+	}
+
+	warn := fmt.Sprintf("skill %q was fetched from a URL base with only SKILL.md; companion files (sub-agents, scripts, meta-prompts) may be missing — use a forge-format URL for multi-file skills", skillPath)
+

Relevance

⭐ Low

Repo recently chose warning + continue for SKILL.md-only URL-base skills (backward compat) rather
than fail-fast (#2697).

PR-#2697

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires (a) URL-base skill resolution to include companion files for multi-file
skills and (b) to fail fast with a clear error if a multi-file skill ends up SKILL.md-only. The new
logic explicitly falls through to a SKILL.md-only fetch after ForgeClient parse/API errors (and when
ForgeClient is nil), emitting only a warning; the added test
TestFetchBaseSkill_ForgeClient_FallbackOnError asserts this warning-and-continue behavior.

URL-base skill resolution includes companion files for multi-file skills
Fail fast with clear error when multi-file URL-based skill is missing companion files
internal/harness/compose.go[821-834]
internal/harness/compose_test.go[2800-2834]

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()` currently degrades to a SKILL.md-only directory when full directory fetch fails (or is unavailable), and proceeds with only a warning. For multi-file skills (e.g., `skills/pr-review`), this should be a hard error with a clear, actionable message identifying missing companion paths.

## Issue Context
- The compliance requirement expects URL-base skill resolution to include companion files for multi-file skills, and to fail fast when they are missing.
- The current tests also encode the non-fail-fast behavior by asserting success + warning on fallback.

## Fix Focus Areas
- internal/harness/compose.go[821-834]
- internal/harness/compose.go[875-942]
- internal/harness/compose_test.go[2800-2838]

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

---

[qodo-code-review]

2. Allowlist path-scope bypass 🐞 Bug ⛨ Security

fetchBaseSkill only validates allowed_remote_resources against the SKILL.md URL, but when
ForgeClient is set it downloads every file in the skill directory without checking that each
companion path is allowed. If an allowlist intentionally whitelists only the SKILL.md URL (or other
narrow prefixes), ForgeClient implicitly expands what can be fetched beyond that policy intent.

Agent Prompt

## Issue description
`fetchBaseSkill()` checks `allowed_remote_resources` only for `.../SKILL.md`, but the ForgeClient path fetches additional sibling/nested files without any allowlist validation. This changes the effective policy from “only this URL/prefix is allowed” to “any file in this directory is allowed” when ForgeClient is available.

## Issue Context
- `allowed_remote_resources` is enforced via prefix matching (`MatchingAllowedPrefixInList`).
- The ForgeClient directory path should respect the same allowlist semantics as the URL fetch path.

## Fix Focus Areas
- internal/harness/compose.go[783-905]

## Suggested fix approach
1. Before doing a ForgeClient directory fetch, ensure the allowlist authorizes directory-wide access (e.g., require that `matchingAllowedPrefix(skillDirURL+"/", allowlist)` is non-empty and use that as `allowedBy`).
2. Additionally (or instead), for each `e.Path` fetched, synthesize the corresponding raw URL (`skillDirURL + "/" + e.Path`) and require `matchingAllowedPrefix(...)` to succeed before fetching/caching that file.
3. If a file is not allowed, either:
   - fail with a clear error stating which path is blocked, or
   - skip the file and attach a warning (but be careful: skipping companion files reintroduces the original bug for multi-file skills).

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

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 9:56 PM UTC · Completed 9:57 PM UTC
Commit: 1c159e0 · View workflow run →

[ggallen]

The core fix is right — threading ForgeClient into ComposeOpts and using ListDirectoryContents + GetFileContentAtRef to fetch full skill directory trees. But the SKILL.md-only fallback needs to go. It preserves exactly the silent-degradation failure mode that caused the v0.22.0 outage. Skills should either fully resolve via the forge API or fail fast — one code path, no fallback.

Removing the fallback also lets you delete isSkillMDOnly, the conditional warning logic, and most of the old SKILL.md-only fetch path. See inline comments for specifics.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:04 AM UTC · Completed 2:05 AM UTC
Commit: e177a2e · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:09 AM UTC · Completed 2:10 AM UTC
Commit: b4a26d3 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:53 PM UTC · Ended 2:01 PM UTC
Commit: 7f5ce3c · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:04 PM UTC · Completed 2:17 PM UTC
Commit: dca90b1 · View workflow run →

[ralphbean]

LGTM. The latest commit addresses all of the prior review feedback — fallback removed, allowlist check added, lock.go wiring fixed, validation added, dead code cleaned up.

[fullsend-ai-review]

Review

Findings

Medium

• [forge-abstraction-violation] internal/cli/run.go:102 — The new composeForgeClient initialization calls gh.New(token) directly in the CLI layer, which AGENTS.md (lines 80–92) prohibits outside internal/forge/github/. The same pattern exists in internal/cli/lock.go:200. While the PR follows the pre-existing pattern (20+ existing gh.New() calls already in internal/cli/), each new instance deepens the coupling. A centralized factory function in internal/forge/ would reduce the surface for a future multi-forge refactor.
  Remediation: Consider extracting a forge.NewClientFromToken(token) helper to centralize GitHub client construction, addressing both this PR and the existing instances.

---

Labels: PR fixes a bug in harness skill directory fetching with Go code changes.

All review threads resolved; changes addressed in dca90b1

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:50 PM UTC · Completed 3:00 PM UTC
Commit: dca90b1 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2707 — Fix URL-base skill companion file resolution

Timeline: Issue #2705 reported that v0.22.0 URL-base skill resolution only fetched SKILL.md, dropping companion files (sub-agents, scripts). The code agent produced PR #2707 at ~21:03 UTC June 26 (~15min run, successful). However, the review agent then hit a systemic infrastructure failure window lasting ~12 hours (23:19 UTC June 26 through ~12:13 UTC June 27) where agents exited in 10-20 seconds without producing output — affecting PR #2707 and at least two other PRs (#2701, #2537). Human reviewer ggallen provided thorough review at 00:07 UTC June 27 finding 5 significant issues (silent fallback preserving the outage failure mode, missing nil check in lock.go, audit log corruption, missing URL validation, wrong error default). By the time the review agent succeeded at 14:01 UTC June 27, all human feedback had been addressed. The agent found 1 valid medium finding (forge-abstraction-violation: gh.New(token) in CLI layer) that the humans did not flag. PR was approved by ralphbean and merged.

Review quality: The review agent's single finding was valid and additive — it caught a repo convention violation (AGENTS.md forge abstraction rule) that the human reviewers didn't mention. However, the agent never reviewed the original problematic code due to infrastructure failures. The 5 issues ggallen found (fail-open fallback, nil check, audit corruption, input validation, error default) all fall within the agent's sub-agent guidance — particularly the correctness and security sub-agents' explicit coverage of fail-open patterns, nil handling, and error paths. We cannot assess whether the agent would have caught them because it never ran against the original code.

Skipped proposals (covered by existing issues):

• Alerting on consecutive infrastructure failures: #2212
• Review agent detecting silent failure paths: #1393
• Review agent detecting fixes that reintroduce their own bug class: #2376
• Failed review runs updating status comment: #837

Proposals filed

• Auto re-dispatch review after consecutive infrastructure failures with backoff (in fullsend-ai/fullsend)
• Code agent should validate its fix eliminates the reported failure mode, not just add the missing capability (in fullsend-ai/fullsend)