Skip to content

Commit 3ce632f

Browse files
sahrizviclaude
andcommitted
fix(workspace): an unknown credentials read is not a scope change; the sync marker never vouches for a snapshot it did not check
- The sidebar's post-commit scope checks answer same/changed/unknown. A credentials read that fails after the initial checks is unknown: the pending commit is skipped (it could pair one account's binding with another's URL or counts) but the tile is not cleared, because it is not known to be wrong. Only a scope that reads as a different one clears. - `lastSuccessfulSyncAt` returns null on any marker read failure when a binding was supplied. The in-memory stamp carries no workspace identity, so falling back to it reported the previous binding's age under the current one. Without a binding to answer for, the map is still an answer. - The unchanged run stamps with the manifest it validated, not with a second read at stamp time: another process can swap a snapshot in between, and the marker would then vouch for a sync this run never made. Verified: 552 pass across the workspace + plugin suites, typecheck clean. Mutation-checked: dropping the binding check on the fallback fails a test. The stamp-time re-read cannot be killed by a unit test — the window needs a second process — so that one is closed by construction rather than pinned. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
1 parent 09dc7bc commit 3ce632f

3 files changed

Lines changed: 70 additions & 11 deletions

File tree

‎packages/opencode/src/altimate/workspace/skill-sync.ts‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ export async function lastSuccessfulSyncAt(
299299
// longer describes what is on disk.
300300
const code = (err as NodeJS.ErrnoException)?.code
301301
if (code === "ENOENT" || code === "ENOTDIR") return null
302+
// Any other read failure (EACCES, a truncated read) with a binding to
303+
// answer for: the in-memory stamp carries no workspace identity, so
304+
// falling back to it reported the previous binding's age under the
305+
// current one. The map is only an answer where the caller asked no
306+
// identity question.
307+
if (binding) return null
302308
return lastSyncedAt.get(path.resolve(directory)) ?? null
303309
}
304310
}
@@ -736,6 +742,9 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean
736742
}
737743
}
738744
let changed = false
745+
/** The manifest an unchanged run validated, so its marker describes that
746+
* snapshot rather than whatever is live when the stamp is written. */
747+
let validated: Manifest | null = null
739748
let failed = false
740749
// Set once the workspace's list has actually been read. Only then has this
741750
// project been "checked", and only then should the poll interval start.
@@ -849,7 +858,14 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean
849858
sawRemote = true
850859
syncedFor.set(canon, accountKeyOf(creds.altimateInstanceName, creds.altimateUrl))
851860

852-
if (!foreign && (await upToDate(canon, manifest, remote))) return
861+
if (!foreign && (await upToDate(canon, manifest, remote))) {
862+
// Remembered for the stamp below, which runs after this block settles.
863+
// Re-reading the manifest there instead would stamp whatever tree is
864+
// live by then — another process can swap a partial snapshot in
865+
// between, and the marker would vouch for a sync this run never made.
866+
validated = manifest
867+
return
868+
}
853869

854870
if (remote.length === 0) {
855871
await removeManaged(canon)
@@ -1074,11 +1090,13 @@ export async function syncSkills(directory: string): Promise<{ changed: boolean
10741090
// nothing: the manifest on disk is unchanged, so stamping beside it
10751091
// cannot pair it with another workspace. Only where a snapshot exists —
10761092
// a clean run against an empty workspace removed the root.
1077-
if (!changed) {
1078-
const current = await readManifest(canon)
1079-
if (current)
1080-
await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), markerFor(current, now)).catch(() => {})
1081-
}
1093+
// Written for the snapshot this run CHECKED, not for whatever is on
1094+
// disk now. If another process swapped a different workspace's tree in
1095+
// meanwhile, the marker names the one that was validated and
1096+
// `lastSuccessfulSyncAt` rejects it for the new binding — no age is
1097+
// better than an age vouching for a sync that did not happen.
1098+
if (!changed && validated)
1099+
await fs.writeFile(path.join(managedRoot(canon), SYNCED_MARKER), markerFor(validated, now)).catch(() => {})
10821100
}
10831101
return { changed }
10841102
})()

‎packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,14 @@ function View(props: { api: TuiPluginApi }) {
158158
// manage base and the status each take a credentials read of their own,
159159
// and a switch during either would otherwise pair one account's binding
160160
// with another's URL or counts.
161-
const stillThisScope = async () => (await currentScope()) === scope
161+
// Three answers, not two: a null read is "unknown this instant", and an
162+
// unknown must neither commit (it could pair the wrong account) nor
163+
// clear (the tile is not known to be wrong). Only a scope that reads as
164+
// another one clears.
165+
const scopeNow = async (): Promise<"same" | "changed" | "unknown"> => {
166+
const now = await currentScope()
167+
return now === null ? "unknown" : now === scope ? "same" : "changed"
168+
}
162169
if (outcome.status === "bound") {
163170
// Counts and the manage URL belong to a SPECIFIC workspace. On a rebind
164171
// they would otherwise keep describing the old one until the new status
@@ -186,8 +193,9 @@ function View(props: { api: TuiPluginApi }) {
186193
// cleared the manage URL, or never set one.
187194
if (!b) return
188195
const base = await resolveManageBase()
189-
if (!(await stillThisScope())) {
190-
clearRendered()
196+
const afterBase = await scopeNow()
197+
if (afterBase !== "same") {
198+
if (afterBase === "changed") clearRendered()
191199
return
192200
}
193201
setManageUrl(base ? buildManageUrl(base, b.datamateId) : null)
@@ -203,8 +211,9 @@ function View(props: { api: TuiPluginApi }) {
203211
// again — during an outage neither answer is memoized, and that was two
204212
// requests where one was already too many.
205213
const detail = await Manage.status(dir, { poll: true, binding: b }).catch(() => null)
206-
if (!(await stillThisScope())) {
207-
clearRendered()
214+
const afterStatus = await scopeNow()
215+
if (afterStatus !== "same") {
216+
if (afterStatus === "changed") clearRendered()
208217
return
209218
}
210219
setDetail(detail)

‎packages/opencode/test/altimate/workspace/skill-sync.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,38 @@ describe("workspace skill sync", () => {
594594
expect((await lastSuccessfulSyncAt(project)) as number).toBeGreaterThan(first as number)
595595
})
596596

597+
test("the unchanged run's marker names the snapshot it checked", async () => {
598+
// Tied to the validated manifest, not to whatever tree is live when the
599+
// stamp is written: another process can swap a partial snapshot in
600+
// between, and a marker read from that tree would vouch for a sync this
601+
// run never made.
602+
serve({ "pub-1": { "SKILL.md": "one" } })
603+
await syncSkills(project)
604+
const manifest = JSON.parse(readFileSync(path.join(project, MANAGED, ".manifest.json"), "utf8"))
605+
await new Promise((r) => setTimeout(r, 5))
606+
await syncSkills(project) // unchanged: publishes nothing, stamps only
607+
608+
const marker = JSON.parse(readFileSync(path.join(project, MANAGED, ".synced-at"), "utf8"))
609+
expect(marker.datamateId).toBe(manifest.datamateId)
610+
expect(marker.tenant).toBe(manifest.tenant)
611+
expect(marker.apiUrl).toBe(manifest.apiUrl)
612+
})
613+
614+
test("an unreadable marker is unknown, never the previous binding's age", async () => {
615+
// The in-memory stamp carries no workspace identity. Falling back to it
616+
// on a read failure reported workspace A's age under B.
617+
serve({ "pub-1": { "SKILL.md": "one" } })
618+
await syncSkills(project)
619+
const marker = path.join(project, MANAGED, ".synced-at")
620+
rmSync(marker)
621+
mkdirSync(marker) // a directory where the file should be: EISDIR, not ENOENT
622+
623+
const asked = { datamateId: 99, tenant: TENANT, apiUrl: API_URL }
624+
expect(await lastSuccessfulSyncAt(project, asked)).toBeNull()
625+
// With no identity asked for, the process's own stamp is still an answer.
626+
expect(await lastSuccessfulSyncAt(project)).not.toBeNull()
627+
})
628+
597629
test("a removed snapshot has no last sync, whatever the process remembers", async () => {
598630
// The in-memory stamp survives the purge; the answer must not. After an
599631
// unlink or a rebind the root is gone, and "synced 2m ago" would describe

0 commit comments

Comments
 (0)