Export safe replacement: manifest-owned prune + --force gate (#108)#110
Conversation
Decides and implements the safe-replacement scheme #104 flagged as blocking E4. The manifest is both index and ownership record: - Adopt a directory that holds a manifest WE wrote; prune every file that manifest listed but this run no longer produces, then clean the dirs that empties — the stale-slug hazard E3/E4's variable products/** and pages/<slug>/** sets introduce. Only ever deletes files a prior rosetta manifest named, so user-added files are untouched. - Write freely into a new/empty directory. - Refuse a non-empty directory with no rosetta manifest unless force (or, on a TTY, an interactive confirm) — never clobber a dir we didn't create. - Manifest last (crash can't leave a manifest naming half-written data); a crashed run needs --force once, then heals. manifest [[files]].name is a directory-relative path, so the manifest shape already describes directory-nested files — no manifest change needed. runExport gains an optional { force, confirmForeign } (default: refuse); the CLI parses --force and supplies a TTY prompt. DB-only boundary preserved: the one new file read is a read-back of our own prior manifest, not a data source — the boundary test now asserts readFileSync reads only manifestPath. Verified live on the schema-v10 corpus: stale nested file pruned + its dir cleaned, foreign file kept, foreign dir refused. Part of #95. Closes #108. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesSafe export replacement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as rosetta export CLI
participant Exporter as runExport
participant FS as export directory
participant Manifest as manifest.toml
User->>CLI: invoke export with optional --force
CLI->>Exporter: pass force and confirmForeign
Exporter->>Manifest: read prior ownership record
Exporter->>FS: inspect and validate output directory
Exporter->>FS: write dataset files
Exporter->>FS: prune stale owned files
Exporter->>Manifest: write updated manifest last
Exporter-->>CLI: return export summary
CLI-->>User: print written datasets and manifest
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
Implements the “safe replacement” export scheme (issue #108) so rosetta export can safely adopt prior exports, prune stale manifest-owned files, and refuse clobbering foreign non-empty directories unless explicitly forced/confirmed—unblocking future variable file-set exports.
Changes:
- Added
--forcehandling and TTY confirmation plumbing in the CLI for exporting into non-empty foreign directories. - Extended
runExportwithExportOptionsand manifest-based ownership/prune logic, plus empty-directory cleanup. - Added tests covering stale-file pruning (including nested paths), foreign-dir refusal/override, and new-directory exports; updated docs/changelog accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/mcp.ts | Parses --force and provides optional TTY confirmation callback when exporting. |
| src/export.ts | Adds manifest-ownership detection, stale-file pruning, and export options to gate foreign directories. |
| src/export.test.ts | Adds coverage for prune/adopt behavior and foreign-dir refusal/force paths; adjusts boundary assertions. |
| MANUAL.md | Documents --force and the safe-replacement semantics for export directories. |
| CHANGELOG.md | Adds an [Unreleased] entry describing the safe-replacement behavior change for rosetta export. |
|
Test is failing gate check: |
- CLI-help ↔ MANUAL parity test: --help now prints `export <dir> [--force]` to match the MANUAL CLI Flags row (the failing check). - Prune before writing the new manifest (Copilot): the manifest still lands last so it never names half-written data, but pruning first means a crash mid-publish leaves the OLD manifest on disk with the full owned set, so the next run re-prunes instead of orphaning stale files. - Validate each pruned path stays under the export root (Copilot): a tampered or hand-edited manifest with a `../` traversal name can no longer delete outside the directory. New test proves an outside file survives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/export.ts`:
- Around line 577-585: The export path handling around dataset writes and stale
cleanup must reject symlink components, not just lexically contained paths. Add
component-by-component lstatSync validation for resolved targets, rejecting any
existing symlink before Bun.write, rmSync, or removeEmptyAncestors executes;
apply this to both generated dataset paths and stale manifest paths, and add a
regression test covering a symlinked directory escaping the export root.
- Around line 507-509: Replace the manifest-last export flow in the export
implementation with a staged-generation approach: write all datasets and the
manifest to an isolated staging location, then atomically commit the generation
as a unit. Add recovery handling for interrupted commits so startup/export
resumption never exposes or accepts a mixed-generation manifest and data set.
- Around line 569-585: Reorder the publish operations so stale entries in
ownedBefore are pruned before the dataset writes in the loop over datasets. Keep
the produced-name exclusion, manifest protection, path-containment defense, and
removeEmptyAncestors behavior unchanged; write the manifest last as currently
intended.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 4c0f022e-8cfa-46eb-ba4e-94a1aac6b132
📒 Files selected for processing (5)
CHANGELOG.mdMANUAL.mdsrc/export.test.tssrc/export.tssrc/mcp.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Bun and TypeScript, including Bun-native runtime APIs such as
bun:sqlite,Bun.serve, andbunx.Rosetta uses Bun and TypeScript; prefer
bunandbun testrather than Node/npm-oriented substitutes.
Files:
src/mcp.tssrc/export.test.tssrc/export.ts
src/{query,mcp,browse}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Route MCP, query, classifier, TUI, and canonicalizer behavior through shared core code, usually
src/query.ts; keepsrc/mcp.tsandsrc/browse.tsthin adapters.
Files:
src/mcp.ts
*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep this Copilot instructions file short; put substantive rules in narrow instruction files under
.github/instructions/*.instructions.md.
Files:
CHANGELOG.mdMANUAL.md
CHANGELOG.md
📄 CodeRabbit inference engine (CLAUDE.md)
Record user-visible shipped changes in
CHANGELOG.mdunder[Unreleased]or release sections.
Files:
CHANGELOG.md
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer each document's canonical home and do not duplicate operational detail, schema blocks, or long rule lists in the routing index.
Files:
CHANGELOG.mdMANUAL.md
{README.md,src/setup.ts,MANUAL.md}
📄 CodeRabbit inference engine (AGENTS.md)
Keep public client setup snippets aligned across
README.md,src/setup.ts, andMANUAL.md, including the Codex commandcodex mcp add rosetta -- bunx@tikoci/rosetta``.
Files:
MANUAL.md
MANUAL.md
📄 CodeRabbit inference engine (CLAUDE.md)
Put installation, operations, release/re-extraction procedures, and schema reference in
MANUAL.md.
Files:
MANUAL.md
🪛 ast-grep (0.44.1)
src/export.ts
[warning] 437-437: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^format = "${EXPORT_FORMAT}"$, "m")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🔇 Additional comments (10)
src/mcp.ts (2)
223-248: LGTM!
184-184: LGTM!MANUAL.md (2)
30-30: LGTM!
314-316: LGTM!CHANGELOG.md (1)
48-48: LGTM!src/export.ts (2)
21-21: LGTM!Also applies to: 407-460
511-530: LGTM!src/export.test.ts (3)
340-388: LGTM!
405-410: LGTM!
419-429: LGTM!
…eRabbit) - Prune before writing datasets (CodeRabbit): a name reused across runs now transitions cleanly between a flat file and a directory (old `pages` file is removed before a new `pages/<slug>/x.tsv` is written, and the inverse). Still manifest-last, so the crash-safety from the prior fix is unchanged. - Contain writes AND prunes to the export root through symlinks, not just lexically (CodeRabbit): `containedTarget` compares the realpath of the deepest existing ancestor to the realpath of the root, so a symlinked component (root/pages -> /outside) can't make a write or delete act outside the dir. A prune escape is skipped; a write escape throws. New regression test. - Docs: state the ordering + symlink containment, and note honestly that the directory is a regenerable audit surface, not a transactional store — a crash can leave a mix of generations until the next run (full staged-atomic-commit was deferred by design in #108). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Implements the safe-replacement scheme #104 flagged as blocking E4, using the strawman ratified on #108: manifest-owned prune-then-publish, with the manifest as the ownership record. Closes #108; part of #95 / B-0022.
Behavior
manifest.tomlis now both the index and the ownership record ([[files]].nameis a directory-relative path, so it already describes directory-nested files — no manifest schema change).products/**andpages/<slug>/**sets need. It only ever deletes files a prior rosetta manifest named — files you added yourself are untouched.--forceoverrides; on a TTY the CLI prompts instead.--forceonce to resume; the next run heals it.runExportgains an optional{ force, confirmForeign }(default: refuse foreign dirs); the CLI parses--forceand supplies a TTY confirm.confirmForeignis a callback soexport.tsstays free of stdin/prompt coupling and fully testable.DB-only boundary
Preserved. The one new file read is a read-back of our own prior manifest — not a data-source read, which is what the boundary forbids. The primitive-scan test is narrowed accordingly: it still bans
fetch/new Database/child_process/Bun.file/require/asyncreadFile, and now asserts the onlyreadFileSyncreadsmanifestPath. The behavioral "importing export.ts opens no database" proof is unchanged.Verification
bun test880 pass / 0 fail; typecheck, biome, markdownlint, cspell clean. New tests cover stale-file pruning (flat + directory-nested, incl. emptied-dir cleanup), the foreign-dir refusal (default + declining confirm +--forceoverride + subsequent adopt), and a brand-new directory. Confirmed live on the schema-v10 corpus: a hand-injected staleproducts/ghost/specs.tsvis pruned and itsproducts/dir removed, a foreignREADME.mdis kept, and a foreign non-empty dir is refused.Notes
Export is
[Unreleased]. This unblocks the #104 chunks: E4 (per-fragment doc tables) and E3 (products) can now build on this scheme.MANUAL.mddocuments the replacement semantics and the--forceflag;CHANGELOG.mdupdated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
rosetta export <dir>replacement behavior using manifest tracking.--forcesupport and interactive confirmation for non-empty directories without a Rosetta manifest.Documentation