fix(skill+memory): plug destructive-on-failure race + flow-style YAML roles - #11
Merged
Merged
Conversation
… roles Bug surfaced by 2026-05-26 dogfood: skillConsolidator dispatched fork sent `SkillWrite(name:X, overwrite:true) + SkillDelete(name:X)` in one batch. SkillWrite failed frontmatter validation; SkillDelete ran anyway and silently dropped the user's prior on-disk skill. Model final text was "nothing to merge this pass" — it didn't even know it had destroyed user data. Same-shape race exists for memoryCurator's MemoryWriteAt + MemoryDelete pair. Three coordinated fixes: 1. `parseFrontmatter` now accepts YAML flow-style arrays (`roles: [main]`, `roles: [main, web]`, `roles: []`). The dogfood `SkillWrite` failed because the LightClaw hand-rolled YAML parser only handled block-style (`-` per line); the LLM wrote the more compact flow-style. Single-line parser respects quoted commas (`["a,b", c]` → `['a,b', 'c']`). 2. New `src/utils/transient-failure-set.ts`: TTL'd in-memory set, used by `src/skill/destructive-guard.ts` and `src/memory/destructive-guard.ts`. SkillWrite catch arm records `(userId, name)` on any validation failure; SkillDelete refuses with actionable message when a same-name record landed within 30s. Same shape for MemoryWriteAt + MemoryDelete with `(memoryDir, absoluteTargetPath)` keys. Covers the class — not just today's instance — so future schema additions can't resurrect the silent-data-loss surface. 3. `parseSkillRoles` error message now names both expected shape (`roles: [main]` flow-style, or block-style) and echoes the `JSON.stringify`'d actual value, so a retrying LLM has enough signal to self-correct. Observability: `[skill-write] validation failed`, `[skill-delete] refused`/`deleted`, `[memory-write-at] validation failed`, `[memory-delete] refused`/`deleted` stderr lines on every destructive boundary — so the next dogfood doesn't need fork-transcript spelunking to find a silent-deletion event. Test plan: - 13 new test cases added; confirmed all 13 fail on pre-fix main. - `pnpm test` 1705/1705 pass post-fix. - `pnpm typecheck` clean. - `pnpm build` clean (1500.81 kB). Prompt-side root causes (`skillConsolidator` step 3 emits the destructive pair; "nothing to merge this pass" final-text doesn't acknowledge destructive ops) are NOT touched here per the prompt-changes-need-separate-PR convention; they will be a follow-up PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8962b6027b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // (typical of the dream-curator destructive pattern) refuses instead of | ||
| // silently dropping the prior on-disk skill. See | ||
| // `src/skill/destructive-guard.ts`. | ||
| recordSkillWriteFailure(userId, input.name) |
There was a problem hiding this comment.
The new destructive guard can be bypassed because SkillWrite records failures with raw input.name, while SkillDelete checks the guard using normalizeSkillName(input.name). If the write request uses a normalizable variant like trailing whitespace (for example "merge-target ") and fails validation, the recorded key differs from the delete key, so the immediate same-name delete is not blocked and can still remove the prior on-disk skill.
Useful? React with 👍 / 👎.
Comment on lines
+39
to
+40
| record(key) { | ||
| recordedAt.set(key, now()) |
There was a problem hiding this comment.
The TTL registry only garbage-collects in isRecent(), but write failures call record() even when no delete checks happen. In long-running processes that see many unique failed writes but few/no deletes, expired entries remain in recordedAt indefinitely, so the map can grow without bound despite a 30s TTL and eventually hurt memory usage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes a class of “destructive-on-failure” races (Write fails → Delete still runs and destroys the prior on-disk content) for both skills and memory, and updates the shared frontmatter parser to accept YAML flow-style arrays (e.g. roles: [main]) that previously caused valid inputs to fail parsing/validation.
Changes:
- Extend the shared
parseFrontmatterimplementation to parse single-line YAML flow-style arrays ([a, b],[], quoted items). - Add a generic TTL-based in-memory failure registry and use it to block same-target deletes shortly after write failures (skills + memory).
- Improve error/diagnostic output (more actionable roles-frontmatter errors + stderr observability) and add regression tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/transient-failure-set.ts | New in-memory TTL registry used to remember recent write failures. |
| src/utils/transient-failure-set.test.ts | Unit tests for TTL behavior + lazy GC. |
| src/skill/destructive-guard.ts | Skill-specific wrapper around transient failure set (userId + normalized skill name). |
| src/memory/destructive-guard.ts | Memory-specific wrapper around transient failure set (memoryDir + absolute target path). |
| src/tools/skill-write.ts | Records recent write failures + adds stderr logging on failure. |
| src/tools/skill-write.test.ts | Regression tests for flow-style roles: [...] + improved roles error message. |
| src/tools/skill-delete.ts | Blocks deletes when a same-name write failed recently + adds stderr logging. |
| src/tools/skill-delete.test.ts | Tests for block behavior (same-name, per-user scoping, allow when clean). |
| src/tools/memory-write-at.ts | Records recent failures keyed by resolved absolute target + stderr logging. |
| src/tools/memory-delete.ts | Blocks deletes when a same-path write failed recently + adds stderr logging. |
| src/tools/memory-delete.test.ts | Tests for block behavior and preservation of existing file when write fails. |
| src/skill/loader.ts | Makes roles frontmatter errors more actionable (examples + actual value). |
| src/memory/auto-memory.ts | Adds flow-style array parsing to the shared frontmatter parser. |
| src/memory/auto-memory.test.ts | Adds parser tests for flow-style arrays + regressions for block lists and scalars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+71
to
+75
| recordSkillWriteFailure(userId, input.name) | ||
| const reason = error instanceof Error ? error.message : String(error) | ||
| process.stderr.write( | ||
| `[skill-write] validation failed user=${userId} name=${input.name} reason=${reason}\n`, | ||
| ) |
Comment on lines
+82
to
+86
| throw new Error( | ||
| `Skill ${filePath} frontmatter "roles" must be a YAML list of role names ` + | ||
| `(e.g. \`roles: [main]\` flow-style, or block-style with one \`- name\` per ` + | ||
| `line). Got: ${JSON.stringify(value)}.`, | ||
| ) |
Comment on lines
+60
to
+64
| output: | ||
| `Refusing to delete skill "${normalizedName}": a SkillWrite for the same name failed ` + | ||
| `${ageSec}s ago. Deleting now would drop the still-present prior version. Retry the ` + | ||
| `SkillWrite (fix the validation error first); once it returns is_error:false, the ` + | ||
| `delete becomes safe.`, |
Comment on lines
+60
to
+63
| process.stderr.write( | ||
| `[memory-write-at] validation failed memoryDir=${memoryDir} path=${input.path} reason=${msg}\n`, | ||
| ) | ||
| return { output: msg, isError: true } |
Comment on lines
+84
to
+88
| recordMemoryWriteAtFailure(memoryDir, target) | ||
| const reason = error instanceof Error ? error.message : String(error) | ||
| process.stderr.write( | ||
| `[memory-write-at] validation failed memoryDir=${memoryDir} path=${input.path} reason=${reason}\n`, | ||
| ) |
Comment on lines
+44
to
+48
| output: | ||
| `Refusing to delete ${input.path}: a MemoryWriteAt for the same path failed ` + | ||
| `${ageSec}s ago. Deleting now would drop the still-present prior version. Retry the ` + | ||
| `MemoryWriteAt (fix the validation error first); once it returns is_error:false, the ` + | ||
| `delete becomes safe.`, |
6 tasks
RowitZou
pushed a commit
that referenced
this pull request
May 26, 2026
…t honesty PR-A (#11) physically blocked the silent data loss with a same-name destructive guard; this PR closes the prompt-side root causes from the 2026-05-26 dogfood so the model stops emitting the destructive pair in the first place. skillConsolidator workflow (`src/agents/bundled/skillConsolidator.ts`): - Step 2: "A single skill is never a merge candidate" — explicit guard against today's degenerate case (model treated size=1 group as X-merges-into-X and SkillDelete'd the same name). - Step 3 split: 3.a SkillWrite is a standalone tool_use; wait for `is_error:false`. 3.b deletes only run after that confirmation, and **never** delete the surviving name. 3.c on `is_error:true`, abort the group; do not retry blindly. Bundling 3.a and 3.b in the same assistant turn is forbidden. - Output discipline: final text must enumerate SkillWrite and SkillDelete success counts plus any aborted groups. Forbids "nothing to merge" if any destructive tool fired — that exact phrase is what the model used in the dogfood while it had just deleted the user's only skill. memoryCurator (`src/agents/bundled/memoryCurator.ts`): mirrors the same sequencing for MemoryWriteAt + MemoryDelete (both in cross-role promotion and within-directory cleanup), plus the same final-text honesty rule. Snapshot test hashes (`role-prompt.snapshot.test.ts` + `prompt/prompt-for-role.test.ts`) updated for memoryCurator (22c073df → c37344ca) and skillConsolidator (3e45d127 → c2700894); deliberate semantic change, not a structural refactor — see [feedback_prompt_changes]. New semantic contract test (`curator-prompt-contract.test.ts`, 13 cases) anchors the key sequencing phrases so a future rewrite that forgets the ordering rules fails loudly. These are intent guards on top of the snapshot's byte-level pin. Test plan: - pnpm test: 1724/1724 PASS (1711 from main + 13 new contract cases) - pnpm typecheck clean - pnpm build clean - Re-confirmed the cleanup-line regex matches by running just that one test before adding the rest Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes Bug 1 from the 2026-05-26 dogfood report —
skillConsolidatordispatched fork silently deleted a user's legitimate skill because:parseFrontmatterrejected flow-style arrays (roles: [main]), so a perfectly validSkillWritefrom the consolidator failed validation.SkillWrite(name:X, overwrite:true) + SkillDelete(name:X)in one batch. SkillWrite failed → SkillDelete ran anyway → user's prior on-disk skill silently gone.Same race shape exists for
memoryCurator'sMemoryWriteAt + MemoryDelete, so this PR fixes the class, not just the instance.What this PR does
parseFrontmatternow accepts flow-style YAML arrays ([main],[main, web],[], quoted commas). Single-element flow-style was the LLM's natural choice — bundled skills all use block-style so the bug stayed latent until now.src/utils/transient-failure-set.ts(generic TTL'd in-memory set) +src/skill/destructive-guard.ts+src/memory/destructive-guard.tsthin wrappers. SkillWrite catch arm records(userId, name); SkillDelete refuses for 30s on same key. Symmetric for MemoryWriteAt + MemoryDelete with(memoryDir, absoluteTargetPath).parseSkillRolesnow names expected shape (both flow- and block-style examples) and echoesJSON.stringify'd actual value — retrying LLM has enough signal to self-correct without re-rolling the same mistake.[skill-write] validation failed,[skill-delete] refused/deleted,[memory-write-at] validation failed,[memory-delete] refused/deleted. Future dogfood won't need fork-transcript spelunking to find a silent-deletion event.What this PR explicitly does NOT do
Prompt-side root causes from the dogfood:
skillConsolidatorprompt step 3 instructs the destructive pairThese need a separate prompt PR per the prompt-changes-must-be-discussed convention. Defense-in-depth here (parser fix + guard) means the next dogfood is safe even before the prompt PR lands.
Test plan
main(pnpm test→ 13 fail before src changes)pnpm test→ 1705/1705 pass post-fixpnpm typecheckcleanpnpm buildclean (1500.81 kB)skillConsolidatorrun a dream pass; verify same-name SkillDelete refused when SkillWrite fails; verify[skill-delete] refusedstderr appears🤖 Generated with Claude Code