fix: resolve relative plugin paths from URL-sourced harnesses

[rh-hemartin]

Summary

• resolveBaseResources in compose.go fetches agent, policy, and skills from the base URL when a harness is loaded from a remote source, but skips plugins
• Relative plugin paths (e.g. plugins/gopls-lsp) pass through to ResolveRelativeTo, which resolves them against the target repo's .fullsend directory where they do not exist, causing ValidateFilesExist to fail
• Add resolveBasePlugins (with fetchBasePlugin/fetchBasePluginDir) using plugin.json as the marker file, and call it from all three resolution sites in LoadWithBase

Failing runs on v0.34.0

• https://github.com/rh-hemartin-fullsendai/my-app/actions/runs/31102858867/job/92620561677
• https://github.com/openkaiden/kaiden/actions/runs/31091696357/job/92584001666

Test plan

• TestLoadWithBase_URLBase_PluginFetchedAsDir -- forge URL parse path is hit
• TestLoadWithBase_URLBase_PluginOfflineCacheHit -- plugins resolve from cache in offline mode
• TestLoadWithBase_SourceURL_Plugins -- SourceURL no-base path resolves plugins
• TestLoadWithBase_SourceURL_PluginPassesValidateFilesExist -- regression test: ResolveRelativeTo + ValidateFilesExist passes when plugin is resolved to cache path
• Full harness test suite passes with -race
• Verified fix in production: https://github.com/rh-hemartin-fullsendai/my-app/actions/runs/31104568468/job/92626328900

Closes #5977

[qodo-code-review]

PR Summary by Qodo

Fix plugin resolution for URL-sourced harness bases via cached plugin directories

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Fetch relative plugin directories from URL-based harnesses into the local cache.
• Resolve plugin paths to cached absolute directories to satisfy file validation.
• Add regression and offline-cache tests covering SourceURL and URL base paths.

Diagram

graph TD
  A["LoadWithBase"] --> B{"Base/Source URL?"} --> C["resolveBasePlugins"] --> D{"Cache hit\nor offline?"}
  D -->|"yes"| E[("URL cache + index")] --> F["Harness.Plugins =\ncache dir"]
  D -->|"no"| G["fetchBasePluginDir"] --> H["gitfetch.FetchTree"] --> E
  subgraph Legend
    direction LR
    _proc["Function"] ~~~ _dec{"Decision"} ~~~ _db[("Cache/Index")] 
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fold plugins into resolveBaseResources

• ➕ Single entrypoint for all URL-sourced base resources (agent/policy/skills/plugins)
• ➕ Less duplicated control flow in LoadWithBase
• ➖ resolveBaseResources currently models file/dir markers differently; adding plugins may complicate it
• ➖ May require broader refactor/testing than necessary for the regression fix

2. Resolve plugins during ResolveRelativeTo using SourceURL context

• ➕ Keeps composition purely declarative; resolution happens at execution time
• ➕ May reduce fetch work if plugins are unused
• ➖ Harder to reason about (resolution side effects later in lifecycle)
• ➖ ValidateFilesExist failures can still occur before resolution depending on call order
• ➖ Would spread URL-specific behavior into runtime codepaths

3. Require plugins to be absolute URLs/paths when using URL bases

• ➕ Simpler implementation; avoids remote directory fetch logic
• ➕ Clearer authoring rule
• ➖ Breaking change for existing harnesses relying on relative plugin paths
• ➖ Worse UX; pushes complexity to users rather than tooling

Recommendation: Keep the PR’s approach: resolve relative plugins during composition by fetching plugin directories using plugin.json as a marker and rewriting harness plugin entries to cached absolute paths. This matches the existing skills pattern, keeps validation behavior consistent, and centralizes URL allowlist/offline/cache handling where other base resource fetching already lives.

Files changed (2) +397 / -0

Bug fix (1) +198 / -0

compose.goFetch and cache relative plugins for URL-based harness composition +198/-0

Fetch and cache relative plugins for URL-based harness composition

• Adds resolveBasePlugins and plugin directory fetching (fetchBasePlugin/fetchBasePluginDir) using plugin.json as the marker file, with allowlist enforcement and offline/stale-cache behavior. Wires plugin resolution into all three URL-resolution sites in LoadWithBase/loadBaseChain so relative plugin paths are rewritten to cached absolute directories before downstream ResolveRelativeTo/ValidateFilesExist.

internal/harness/compose.go

Tests (1) +199 / -0

compose_test.goAdd regression tests for URL-sourced plugin path resolution +199/-0

Add regression tests for URL-sourced plugin path resolution

• Introduces tests covering URL base parsing behavior, offline cache hits for plugin directories, SourceURL plugin resolution, and a regression scenario ensuring ResolveRelativeTo + ValidateFilesExist succeeds when plugins are rewritten to cache paths.

internal/harness/compose_test.go

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:13 PM UTC · Ended 1:23 PM UTC
Commit: 8998985 · View workflow run →

[rh-hemartin]

The agents repo harness (harness/code.yaml) references plugins/gopls-lsp as a relative path. When fullsend fetches that harness remotely, it resolves relative paths for agent, policy, skills, scripts, host_files, profiles, and providers against the source URL and caches them locally. Plugins were not included in that resolution, so the relative path was resolved against the target repo's .fullsend directory instead -- where the plugin directory does not exist.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Chmod errors ignored ✓ Resolved 🐞 Bug ☼ Reliability

Description

In fetchBasePluginDir, filepath.Walk/Chmod errors are discarded, so plugin permission setting can
fail silently and later plugin execution can break with a non-obvious error. This also makes audit
logs misleading because the fetch is recorded as successful even if permissions weren’t applied.

Code

internal/harness/compose.go[R1633-1636]

+	_ = filepath.Walk(resolved, func(p string, info os.FileInfo, walkErr error) error {
+		if walkErr != nil || info.IsDir() {
+			return walkErr
+		}

Relevance

●●● Strong

Repo trends toward surfacing important filesystem permission/cleanup failures (at least warn; often
error) rather than silent success.

PR-#5474
PR-#761

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code explicitly discards the error from the chmod walk, while other call sites treat chmod
failures as errors—showing this path should not silently succeed when permissions aren’t applied.

internal/harness/compose.go[1627-1639]
internal/cli/lock.go[1067-1080]
internal/resolve/resolve.go[361-368]

Agent prompt

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

### Issue description
`fetchBasePluginDir` attempts to make all plugin files executable, but it ignores the error returned by `filepath.Walk` (assigned to `_`). This can leave plugins non-executable while still returning success.

### Issue Context
Other code paths treat plugin chmod failures as fatal (e.g., `resolve.ResolveHarness` and lockfile resolution), so this should be consistent.

### Fix Focus Areas
- internal/harness/compose.go[1627-1639]

### Expected change
- Capture the return value from `filepath.Walk` and return an error when non-nil.
- Consider extracting/reusing a shared helper (similar to `chmodDirFiles` in `internal/cli/lock.go`) to avoid drift and ensure consistent behavior.

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

---

2. Cache-hit plugin not chmodded ✓ Resolved 🐞 Bug ≡ Correctness

Description

fetchBasePlugin’s cache-hit/stale-fallback path returns a cached plugin directory without applying
executable permissions, even though cached directories are written with restrictive modes (0600).
This can break offline/cache-hit runs for plugins that include binaries/scripts (the primary
motivation for chmod in the fetch-miss path).

Code

internal/harness/compose.go[R1543-1546]

+					if aErr := auditBaseFetch(opts, pluginFileURL, treeHash, allowedBy, true, entry.FetchTime, "plugin"); aErr != nil {
+						return Dependency{}, "", aErr
+					}
+					return cachedDep, treePath, nil

Relevance

●●● Strong

They’ve previously accepted aligning cache-hit behavior with cache-miss to avoid drift/breakage
(chmod fits same pattern).

PR-#2626

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cache-hit path returns the directory without chmod, while cache writes force 0600 permissions
and other resolution flows explicitly chmod plugin directories to 0755 to make them runnable.

internal/harness/compose.go[1522-1547]
internal/fetch/cache.go[409-425]
internal/resolve/resolve.go[361-368]

Agent prompt

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

### Issue description
On a cache hit, `fetchBasePlugin` returns the cached directory immediately without ensuring files are executable. But directory cache entries are written via `atomicWrite`, which chmods files to `0600`, and other parts of the system explicitly chmod plugin directories after resolution.

### Issue Context
This PR adds chmod logic only in the cache-miss path (`fetchBasePluginDir`). If the cache was populated by another mechanism (or before chmod logic existed), cached plugin files can remain non-executable.

### Fix Focus Areas
- internal/harness/compose.go[1522-1547]
- internal/fetch/cache.go[409-425]

### Expected change
- Before returning `cachedDep` (and before returning `staleFallback`), run the same directory chmod logic used on cache-miss.
- Ensure chmod errors are propagated (and covered by tests), and ideally share a helper to avoid duplicating chmod logic across packages.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[codecov]

Codecov Report

❌ Patch coverage is 84.81013% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/harness/compose.go	80.48%	12 Missing and 12 partials ⚠️

📢 Thoughts on this report? Let us know!

[waynesun09]

Review sweep findings (2 new items; 1 additional candidate finding was a duplicate of an existing bot review comment on the same lines and was skipped).

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:24 PM UTC · Ended 1:29 PM UTC
Commit: b0bc046 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:30 PM UTC · Ended 1:43 PM UTC
Commit: d622cc8 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:44 PM UTC · Completed 1:59 PM UTC
Commit: 63cffda · View workflow run →

[waynesun09]

Review sweep findings (2 new items, both MEDIUM). Checked against existing review comments on this PR (bot chmod findings, prior sweep's test-coverage and chmod-duplication findings) — no overlap found.

[fullsend-ai-review]

Review

Findings

Low

• [defensive consistency] internal/harness/compose.go:1560 — fetchBasePlugin's offline-mode error path does not check staleFallback before returning, unlike fetchBaseSkill. The guard is dead code in both functions because staleFallback is only assigned when Offline=false, so the behavior is equivalent. The divergence is a minor consistency issue, not a functional bug or security vulnerability.

Previous run

Review

Findings

Low

• [defensive consistency] internal/harness/compose.go:1560 — fetchBasePlugin's offline-mode error path does not check staleFallback before returning, unlike fetchBaseSkill (line 1413). In fetchBaseSkill, the offline path returns the stale fallback when available before falling through to the cache-miss error. In fetchBasePlugin, the offline path unconditionally returns an error. The inline comment correctly notes that staleFallback is always nil here (it is only set when !opts.FetchPolicy.Offline), so the behavior is equivalent — but the same invariant holds in fetchBaseSkill, making that function's nil guard dead code. The divergence is a consistency issue, not a functional bug.

Previous run (2)

Review

Findings

Medium

• [variable shadowing] internal/harness/compose.go:1070 — In resolveBasePlugins, the line if base := filepath.Base(p); !ValidPluginBasename(base) shadows the function parameter base *Harness. While Go's scoping rules make this technically correct, it is a maintenance hazard: future code added inside the if block that references base.Plugins would use the wrong base (a string, not a *Harness) and fail to compile. The analogous resolveBaseProviders avoids this pattern.
  Remediation: Rename the inner variable: if baseName := filepath.Base(p); !ValidPluginBasename(baseName).

Low

• [defensive consistency] internal/harness/compose.go:1560 — fetchBasePlugin's offline-mode error path does not check staleFallback before returning, unlike fetchBaseSkill (line 1413). In fetchBaseSkill, the offline path returns the stale fallback when available before falling through to the cache-miss error. In fetchBasePlugin, the offline path unconditionally returns an error. The inline comment correctly notes that staleFallback is always nil here (it is only set when !opts.FetchPolicy.Offline), so the behavior is equivalent — but the same invariant holds in fetchBaseSkill, making that function's nil guard dead code. The divergence is a consistency issue, not a functional bug.

Previous run (3)

Review

Findings

Medium

• [variable shadowing] internal/harness/compose.go:1070 — In resolveBasePlugins, the line if base := filepath.Base(p); !ValidPluginBasename(base) shadows the function parameter base *Harness. While Go’s scoping rules make this technically correct, it is a maintenance hazard: future code added inside the if block that references base.Plugins would use the wrong base (a string, not a *Harness) and fail to compile. The analogous resolveBaseProviders avoids this pattern.
  Remediation: Rename the inner variable: if baseName := filepath.Base(p); !ValidPluginBasename(baseName).

Low

• [defensive consistency] internal/harness/compose.go:1091 — fetchBasePlugin’s offline-mode error path does not check staleFallback before returning, unlike fetchBaseSkill. Currently unreachable since staleFallback is only set when !opts.FetchPolicy.Offline, but this diverges from the established pattern and could become a bug if the condition were relaxed.

• [comment accuracy] internal/harness/compose.go:191 — The updated comment enumerates resource types in a different order from code execution order. The comment lists types that may have relative paths (not execution order), so this is a minor discrepancy.

• [doc comment precision] internal/harness/compose.go:1045 — The resolveBasePlugins doc comment says “following the same pattern as resolveBaseResources” but resolveBaseResources handles agent, policy, and skills — not just plugins. The phrasing refers to the structural pattern, which is correct, but could be more precise.

Previous run (4)

Review

Findings

Low

• [comment accuracy] internal/harness/compose.go:191 — The updated comment enumerates resource types in the order "agent, policy, skills, plugins, host_files, scripts, profiles, providers" but the code resolves them in a different order: scripts, resources (agent+policy+skills), host_files, profiles, providers, plugins. However, this is a parenthetical list identifying which resource types have relative paths, not documenting execution order — the comment does not claim to describe resolution sequence.

• [doc comment precision] internal/harness/compose.go:1045 — The resolveBasePlugins doc comment says "the same way resolveBaseResources handles skills" but resolveBaseResources handles agent, policy, and skills — not just skills. A more precise phrasing would be "following the same pattern as resolveBaseResources" or "the same way resolveBaseResources handles skills (among other resources)."

Previous run (5)

Review

Findings

Low

• [comment accuracy] internal/harness/compose.go:191 — The updated comment lists resource types in a different order from the actual resolve call order below it. The comment enumerates "agent, policy, skills, plugins, host_files, scripts, profiles, providers" but the calls below resolve in order: scripts, resources (agent+policy+skills), host_files, profiles, providers, plugins. Consider either matching call order or using "including" rather than a definitive enumeration.

• [naming consistency] internal/harness/compose.go:1042 — The doc comment for resolveBasePlugins says "the same way resolveBaseResources handles skills", but resolveBaseResources handles agent, policy, and skills — not just skills. A more precise phrasing would be "following the same pattern as resolveBaseResources".

• [excessive permissions] internal/harness/harness.go:32 — ChmodPluginDir is now exported (previously unexported in resolve.go), slightly increasing the API surface for filesystem mutation. The function correctly resolves symlinks before walking and is consistent with existing patterns. No action required.

---

Labels: PR modifies harness composition internals (compose.go, harness.go) and is a bug fix for plugin path resolution

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:20 PM UTC · Completed 2:37 PM UTC
Commit: 5358e6e · View workflow run →

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 2:46 PM UTC · Ended 3:03 PM UTC
Commit: 4b73425 · View workflow run →

[waynesun09]

Local end-to-end verification

Built the PR branch locally and ran the code agent against issue #5954 with the harness fetched by URL (production shape) — the exact code path that triggers #5977.

Setup

• Empty --fullsend-dir (no config.yaml, no lock.yaml) → forces the tryAgentsRepoFallback URL-sourced path
• --no-post-script to suppress push/PR side effects
• code.yaml from fullsend-ai/agents@v0 — the only harness with plugins: [plugins/gopls-lsp]

Baseline repro (main, pre-fix) — bug confirmed

✗ File validation failed
Error: validating files: plugins[0]: stat /…/fs-dir/plugins/gopls-lsp: no such file or directory

Relative plugin path resolved against the local --fullsend-dir instead of the URL source — exact #5977 failure.

Fix run (PR branch) — Checkpoint A PASS

Base: https://raw.githubusercontent.com/fullsend-ai/agents/ff0e29b…/plugins/gopls-lsp/plugin.json (fetched)
✓ Harness loaded (1.1s)
Plugins: /…/.fullsend-cache/resources/sha256/489be6d…/gopls-lsp

• Plugin correctly fetched from the remote agents repo URL
• Resolved to an absolute cache path under .fullsend-cache/resources/sha256/…
• No file validation error — run advanced past ValidateFilesExist → openshell check → pre-script
• Post-script confirmed skipped: (SKIPPED: --no-post-script)

Checkpoint B (full agent run)

Blocked by pre-script: issue #5954 already has an open PR (#5956), so the pre-script correctly skipped the agent run. This is a legitimate guard, not a fix failure.

Conclusion

The fix conclusively resolves #5977. resolveBasePlugins now correctly fetches and caches plugins from the URL-sourced harness base instead of trying to stat them locally.

[waynesun09]

Review sweep finding (1 new item, HIGH). Checked against all 14 existing review comments and 8 prior reviews on this PR (bot chmod/coverage/duplication findings, and this account's two prior sweep passes) — no overlap; this traces a new interaction between the PR's new base-plugin URL format and the existing (unmodified) lock-file resolution path.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:46 PM UTC · Completed 3:03 PM UTC
Commit: 4b73425 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:09 PM UTC · Completed 3:30 PM UTC
Commit: d61055a · View workflow run →

[waynesun09]

Review sweep findings (2 new items: 1 HIGH, 1 MEDIUM). Checked against all 17 existing review comments and 11 reviews on this PR — no overlap found (existing comments cover the plugins[] fix, chmod issues, and doc-comment nits, but not the skills[] lock-resolution gap or the resolveBasePlugins variable shadowing).

[waynesun09]

Approved after multi-agent review (3 agents: 2 Claude, 1 Grok) and local end-to-end verification.

Local verification:

• Bug reproduced on main: validating files: plugins[0]: stat .../plugins/gopls-lsp: no such file or directory
• Fix verified on PR branch: plugin fetched from URL source → absolute cache path, no file validation error, harness loaded successfully
• Lock resolution fix (df9c9a7) verified with reproducing test: lockTreeDirName correctly strips marker files from raw-content URLs for both plugins and skills

Review squad result: 0 critical, 0 high, 0 medium. 3 low findings (filepath.Base/path.Base mixing in lockTreeDirName, stale-fallback guard asymmetry, ChmodPluginDir godoc missing cache-mutation warning) — all non-blocking. Structural duplication deferred to #5982.

All prior HIGH/MEDIUM findings from 5 review rounds resolved in subsequent commits.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:12 PM UTC · Completed 6:25 PM UTC
Commit: df9c9a7 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:14 AM UTC · Completed 6:27 AM UTC
Commit: 78f54ed · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:53 AM UTC · Completed 7:18 AM UTC
Commit: 78f54ed · View workflow run →

[fullsend-ai-retro]

Retro: PR #5978 — fix: resolve relative plugin paths from URL-sourced harnesses

Timeline

rh-hemartin opened issue #5977 and PR #5978 simultaneously (Aug 6 13:11 UTC), fixing a bug where resolveBaseResources in compose.go resolved agent, policy, and skills from remote base URLs but skipped plugins. The PR went through 5 review rounds and 8 commits over ~18 hours before merging.

Key review sequence:

• 13:17–13:21 — qodo-code-review[bot] and waynesun09 post initial findings (chmod errors silently ignored, 12.5% test coverage, code duplication).
• 13:28–13:42 — Author pushes 3 commits fixing chmod propagation, extracting ChmodPluginDir helper, increasing test coverage.
• 13:59 — fullsend-ai-review[bot] APPROVES commit 63cffda with only 3 low findings, adds ready-for-merge label. At this point, waynesun09's 2 MEDIUM findings from 13:50 on the same commit are unaddressed.
• 14:52 — waynesun09 discovers a HIGH bug: fetchBasePlugin records raw-content URLs with /plugin.json suffix that break resolveFromLock's basename parsing, silently bypassing lock-file content pinning.
• 15:03 — waynesun09 confirms the bug with a reproducing test. ready-for-merge removed, requires-manual-review added.
• 15:27 — waynesun09 discovers the same HIGH bug exists in resolveFromLock's skills[ branch (pre-existing latent bug).
• 15:05–15:39 — Both bugs fixed (rh-hemartin fixes plugin lock resolution, waynesun09 pushes direct fix for skill lock resolution + variable shadowing).
• 17:21 — waynesun09 APPROVES after multi-agent verification (2 Claude + 1 Grok) and local end-to-end testing.
• Aug 7 06:51 — PR merged via merge queue.

What went well

• The iterative review process caught 2 HIGH lock-file bypass bugs and 4 MEDIUM issues that would have shipped otherwise.
• waynesun09's multi-agent review approach was thorough: structured dedup checks against prior comments, reproducing tests for HIGH findings, and direct fix commits.
• Cancel-in-progress worked correctly — 3 review runs were cancelled on rapid pushes, avoiding token waste.
• Triage agent correctly identified the PR already addressed issue fix: resolve relative plugin paths from URL-sourced harnesses #5977.

Gaps identified

1. fullsend-ai-review[bot] never found the HIGH lock-file bypass bugs across 5 successful review runs (run 31107238246, run 31122930612, run 31153203505). Its findings were limited to low-severity surface issues (comment accuracy, naming consistency, defensive consistency). The cross-module data flow from fetchBasePlugin → lock file → resolveFromLock was never traced.

2. Premature ready-for-merge label — Bot approved and labeled at 13:59 while waynesun09 had outstanding MEDIUM findings (13:50) on the same commit. The label was later removed only incidentally — the bot's own unrelated MEDIUM variable-shadowing finding caused a COMMENT verdict, which triggered label removal. If the bot hadn't found that separate issue, ready-for-merge would have remained while the HIGH lock-file bypass was present. Evidence for existing issues: #447 (agents) (incorporate outstanding human reviews), #5065 (don't label while CHANGES_REQUESTED outstanding), #1574 (gate ready-for-merge on human approval).

3. Repeated low finding — The "defensive consistency" finding about fetchBasePlugin's offline error path was posted 3 times across review runs on commits d61055a and 78f54ed. Evidence for existing dedup issues: #2959, #1013, #1285.

4. Harness pipeline checklist — The author's initial implementation covered resolve/fetch/cache stages for plugins but missed the lock-resolution stage. This is evidence for existing issue #5579 (document harness field integration pipeline as a contributor checklist), which would help both humans and agents catch completeness gaps when new resource types are added.

Proposals filed

• Review correctness sub-agent should trace producer-consumer format compatibility for new struct-field writes (in fullsend-ai/agents)