Add README scaffolding to create wizard: default-on toggle, seeded template, authored-content preservation, and atomic write transaction - #457
Conversation
|
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
✨ 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 |
![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: d301e3786b
ℹ️ 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".
| if (mutation.kind === 'write') { | ||
| mkdirSync(dirname(mutation.path), { recursive: true }) | ||
| // tmp + rename so a reader never observes a half-written file. | ||
| const tmp = `${mutation.path}.tmp` |
There was a problem hiding this comment.
Use a unique transaction-owned temporary path (and clean it up) instead of ${mutation.path}.tmp. When facet create --force targets a project that already has, for example, README.md.tmp or facet.json.tmp, this write overwrites that unrelated authored file and the subsequent rename removes it; the transaction never captures or restores temporary-path preimages, so the promised rollback cannot recover the data.
Useful? React with 👍 / 👎.
| if (!result.ok) { | ||
| throw new Error( | ||
| `scaffold write failed at ${result.failedPath}: ${result.reason}` + | ||
| (result.rollback.ok ? '' : ` (rollback incomplete: ${result.rollback.failedPaths.join(', ')})`), | ||
| ) |
There was a problem hiding this comment.
Expose the transaction failure as a discriminated writeScaffold result instead of throwing it. Filesystem failures such as an unwritable target or a conflicting path are expected create outcomes that the command can render and exit from normally; this throw instead bypasses both the headless and wizard command paths and reaches the process-level catch as an “unexpected” exit (code 2).
AGENTS.md reference: AGENTS.md:L262-L266
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const applied: FsMutation[] = [] | ||
| for (const mutation of mutations) { | ||
| try { | ||
| applyMutation(mutation) | ||
| applied.push(mutation) |
There was a problem hiding this comment.
applied tracking array
applied is pushed to on every successful mutation but is never read. rollbackApplied always receives preimages (the full pre-transaction preimage set), not applied, so the tracking does nothing. Restoring every preimage when only a subset was applied is still correct — restoring a preimage for an untouched path is a no-op (rmSync with { force: true } on a path that was never written). The array and its push call can be removed entirely to avoid misleading future readers into thinking it drives rollback selection.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
d301e37 to
b3293d1
Compare
e62f6c5 to
fc27156
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3293d10ed
ℹ️ 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".
| }, | ||
| ] | ||
| if (opts.readme.kind === 'enabled') { | ||
| mutations.push({ kind: 'write', path: join(targetDir, README_MD), bytes: encoder.encode(opts.readme.content) }) |
There was a problem hiding this comment.
When the target directory has no facet.json but does have a pre-existing README.md (for example, creating a facet in an existing repository root), overwrite protection permits the operation because it checks only facet.json in create/index.ts:85. This new default mutation then silently replaces the authored README even though the user did not pass --force; guard planned output paths or require explicit overwrite consent before writing it.
Useful? React with 👍 / 👎.
|
|
||
| function applyMutation(mutation: FsMutation): void { | ||
| if (mutation.kind === 'write') { | ||
| mkdirSync(dirname(mutation.path), { recursive: true }) |
There was a problem hiding this comment.
If a later mutation fails after this call has created parent directories—for example, a pre-existing commands file makes the command write fail after a skill was written—the rollback removes the planned files but never removes newly created skills/<name> directories. The target is therefore not restored to its pre-transaction state despite the transaction contract; record which directories were newly created and prune them after restoring preimages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b3293d1. Configure here.
Merge activity
|
…mplate, authored-content preservation, and atomic write with rollback
b3293d1 to
3f65211
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f652119c7
ℹ️ 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".
| skill: { type: 'array', description: 'Skill to scaffold, repeatable (headless mode)' }, | ||
| agent: { type: 'array', description: 'Agent to scaffold, repeatable (headless mode)' }, | ||
| command: { type: 'array', description: 'Command to scaffold, repeatable (headless mode)' }, | ||
| readme: { type: 'boolean', description: 'Scaffold a README.md (default on; pass --no-readme to skip)' }, |
There was a problem hiding this comment.
When users run facet create --no-readme without another headless content flag, decideCreate deliberately selects the wizard, but the flag is never passed into its initial form state. The wizard therefore starts with README enabled and will create and declare README.md unless the user manually disables it, despite the advertised --no-readme opt-out. Apply this flag to the wizard state (or make it select an appropriate noninteractive path) as well as the headless options.
Useful? React with 👍 / 👎.
| }, [steps, ctx]) | ||
| report(ctx.toCreateOptions().readme) | ||
| return null |
There was a problem hiding this comment.
For each test whose steps callback changes form state, including all of the new mutation tests, ctx gets a new identity after the update, so this effect runs again and invokes the setters again. Because each setter returns a new form object even when the values are unchanged, this produces an unbounded render/effect loop rather than a settled result. Depend only on the stable steps callback (or otherwise guard the setup effect) so the probe performs its sequence once.
Useful? React with 👍 / 👎.
1 participant
Why
Tasks 12 and 13.1–13.2 are complete. This adds first-class README scaffolding to
facet create: the wizard and headless paths both produce aREADME.mdby default, seeded from the facet name and description, with an explicit opt-out via--no-readme.Details
README state model (
ReadmeState/ScaffoldReadme)The form tracks a
draftwith anoriginfield (seeded|authored). Whileseeded, identity edits (name, description) automatically regenerate the template. The first explicit edit — either through the in-wizard editor button or the external editor round-trip — flips origin toauthored, after which the content is frozen regardless of later identity changes. Toggling README off and back on never discards the draft, so authored content survives a disable/re-enable cycle.Manifest declaration
An enabled README is declared as an ordinary
files: ["README.md"]top-level entry — no README-specific manifest field. The declaration is written last, after asset sections. A disabled README produces nofileskey.Atomic scaffold writes (
fs-transaction.ts)writeScaffoldpreviously wrote files sequentially with no rollback. It now builds a list ofFsMutationvalues and applies them throughapplyFsTransaction, which captures a preimage of every affected path before touching disk, applies each mutation via tmp-then-rename, and restores all preimages in reverse order on any failure. The transaction result is discriminated data for callers that can recover (the future edit apply path);writeScaffolditself throws on failure since create has no fallback.Wizard editor round-trips
onRequestEditorpreviously took(section, name, description)positional arguments. It now receives a taggedEditorRequestunion (asset-description|readme) so the two editor flows cannot be confused and each arm carries exactly the fields it needs.mergeEditorResultinwizard.tsxdispatches on the tag: asset descriptions are trimmed (single-line semantics); README content is stored verbatim and markedauthored.Headless parity
decideCreateseeds the samereadmeTemplate(name, description)the wizard uses, so headless and interactive create never diverge by default.--no-readme(parsed asreadme: false) produces{ kind: 'disabled' }.Edit isolation
manifestToFormStatesets an inertreadme: { enabled: false, draft: { origin: 'seeded', content: '' } }to satisfy the sharedFormStateshape. The edit flow routes README through its own dedicated panel and is unaffected by this field.Verification
New and updated unit tests cover: default-enabled README in headless mode,
--no-readmeopt-out, authored-content freeze across identity edits, disable/re-enable draft preservation, manifest declaration presence and ordering,previewScaffoldFilesordering,writeScaffoldverbatim content, disabled-README omission, and transactional rollback when a write fails mid-batch. CI covers the rest.Note
Low Risk
Create/scaffold path only; behavior is additive with explicit opt-out and transactional writes reduce partial-failure risk. Edit authoring for supplementary files remains future work.
Overview
Adds default-on
README.mdscaffolding tofacet create, aligned with the non-asset-files work (tasks 13.1–13.2).Engine:
ScaffoldOptionsnow requires a taggedreadme(enabledwith verbatim content, ordisabled). Enabled READMEs are written asREADME.mdand declared via ordinary top-levelfiles: ["README.md"](no README-specific manifest field). Shared helpers live inreadme.ts(readmeTemplate, path constants).writeScaffoldapplies manifest + README + assets through newapplyFsTransaction(preimage capture, tmp+rename writes, rollback on failure).CLI: Interactive create gets README toggle, Edit README (external editor), and form state with seeded vs authored drafts—identity edits re-seed only while seeded; authored bytes stay frozen. Headless create seeds the same template by default;
--no-readmeopts out without forcing headless mode. Editor round-trips use a taggedEditorRequestunion (asset-description|readme). Existing e2e scaffolds passreadme: { kind: 'disabled' }where README is irrelevant.Tests: Engine scaffold/manifest/preview/rollback coverage; headless README defaults; TUI README state tests.
Reviewed by Cursor Bugbot for commit 3f65211. Bugbot is set up for automated code reviews on this repo. Configure here.