Skip to content

feat(setup): recover rejected client imports safely - #258

Merged
mohanagy merged 3 commits into
developmentfrom
feat/204-advanced-manual-setup
Jul 26, 2026
Merged

feat(setup): recover rejected client imports safely#258
mohanagy merged 3 commits into
developmentfrom
feat/204-advanced-manual-setup

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • keep client-entry import fail-closed while guided setup recovers only through the existing manual local or remote transport paths
  • preserve only the non-secret configuration name and output location; never copy or render rejected source arguments, environment values, headers, or credentials
  • add Console recovery controls that clear pasted client JSON, plus README and CLI guidance

Security impact

  • rejected client-entry source data is never published, reused as launch input, or reflected in recovery output
  • direct scripted import remains fail-closed; only the interactive flow can continue through reviewed manual transport setup
  • Console recovery clears the pasted document before selecting the existing local or remote form

Validation results

  • focused import and Console suite: 222 passed, 1 platform skip
  • npm test: passed
  • npm run test:core: passed on clean rerun with 415 passed and 22 platform skips
  • npm run test:coverage, npm run lint, npm run typecheck, npm run build, npm run smoke:cli, npm run check:pack, and npm run test:package: passed
  • exact current-head GitHub Actions matrix: all Linux, macOS, Windows Node 20/22/24, and Verify checks green; the initial Windows Node 20 migration timeout is tracked in ci: diagnose intermittent Windows config-migration transaction timeouts #247 and its unchanged rerun passed

Part of #204.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 272f7294-4d1a-4d74-87bd-91ac8c31a377

📥 Commits

Reviewing files that changed from the base of the PR and between 9ab88f1 and cd9e9c9.

📒 Files selected for processing (4)
  • src/cli/setup-client-entry-import.ts
  • src/console/console-assets.ts
  • tests/console-server.test.ts
  • tests/setup-command.test.ts
📝 Walkthrough

Walkthrough

Unsafe guided client-entry imports now preserve failure reasons, retain only safe setup inputs, and route users to manual transport setup. CLI and Console flows clear rejected imported data, while documentation and tests cover the recovery behavior and redaction guarantees.

Changes

Guided import recovery

Layer / File(s) Summary
Import failure reason propagation
src/cli/setup-client-entry-import.ts
Import setup errors now retain their underlying failure reason for guided recovery handling.
CLI manual recovery flow
src/cli/setup.ts, tests/setup-command.test.ts
Refused imports return a manual-recovery outcome, allowing reviewed local setup without copying rejected arguments, credentials, or environment values.
Console controls and product contracts
src/console/console-assets.ts, src/console/console-server.ts, tests/console-server.test.ts, README.md, docs/cli.md, tests/readme-product-contract.test.ts
Console recovery buttons clear pasted JSON and select local or remote setup; messages, documentation, and assertions describe discarded rejected values.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant runSetupCommand
  participant runGuidedClientEntryImport
  participant runInitCommand
  participant ConfigFile
  User->>runSetupCommand: Select import and client entry
  runSetupCommand->>runGuidedClientEntryImport: Attempt guided import
  runGuidedClientEntryImport-->>runSetupCommand: Return manual-recovery outcome
  runSetupCommand->>runInitCommand: Start reviewed manual transport setup
  runInitCommand->>ConfigFile: Write manual configuration
  ConfigFile-->>User: Complete setup
Loading

Possibly related PRs

  • mohanagy/miftah#215: Extends the no-secret client-entry import flow with failure propagation and manual recovery.
  • mohanagy/miftah#252: Shares the guided import control flow updated for recovery handling.
  • mohanagy/miftah#256: Shares CLI and Console setup-source wiring for import, local, and remote flows.

Poem

A bunny found an unsafe scroll,
And kept no secrets in its hole.
“Try local paths, or HTTPS bright—”
The form was cleared and set aright.
Safe setup hops into the night!


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Docstring Coverage ❌ Error Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: safe recovery for rejected client imports.
Description check ✅ Passed The description includes Summary, Security impact, and Validation sections with relevant details and results.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/204-advanced-manual-setup

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 26, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/cli/setup-client-entry-import.ts`:
- Around line 26-34: Update the ClientEntryImportSetupError constructor to set
this.name to "ClientEntryImportSetupError" after calling the CliUsageError
constructor, matching the naming convention used by ClientEntryImportError while
preserving existing inheritance and importReason behavior.

In `@src/console/console-assets.ts`:
- Around line 1186-1198: The manual recovery handlers redundantly query the
onboarding form instead of using the existing closure reference. Update
bindClientEntryManualRecoveryAction and the corresponding code around the other
referenced handler to accept or reuse clientEntryOnboardingForm, removing the
duplicate byId("client-entry-onboarding-form") lookup while preserving the
textarea-clearing behavior.
🪄 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: 5f26cc86-d4d2-4962-bf32-3be797cc1826

📥 Commits

Reviewing files that changed from the base of the PR and between 722d0ca and 9ab88f1.

📒 Files selected for processing (9)
  • README.md
  • docs/cli.md
  • src/cli/setup-client-entry-import.ts
  • src/cli/setup.ts
  • src/console/console-assets.ts
  • src/console/console-server.ts
  • tests/console-server.test.ts
  • tests/readme-product-contract.test.ts
  • tests/setup-command.test.ts

Comment thread src/cli/setup-client-entry-import.ts
Comment thread src/console/console-assets.ts Outdated
@mohanagy

Copy link
Copy Markdown
Owner Author

CodeRabbit rate-limit exception for the current head cd9e9c9:

  • Both actionable inline threads from the prior review are resolved in GitHub and addressed by d49fa4a.
  • cd9e9c9 adds the requested helper docstrings; the PR description now includes Security impact and validation results.
  • The fresh CodeRabbit attempt is explicitly rate-limited for 22 minutes, so it cannot re-evaluate the current head. Its remaining docstring banner has no uncovered-symbol detail.
  • All current-head GitHub Actions checks are green, including Linux quality, Verify, and Windows Node 20/22/24. The transient unrelated Windows migration timeout is tracked in ci: diagnose intermittent Windows config-migration transaction timeouts #247 and passed an unchanged rerun.
  • Local Fable review did not produce a report after a bounded retry, and cloud ultrareview is quota-exhausted. Manual diff review and focused/full validation are complete.

The existing CHANGES_REQUESTED decision applies only to superseded head 9ab88f1 and has no unresolved threads. Dismissing that stale bot review under the documented rate-limit exception.

@mohanagy
mohanagy dismissed coderabbitai[bot]’s stale review July 26, 2026 15:21

Dismissed under the documented CodeRabbit rate-limit exception: the two actionable threads are resolved, the review applies to superseded head 9ab88f1, and current head cd9e9c9 has green CI.

@mohanagy
mohanagy merged commit e0735da into development Jul 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant