fix: use skill directory name from URL instead of cache-internal "tree"

[ralphbean]

Summary

• URL-resolved skills were named "tree" in the sandbox instead of their actual name (e.g., "architecture") because the cache stores content under a tree/ subdirectory and filepath.Base() picked that up
• Fix creates a symlink named after the skill directory (from the URL path) alongside tree in the cache, so all downstream consumers (sandbox upload, logging) see the correct name
• Multiple URL-resolved skills no longer collide on the name "tree"

Discovered via https://github.com/konflux-ci/refinement/actions/runs/28116641395/job/83257718265 where the architecture skill showed up as Skill "tree": uploaded to sandbox.

Test plan

• Added basename assertions to TestResolveHarness_SkillDirFetchAndCache (fresh fetch)
• Added basename assertion to TestResolveHarness_SkillDirCacheHit (cache hit path)
• Added basename assertions to TestResolveHarness_MultipleSkills (two URL skills get distinct names)
• All 27 resolve package tests pass
• internal/runtime and internal/fetchsvc tests pass

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix URL skill name resolution by symlinking cache tree to URL basename
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Description

• Create a URL-derived symlink in the directory cache so resolved skills keep their real names.
• Prevent multiple URL-resolved skills from colliding on the cache-internal "tree" basename.
• Add tests asserting resolved skill directory basenames for fetch, cache-hit, and multi-skill
 cases.

Diagram

graph TD
  A["Harness skills (URL)"] --> B["resolveSkillDirURL"] --> C[("Dir cache entry")] --> D["tree/ content"] --> E["<skillName> symlink"] --> F["Sandbox upload & logs"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Propagate explicit skill name metadata (no symlink)

• ➕ Avoids filesystem side effects and symlink portability concerns
• ➕ Makes naming independent of path semantics
• ➖ Requires API/struct changes across resolver/harness and all downstream consumers
• ➖ Higher refactor cost and broader review surface than this bug fix

2. Change downstream consumers to ignore "tree" and derive name differently

• ➕ Keeps cache layout untouched
• ➕ Potentially simpler if only one consumer exists
• ➖ Name derivation logic would be duplicated across consumers
• ➖ Doesn't fix other callers that use filepath.Base() today or in the future

3. Change cache layout to store content under the real directory name

• ➕ Eliminates the need for a symlink and makes paths self-describing
• ➖ Risky migration for existing cache entries and tooling assumptions
• ➖ More invasive change for a localized symptom

Recommendation: The symlink approach is a good fit for a narrow bug fix: it centralizes the correction at the resolution boundary, preserves the existing cache format (tree/), and avoids changing downstream APIs. If symlink support becomes a concern (e.g., Windows environments), consider the metadata-propagation alternative as the longer-term design.

Files changed (2) +27 / -1

Bug fix (1) +17 / -0

resolve.goSymlink cached tree to URL-derived skill directory name +17/-0

Symlink cached tree to URL-derived skill directory name

• After resolving a URL-based skill directory, derive the skill name from the URL path basename and create an idempotent symlink alongside the cached "tree" directory. Return the symlinked path so downstream consumers using filepath.Base() see the intended skill name and distinct URL skills no longer collide on "tree".

internal/resolve/resolve.go

Tests (1) +10 / -1

resolve_test.goAssert resolved skill basenames match URL path names +10/-1

Assert resolved skill basenames match URL path names

• Update resolver harness tests to assert that the resolved skill directory path basename matches the URL directory name for both fresh fetch and cache-hit paths. Add a multi-skill assertion ensuring distinct URL skills resolve to distinct basenames (e.g., "one" and "two") rather than "tree".

internal/resolve/resolve_test.go

[github-actions]

Site preview

Preview: https://26cb7791-site.fullsend-ai.workers.dev

Commit: c17bf615ee9bf8cf2c0403826043f680041c725d

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:42 PM UTC · Completed 6:53 PM UTC
Commit: 71dc217 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 51 rules

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

1. Stale /tree lock tests 🐞 Bug ☼ Reliability

Description

The change makes resolved skill paths end with the URL basename instead of /tree, but
internal/cli/lock_test.go still asserts a /tree suffix. This will break go test ./... and should
be updated to the new path shape.

Code

internal/resolve/resolve.go[R371-386]

+	// Create a symlink named after the skill directory so downstream consumers
+	// (sandbox upload, logging) see the real skill name instead of "tree".
+	skillName := filepath.Base(forgeInfo.Path)
+	if skillName == "" || skillName == "." {
+		skillName = "tree"
+	}
+	namedPath := filepath.Join(filepath.Dir(treePath), skillName)
+	if namedPath != treePath {
+		// Idempotent: only create if it doesn't already exist.
+		if _, err := os.Lstat(namedPath); os.IsNotExist(err) {
+			if err := os.Symlink("tree", namedPath); err != nil {
+				return Dependency{}, "", fmt.Errorf("creating named symlink for %s: %w", field, err)
+			}
+		}
+		treePath = namedPath
+	}

Relevance

⭐⭐⭐ High

They routinely update brittle tests when path shapes change; lock/resolve tests maintained in
#2139/#2082.

PR-#2139
PR-#2082

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR explicitly changes resolve to return a sibling path named after the URL’s directory basename.
CLI lock tests still hard-code /tree as the suffix, so they will no longer match the new resolved
paths.

internal/resolve/resolve.go[371-386]
internal/cli/lock_test.go[298-302]
internal/cli/lock_test.go[721-750]

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 resolve behavior now returns a skill directory path whose basename matches the URL directory name, not the cache-internal `tree` directory name. CLI lock tests still assume the old behavior.

## Issue Context
The PR updates `internal/resolve/resolve_test.go` to assert basenames like `review/cached/one/two`, but `internal/cli/lock_test.go` still asserts paths end with `/tree`.

## Fix Focus Areas
- internal/cli/lock_test.go[280-302]
- internal/cli/lock_test.go[721-750]

## Implementation notes
- Replace `HasSuffix(..., "/tree")` assertions with basename assertions that match the URL path (e.g., `filepath.Base(h.Skills[0]) == "test"`).
- If the CLI intends to accept both old and new layouts, loosen the assertion accordingly (but ensure it still validates the returned path is a directory).

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

---

2. Unsafe cache symlink reuse ✗ Dismissed 🐞 Bug ⛨ Security

Description

resolveSkillDirURL rewrites the verified cache treePath to namedPath even when namedPath
already exists, without verifying it’s a symlink to tree (or even a directory). If the URL-derived
basename is .. or collides with an existing file/symlink, downstream consumers (e.g., sandbox
upload) can operate on an unintended path outside the verified cache tree.

Code

internal/resolve/resolve.go[R371-386]

+	// Create a symlink named after the skill directory so downstream consumers
+	// (sandbox upload, logging) see the real skill name instead of "tree".
+	skillName := filepath.Base(forgeInfo.Path)
+	if skillName == "" || skillName == "." {
+		skillName = "tree"
+	}
+	namedPath := filepath.Join(filepath.Dir(treePath), skillName)
+	if namedPath != treePath {
+		// Idempotent: only create if it doesn't already exist.
+		if _, err := os.Lstat(namedPath); os.IsNotExist(err) {
+			if err := os.Symlink("tree", namedPath); err != nil {
+				return Dependency{}, "", fmt.Errorf("creating named symlink for %s: %w", field, err)
+			}
+		}
+		treePath = namedPath
+	}

Relevance

⭐⭐⭐ High

Prior accepted symlink/path hardening (sanitizeDownload EvalSymlinks) and rejecting symlink inputs
(cred file).

PR-#1177
PR-#215

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new block in resolveSkillDirURL computes a URL-derived skillName, builds namedPath, and then
sets treePath = namedPath even when namedPath already exists (no validation of type/target).
Because forge URL parsing preserves path segments verbatim, skillName can be .. or collide with
existing cache files. Downstream, the runtime uploads skillPath to the sandbox, so a wrong
treePath changes what gets uploaded.

internal/resolve/resolve.go[371-386]
internal/forge/url.go[48-84]
internal/runtime/claude.go[53-62]
PR-#1177

Agent prompt

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

## Issue description
`resolveSkillDirURL` derives `skillName` from `forgeInfo.Path` and then unconditionally switches `treePath` to `namedPath` as long as `namedPath != treePath`, even if `namedPath` already exists as a regular file or an unexpected symlink. This bypasses the integrity-verified `tree/` directory returned by the cache and can cause sandbox upload/logging to act on the wrong path.

## Issue Context
- `forgeInfo.Path` is built from URL path segments and is not normalized to reject special segments like `..`.
- The runtime uploads whatever directory path it receives for a skill.

## Fix Focus Areas
- internal/resolve/resolve.go[371-386]

## Implementation notes
- Sanitize `skillName`: explicitly reject/override `".."` (and consider rejecting empty/`.` as already handled).
- If `namedPath` exists:
 - Require it to be a symlink that resolves to the same target as `treePath` (e.g., compare `filepath.EvalSymlinks(namedPath)` to `filepath.EvalSymlinks(treePath)`), otherwise return a clear error (or ignore `namedPath` and keep using `treePath`).
 - Do **not** set `treePath = namedPath` unless the symlink is newly created or verified.
- If you keep the “reuse existing” behavior, verify containment similarly to prior symlink-hardening work (resolve symlinks before trusting the path).

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

---

3. Symlink creation TOCTOU ✓ Resolved 🐞 Bug ☼ Reliability

Description

The code performs Lstat(namedPath) and then os.Symlink(...), so another process can create
namedPath between the two calls and cause os.Symlink to fail with EEXIST. This can produce
intermittent resolve failures when multiple runs share the same cache directory.

Code

internal/resolve/resolve.go[R379-383]

+		// Idempotent: only create if it doesn't already exist.
+		if _, err := os.Lstat(namedPath); os.IsNotExist(err) {
+			if err := os.Symlink("tree", namedPath); err != nil {
+				return Dependency{}, "", fmt.Errorf("creating named symlink for %s: %w", field, err)
+			}

Relevance

⭐⭐ Medium

No clear history on handling Symlink EEXIST/TOCTOU; reliability recommendations inconsistent across
past reviews.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The symlink creation is guarded by an Lstat existence check, but the actual creation is a separate
syscall, making it vulnerable to a race where another process creates namedPath between the check
and the symlink call.

internal/resolve/resolve.go[379-383]

Agent prompt

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

## Issue description
There is a TOCTOU window between checking `os.Lstat(namedPath)` and creating the symlink. If another process creates the symlink (or any entry) at `namedPath` between those operations, `os.Symlink` may return an `EEXIST` error and abort resolution.

## Issue Context
The comment says the operation is idempotent, but it is not idempotent under concurrency due to the separate existence check.

## Fix Focus Areas
- internal/resolve/resolve.go[379-383]

## Implementation notes
- Prefer attempting `os.Symlink("tree", namedPath)` directly and:
 - if it succeeds, proceed;
 - if it fails with already-exists (`os.IsExist(err)`), treat it as success (optionally verify the existing path is the expected symlink target as part of the other fix);
 - otherwise, return the error.

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

---

[codecov]

Codecov Report

❌ Patch coverage is 57.14286% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/fetch/cache.go	66.66%	2 Missing and 2 partials ⚠️
internal/harness/compose.go	33.33%	2 Missing and 2 partials ⚠️
internal/resolve/resolve.go	66.66%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Verdict: Approve

The prior review's code-duplication finding is resolved. The latest commit extracts the symlink creation logic into a shared helper fetch.CacheNamedSymlink in internal/fetch/cache.go, replacing the three inline copies across fetchBaseSkill (cache-hit path), fetchBaseSkillDir, and resolveSkillDirURL. The extraction is correct — the helper handles reserved names, TOCTOU races via os.IsExist, non-IsNotExist Lstat errors, and the identity case (skillName == "tree"). All three call sites pass the error through with contextual wrapping.

The stale-fallback paths in fetchBaseSkill are correctly covered because CacheNamedSymlink is called before cachedDep and staleFallbackPath are assigned, so all exit points return the named path.

Test coverage is thorough: the helper has direct unit tests (creation, idempotency, reserved-name fallback, concurrent EEXIST tolerance), and integration-level tests cover both cache-hit and fresh-fetch paths in compose_test.go and resolve_test.go, including edge-case tests for ".." and "metadata.json" fallback and multi-skill disambiguation.

Findings

Low

• [edge-case] internal/fetch/cache.go — CacheNamedSymlink does not sanitize skillName values containing path separators (e.g., "sub/dir"). If a caller passes such a name, filepath.Join(filepath.Dir(treePath), skillName) would create a symlink outside the hash directory. All current callers use filepath.Base() before calling, so this is not an active bug. However, the function is exported and its docstring claims it "sanitizes the skill name," which is slightly misleading — it guards against "", ".", "..", and "metadata.json", but not path-separator-containing names.
  Remediation: Add skillName = filepath.Base(skillName) at the top of CacheNamedSymlink for defense-in-depth, or update the docstring to clarify which sanitizations are performed.

---

Prior review (49e7a47): 1 low finding (code-duplication across 3 call sites) → resolved (extracted into shared CacheNamedSymlink helper).

Previous run

Review

Verdict: Approve

All prior review findings are resolved. The latest commit addresses the medium-severity cache-hit path inconsistency by applying the same symlink logic to fetchBaseSkill's cache-hit branch, with a dedicated test verifying the fix. The stale-fallback return paths are also correctly covered because the symlink block executes before cachedDep and staleFallbackPath are set, so all exit points from fetchBaseSkill return the named path.

The security sub-agent raised a concern about null-byte truncation bypassing the "metadata.json" guard (e.g., a URL path like metadata.json%00suffix). This is a false positive: Go's standard library validates all filesystem paths for embedded null bytes since Go 1.6 — os.Lstat and os.Symlink both call syscall.BytePtrFromString, which returns EINVAL for any path containing \x00. The existing error-handling branches would catch this before any filesystem mutation occurs.

Findings

Low

• [code-duplication] internal/harness/compose.go, internal/resolve/resolve.go — The symlink creation block (skillName extraction, guard list, Lstat/Symlink logic) is now duplicated across three call sites: fetchBaseSkill cache-hit path, fetchBaseSkillDir, and resolveSkillDirURL. The guard list ("", ".", "..", "metadata.json") is a maintenance risk if updated in only one location.
  Remediation: Extract the symlink-creation logic into a shared helper (e.g., func ensureNamedSymlink(treePath, skillName string) (string, error) in the fetch package), called from all three sites.

---

Prior review (8ff231c): 1 medium finding (inconsistency — cache-hit path bypassed symlink logic) → resolved. 2 low findings (test-adequacy, code-duplication) → test-adequacy resolved (new test TestFetchBaseSkill_CacheHit_UsesSkillNameNotTree); code-duplication carried (now in 3 locations).

Previous run

Review

Verdict: Approve

All prior review findings are resolved. The latest commit addresses the medium-severity cache-hit path inconsistency by applying the same symlink logic to fetchBaseSkill's cache-hit branch, with a dedicated test verifying the fix. The stale-fallback return paths are also correctly covered because the symlink block executes before cachedDep and staleFallbackPath are set, so all exit points from fetchBaseSkill return the named path.

The security sub-agent raised a concern about null-byte truncation bypassing the "metadata.json" guard (e.g., a URL path like metadata.json%00suffix). This is a false positive: Go's standard library validates all filesystem paths for embedded null bytes since Go 1.6 — os.Lstat and os.Symlink both call syscall.BytePtrFromString, which returns EINVAL for any path containing \x00. The existing error-handling branches would catch this before any filesystem mutation occurs.

Findings

Low

• [code-duplication] internal/harness/compose.go, internal/resolve/resolve.go — The symlink creation block (skillName extraction, guard list, Lstat/Symlink logic) is now duplicated across three call sites: fetchBaseSkill cache-hit path, fetchBaseSkillDir, and resolveSkillDirURL. The guard list ("", ".", "..", "metadata.json") is a maintenance risk if updated in only one location.
  Remediation: Extract the symlink-creation logic into a shared helper (e.g., func ensureNamedSymlink(treePath, skillName string) (string, error) in the fetch package), called from all three sites.

---

Prior review (8ff231c): 1 medium finding (inconsistency — cache-hit path bypassed symlink logic) → resolved. 2 low findings (test-adequacy, code-duplication) → test-adequacy resolved (new test TestFetchBaseSkill_CacheHit_UsesSkillNameNotTree); code-duplication carried (now in 3 locations).

Previous run (2)

Review

Verdict: Comment

The bug fix correctly addresses URL-resolved skills appearing as "tree" in sandbox paths and logs. The symlink approach is sound — creating a named symlink alongside the cache-internal tree/ directory is minimal and preserves the content-addressed cache structure. Edge case guards ("", ".", "..", "metadata.json") are correct and tested. The prior review's two low findings (race-condition and error-handling-gap) are both resolved in this revision — the os.IsExist(err) check on os.Symlink handles the TOCTOU race, and the else if err != nil branch returns an error for non-IsNotExist Lstat failures.

However, the fix is incomplete in compose.go: the cache-hit path in fetchBaseSkill bypasses the new symlink logic entirely.

Findings

Medium

• [inconsistency] internal/harness/compose.go — The cache-hit path in fetchBaseSkill (lines 900–923) returns treePath directly from CacheGetDir without creating or using a named symlink. On a cache hit, Dependency.LocalPath has basename "tree"; on a cache miss (via fetchBaseSkillDir), the symlink is created and the basename is the real skill name. The stale-fallback paths (lines 929–931 and 940–941) also bypass the symlink logic. By contrast, resolve.go correctly places the symlink block after both cache-hit and cache-miss branches have merged, so it works consistently in both cases.
  Remediation: Add the same Lstat-then-Symlink block in fetchBaseSkill after the CacheGetDir call succeeds (around line 904), using filepath.Base(skillPath) for the skill name. Apply the same treatment to the stale-fallback return paths.

Low

• [test-adequacy] internal/harness/compose.go — The symlink logic in fetchBaseSkillDir has no corresponding test coverage. All new tests are in resolve_test.go only. If the compose.go code path regresses (or the cache-hit gap above is fixed), no test would catch it.
  Remediation: Add a test in compose_test.go exercising fetchBaseSkillDir (or fetchBaseSkill end-to-end) and asserting the returned path basename matches the URL-derived skill name.

• [code-duplication] internal/harness/compose.go, internal/resolve/resolve.go — The symlink creation block (skillName extraction, guard list, Lstat/Symlink logic) is duplicated verbatim across both files. The blocklist ("", ".", "..", "metadata.json") is a maintenance risk if updated in only one location. The cache-hit gap in compose.go already demonstrates how the two code paths can diverge in behavior.
  Remediation: Extract the symlink-creation logic into a shared helper (e.g., in the fetch package), called from both sites.

---

Prior review (735a7ce): 2 low findings (race-condition, error-handling-gap) → both resolved in this revision.

Previous run

Review

Verdict: Approve

Findings

Low

• [race-condition] internal/resolve/resolve.go — The Lstat-then-Symlink pattern has a narrow TOCTOU race across concurrent processes sharing the same workspace cache.
  Remediation: After os.Symlink fails, check os.IsExist(err) and treat it as success.

• [error-handling-gap] internal/resolve/resolve.go — If os.Lstat(namedPath) returns a non-nil, non-IsNotExist error, the code skips symlink creation but still executes treePath = namedPath.
  Remediation: Add an explicit branch for non-IsNotExist errors.

---

Prior review (c17bf61): 1 low finding (error-handling-gap) → carried (unchanged code). 1 low finding (race-condition) added after challenger downgrade from medium.

Previous run

Review

Verdict: Approve

Findings

Low

• [error-handling-gap] internal/resolve/resolve.go — The os.Lstat(namedPath) result is only checked for os.IsNotExist. If Lstat returns a different error (e.g., I/O error), the code skips symlink creation but still executes treePath = namedPath, pointing treePath at a path that may not exist.

---

Prior review (71dc217): 1 medium finding (path-traversal via missing ".." guard) → resolved in this revision.

Previous run

Review

Findings

Medium

• [path-traversal] internal/resolve/resolve.go:373 — The skillName guard checks for "" and "." but does not check for "..". If forgeInfo.Path ends with .. (e.g., skills/..), filepath.Base returns "..", and namedPath resolves to the parent of the cache hash directory via filepath.Join(filepath.Dir(treePath), ".."). A symlink would be created outside the intended cache directory.
  Remediation: Add skillName == ".." to the guard condition.

---

Labels: Bug fix in skill URL resolution within the harness component.

Previous run (3)

Review

Verdict: Comment

The bug fix correctly addresses URL-resolved skills appearing as "tree" in sandbox paths and logs. The symlink approach is sound — creating a named symlink alongside the cache-internal tree/ directory is minimal and preserves the content-addressed cache structure. Edge case guards ("", ".", "..", "metadata.json") are correct and tested. The prior review's two low findings (race-condition and error-handling-gap) are both resolved in this revision — the os.IsExist(err) check on os.Symlink handles the TOCTOU race, and the else if err != nil branch returns an error for non-IsNotExist Lstat failures.

However, the fix is incomplete in compose.go: the cache-hit path in fetchBaseSkill bypasses the new symlink logic entirely.

Findings

Medium

• [inconsistency] internal/harness/compose.go — The cache-hit path in fetchBaseSkill (lines 900–923) returns treePath directly from CacheGetDir without creating or using a named symlink. On a cache hit, Dependency.LocalPath has basename "tree"; on a cache miss (via fetchBaseSkillDir), the symlink is created and the basename is the real skill name. The stale-fallback paths (lines 929–931 and 940–941) also bypass the symlink logic. By contrast, resolve.go correctly places the symlink block after both cache-hit and cache-miss branches have merged, so it works consistently in both cases.
  Remediation: Add the same Lstat-then-Symlink block in fetchBaseSkill after the CacheGetDir call succeeds (around line 904), using filepath.Base(skillPath) for the skill name. Apply the same treatment to the stale-fallback return paths.

Low

• [test-adequacy] internal/harness/compose.go — The symlink logic in fetchBaseSkillDir has no corresponding test coverage. All new tests are in resolve_test.go only. If the compose.go code path regresses (or the cache-hit gap above is fixed), no test would catch it.
  Remediation: Add a test in compose_test.go exercising fetchBaseSkillDir (or fetchBaseSkill end-to-end) and asserting the returned path basename matches the URL-derived skill name.

• [code-duplication] internal/harness/compose.go, internal/resolve/resolve.go — The symlink creation block (skillName extraction, guard list, Lstat/Symlink logic) is duplicated verbatim across both files. The blocklist ("", ".", "..", "metadata.json") is a maintenance risk if updated in only one location. The cache-hit gap in compose.go already demonstrates how the two code paths can diverge in behavior.
  Remediation: Extract the symlink-creation logic into a shared helper (e.g., in the fetch package), called from both sites.

---

Prior review (735a7ce): 2 low findings (race-condition, error-handling-gap) → both resolved in this revision.

Previous run (4)

Review

Verdict: Approve

Findings

Low

• [race-condition] internal/resolve/resolve.go — The Lstat-then-Symlink pattern has a narrow TOCTOU race across concurrent processes sharing the same workspace cache.
  Remediation: After os.Symlink fails, check os.IsExist(err) and treat it as success.

• [error-handling-gap] internal/resolve/resolve.go — If os.Lstat(namedPath) returns a non-nil, non-IsNotExist error, the code skips symlink creation but still executes treePath = namedPath.
  Remediation: Add an explicit branch for non-IsNotExist errors.

---

Prior review (c17bf61): 1 low finding (error-handling-gap) → carried (unchanged code). 1 low finding (race-condition) added after challenger downgrade from medium.

Previous run

Review

Verdict: Approve

Findings

Low

• [error-handling-gap] internal/resolve/resolve.go — The os.Lstat(namedPath) result is only checked for os.IsNotExist. If Lstat returns a different error (e.g., I/O error), the code skips symlink creation but still executes treePath = namedPath, pointing treePath at a path that may not exist.

---

Prior review (71dc217): 1 medium finding (path-traversal via missing ".." guard) → resolved in this revision.

Previous run

Review

Findings

Medium

• [path-traversal] internal/resolve/resolve.go:373 — The skillName guard checks for "" and "." but does not check for "..". If forgeInfo.Path ends with .. (e.g., skills/..), filepath.Base returns "..", and namedPath resolves to the parent of the cache hash directory via filepath.Join(filepath.Dir(treePath), ".."). A symlink would be created outside the intended cache directory.
  Remediation: Add skillName == ".." to the guard condition.

---

Labels: Bug fix in skill URL resolution within the harness component.

Previous run (5)

Review

Verdict: Approve

Clean bug fix that resolves URL-resolved skills incorrectly appearing as "tree" in sandbox paths and logs. The approach — creating a named symlink alongside the cache-internal tree/ directory — is minimal, preserves the content-addressed cache structure, and handles edge cases ("", ".", "..", "metadata.json") correctly. Since the prior review, the only change is adding "metadata.json" to the reserved-name guard to prevent collision with the cache metadata file, plus a dedicated test.

Test coverage is thorough: fresh fetch, cache hit, ".." fallback, "metadata.json" fallback, and multiple-skill disambiguation are all exercised. No existing assertions were weakened.

Findings

Low

• [race-condition] internal/resolve/resolve.go — The Lstat-then-Symlink pattern has a narrow TOCTOU race across concurrent processes sharing the same workspace cache. If two fullsend processes resolve the same skill URL simultaneously, both can observe os.IsNotExist from Lstat, then the second os.Symlink call fails with EEXIST, propagating as a hard error. Within a single process this is safe — ResolveHarness iterates skills sequentially with no goroutines. The cross-process scenario requires concurrent first-fetch of the same skill URL and a microsecond timing window.
  Remediation: After os.Symlink fails, check os.IsExist(err) and treat it as success: if err := os.Symlink("tree", namedPath); err != nil && !os.IsExist(err) { return ... }.

• [error-handling-gap] internal/resolve/resolve.go — If os.Lstat(namedPath) returns a non-nil, non-IsNotExist error (e.g., I/O error), the code skips symlink creation but still executes treePath = namedPath, pointing treePath at a path that may not exist. Downstream code would then reference a broken path. The practical likelihood is very low — the cache directory was just successfully accessed — but the fix is trivial.
  Remediation: Add an explicit branch for non-IsNotExist errors: either return the error (fail fast) or fall back to keeping treePath as the original tree path.

---

Prior review (c17bf61): 1 low finding (error-handling-gap) → carried (unchanged code). 1 low finding (race-condition) added after challenger downgrade from medium.

Previous run (6)

Review

Verdict: Approve

Findings

Low

• [error-handling-gap] internal/resolve/resolve.go — The os.Lstat(namedPath) result is only checked for os.IsNotExist. If Lstat returns a different error (e.g., I/O error), the code skips symlink creation but still executes treePath = namedPath, pointing treePath at a path that may not exist.

---

Prior review (71dc217): 1 medium finding (path-traversal via missing ".." guard) → resolved in this revision.

Previous run

Review

Findings

Medium

• [path-traversal] internal/resolve/resolve.go:373 — The skillName guard checks for "" and "." but does not check for "..". If forgeInfo.Path ends with .. (e.g., skills/..), filepath.Base returns "..", and namedPath resolves to the parent of the cache hash directory via filepath.Join(filepath.Dir(treePath), ".."). A symlink would be created outside the intended cache directory.
  Remediation: Add skillName == ".." to the guard condition.

---

Labels: Bug fix in skill URL resolution within the harness component.

Previous run (7)

Review

Verdict: Approve

Clean bug fix that addresses URL-resolved skills incorrectly appearing as "tree" in sandbox paths and logs. The approach — creating a named symlink alongside the cache-internal tree/ directory — is minimal, preserves the content-addressed cache structure, and handles edge cases ("", ".", "..") correctly. The prior review's medium-severity path-traversal finding (missing ".." guard) has been resolved in this revision, with a dedicated test (TestResolveHarness_SkillDirDotDotFallsBackToTree) confirming the fix.

Test coverage is good: fresh fetch, cache hit, ".." fallback, and multiple-skill disambiguation are all exercised. No existing assertions were weakened.

Findings

Low

• [error-handling-gap] internal/resolve/resolve.go — The os.Lstat(namedPath) result is only checked for os.IsNotExist. If Lstat returns a different error (e.g., I/O error), the code skips symlink creation but still executes treePath = namedPath, pointing treePath at a path that may not exist. Downstream code (resolveSkillTransitiveDeps, Dependency.LocalPath) would then reference a broken path. The practical likelihood is very low — the cache directory is freshly created with 0700 permissions — but the fix is trivial.
  Remediation: Add an explicit branch for non-IsNotExist errors: either return the error (fail fast) or fall back to keeping treePath as the original tree path.

---

Prior review (71dc217): 1 medium finding (path-traversal via missing ".." guard) → resolved in this revision.

Previous run (8)

Review

Findings

Medium

• [path-traversal] internal/resolve/resolve.go:373 — The skillName guard checks for "" and "." but does not check for "..". If forgeInfo.Path ends with .. (e.g., skills/..), filepath.Base returns "..", and namedPath resolves to the parent of the cache hash directory via filepath.Join(filepath.Dir(treePath), ".."). A symlink would be created outside the intended cache directory. The risk is mitigated by forge URL validation upstream, but the check is cheap to add.
  Remediation: Add skillName == ".." to the guard condition, or use a more restrictive check: if skillName == "" || skillName == "." || skillName == ".." || strings.ContainsAny(skillName, "/\\") { skillName = "tree" }

---

Labels: Bug fix in skill URL resolution within the harness component.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:26 PM UTC · Completed 8:36 PM UTC
Commit: c17bf61 · View workflow run →

[waynesun09]

Review squad findings (medium+ only; see full report for low-severity items and previously-addressed concerns).

[HIGH] Symlink naming fix not applied to base-composed harness skills — internal/harness/compose.go's fetchBaseSkillDir (line 957) independently resolves a URL-referenced skill directory and returns the raw treePath (ending in /tree), assigned directly to base.Skills[i] at line 727, with no renaming logic. This file isn't part of this PR's diff so I couldn't leave an inline comment on the exact line — flagging here instead. Any harness using base: composition with a URL-resolved skill will still show up as "tree" in the sandbox/logs, the same bug this PR fixes via a different code path.

The MEDIUM finding (reserved metadata.json filename collision) is posted inline on internal/resolve/resolve.go.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:15 PM UTC · Completed 6:25 PM UTC
Commit: 735a7ce · View workflow run →

[waynesun09]

Following up on the HIGH finding from the earlier squad review (fetchBaseSkillDir in internal/harness/compose.go bypassing this fix) — it looks like it got missed since it was posted as a review-body comment rather than inline (that file isn't in this PR's diff, so GitHub wouldn't let me anchor it to a line).

Suggested fix, if you'd like to fold it into this PR:

1. Extract a shared helper, e.g. in internal/fetch/cache.go (colocated with CachePath/CacheGetDir/CachePutDir):

// NamedSkillPath returns treePath renamed to a symlink using rawName as the
// basename (falling back to treePath unchanged for reserved/unsafe names),
// creating the symlink idempotently if it doesn't already exist.
func NamedSkillPath(treePath, rawName string) (string, error) {
	skillName := filepath.Base(rawName)
	switch skillName {
	case "", ".", "..", "tree", "metadata.json":
		return treePath, nil
	}
	namedPath := filepath.Join(filepath.Dir(treePath), skillName)
	if _, err := os.Lstat(namedPath); os.IsNotExist(err) {
		if err := os.Symlink("tree", namedPath); err != nil && !os.IsExist(err) {
			return "", fmt.Errorf("creating named symlink: %w", err)
		}
	} else if err != nil {
		return "", fmt.Errorf("checking named symlink: %w", err)
	}
	return namedPath, nil
}

(this also folds in the two LOW findings from the review — tolerating EEXIST from a concurrent Symlink race, and explicitly handling non-NotExist Lstat errors instead of silently falling through.)

2. Call it from internal/resolve/resolve.go, replacing the inline block added by this PR:

treePath, err = fetch.NamedSkillPath(treePath, forgeInfo.Path)
if err != nil {
    return Dependency{}, "", fmt.Errorf("%s: %w", field, err)
}

3. Call it from internal/harness/compose.go at the two places that currently return a raw treePath for URL-resolved skills:

• fetchBaseSkill's cache-hit branch (~line 902), before return cachedDep, treePath, nil
• fetchBaseSkillDir (~line 1000), before its final return Dependency{...}, treePath, nil

Both already have the URL path in scope (skillFileURL/forgeInfo.Path via forge.ParseRawContentURL(skillDirURL)), so it's a one-line call plus updating the returned LocalPath.

Happy to see this land in this PR since it's the same root cause, or as a fast-follow if you'd rather keep this one scoped — your call given you own the tradeoff on merge velocity vs. completeness here.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 7:14 PM UTC · Ended 7:28 PM UTC
Commit: b8a817e · View workflow run →

[ralphbean]

Nice catch on the compose path, @waynesun09. Applied the same symlink naming in fetchBaseSkillDir in 8ff231c — also incorporated the TOCTOU and Lstat error handling improvements from the bot feedback while I was at it.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 7:14 PM UTC · Completed 7:28 PM UTC
Commit: 8ff231c · View workflow run →

[waynesun09]

Confirmed the resolve.go fixes (path-traversal, TOCTOU, Lstat error handling) are solid in this revision. One remaining gap found — see inline comment. Not approving until the cache-hit path in fetchBaseSkill gets the same treatment, since it's the code path that actually runs on every re-resolution after the first fetch.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:42 AM UTC · Completed 11:54 AM UTC
Commit: 49e7a47 · View workflow run →

[waynesun09]

Approving — the fix is well-scoped (no scope creep) and the commit history shows good iterative hardening (path traversal, metadata.json collision). Left a few non-blocking findings inline plus one general note:

• The core fix's premise — that openshell sandbox upload follows directory symlinks when uploading — isn't verified anywhere in this diff. If it doesn't resolve symlinks, the original bug may not actually be fixed by the sandbox-upload path. Similarly, resolveSkillDisplayName in internal/runtime/claude.go only falls back to filepath.Base when SKILL.md frontmatter is absent, so the "logging sees the correct name" claim in the commit message is only exercised in that fallback case, and it isn't covered by a new test. Worth a quick manual/integration check that the symlink is actually followed end-to-end, even if it lands as a fast-follow rather than blocking this PR.

None of this blocks merging — flagging for awareness and possible fast-follow.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:06 PM UTC · Completed 5:15 PM UTC
Commit: e254209 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:42 PM UTC · Completed 6:51 PM UTC
Commit: e254209 · View workflow run →

[fullsend-ai-retro]

PR #2626 fixed a bug where URL-resolved skills were named 'tree' instead of their actual name. The fix required 6 commits over 19 days and 7 review runs due to findings surfacing incrementally. The review bot caught path-traversal in the first review but missed three critical cross-file gaps caught only by human reviewer waynesun09: metadata.json collision, compose.go cache-miss bypass, and cache-hit bypass. One review run failed with a 422 post-review error. The human also drove the code duplication extraction. These patterns strengthen existing tracked issues: #1525 (cross-file impact analysis), #1582 (first-pass completeness), and #2569 (post-review 422 regression).

Evidence notes (not filed as issues)

• Evidence for Review agent misses cross-file impact analysis that Review Squad catches #1525: review agent missed compose.go parallel paths that human caught twice on PR fix: use skill directory name from URL instead of cache-internal "tree" #2626 (fullsend-ai/fullsend): On PR fix: use skill directory name from URL instead of cache-internal "tree" #2626 (fix: use skill directory name from URL instead of cache-internal "tree" #2626), the review bot reviewed all 6 commits across 7 runs but never flagged that fetchBaseSkillDir and fetchBaseSkill (cache-hit path) in internal/harness/compose.go independently resolve URL-based skill directories and return the raw 'tree' path — the exact bug the PR was fixing in internal/resolve/resolve.go. Human reviewer waynesun09 caught this twice: the cache-miss path on July 10 (fix: use skill directory name from URL instead of cache-internal "tree" #2626 (comment), leading to commit 8ff231c) and the cache-hit path on July 10 (fix: use skill directory name from URL instead of cache-internal "tree" #2626 (comment), leading to commit 49e7a47). The bot approved on July 9 (run 29047861091) and July 10 (run 29113847679) without examining compose.go. These were the most impactful findings — without them, cached skills in the compose path would still surface as 'tree' in steady state.
• Evidence for Review agent should catch all findings in the first pass to reduce rework cycles #1582: bot found 1 of 5 findings in first pass on PR fix: use skill directory name from URL instead of cache-internal "tree" #2626, driving 6 commits over 19 days (fullsend-ai/fullsend): On PR fix: use skill directory name from URL instead of cache-internal "tree" #2626, the review bot's first review (June 24, run 28121356146) found only 1 of 5 total distinct findings: the '..' path-traversal guard gap. The remaining 4 findings surfaced across later reviews: (1) metadata.json reserved-name collision — caught by human reviewer waynesun09 on July 10, never by bot. (2) TOCTOU race in Lstat/Symlink — caught by bot on review 3 (July 10, run 29113847679). (3) Lstat non-IsNotExist error handling gap — caught by bot on review 3. (4) compose.go parallel path bypass — caught by human on July 10, never by bot. The PR required 6 commits (98fff12, c17bf61, 735a7ce, 8ff231c, 49e7a47, e254209) and 7 review runs. If all findings had been caught in pass 1, the PR could have been completed in 2-3 commits.
• Evidence for post-review 422 regression: diff-hunk validation does not cover all edge cases #2569: post-review 422 on PR fix: use skill directory name from URL instead of cache-internal "tree" #2626 run 29117268669 wasted full agent invocation (fullsend-ai/fullsend): Review run 29117268669 on PR fix: use skill directory name from URL instead of cache-internal "tree" #2626 (commit 8ff231c, July 10 19:14-19:28 UTC) completed agent analysis successfully but failed at the post-review step. The log shows: the diff-hunk filter ran and downgraded 1 finding to a file-level comment ('line outside diff hunk'), then attempted to submit a COMMENT review with 2 remaining inline comments. The GitHub API rejected the submission with '422 Unprocessable Entity'. The error message: 'Error: submitting review: create pull request review on fix: use skill directory name from URL instead of cache-internal "tree" #2626: github api: 422 Unprocessable Entity'. The review was never posted. This is the same failure mode as PR feat: add ESLint, Prettier, and Stylelint for web apps #625 (described in issue post-review 422 regression: diff-hunk validation does not cover all edge cases #2569) where the diff-hunk validation runs but doesn't catch all edge cases.