feat(setup): add safe resumable connector drafts - #272
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughChangesThe PR adds a private, resumable connector-setup checkpoint shared by Setup draft persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI_or_Console
participant SetupDraftStore
participant ConfigurationPublisher
Operator->>CLI_or_Console: Start or resume connector setup
CLI_or_Console->>SetupDraftStore: Save or load connector metadata
SetupDraftStore-->>CLI_or_Console: Validated draft
CLI_or_Console->>ConfigurationPublisher: Collect required inputs and publish configuration
ConfigurationPublisher-->>CLI_or_Console: Publication result
CLI_or_Console->>SetupDraftStore: Discard draft after successful publication
Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 10
🤖 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 `@docs/cli.md`:
- Around line 81-82: The resume-option wording is ambiguous and must explicitly
prohibit combining --resume with any setup-input option. Update docs/cli.md
lines 81-82 to state that restriction, and update
tests/readme-product-contract.test.ts lines 121-122 to assert the same clarified
wording.
In `@docs/console-api.md`:
- Line 40: Document the setup-draft endpoint payloads separately in
docs/console-api.md: specify GET’s response, PUT’s request/response including
optional expectedRevision, and DELETE’s required { revision } request body and
response. In tests/readme-product-contract.test.ts, add assertions covering
these distinct GET, PUT, and DELETE request/response contracts rather than only
the shared field summary.
In `@README.md`:
- Line 119: Update README.md at lines 119-119 to document that the private
checkpoint persists the connector source, configuration name, preset, and setup
stage, while preserving the existing exclusions and lifecycle wording. Update
tests/readme-product-contract.test.ts at lines 91-95 to assert the corrected
README wording and explicitly require setup-stage persistence.
In `@src/cli/init.ts`:
- Line 430: Update the cancellation catch surrounding the setup flow in the init
handler containing onSetupDraftIntent to rethrow MiftahError unchanged alongside
CliUsageError. Ensure SETUP_DRAFT_CONFLICT and SETUP_DRAFT_UNAVAILABLE errors
from save() propagate to the caller and retain their mapped exit codes, while
other errors continue using the existing “Interactive init was cancelled.”
conversion; import MiftahError if needed.
In `@src/cli/setup.ts`:
- Around line 420-426: In src/cli/setup.ts lines 420-426, update the
post-runInitCommand store.discard(draft.revision) cleanup to catch MiftahError,
report it as a warning, and always continue to finishCreatedSetup. Apply the
same handling in src/cli/setup.ts lines 652-654 for
context.setupDraftStore!.discard(checkpointedDraft.revision), first capturing
the setup draft store in a local const to remove the non-null assertion.
In `@src/console/console-assets.ts`:
- Around line 1635-1674: Update the click handlers for saveSetupDraft,
resumeSetupDraft, and discardSetupDraft to disable their respective buttons
before starting each async api request and re-enable them in a finally block
after completion. Preserve the existing success and error behavior while
preventing concurrent requests from rapid repeated clicks.
In `@src/console/console-dashboard-application-service.ts`:
- Around line 385-390: Make clearSetupDraftAfterFirstRunPublication best-effort
by handling discard conflicts or other cleanup failures without propagating them
to the successful onboarding flows. Preserve the existing load-and-discard
behavior when no concurrent modification occurs, and ensure onboardNativeOAuth,
onboardDiscoveredNativeOAuth, onboardPreset, and onboardClientEntry continue to
reach confirmCreatedFirstRunConfiguration even when draft cleanup fails.
In `@src/setup/setup-draft.ts`:
- Around line 340-344: Update the catch block in the setup-draft operation to
remove the redundant OAuthLocalLockUnavailableError branch and preserve the
original non-MiftahError as the cause when calling setupDraftUnavailable.
Confirm and use the existing MiftahErrorDetails shape for the details argument,
while continuing to rethrow MiftahError unchanged.
- Around line 299-320: Update save() in the setup draft store so an omitted
expectedRevision is treated as an unconditional replace: only perform revision
conflict validation when expectedRevision is defined, while preserving
validation for explicitly supplied revisions and existing revision increment
behavior. Add coverage for saving without expectedRevision over an existing
draft if the surrounding tests are available.
In `@tests/setup-command.test.ts`:
- Around line 229-242: Extend the setup command tests near the existing
resume/output rejection test to cover both remaining guards in src/cli/setup.ts:
assert that --resume combined with --discard-draft rejects, and that --resume
without a TTY rejects. Reuse the existing stream and command setup patterns,
ensuring each assertion exercises its intended branch and cleans up the input
stream.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88102f70-34dd-4006-bf15-1243187a3be3
📒 Files selected for processing (22)
CHANGELOG.mdREADME.mddocs/cli.mddocs/console-api.mdsrc/cli/exit-codes.tssrc/cli/init.tssrc/cli/main.tssrc/cli/parse.tssrc/cli/setup.tssrc/console/console-application-service.tssrc/console/console-assets.tssrc/console/console-dashboard-application-service.tssrc/console/console-server.tssrc/setup/setup-draft.tssrc/utils/errors.tstests/cli-exit-codes.test.tstests/cli-parse.test.tstests/console-dashboard-application-service.test.tstests/console-server.test.tstests/readme-product-contract.test.tstests/setup-command.test.tstests/setup-draft.test.ts
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 `@docs/cli.md`:
- Line 81: Update the `miftah setup --resume` documentation in `docs/cli.md` to
state that resume restores the configuration name, catalog preset, connector
source, and setup stage from the private checkpoint. Keep the existing
description that all remaining connection and OAuth details are collected again,
and align the wording with the shared checkpoint behavior documented elsewhere.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a2502ed-3303-4fce-b4de-817881d4b22a
📒 Files selected for processing (15)
README.mddocs/cli.mddocs/console-api.mdsrc/cli/init.tssrc/cli/setup.tssrc/console/console-assets.tssrc/console/console-dashboard-application-service.tssrc/setup/setup-draft.tstests/console-dashboard-application-service.test.tstests/console-server.test.tstests/console-windows-first-run.test.tstests/init-command.test.tstests/readme-product-contract.test.tstests/setup-command.test.tstests/setup-draft.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/setup/setup-draft.ts (2)
341-345: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
discard()swallows the underlyingrm()failure cause.Same class of issue previously fixed in
withLock()(nowsetupDraftUnavailable(error)): here therm()catch block still callssetupDraftUnavailable()with no argument, discarding the real filesystem error (EACCES/EBUSY/etc.).♻️ Preserve the cause
try { await rm(this.path, { force: true }); - } catch { - setupDraftUnavailable(); + } catch (error) { + setupDraftUnavailable(error); }🤖 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 `@src/setup/setup-draft.ts` around lines 341 - 345, Update the catch block in discard() to capture the rm() failure and pass that error to setupDraftUnavailable(error), preserving the underlying filesystem failure cause consistently with withLock().
420-453: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCause-preservation fix from
withLock()wasn't applied todiscard(),loadUnchecked(), orpersist(). All three still callsetupDraftUnavailable()with no argument in their catch paths, discarding the real underlying error (filesystem errors, ACL failures, decode failures) that the already-fixedwithLock()/ensurePrivateDirectory()pattern preserves.
src/setup/setup-draft.ts#L420-L453: in thepersist()catch block (around L449-L450), pass the caughterrorintosetupDraftUnavailable(error)instead of calling it bare.src/setup/setup-draft.ts#L335-L347: indiscard()'srm()catch (L341-L345), capture the error and callsetupDraftUnavailable(error).src/setup/setup-draft.ts#L359-L418: inloadUnchecked()(L410), pass the already-capturedfailureintosetupDraftUnavailable(failure).🤖 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 `@src/setup/setup-draft.ts` around lines 420 - 453, Preserve the underlying errors when converting setup-draft failures: in src/setup/setup-draft.ts lines 420-453, update persist() to pass its caught error to setupDraftUnavailable(error); in lines 335-347, update discard()’s rm() catch to capture and pass its error; and in lines 359-418, update loadUnchecked() to pass the existing failure value to setupDraftUnavailable(failure).
🤖 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/console/console-application-service.ts`:
- Around line 429-436: The Windows configuration-directory setup must also
verify directory identity after successful creation. Update the flow around
createWindowsPrivateDirectory and verifyWindowsConfigPathSecurity to perform the
lightweight Node-side post-create directory recheck used by setup-draft.ts
before publishSetupConfigurationPlan(), while preserving the existing verifier
for the failed-create path.
---
Outside diff comments:
In `@src/setup/setup-draft.ts`:
- Around line 341-345: Update the catch block in discard() to capture the rm()
failure and pass that error to setupDraftUnavailable(error), preserving the
underlying filesystem failure cause consistently with withLock().
- Around line 420-453: Preserve the underlying errors when converting
setup-draft failures: in src/setup/setup-draft.ts lines 420-453, update
persist() to pass its caught error to setupDraftUnavailable(error); in lines
335-347, update discard()’s rm() catch to capture and pass its error; and in
lines 359-418, update loadUnchecked() to pass the existing failure value to
setupDraftUnavailable(failure).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e2b0270e-ed18-474b-a0fc-fb07cb5b463c
📒 Files selected for processing (7)
src/cli/windows-config-acl.tssrc/console/console-application-service.tssrc/console/console-dashboard-application-service.tssrc/setup/setup-draft.tstests/console-dashboard-application-service.test.tstests/console-windows-first-run.test.tstests/windows-config-acl.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/console/console-application-service.ts (1)
417-443: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPost-create directory recheck dropped again on the Windows fast path.
When
created === true,ensureFirstRunConfigDirectoryreturns immediately with no re-verification. The siblingensurePrivateDirectoryinsrc/setup/setup-draft.tsdeliberately keeps a lightweight recheck after a successful trusted create:if (await privateDirectoryMetadata(directory, false) === undefined) setupDraftUnavailable();with the comment "Retain the Node metadata check without launching it again." A prior review on this exact function required the same defense (fixed in commit 4402bdf, for the old
createWindowsPrivateDirectory/verifyWindowsConfigPathSecuritypair); this refactor tocreateWindowsPrivateDirectoryInPrivateParent/verifyWindowsConfigPathsSecurityappears to have dropped it again, leaving a window between the trusted helper's internal verification and Node's first write where a same-user directory swap could go unnoticed.tests/console-windows-first-run.test.ts's ACL-mock call-count assertions can't detect this gap since a locallstat-based recheck wouldn't touch those mocks.🛡️ Mirror the setup-draft.ts recheck
const created = await createWindowsPrivateDirectoryInPrivateParent(parent, directory); // The trusted creator applies and verifies the private owner/DACL/reparse // boundary, including its parent, before it reports success. A failed // exclusive create may be a safe concurrent Miftah creation, so both path // components need an independent verifier before that race is accepted. - if (!created && !(await verifyWindowsConfigPathsSecurity([ - { path: parent, kind: "directory" }, - { path: directory, kind: "directory" } - ]))) { - throw new Error("unsafe configuration directory or parent"); - } + if (created) { + // Retain a lightweight local identity check after a successful trusted + // create, mirroring src/setup/setup-draft.ts's ensurePrivateDirectory. + if ((await lstat(directory).catch(() => undefined))?.isDirectory() !== true) { + throw new Error("unsafe configuration directory or parent"); + } + } else if (!(await verifyWindowsConfigPathsSecurity([ + { path: parent, kind: "directory" }, + { path: directory, kind: "directory" } + ]))) { + throw new Error("unsafe configuration directory or parent"); + }Exact recheck primitive (ownership/symlink check, not just
isDirectory) should mirror whateverprivateDirectoryMetadatadoes in setup-draft.ts; a plainlstathere is illustrative only.Please confirm no equivalent recheck exists earlier in the file (outside the shown range) before applying this fix.
🤖 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 `@src/console/console-application-service.ts` around lines 417 - 443, Confirm there is no existing post-create security recheck elsewhere in the file, then update ensureFirstRunConfigDirectory so the Windows path performs the same lightweight ownership/symlink metadata verification as setup-draft.ts after createWindowsPrivateDirectoryInPrivateParent reports created === true. Reuse the established privateDirectoryMetadata-style primitive, preserve the existing verifier for the created === false race path, and convert any failed recheck into the current CONFIG_CREATE_FAILED error flow.
🤖 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.
Outside diff comments:
In `@src/console/console-application-service.ts`:
- Around line 417-443: Confirm there is no existing post-create security recheck
elsewhere in the file, then update ensureFirstRunConfigDirectory so the Windows
path performs the same lightweight ownership/symlink metadata verification as
setup-draft.ts after createWindowsPrivateDirectoryInPrivateParent reports
created === true. Reuse the established privateDirectoryMetadata-style
primitive, preserve the existing verifier for the created === false race path,
and convert any failed recheck into the current CONFIG_CREATE_FAILED error flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2aa8b94-e8ca-429b-a1b0-d0d477db88ee
📒 Files selected for processing (8)
docs/cli.mdsrc/cli/windows-config-acl.tssrc/console/console-application-service.tssrc/setup/setup-draft.tstests/console-windows-first-run.test.tstests/readme-product-contract.test.tstests/windows-config-acl.test.tstests/windows-config-migration-acl.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/console/console-application-service.ts (1)
455-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the underlying cause when wrapping
CONFIG_CREATE_FAILED.Every non-
MiftahErrorfailure — including the new stability-check message ("created configuration directory changed before first write") — collapses into a genericCONFIG_CREATE_FAILEDwith no cause. This is the same discard-the-cause pattern already identified and fixed elsewhere in this PR (src/setup/setup-draft.ts'swithLock, which now forwardserrorintosetupDraftUnavailable(error)).♻️ Preserve the cause consistently with the setup-draft fix
} catch (error) { if (error instanceof MiftahError) throw error; throw new MiftahError( "CONFIG_CREATE_FAILED", - "CONFIG_CREATE_FAILED: unable to create a safe first-run configuration directory" + "CONFIG_CREATE_FAILED: unable to create a safe first-run configuration directory", + { cause: error } ); }Confirm the
MiftahErrorconstructor's details/cause shape before adopting this exact form.🤖 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 `@src/console/console-application-service.ts` around lines 455 - 461, Update the non-MiftahError wrapping path in the configuration creation catch block to preserve the original error as the MiftahError details/cause, matching the constructor shape and the existing withLock/setupDraftUnavailable pattern. Keep existing MiftahError rethrow behavior and the CONFIG_CREATE_FAILED code unchanged.
🤖 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.
Outside diff comments:
In `@src/console/console-application-service.ts`:
- Around line 455-461: Update the non-MiftahError wrapping path in the
configuration creation catch block to preserve the original error as the
MiftahError details/cause, matching the constructor shape and the existing
withLock/setupDraftUnavailable pattern. Keep existing MiftahError rethrow
behavior and the CONFIG_CREATE_FAILED code unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22e70f22-f8c1-417d-bf54-5c914b665da5
📒 Files selected for processing (6)
src/cli/windows-config-acl.tssrc/console/console-application-service.tssrc/setup/setup-draft.tstests/console-windows-first-run.test.tstests/windows-config-acl.test.tstests/windows-config-migration-acl.test.ts
13 tasks
This was referenced Aug 9, 2026
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
1 participant
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.
Scope
Implements the safe, resumable connector-choice slice of #204. This does not close the parent issue.
miftah setup --resumeandmiftah setup --discard-draft; resume re-prompts all connection-sensitive values.Validation
npm test— 1,738 passed, 27 skippednpm run test:core— 415 passed, 22 skippednpm run test:coverage— 1,738 passed, thresholds metnpm run lintnpm run typechecknpm run buildnpm run smoke:clinpm run check:packnpm run test:packageReview note
Manual security/API review completed. Claude Code Opus did not return from the read-only review attempt, and the Fable fallback reported an exhausted quota; no automated-review approval is claimed.
Summary by CodeRabbit
miftah setup --resumeor discarded with--discard-draft.