Skip to content

Commit 5b1e3b7

Browse files
sahibzada-allahyarsahibzada-allahyar
committed
fix(skills): preserve proposal status during stale reconciliation
1 parent 01f0e97 commit 5b1e3b7

2 files changed

Lines changed: 143 additions & 13 deletions

File tree

‎src/skills/workshop/service.test.ts‎

Lines changed: 105 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,11 @@ import {
2222
resolvePendingSkillProposal,
2323
reviseSkillProposal,
2424
} from "./service.js";
25-
import { readSkillProposalManifest, resolveProposalDraftPath } from "./store.js";
25+
import {
26+
readSkillProposalManifest,
27+
readSkillProposalRecord,
28+
resolveProposalDraftPath,
29+
} from "./store.js";
2630

2731
const tempDirs = createTrackedTempDirs();
2832
let envSnapshot: ReturnType<typeof captureEnv>;
@@ -941,4 +945,104 @@ describe("skill workshop proposals", () => {
941945
expect(result?.record.status).toBe("stale");
942946
expect(result?.record.statusReason).toBe("Target skill was created after proposal creation.");
943947
});
948+
949+
it("preserves concurrent applied record during listSkillProposals stale reconciliation (lock-and-reread)", async () => {
950+
// Regression: when a concurrent apply changes the record to "applied" before
951+
// stale reconciliation runs, the re-read inside the lock should preserve the
952+
// "applied" status instead of overwriting it to "stale".
953+
const workspaceDir = await makeWorkspace();
954+
const proposal = await proposeCreateSkill({
955+
workspaceDir,
956+
name: "Concurrent Apply",
957+
description: "Applied before stale check",
958+
content: "# Concurrent Apply\n\nApplied before stale check.\n",
959+
});
960+
961+
// Manually create the target skill file on disk (triggers stale path)
962+
const skillDir = path.dirname(proposal.record.target.skillFile);
963+
await fs.mkdir(skillDir, { recursive: true });
964+
await fs.writeFile(proposal.record.target.skillFile, "# Concurrent Apply\n", "utf8");
965+
966+
// Manually apply the proposal by writing "applied" status directly to the record.
967+
// This simulates what applySkillProposal does before the stale lock re-read.
968+
const staleRecord = {
969+
...proposal.record,
970+
status: "applied" as const,
971+
appliedAt: new Date().toISOString(),
972+
updatedAt: new Date().toISOString(),
973+
};
974+
const { updateSkillProposalRecord } = await import("./store.js");
975+
await updateSkillProposalRecord({ record: staleRecord });
976+
977+
// listSkillProposals triggers reconcileStaleCreateProposals.
978+
// With the lock-and-reread fix, the re-read inside the lock sees "applied"
979+
// and skips marking stale. Without the fix, it would use the stale outer
980+
// record and overwrite to "stale".
981+
await listSkillProposals({ workspaceDir });
982+
983+
const record = await readSkillProposalRecord(proposal.record.id);
984+
expect(record?.status).toBe("applied");
985+
});
986+
987+
it("preserves concurrent applied record during inspectSkillProposal stale reconciliation (lock-and-reread)", async () => {
988+
// Regression: same invariant as above but through the inspect path.
989+
const workspaceDir = await makeWorkspace();
990+
const proposal = await proposeCreateSkill({
991+
workspaceDir,
992+
name: "Inspect Concurrent",
993+
description: "Applied before inspect stale check",
994+
content: "# Inspect Concurrent\n\nApplied before inspect stale check.\n",
995+
});
996+
997+
// Manually create the target skill file on disk
998+
const skillDir = path.dirname(proposal.record.target.skillFile);
999+
await fs.mkdir(skillDir, { recursive: true });
1000+
await fs.writeFile(proposal.record.target.skillFile, "# Inspect Concurrent\n", "utf8");
1001+
1002+
// Manually apply the record to "applied" status
1003+
const staleRecord = {
1004+
...proposal.record,
1005+
status: "applied" as const,
1006+
appliedAt: new Date().toISOString(),
1007+
updatedAt: new Date().toISOString(),
1008+
};
1009+
const { updateSkillProposalRecord } = await import("./store.js");
1010+
await updateSkillProposalRecord({ record: staleRecord });
1011+
1012+
// inspectSkillProposal triggers its own stale check with lock-and-reread.
1013+
// The re-read should see "applied" and skip marking stale.
1014+
const result = await inspectSkillProposal(proposal.record.id, { workspaceDir });
1015+
1016+
expect(result?.record.status).toBe("applied");
1017+
});
1018+
1019+
it("preserves concurrent rejected record during listSkillProposals stale reconciliation (lock-and-reread)", async () => {
1020+
// Regression: concurrent "rejected" status should also be preserved.
1021+
const workspaceDir = await makeWorkspace();
1022+
const proposal = await proposeCreateSkill({
1023+
workspaceDir,
1024+
name: "Concurrent Reject",
1025+
description: "Rejected before stale check",
1026+
content: "# Concurrent Reject\n\nRejected before stale check.\n",
1027+
});
1028+
1029+
// Manually create the target skill file on disk
1030+
const skillDir = path.dirname(proposal.record.target.skillFile);
1031+
await fs.mkdir(skillDir, { recursive: true });
1032+
await fs.writeFile(proposal.record.target.skillFile, "# Concurrent Reject\n", "utf8");
1033+
1034+
// Manually reject the record
1035+
const staleRecord = {
1036+
...proposal.record,
1037+
status: "rejected" as const,
1038+
updatedAt: new Date().toISOString(),
1039+
};
1040+
const { updateSkillProposalRecord } = await import("./store.js");
1041+
await updateSkillProposalRecord({ record: staleRecord });
1042+
1043+
await listSkillProposals({ workspaceDir });
1044+
1045+
const record = await readSkillProposalRecord(proposal.record.id);
1046+
expect(record?.status).toBe("rejected");
1047+
});
9441048
});

‎src/skills/workshop/service.ts‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ const MAX_PROPOSAL_DIRECTORY_ENTRIES = MAX_PROPOSAL_SUPPORT_FILES * 4;
7777
const MAX_SKILL_PROPOSAL_DESCRIPTION_BYTES = 160;
7878

7979
/** Reconcile pending `create` proposals whose target skill file already exists
80-
* on disk (e.g. manually installed) by marking them stale. */
80+
* on disk (e.g. manually installed) by marking them stale.
81+
* Uses the proposal target lock and re-reads the record inside the lock to
82+
* avoid racing with concurrent apply/mutate operations. */
8183
async function reconcileStaleCreateProposals(workspaceDir: string): Promise<void> {
8284
const manifest = await readSkillProposalManifest();
8385
for (const proposal of manifest.proposals) {
@@ -91,10 +93,22 @@ async function reconcileStaleCreateProposals(workspaceDir: string): Promise<void
9193
if (record.kind !== "create") {
9294
continue;
9395
}
94-
const currentContent = await readWorkspaceSkillFile(record.target.skillFile);
95-
if (currentContent !== null) {
96-
await markProposalStale(record, "Target skill was created after proposal creation.");
97-
}
96+
await withSkillProposalTargetLock(record, async () => {
97+
// Re-read the record inside the lock so we see the latest state
98+
const lockedRecord = await readSkillProposalRecord(proposal.id);
99+
if (
100+
!lockedRecord ||
101+
lockedRecord.status !== "pending" ||
102+
lockedRecord.kind !== "create" ||
103+
!isProposalInWorkspace(lockedRecord, workspaceDir)
104+
) {
105+
return;
106+
}
107+
const currentContent = await readWorkspaceSkillFile(lockedRecord.target.skillFile);
108+
if (currentContent !== null) {
109+
await markProposalStale(lockedRecord, "Target skill was created after proposal creation.");
110+
}
111+
});
98112
}
99113
}
100114

@@ -132,15 +146,27 @@ export async function inspectSkillProposal(
132146
return null;
133147
}
134148
// Reconcile a pending create proposal whose target skill file now
135-
// exists on disk (manually installed).
149+
// exists on disk (manually installed). Uses the proposal target lock and
150+
// re-reads the record inside the lock to avoid racing with concurrent apply.
136151
if (read.record.status === "pending" && read.record.kind === "create" && options.workspaceDir) {
137-
const currentContent = await readWorkspaceSkillFile(read.record.target.skillFile);
138-
if (currentContent !== null) {
139-
await markProposalStale(read.record, "Target skill was created after proposal creation.");
140-
const refreshed = await readSkillProposal(proposalId);
141-
if (refreshed) {
142-
return await hydrateProposalSupportFiles(refreshed);
152+
await withSkillProposalTargetLock(read.record, async () => {
153+
const lockedRecord = await readSkillProposalRecord(proposalId);
154+
if (
155+
!lockedRecord ||
156+
lockedRecord.status !== "pending" ||
157+
lockedRecord.kind !== "create" ||
158+
!isProposalInWorkspace(lockedRecord, options.workspaceDir!)
159+
) {
160+
return;
143161
}
162+
const currentContent = await readWorkspaceSkillFile(lockedRecord.target.skillFile);
163+
if (currentContent !== null) {
164+
await markProposalStale(lockedRecord, "Target skill was created after proposal creation.");
165+
}
166+
});
167+
const refreshed = await readSkillProposal(proposalId);
168+
if (refreshed) {
169+
return await hydrateProposalSupportFiles(refreshed);
144170
}
145171
}
146172
return await hydrateProposalSupportFiles(read);

0 commit comments

Comments
 (0)