feat: add the optional local OAuth dashboard - #191
Conversation
📝 WalkthroughWalkthroughThe PR adds ChangesConsole dashboard and OAuth onboarding
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant DashboardCLI
participant ConsoleServer
participant ConsoleApplicationService
participant ConfigFile
Operator->>DashboardCLI: run miftah dashboard
DashboardCLI->>ConsoleServer: start foreground loopback server
ConsoleServer-->>DashboardCLI: URL and one-time bootstrap code
DashboardCLI-->>Operator: print startup details and optionally open browser
Operator->>ConsoleServer: submit native OAuth onboarding
ConsoleServer->>ConsoleApplicationService: validate and create connection
ConsoleApplicationService->>ConfigFile: write new configuration
ConfigFile-->>ConsoleApplicationService: configuration created
ConsoleApplicationService-->>ConsoleServer: onboarding report
ConsoleServer-->>Operator: return dashboard response
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Self-review follow-up on current head c074c94:
Focused Console suite: 14/14 passed. Lint, typecheck, build, and diff checks pass. A simultaneous full-suite run was invalidated by a confirmed unrelated Madar Vitest worker saturating the host and causing 17 broad 5-second timeouts; that evidence is recorded under existing #113, and no timeout was increased. A quiet-host full repeat is pending while current-head CI runs. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/main.ts (1)
94-106: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract shared shutdown/SIGHUP wiring from
consoleServeanddashboardServe.The shutdown handler and SIGINT/SIGTERM/SIGHUP registration in
dashboardServe(Lines 134-146) is a verbatim copy of the same block inconsoleServe(Lines 94-106). Any future change to lifecycle handling has to be applied in both places.♻️ Proposed refactor to share lifecycle wiring
+function registerServerLifecycle(server: { close: () => Promise<void>; rotateCredential: () => string }): void { + const shutdown = (): void => { + void server.close().catch(() => { + process.stderr.write("Miftah Console shutdown failed.\n"); + process.exitCode = 1; + }); + }; + process.once("SIGINT", shutdown); + process.once("SIGTERM", shutdown); + if (process.platform !== "win32") { + process.on("SIGHUP", () => { + process.stdout.write(`Replacement one-time bootstrap code: ${server.rotateCredential()}\n`); + }); + } +} + async function consoleServe(configPath: string, port: string | undefined): Promise<void> { const server = await startConsoleServer(configPath, { port: consolePort(port), launcher: { command: process.execPath, args: [fileURLToPath(import.meta.url), "serve"] } }); process.stdout.write(...); - const shutdown = (): void => { ... }; - process.once("SIGINT", shutdown); - process.once("SIGTERM", shutdown); - if (process.platform !== "win32") { ... } + registerServerLifecycle(server); }Apply the same replacement in
dashboardServe.Also applies to: 134-146
🤖 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/cli/main.ts` around lines 94 - 106, Extract the duplicated shutdown and signal-registration logic from consoleServe and dashboardServe into a shared lifecycle-wiring helper, preserving the existing server.close error handling, SIGINT/SIGTERM registration, and non-Windows SIGHUP credential rotation behavior. Replace both inline blocks with calls to the shared helper and pass each serve function’s server instance.src/console/console-server.ts (1)
162-188: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CONSOLE_LAUNCHER_UNAVAILABLEhas no dedicated HTTP mapping.This new error code (thrown by
clientSnippetswhen no launcher is configured) falls through to the generic502 "operation could not be completed"branch. 502 implies an upstream/gateway failure, but this is a local console misconfiguration — closer toCONFIG_CREATE_FAILED's503treatment.🐛 Proposed fix
if (error.code === "CONFIG_CREATE_FAILED") { return new ConsoleHttpError(503, "config_create_failed", "The initial configuration could not be created."); } + if (error.code === "CONSOLE_LAUNCHER_UNAVAILABLE") { + return new ConsoleHttpError(503, "console_launcher_unavailable", "Client snippets are unavailable because no launcher is configured."); + }🤖 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-server.ts` around lines 162 - 188, Add a dedicated CONSOLE_LAUNCHER_UNAVAILABLE case in publicApplicationError, mapping it to a 503 response with a stable launcher-unavailable error identifier and a message indicating the Console launcher is unavailable. Keep the existing generic mappings 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.
Inline comments:
In `@docs/oauth-console-threat-model.md`:
- Line 70: Update docs/oauth-console-threat-model.md:70 and docs/security.md:41
to state that Console requests require the exact listener Origin, including
scheme, host, and port. Extend the contract assertions in
tests/oauth-console-threat-model-docs-contract.test.ts:35-46 to verify exact
listener Host and full Origin validation, CSRF requirements for state-changing
requests, and the existing omission rule for permitted reads.
In `@src/console/console-application-service.ts`:
- Around line 125-129: Consolidate the duplicate error-code narrowing by
exporting the existing errorCode(error) helper from migrate-config and reusing
it in console-application-service.ts, including within the fileErrorCode call
sites. Remove the local fileErrorCode implementation while preserving the
current undefined behavior for errors without a string code.
In `@tests/oauth-console-threat-model-docs-contract.test.ts`:
- Around line 29-46: Extend the contract assertions in the OAuth console
threat-model test to require that mutation requests use the exact listener Host
and matching Origin, and enforce the documented CSRF checks. Keep the existing
authenticated GET/HEAD Origin-omission assertion, and add consoleApi
expectations covering the exact-origin mutation rule so documentation cannot
weaken this boundary.
---
Outside diff comments:
In `@src/cli/main.ts`:
- Around line 94-106: Extract the duplicated shutdown and signal-registration
logic from consoleServe and dashboardServe into a shared lifecycle-wiring
helper, preserving the existing server.close error handling, SIGINT/SIGTERM
registration, and non-Windows SIGHUP credential rotation behavior. Replace both
inline blocks with calls to the shared helper and pass each serve function’s
server instance.
In `@src/console/console-server.ts`:
- Around line 162-188: Add a dedicated CONSOLE_LAUNCHER_UNAVAILABLE case in
publicApplicationError, mapping it to a 503 response with a stable
launcher-unavailable error identifier and a message indicating the Console
launcher is unavailable. Keep the existing generic mappings unchanged.
🪄 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: 8754d23d-95c7-456a-865f-9a40c257d48e
📒 Files selected for processing (23)
README.mddocs/architecture.mddocs/cli.mddocs/console-api.mddocs/oauth-console-threat-model.mddocs/oauth-support.mddocs/security.mdsrc/cli/exit-codes.tssrc/cli/main.tssrc/cli/migrate-config.tssrc/cli/parse.tssrc/console/console-application-service.tssrc/console/console-assets.tssrc/console/console-server.tssrc/console/open-browser.tssrc/utils/errors.tstests/cli-exit-codes.test.tstests/cli-parse.test.tstests/console-application-service.test.tstests/console-open-browser.test.tstests/console-server.test.tstests/oauth-console-threat-model-docs-contract.test.tstests/package-contract.test.ts
|
Current-head quiet-host validation is complete on c074c94:
The quiet repeat passed every case that failed while the unrelated worker was active. No timeout, assertion, coverage threshold, or platform behavior was changed. |
Review findings resolved on current head
|
Dismissed only after all five findings were verified: four were fixed in d2c93b9, the local helper consolidation was declined with an architectural rationale, all three inline threads are resolved, and the current-head CodeRabbit status is rate-limited rather than a new review. Full local validation is green; exact-head CI remains the merge gate.
1 participant
Closes #86
Outcome
Adds an optional, foreground-only browser-local Console launched with
miftah dashboard. It turns first-run connection/profile setup, OAuth onboarding, status, recovery, and copyable client configuration into a guided local workflow while keeping the CLI and hand-written config fully supported.Product and security boundary
Tests and documentation
Adds focused CLI parsing/exit-code, browser launch, Console service, loopback HTTP security, accessibility/recovery, package-contract, and threat-model contract coverage. Updates the README and Console/OAuth/CLI/security/architecture documentation.
Validation on rebased head e32bac9
npm test: 1,261 passed, 20 platform-gated skipsnpm run test:core: 404 passed, 20 platform-gated skipsnpm run test:coverage: 95.41% statements, 91.90% branchesnpm run lintnpm run typechecknpm run buildnpm run smoke:clinpm run check:pack: 46-file package contract verifiednpm run test:package: 20 passedgit diff --checkThe first full-suite attempt hit the already tracked intermittent synchronization boundary in #113. The exact case then passed 11/11 focused quiet-host repetitions and passed the clean full-suite and coverage runs; no timeout or source workaround was applied.
The base merge commit e6a53bf also passed exact-head development run 29958797299 across Linux, macOS, Windows, Node 20/22/24, and final Verify.
Summary by CodeRabbit
New Features
miftah dashboard.--config,--port, and--no-open.Security
Documentation