fix(workspace): count an IDE pin and this client's manifest as binding evidence - #1376
Conversation
…g evidence #1374 stays quiet when a binding lookup fails for a project with no sign of a workspace. Two cases slipped through that check: - An IDE pin (`ALTIMATE_PINNED_WORKSPACE_*`) is a workspace the user chose, but it is never written to disk. An inaccessible pinned workspace, or an offline first validation, was silent — including in the IDE's Refresh report. A pin for this tree now counts, and so does a malformed one: the extension set it, and resolution fails closed on it. - Any `_workspace` folder counted, including one the user or another tool created. Only this client's manifest does now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1373 added its own `readPin` import and a pin purge on the same unknown branch. Keep one import, and scope the pin evidence with the same `resolveWithinRoot` as that purge, so the two agree on which folders a pin speaks for. The purge removes the manifest before the evidence check; the pin still counts, so a user loses the skills and is told why. Tested together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWorkspace binding evidence now includes IDE pins and a readable manifest. Tests cover offline binding lookups with unmanaged workspace folders and project-rooted or incomplete IDE pins. ChangesWorkspace binding evidence
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No merge-blocking issue was established for the workspace-binding evidence change. Merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to A pin that cannot be confirmed is now reported, including after its skills are removed. One narrow case appears less visible: if a previously synced manifest becomes unreadable, an offline check may leave the skills in place without reporting that the workspace could not be confirmed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the pin at dawn, Comment |
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by gpt-sol-latest · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/workspace/skill-sync.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/skill-sync.ts:790">
P2: This counts any shallowly parseable `.manifest.json` as this client’s evidence, so a user or another tool can create a lookalike manifest and make an unlinked project emit offline warnings. Tighten manifest validation or ownership before using it for evidence.</violation>
<violation number="2" location="packages/opencode/src/altimate/workspace/skill-sync.ts:790">
P3: The new `readManifest` arm replaces the old `hasManagedSnapshot` lstat check, so a snapshot this client published whose manifest no longer validates (corrupt write, partial truncation) stops counting as evidence. Its skills still load from disk (discovery ignores the manifest) and `deactivate` refuses to remove an unreadable manifest, so the workspace keeps serving skills while the binding-lookup warning goes quiet. If narrowing to a fully valid manifest is deliberate, this tradeoff is worth a note; otherwise, damaged snapshots now lose their warning.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (pin.kind === "valid" && resolveWithinRoot(directory, pin.root) !== null) return true | ||
| // This client's manifest, not merely a `_workspace` folder: a tree the user | ||
| // or another tool created says nothing about a binding. | ||
| if ((await readManifest(directory).catch(() => null)) !== null) return true |
There was a problem hiding this comment.
P2: This counts any shallowly parseable .manifest.json as this client’s evidence, so a user or another tool can create a lookalike manifest and make an unlinked project emit offline warnings. Tighten manifest validation or ownership before using it for evidence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/workspace/skill-sync.ts, line 790:
<comment>This counts any shallowly parseable `.manifest.json` as this client’s evidence, so a user or another tool can create a lookalike manifest and make an unlinked project emit offline warnings. Tighten manifest validation or ownership before using it for evidence.</comment>
<file context>
@@ -772,11 +772,22 @@ async function hasManagedSnapshot(directory: string): Promise<boolean> {
+ if (pin.kind === "valid" && resolveWithinRoot(directory, pin.root) !== null) return true
+ // This client's manifest, not merely a `_workspace` folder: a tree the user
+ // or another tool created says nothing about a binding.
+ if ((await readManifest(directory).catch(() => null)) !== null) return true
return (await readLocalBinding(directory).catch(() => null)) !== null
}
</file context>
| if (pin.kind === "valid" && resolveWithinRoot(directory, pin.root) !== null) return true | ||
| // This client's manifest, not merely a `_workspace` folder: a tree the user | ||
| // or another tool created says nothing about a binding. | ||
| if ((await readManifest(directory).catch(() => null)) !== null) return true |
There was a problem hiding this comment.
P3: The new readManifest arm replaces the old hasManagedSnapshot lstat check, so a snapshot this client published whose manifest no longer validates (corrupt write, partial truncation) stops counting as evidence. Its skills still load from disk (discovery ignores the manifest) and deactivate refuses to remove an unreadable manifest, so the workspace keeps serving skills while the binding-lookup warning goes quiet. If narrowing to a fully valid manifest is deliberate, this tradeoff is worth a note; otherwise, damaged snapshots now lose their warning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/workspace/skill-sync.ts, line 790:
<comment>The new `readManifest` arm replaces the old `hasManagedSnapshot` lstat check, so a snapshot this client published whose manifest no longer validates (corrupt write, partial truncation) stops counting as evidence. Its skills still load from disk (discovery ignores the manifest) and `deactivate` refuses to remove an unreadable manifest, so the workspace keeps serving skills while the binding-lookup warning goes quiet. If narrowing to a fully valid manifest is deliberate, this tradeoff is worth a note; otherwise, damaged snapshots now lose their warning.</comment>
<file context>
@@ -772,11 +772,22 @@ async function hasManagedSnapshot(directory: string): Promise<boolean> {
+ if (pin.kind === "valid" && resolveWithinRoot(directory, pin.root) !== null) return true
+ // This client's manifest, not merely a `_workspace` folder: a tree the user
+ // or another tool created says nothing about a binding.
+ if ((await readManifest(directory).catch(() => null)) !== null) return true
return (await readLocalBinding(directory).catch(() => null)) !== null
}
</file context>
1 participant
Issue for this PR
No GitHub issue. Follow-up to #1374, from review comments that arrived after it merged.
Type of change
What does this PR do?
#1374 stays quiet when a binding lookup fails in a project that shows no sign of having a workspace. Without that check, being offline produced a warning in every opted-in repo, including repos never linked. The check missed two cases.
An IDE pin now counts. The extension can choose a workspace with
ALTIMATE_PINNED_WORKSPACE_*. That choice is never written to disk. If the pinned workspace was inaccessible, or offline on its first check, the sync said nothing — including in the IDE's Refresh report. A pin for this folder now counts as evidence. A malformed pin counts too, because the extension set it and resolution fails closed on it everywhere.Only this client's manifest counts. Previously any
_workspacefolder counted, including one the user or another tool created. Now only the manifest this client writes counts.Interaction with #1373. #1373 landed while this was open. It removes the skill snapshot when a pin cannot be honoured. That removal deletes the manifest before the evidence check runs, so the pin has to count on its own. The user loses the skills and is told why. Both checks now use the same
resolveWithinRootto decide which folders a pin applies to, so they always agree. One test covers the two together.How did you verify your code works?
_workspacefolder does not countflushPendingSyncs … would abandon(a 5s timeout); it fails the same way onmain.Screenshots / recordings
N/A. No UI change beyond when the existing warning appears.
Checklist
🤖 Generated with Claude Code
Summary by cubic
Fixes the workspace binding evidence check in skill sync so a failed lookup warns the user in two cases it previously missed.
ALTIMATE_PINNED_WORKSPACE_*) now counts as evidence. Pins are never written to disk, so an inaccessible pin was silent before; a malformed pin counts too._workspacefolder the user or another tool created no longer triggers the warning.resolveWithinRootas the pin purge from fix(workspace): relink memory, consent before seeding, pin skills, clearer link and /workspace #1373, so after the purge removes the manifest, the warning still fires and the user is told why.Written for commit def6080. Summary will update on new commits.
Summary by CodeRabbit