fix(schema): preserve YAML formatting when forking a schema - #1607
Conversation
Rename a forked schema via yaml's Document API (parseDocument + doc.set) instead of round-tripping through parseSchema/stringifyYaml, so block scalars, comments, and key order in the source schema.yaml survive the fork. Keep the structural parseSchema validation before the document mutation so an invalid source is still rejected (addresses PR #1130 review). Adds fork-level regression coverage for both formatting preservation and invalid-source rejection. Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesSchema fork safety and fidelity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SchemaForkCommand
participant SourceSchema
participant StagingDirectory
participant DestinationSchema
User->>SchemaForkCommand: request schema fork
SchemaForkCommand->>SourceSchema: parse and validate source
SourceSchema-->>SchemaForkCommand: validation result
SchemaForkCommand->>StagingDirectory: copy source schema
SchemaForkCommand->>StagingDirectory: update YAML document name
SchemaForkCommand->>DestinationSchema: install staged schema
DestinationSchema-->>SchemaForkCommand: restore backup on failure
StagingDirectory-->>SchemaForkCommand: clean up temporary data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/schema.ts`:
- Around line 710-712: Move the trusted-source `schema.yaml` read and
`parseSchema` validation before the destination existence/removal logic in the
schema command flow, while keeping Document API mutation after the copy. Update
the invalid-source regression to assert no destination remains, and add a
`--force` case confirming an existing destination is unchanged when validation
fails; run the focused Vitest command specified in the comment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 321f939e-2a8a-47ea-8e70-7656808c92b7
📒 Files selected for processing (2)
src/commands/schema.tstest/commands/schema-fork-fidelity.test.ts
If the source schema is structurally invalid, parseSchema throws after copyDirRecursive has already created the destination directory, leaving a broken half-schema on disk that made the next fork report "already exists". Wrap the read/validate/rename in a try/catch that removes the just-created destination on any failure and rethrows so the original error still drives the JSON/exit-code reporting. The cleanup can only ever delete a directory this run created: the no-force existing-dest path returns before the copy, and the --force path removes the prior directory first. This also closes a mid-write truncation window for free. Adds regression coverage: cleanup + retryability on invalid source, the pre-existing-destination-is-never-touched invariant, and a lock-in that YAML-ambiguous names (true/false/null/off) round-trip as strings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-up hardening (7424043): an adversarial edge-case pass turned up no data-loss or type-corruption reachable through the Document-API rename (multi-doc YAML makes |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/schema.ts`:
- Around line 708-730: Move source validation in the schema fork flow before the
--force destination removal logic, using the schema at
trustedSourceDir/schema.yaml rather than the copied destination. Ensure parsing
and Document API serialization succeed before replacing an existing destination;
stage the copy if necessary, then atomically remove/replace the destination only
after validation and serialization complete, preserving the original destination
on failure. Update the surrounding logic anchored by parseSchema,
copyDirRecursive, and the destination cleanup catch block.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05ff4b54-2960-4642-a423-6b0d7f517e78
📒 Files selected for processing (2)
src/commands/schema.tstest/commands/schema-fork-fidelity.test.ts
Second hardening pass on the fork command, from an adversarial review of the previously-added cleanup. 1. Atomicity: validate the source's schema.yaml up front, immediately after assertSchemaTreeCanBeCopied and BEFORE the --force removal of an existing destination. Previously the source was validated only after the copy, so `fork --force <invalid-source> <existing-valid-dest>` destroyed the existing destination and then failed, leaving nothing. This matches `schema init`, which already validates before it overwrites. Behavior is unchanged for valid sources, and the redundant post-copy validation is dropped. 2. Never mask the real error: the failure-cleanup rmSync is now wrapped in its own try/catch. fs.rmSync's `force` only suppresses ENOENT, not EPERM/EBUSY/ENOTEMPTY (e.g. a locked file on Windows or a concurrent process), so a failed cleanup could previously replace the real "Invalid schema" diagnostic with a confusing filesystem error. The original error is now always rethrown. Adds regression coverage: --force with an invalid source leaves a valid destination intact; the pre-existing-destination test now uses a valid source so it exercises the no-force "already exists" guard directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Second hardening pass (6498ef5), from an adversarial review of the cleanup I added in 7424043:
New regression coverage: |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
schema fork with identical source and destination plus --force deletes its own source before failing, and replacement removes an existing destination before a complete fork is staged. Reject identical paths, stage and validate before replacement, and add regressions proving self-fork and copy failures preserve existing content.
…ination Two data-loss defects in `schema fork --force` (per alfred-openspec review): 1. Self-fork: forking a schema onto itself removed the destination (which IS the source) before the copy, so the copy then read a directory it had just deleted — destroying the only copy. Now rejected up front by comparing the real (symlink-resolved) source and destination paths before any removal. 2. Non-atomic replacement: an existing destination was removed before the new fork was fully copied and name-updated, so a mid-copy failure left the user with nothing. The fork is now staged in a temporary sibling directory and only swapped into place once complete; any failure while staging leaves both the source and the existing destination untouched. Adds regressions: self-fork is rejected with the source intact; a forced fork whose copy fails leaves the existing destination byte-identical with no staging leftovers. Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec Addressed both data-loss defects in 1. Reject identical source/destination (self-fork). Before any removal, the destination is resolved to its real path ( 2. Stage-then-replace (atomic replacement). The fork is now built in a temporary sibling directory ( New regressions in
Full suite: 3867 passed; the only 2 failures ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/commands/schema-fork-fidelity.test.ts (2)
260-283: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an alias-path regression test for the self-fork guard.
This test covers only the identical-name case. The implementation comment in
src/commands/schema.tsat Lines 691-696 claims that a symlinked destination or a./..spelling of the source is also rejected. No test proves that claim.Add a case that creates a symlink inside
openspec/schemaspointing atsrc-schema, then forkssrc-schemaonto the symlink name with--force. Assert the rejection and assert that the source stays byte-identical. Canonicalize both sides withfs.realpathSync.native()when you compare paths. Guard the symlink creation so the case skips on platforms without symlink support.As per coding guidelines, "Add an alias-path regression test when touching path identity logic" and "When asserting existing filesystem paths as identities, canonicalize both actual and expected paths first using
FileSystemUtils.canonicalizeExistingPath()in project code andfs.realpathSync.native()in test-only expectations".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/commands/schema-fork-fidelity.test.ts` around lines 260 - 283, Add a regression case alongside the existing self-fork test that creates a symlink in openspec/schemas targeting src-schema, skips when symlink creation is unsupported, and forks src-schema to the symlink with --force. Assert the command rejects the alias and verify the source schema remains byte-identical; compare the symlink and source identities using fs.realpathSync.native().Source: Coding guidelines
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the patched module for the default export.
default: actualbypasses the patchedcopyFileSync. Return one patched object as both the named exports anddefault.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/commands/schema-fork-fidelity.test.ts` around lines 13 - 25, Update the node:fs mock in the vi.mock factory so the default export references the same patched object as the named exports; avoid assigning default to the unmodified actual module, ensuring default.copyFileSync uses the failCopyFileSync behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/schema.ts`:
- Around line 729-732: Update schema discovery in isSchemaDir and listSchemas to
ignore dot-prefixed entries, including temporary .fork-staging-* directories, so
schema validate --all cannot treat staging contents as schemas. Preserve
discovery of non-hidden schema directories and the existing staging copy flow.
---
Nitpick comments:
In `@test/commands/schema-fork-fidelity.test.ts`:
- Around line 260-283: Add a regression case alongside the existing self-fork
test that creates a symlink in openspec/schemas targeting src-schema, skips when
symlink creation is unsupported, and forks src-schema to the symlink with
--force. Assert the command rejects the alias and verify the source schema
remains byte-identical; compare the symlink and source identities using
fs.realpathSync.native().
- Around line 13-25: Update the node:fs mock in the vi.mock factory so the
default export references the same patched object as the named exports; avoid
assigning default to the unmodified actual module, ensuring default.copyFileSync
uses the failCopyFileSync behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15e2e270-c388-4635-9f41-d0f9228dffcf
📒 Files selected for processing (2)
src/commands/schema.tstest/commands/schema-fork-fidelity.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The staging change fixes self-forks and copy or validation failures, but the final swap still deletes the existing destination before renameSync. A forced final-rename failure on 87d5da4 leaves the destination schema missing, so please move the destination to a sibling backup, install staging, restore on failure, and add a final-move regression before approval.
…d final move restores The stage-then-swap still removed the destination and then renamed staging into place; if that final rename failed (e.g. a Windows lock) the destination was gone with no restore. Now, when a destination exists, `fork --force` moves it to a sibling backup, installs the staged fork, and only then discards the backup. If the install rename throws, the backup is moved back so the original destination is never lost. Non-existing destinations keep the simple staging rename. Adds a regression: forcing the final staging->destination move to fail leaves the pre-existing destination byte-identical with no staging/backup leftovers. Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec Addressed the final-move edge case (commit 2c20d4c). Backup/restore swap. When a destination already exists,
When the destination does not exist, the simple New regression in Full suite: 3868 passed; only the 2 pre-existing unrelated failures remain ( |
Deploying openspec-docs with
|
| Latest commit: |
cc4fd4b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e76c1df2.openspec-docs.pages.dev |
| Branch Preview URL: | https://claude-pr-1130-merge-ready-5.openspec-docs.pages.dev |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The backup now restores the common final-move failure, but recovery remains unsafe in two exact cases: a failed restore is swallowed with no backup path reported, and staging plus backup directories are accepted by schema discovery, so concurrent list or validate can consume in-progress artifacts. Please exclude owned temporary directories, surface restore failure and its recovery path, and add regressions before approval.
…irs from discovery Two more edge cases from alfred's review: 1. A failed backup->destination restore was silently swallowed, so if the final install AND the restore both failed the user lost the destination with no clue the backup existed. Now that case throws an error naming the backup directory and how to move it back, with the original install error attached as cause. 2. The transient `.fork-staging-*` / `<name>.fork-backup-*` directories live inside the schemas dir, so a concurrent scan could surface them as real schemas. isSchemaDir (the single discovery chokepoint) now excludes them; real schema names are kebab-case (no dots) so this can never hide a schema. Adds regressions: an unrecoverable restore surfaces the backup path (and the rescued content is really there); fork temp dirs are excluded from listSchemas and listSchemasWithInfo. Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec Addressed both remaining issues (commit b6c2206, rebased onto the latest main merge). 1. Unrecoverable restore is no longer swallowed ( 2. Fork temp dirs are hidden from schema discovery ( New regressions in
Full suite: 3909 passed; only the 2 pre-existing unrelated failures remain ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/commands/schema-fork-fidelity.test.ts`:
- Around line 422-441: Update the test around the backup directory lookup to
assert that output contains the actual backupDir value after it is discovered.
Keep the existing generic backup-path assertions and recovery-content validation
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e1a6f1e-4cf0-4029-8dc2-031ad52837c3
📒 Files selected for processing (3)
src/commands/schema.tssrc/core/artifact-graph/resolver.tstest/commands/schema-fork-fidelity.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/schema.ts
| const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); | ||
| expect(process.exitCode).toBeTruthy(); | ||
| expect(output).toContain('"forked": false'); | ||
| // The destination loss is surfaced, not silent: the error names a | ||
| // `.fork-backup-` directory and how to restore it. | ||
| expect(output).toMatch(/\.fork-backup-/); | ||
| expect(output).toMatch(/preserved at/i); | ||
| expect(output).toMatch(/could not restore/i); | ||
|
|
||
| // The original content really is still on disk in the backup dir the error | ||
| // points at (recovery is genuinely possible). | ||
| const backupDir = fs | ||
| .readdirSync(path.join(tempDir, 'openspec', 'schemas')) | ||
| .find((entry) => entry.includes('.fork-backup-')); | ||
| expect(backupDir).toBeTruthy(); | ||
| const rescued = fs.readFileSync( | ||
| path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'), | ||
| 'utf-8' | ||
| ); | ||
| expect(rescued).toBe('name: keep-me\nversion: 3\n'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the actual backup directory in the error output.
Lines 427-429 accept any .fork-backup- text. The test can pass if the command reports a wrong backup location while preserving the real backup directory on disk. After backupDir is found, assert that output contains backupDir.
Proposed test assertion
expect(backupDir).toBeTruthy();
+ expect(output).toContain(backupDir!);
const rescued = fs.readFileSync(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); | |
| expect(process.exitCode).toBeTruthy(); | |
| expect(output).toContain('"forked": false'); | |
| // The destination loss is surfaced, not silent: the error names a | |
| // `.fork-backup-` directory and how to restore it. | |
| expect(output).toMatch(/\.fork-backup-/); | |
| expect(output).toMatch(/preserved at/i); | |
| expect(output).toMatch(/could not restore/i); | |
| // The original content really is still on disk in the backup dir the error | |
| // points at (recovery is genuinely possible). | |
| const backupDir = fs | |
| .readdirSync(path.join(tempDir, 'openspec', 'schemas')) | |
| .find((entry) => entry.includes('.fork-backup-')); | |
| expect(backupDir).toBeTruthy(); | |
| const rescued = fs.readFileSync( | |
| path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'), | |
| 'utf-8' | |
| ); | |
| expect(rescued).toBe('name: keep-me\nversion: 3\n'); | |
| const output = consoleLogSpy.mock.calls.map((c) => String(c[0])).join('\n'); | |
| expect(process.exitCode).toBeTruthy(); | |
| expect(output).toContain('"forked": false'); | |
| // The destination loss is surfaced, not silent: the error names a | |
| // `.fork-backup-` directory and how to restore it. | |
| expect(output).toMatch(/\.fork-backup-/); | |
| expect(output).toMatch(/preserved at/i); | |
| expect(output).toMatch(/could not restore/i); | |
| // The original content really is still on disk in the backup dir the error | |
| // points at (recovery is genuinely possible). | |
| const backupDir = fs | |
| .readdirSync(path.join(tempDir, 'openspec', 'schemas')) | |
| .find((entry) => entry.includes('.fork-backup-')); | |
| expect(backupDir).toBeTruthy(); | |
| expect(output).toContain(backupDir!); | |
| const rescued = fs.readFileSync( | |
| path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'), | |
| 'utf-8' | |
| ); | |
| expect(rescued).toBe('name: keep-me\nversion: 3\n'); |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 436-439: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(
path.join(tempDir, 'openspec', 'schemas', backupDir!, 'schema.yaml'),
'utf-8'
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/commands/schema-fork-fidelity.test.ts` around lines 422 - 441, Update
the test around the backup directory lookup to assert that output contains the
actual backupDir value after it is discovered. Keep the existing generic
backup-path assertions and recovery-content validation unchanged.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The restore reporting and temp-directory filtering are fixed, but the destructive swap still trusts destination state captured before staging. On exact head a8fd05a, I changed destination schema.yaml during copy; --force completed and deleted the newer content. Please fingerprint the authorized destination, revalidate before moving it aside and before deleting its backup, abort while preserving concurrent changes, and add a regression for this race.
…dits A concurrent process could edit an existing fork destination between the moment --force authorized the overwrite and the moment the destructive swap ran, and those edits were silently destroyed (reproduced by alfred: mutate destination schema.yaml during copy; --force completed and deleted the newer content). Now, when overwriting an existing destination, the fork: - fingerprints the authorized destination (SHA-256 over every file's relative path and bytes) BEFORE staging; - re-fingerprints and compares immediately before moving the destination aside; on mismatch it ABORTS without touching the destination, preserving the concurrent changes and telling the user to re-run; - re-fingerprints the backup before discarding it on the success path; if it changed during the install window it is kept, not deleted, and its location is surfaced. All prior guarantees remain: self-fork rejection, stage-then-swap, backup/restore on failed install with the backup path surfaced, and the temp-dir discovery filter. Adds regressions: a destination edited concurrently during staging aborts the fork and preserves the edit; a backup modified during the install window is kept and its location surfaced. Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec Fixed the concurrent-edit race you reproduced (commit c59d025). Root cause. The destructive swap trusted destination state captured before staging. A process editing the destination between the Fix (
All prior guarantees remain intact: self-fork rejection, stage-then-swap, backup/restore on failed install with the backup path surfaced, and the New regressions in
Full suite: 3963 passed; only the 2 pre-existing unrelated failures remain ( |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…-system-race)
fingerprintDir called fs.lstatSync then fs.readFileSync on the same path,
which CodeQL flags as a file-system race (the file may change between the
check and the read). Use the Dirent type already returned by readdirSync
({ withFileTypes: true }) instead of a separate lstat, and read files
directly, deriving the size from the bytes read. Behavior is unchanged
(13/13 fork-fidelity tests, incl. the concurrent-edit race regressions,
still pass); one fewer syscall per entry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed the CodeQL |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The destination fingerprinting fixes the reported race, but the staged copy still relies on source validation performed before copying. On ce9039e I changed source schema.yaml to a structurally invalid schema inside copyFileSync; --force succeeded, installed the invalid fork, and deleted the valid destination. Please validate the completed staged schema after the Document edit and before any destination displacement, with a regression preserving the destination when the source changes during staging.
…tive step
The up-front parseSchema only checks the SOURCE, but copyDirRecursive reads
source files that can change mid-copy, so the staged result can be invalid even
though the source was valid at the pre-check (reproduced by alfred: mutate source
schema.yaml to invalid inside copyFileSync; --force installed the invalid fork
and deleted the valid destination).
Now, after copying and the Document-API name edit, the fork validates the
COMPLETED staged schema.yaml (the exact bytes about to be installed) with
parseSchema BEFORE any destination displacement. On failure it aborts, cleans up
staging, and rethrows a clear error ("the staged fork of '<source>' is not a
valid schema ...; aborted, '<dest>' was not modified") chaining the parse error.
The up-front source parseSchema stays as a fail-fast; this is the authoritative
gate. Order before the swap: validate staged -> fingerprint-revalidate dest ->
rename dest->backup -> rename staging->dest -> revalidate+rm backup.
Adds a regression: a source that becomes structurally invalid during staging
aborts the fork and leaves the valid destination byte-identical, no leftovers.
Co-Authored-By: JinzeLin <linjinze999@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec Fixed the mid-copy source-invalidation race (commit 2d10a1c). Root cause. The up-front Fix (
with the parse error chained as New regression in Full suite: 3965 passed; only the 2 pre-existing unrelated failures remain ( |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The completed staging tree is now validated after the Document edit and before any destination move, so a source changed mid-copy cannot replace a valid destination. Exact-head build, lint, 63 focused recovery/discovery tests, and the full Linux/macOS/Windows/security matrix pass. Approved.
3 participants
Status: LGTM — verified merge-ready. Supersedes #1130 (by @linjinze999) with the maintainer-requested validation restored and regression coverage added.
What was missing / motivation
openspec schema forkrenamed the forkedschema.yamlby round-tripping throughparseSchema→stringifyYaml. That re-serialization dropped comments and could rewrite block-scalar style (a long literal|folded to>) and reorder keys, so a forked schema no longer matched its source. #1130 (thanks @linjinze999) fixed this by switching to yaml's Document API.What it does
parseDocument+doc.set('name', …)+doc.toString()), so block scalars, comments, and key order in the sourceschema.yamlsurvive the fork.parseSchemavalidation before the document mutation, so an invalid source (e.g. an artifact missing required fields) is still rejected rather than silently serialized. This addresses thealfred-openspecCHANGES_REQUESTED review on fix(schema): preserve YAML formatting when forking a schema #1130.Proof it works
test/commands/schema-fork-fidelity.test.ts(new) covers both halves alfred asked for:instruction: |block; asserts the fork keeps|(not flattened), preserves both comments, updatesname:, and is byte-identical to the source except the name line."forked": false,Invalid schema). Verified this test fails without the restoredparseSchemaline and passes with it.Full gate:
npm run build+npx vitest run→ 2791 passed; the only failures (config-profile,artifact-workflow,adapters.testmissing-dep) are pre-existing onmainand PR-independent (confirmed by stash/revert compare). Lint + typecheck clean.Notes / nits
Closes #N.A: |flattens toA: xxx yyy; on the pinned yaml 2.9.0 that specific case was already preserved — the real regressions fixed are comment loss and key-order/scalar-style drift. The fix is sound regardless.Co-authored with the original author.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests