Extend edit reconciliation to structured item identities, supplementary file scanning, transactional apply, and exact-path confirmation preview - #458
Conversation
|
|
Warning Review limit reached
Next review available in: 56 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 (21)
✨ 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
facets/packages/cli/src/tui/views/edit/wizard.tsx
Lines 97 to 101 in 28c186f
Do not show successful apply until the transaction succeeds
When applyEditOperations returns { ok: false } (for example, a later file write fails due to permissions or a path conflict), onApply records the error but resolves normally. This callback then unconditionally marks the edit complete and moves to the success view, so the user sees “Changes applied” before the command reports the failure after unmounting. Have the callback return an outcome and only call onComplete/enter done after a successful apply.
AGENTS.md reference: AGENTS.md:L172-L178
ℹ️ 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".
| const result = applyFsTransaction(mutations) | ||
| if (result.ok) return { ok: true } | ||
| return { ok: false, failedPath: result.failedPath, reason: result.reason, rollbackOk: result.rollback.ok } |
There was a problem hiding this comment.
A declared companion or supplementary path can be a directory on disk: the scanners treat it as missing because they scan only files, so choosing Scaffold queues a write at that path. applyFsTransaction captures preimages before its mutation try and readFileSync on that directory throws, causing this result-shaped API to reject instead of returning { ok: false }; the edit command only handles false results. Convert preimage-capture failures into the transaction result as well.
AGENTS.md reference: AGENTS.md:L172-L178
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| * `files` array (repo-relative paths); `skill` is a skill descriptor's `files` | ||
| * array (paths relative to that skill's directory). | ||
| */ | ||
| export type DeclarationSite = { kind: 'root' } | { kind: 'skill'; skill: string } |
There was a problem hiding this comment.
Unused exported declaration type
Low Severity
The DeclarationSite type in packages/engine/src/edit/types.ts is exported but unused. Its role for tagging declaration sites is already handled by ReconciliationItem variants, making it redundant API surface.
Reviewed by Cursor Bugbot for commit 28c186f. Configure here.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (phase === 'confirmation') { | ||
| const pending = buildResult(form) | ||
| const operations = pending.outcome === 'applied' ? pending.operations : [] | ||
| return ( |
There was a problem hiding this comment.
buildResult(form) is called as a plain expression in the render body of the confirmation branch. Every focus change (arrow key between the two buttons) triggers a re-render that recomputes buildManifest, applySupplementaryDeltas, and buildOperations in full. useMemo would pin the result to the form/resolutions values that actually drive it, which are stable during the confirmation phase.
| if (phase === 'confirmation') { | |
| const pending = buildResult(form) | |
| const operations = pending.outcome === 'applied' ? pending.operations : [] | |
| return ( | |
| const confirmationOperations = useMemo(() => { | |
| if (phase !== 'confirmation') return [] | |
| const pending = buildResult(form) | |
| return pending.outcome === 'applied' ? pending.operations : [] | |
| }, [phase, buildResult, form]) | |
| if (phase === 'confirmation') { | |
| const operations = confirmationOperations | |
| return ( |
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
28c186f to
a27000a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a27000a338
ℹ️ 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 (!applied.ok) { | ||
| applyError = `Failed to apply changes at ${applied.failedPath}: ${applied.reason}${ | ||
| applied.rollbackOk ? ' (rolled back)' : ' (rollback incomplete)' | ||
| }` | ||
| } |
There was a problem hiding this comment.
When a filesystem mutation fails (for example, a scaffold target has a file as its parent), this callback records applyError but still fulfills its Promise<void>. EditWizard.handleConfirm consequently marks the run complete and renders “Changes applied.” before the command prints the failure and exits 1. Return the apply result (or throw/handle a failure at the wizard boundary) so the success view and completion state are reached only after a successful transaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a27000a. Configure here.
| /** The two option labels for an item, in index order. */ | ||
| export function optionLabelsFor(item: ReconciliationItem): [string, string] { | ||
| return isAdditionItem(item) ? ['Add to manifest', 'Ignore for now'] : ['Scaffold template', 'Remove from manifest'] | ||
| } |
There was a problem hiding this comment.
Misleading supplementary scaffold labels
Medium Severity
optionLabelsFor labels every missing item as Scaffold template, including companion-missing and root-missing. Those resolutions write empty bytes at the declared path, not an asset template, so the reconciliation UI misstates what Apply will do.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a27000a. Configure here.
Merge activity
|
…ry file scanning, transactional apply, and exact-path confirmation preview
a27000a to
5eaf99e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5eaf99e920
ℹ️ 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".
| try { | ||
| rmSync(preimage.path, { force: true }) | ||
| } catch { | ||
| // Already absent for all practical purposes. | ||
| } |
There was a problem hiding this comment.
Do not swallow every rmSync error here. If a transaction has already created a previously absent file and a later mutation fails, a cleanup failure such as EACCES (for example, after concurrent permission changes) leaves that new file behind, yet rollbackApplied reports rollback.ok: true and the CLI tells the user the changes were rolled back. Only suppress errors that prove the path is already absent (such as ENOENT/the intended non-directory case), and let other errors reach rollbackApplied.
AGENTS.md reference: AGENTS.md:L172-L178
Useful? React with 👍 / 👎.
1 participant
Why
The edit reconciliation system only understood primary asset files (skills, agents, commands). Skill companion files declared under a skill's
filesarray, common root-level files likeLICENSEandCHANGELOG.md, and the two conventional README paths (README.mdandREADME) were invisible to the scanner and reconciler, meaning undeclared files went unnoticed and declared-but-missing files were silently ignored.Details
Structured reconciliation item types.
ReconciliationItemis now a tagged union with six variants (asset-addition,asset-missing,companion-addition,companion-missing,root-addition,root-missing) instead of two string-keyed variants. Each variant carries its own typed fields so no consumer needs to parse a colon-joined key back into structured data.reconciliationItemKeyproduces a stable one-way key for React lists and focus IDs only.Resolution actions are renamed and narrowed.
add-to-manifest/scaffold-template/remove-from-manifestbecomeadd/scaffold/remove/ignore.resolutionForOptionis the single place that maps an option index to a resolution, making illegal item/action pairings unrepresentable.Supplementary scanning and reconciliation.
scanSkillCompanionswalks a skill's directory and returns companion paths relative to the skill dir.scanCommonRootFileschecks a curated list of conventional root files (LICENSE,CHANGELOG.md,CONTRIBUTING.md, etc.).reconcileSupplementarycompares these disk facts against manifest declarations and emits the appropriate addition/missing items. README paths are explicitly excluded from this path and routed to a new dedicated README panel.README panel state.
computeReadmeStatesclassifies each of the two conventional README paths into one of four states (present-declared,present-undeclared,declared-missing,absent-undeclared) independently.EditContextnow carries areadmefield alongsidereconciliationItems.Pure manifest mutation helpers.
addTopLevelFile,removeTopLevelFile,addSkillCompanion, andremoveSkillCompanionare pure functions that return new manifests with sorted, de-duplicatedfilesarrays and drop empty arrays rather than leavingfiles: [].Transactional apply.
applyEditOperationsno longer takes a separate manifest argument — the final manifest travels inside thewrite-manifestoperation so the entire operation list drives oneapplyFsTransactioncall.delete-assetcarries explicitcompanionPathsso the transaction deletes only declared companions and leaves undeclared files untouched.write-fileanddelete-fileoperations handle supplementary file writes and removals. Rollback now swallows errors when restoring a pre-image that is already absent (e.g. when a parent path is not a directory).Operation preview.
previewEditOperationsexpands the queued operation list into exact repo-relative path lines for the confirmation and success views, including each deleted companion path individually.EditConfirmViewnow receives the pending operations and renders the file-change preview before the user confirms, satisfying the "show every queued exact-path operation before Apply" requirement.useEditSessionresolution map now storesResolvedItem(structured item + resolution) instead of a bare resolution, sobuildOperationscan act on the item's typed fields without re-parsing a string key. Resolutions are seeded from the wizard snapshot so they survive external-editor round-trips that unmount and remount the wizard.Verification
New tests in
edit-supplementary.test.tscover the scanner, supplementary reconciler, README state classifier, declaration mutation helpers, transactional apply (including rollback), operation preview, and the fullbuildEditContextintegration path. Existing integration tests are updated to use the renamed operation and item kinds.Note
Medium Risk
Changes the edit apply contract and filesystem transaction behavior for manifest + multi-file writes; mistakes could corrupt facet projects, though rollback and tests mitigate this.
Overview
facet editnow reconciles skill companions, common root files (e.g.LICENSE), and declared-but-missing supplementary paths—not only primary skills/agents/commands. Reconciliation items are a six-variant tagged union with stable lookup keys (not parsed back from strings); resolutions are centralized asadd/ignore/scaffold/remove.README.mdandREADMEare excluded from generic reconciliation;EditContext.readmecarries independent per-path states fromcomputeReadmeStates(dedicated README panel UI is still outstanding per tasks).Apply path:
applyEditOperations(operations, rootDir)runs manifest + file ops throughapplyFsTransaction; the manifest lives onwrite-manifest. Ops includescaffold-asset,delete-asset(with explicitcompanionPathsso undeclared skill files survive),write-file, anddelete-file.previewEditOperationsdrives confirm/success exact-path listings; the CLI surfaces apply failures and rollback status.The edit session applies companion/root declaration deltas via pure manifest helpers, avoids scaffolding over on-disk asset additions, and keeps structured
ResolvedItemresolutions across wizard snapshots.Reviewed by Cursor Bugbot for commit 5eaf99e. Bugbot is set up for automated code reviews on this repo. Configure here.