Skip to content

fix(#5396): add .yaml extension to cached profile paths for openshell#5397

Merged
waynesun09 merged 1 commit into
mainfrom
agent/5396-profile-cache-extension
Jul 21, 2026
Merged

fix(#5396): add .yaml extension to cached profile paths for openshell#5397
waynesun09 merged 1 commit into
mainfrom
agent/5396-profile-cache-extension

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

URL-fetched openshell profiles are cached at content-addressed paths (.fullsend-cache/resources/sha256/<hash>/content) with no file extension. OpenShell's CLI selects its YAML parser strictly by file extension, causing an immediate unsupported provider profile file format error for any URL-referenced profile. This fix creates a <id>.yaml symlink next to the cached content file, giving openshell the extension it expects.

Changes

  • Generalize CacheNamedSymlink to derive its symlink target from filepath.Base(cachePath) instead of hardcoding "tree", enabling it to work for both directory caches and single-file caches
  • Apply CacheNamedSymlink in ResolveHarness (live fetch path) to symlink cached profiles as <id>.yaml
  • Apply CacheNamedSymlink in resolveFromLock (lock-file replay path) for the same treatment
  • Strengthen TestResolveHarness_ProfileURL to assert .yaml suffix and verify readability through the symlink
  • Add CacheNamedSymlink tests for content-path symlinks and reserved-name fallback behavior

Testing

  • All internal/fetch cache tests pass (including new CacheNamedSymlink content-path tests)
  • All internal/resolve tests pass (including strengthened TestResolveHarness_ProfileURL)
  • All internal/cli lock tests pass
  • go vet and go build clean
  • Secret scan passed

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • I wrote this contribution myself and can explain all changes in it

Closes #5396

Post-script verification

  • Branch is not main/master (agent/5396-profile-cache-extension)
  • Secret scan passed (gitleaks — 3cfa255ab4cc8190670585ea42da529119251632..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner July 21, 2026 00:45
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Agent PR ready for human review label Jul 21, 2026
@github-actions

Copy link
Copy Markdown

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:47 AM UTC · Completed 12:59 AM UTC
Commit: 7a73c60 · View workflow run →

@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

Site preview

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

Commit: 287637bda8d64c32f158384e36d1d54f1b74c97a

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.41176% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/fetch/cache.go 70.83% 4 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Low

  • [test adequacy] internal/cli/lock.go:671 — The resolveFromLock path now creates a named symlink for cached profiles (parallel to the change in resolve.go), but the existing TestResolveFromLock_ProfileReconstruction test does not verify that the profile's LocalPath ends with .yaml. The resolve_test.go test was updated with this assertion, but the equivalent lock_test.go test was not. If the lock.go symlink call were accidentally removed or broken, no test would catch it.
    Remediation: Add assertions to TestResolveFromLock_ProfileReconstruction verifying strings.HasSuffix(lockResult.Profiles[0].LocalPath, ".yaml") and that the path is readable via os.ReadFile.

Labels: PR fixes a bug in harness profile resolution caching

Comment thread internal/cli/lock.go
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge component/harness Agent harness, config, and skills loading type/bug Confirmed defect in existing behavior labels Jul 21, 2026
@waynesun09
waynesun09 force-pushed the agent/5396-profile-cache-extension branch from 7a73c60 to a624b2c Compare July 21, 2026 15:42
URL-fetched openshell profiles are cached at content-addressed paths
(.fullsend-cache/resources/sha256/<hash>/content) with no file
extension. OpenShell's CLI selects its YAML parser strictly by file
extension, so the extensionless "content" filename causes an immediate
"unsupported provider profile file format" error at profile import.

Generalize CacheNamedSymlink to derive its symlink target from
filepath.Base(cachePath) instead of hardcoding "tree", making it work
for both directory caches ("tree") and single-file caches ("content").
Apply it in both ResolveHarness (live fetch) and resolveFromLock
(lock-file replay) to create a <id>.yaml symlink next to the cached
content file, giving openshell the file extension it requires.

Strengthen TestResolveHarness_ProfileURL to assert the .yaml suffix
and verify readability through the symlink. Add CacheNamedSymlink
tests for content-path handling and reserved-name fallback.

Closes #5396

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via multi-agent review squad (Claude, Grok). Two MEDIUM findings on the cached-profile symlink helper addressed: replacement is now atomic (temp symlink + rename), and link names are validated as single path segments. Error-branch coverage added — codecov/patch now green. All CI checks pass.

@waynesun09
waynesun09 added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit 6ef9152 Jul 21, 2026
16 checks passed
@waynesun09
waynesun09 deleted the agent/5396-profile-cache-extension branch July 21, 2026 21:31
@fullsend-ai-retro

fullsend-ai-retro Bot commented Jul 21, 2026

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 9:34 PM UTC · Completed 9:43 PM UTC
Commit: 287637b · View workflow run →

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #5397 — fix(#5396): add .yaml extension to cached profile paths for openshell

Timeline

  1. Issue URL-referenced openshell profile import fails: cached content has no file extension #5396 filed by waynesun09 (2026-07-20 23:54 UTC) — URL-fetched openshell profiles cached without file extension, causing parser failures.
  2. Triage agent (run 29789390704) ran ~5 min, correctly classified as High priority, recommended generalizing CacheNamedSymlink.
  3. Code agent (run 29790790027) ran ~9 min, produced PR fix(#5396): add .yaml extension to cached profile paths for openshell #5397 with 6 files changed (+376/-20).
  4. Review agent (run 29791254060) ran ~13 min, found 1 LOW (test adequacy in lock_test.go), approved.
  5. Human reviewer (waynesun09) used a multi-agent review squad (Claude + Grok) and identified 2 MEDIUM findings the review agent missed: (a) non-atomic symlink replacement (check-then-act race), (b) missing path-segment validation on linkName. Human force-pushed twice over ~20 hours to fix these plus add error-branch coverage (codecov went from 79.41% to passing).
  6. PR merged 2026-07-21 21:31 UTC, ~21 hours after creation.

Review gap

The review agent approved at commit 7a73c60 with only a LOW finding. The human subsequently found and fixed two MEDIUM issues:

  • Non-atomic symlink replacement: CacheNamedSymlink used a check-then-act pattern (os.Lstatos.Symlink) that races under concurrency. The atomicWrite function using temp+rename was literally in the same file (cache.go). The correctness sub-agent lists "race conditions" in its scope but did not apply it here.
  • Missing path-segment validation: The linkName parameter (derived from profile YAML, untrusted input) was passed to filepath.Join without checking for path separators, allowing potential cache directory escape. The security sub-agent covers "path traversal" as an injection category but did not flag this.

Existing issue evidence

What went well

  • Triage agent correctly identified severity, root cause, and recommended fix approach.
  • Code agent produced a working solution in ~9 min that addressed the core problem.
  • Review agent correctly found the lock_test.go test adequacy gap.
  • The full pipeline (triage → code → review) completed in under 30 min.

Proposals filed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/harness Agent harness, config, and skills loading ready-for-merge All reviewers approved — ready to merge ready-for-review Agent PR ready for human review type/bug Confirmed defect in existing behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

URL-referenced openshell profile import fails: cached content has no file extension

1 participant