fix(web/console): gate /console behind auth + correct run-event normalizer#1878
Merged
Conversation
…lizer Two correctness/security fixes that block the console from replacing the old chat + dashboard UI. Security: /console/* mounted OUTSIDE SessionGate, so with web auth enabled the console was reachable without login while every other route is gated. Wrap it in SessionGate — still outside Layout so it keeps omitting TopNav. SessionGate is a passthrough when web auth is disabled (the solo default), so this is a no-op there. Correctness: the run-event normalizer read keys the server never writes — - node duration read `duration`, but the row carries `duration_ms`, so every rendered node duration was null; - approvals checked `approval_pending`/`approval_resolved` with a `resolution` key, but the server writes `approval_requested`/`approval_received` with `decision` + `comment`/`reason`, so approvals fell through to the raw-JSON text fallback; - `node_skipped_prior_success` (emitted on resume for already-done nodes) was dropped entirely. Also carry the node_completed enrichment already in the payload (output preview, cost, stop reason, turns) for the upcoming per-node detail view. Adds the first test under experiments/console and wires src/experiments/console/ into the web test script so CI starts covering the console as it is promoted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWraps the console route in SessionGate for auth; enriches node transition events with outputPreview, costUsd, stopReason, numTurns; changes approval normalization to approval_requested/approval_received pairs; adds Bun tests and runs them via the web package test script. ChangesConsole Authentication & Event Normalization
sequenceDiagram
participant Client as Console Route
participant SessionGate as SessionGate
participant toRunEvent as toRunEvent
participant DB as Server Rows
participant UI as Console UI
Client->>SessionGate: request /console/*
SessionGate->>Client: allow/deny based on web auth
DB->>toRunEvent: emit event rows (node_*, tool_*, approval_*, workflow_*)
toRunEvent->>UI: normalized events (node_transition with enrichment, tool_call/completion, approval request/received, system/text fallbacks)
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/experiments/console/primitives/event.ts (1)
228-246: 💤 Low valueConsider explicit handling for unexpected
decisionvalues.If
decisionis empty, missing, or an unexpected value (e.g.,'pending'),readStringreturns'', which silently falls through to the'approved'branch. Per coding guidelines, fallback behavior should be documented when intentional, or handled explicitly.A brief comment would suffice if this is intentional, or add an explicit check:
Option A: Document the fallback
if (et === 'approval_received') { - const decision = readString(data, 'decision'); // 'approved' | 'rejected' + // Server sends 'approved' | 'rejected'; default to approved if malformed + // (safe fallback — doesn't block workflow, and backend state is authoritative). + const decision = readString(data, 'decision'); return {Option B: Explicit check
if (et === 'approval_received') { const decision = readString(data, 'decision'); // 'approved' | 'rejected' return { ...base, kind: 'approval', prompt: '', resolution: decision === 'rejected' ? { kind: 'rejected', at: raw.created_at, reason: readString(data, 'reason'), } + : decision === 'approved' + ? { + kind: 'approved', + at: raw.created_at, + comment: readStringOrNull(data, 'comment'), + } : { kind: 'approved', at: raw.created_at, - comment: readStringOrNull(data, 'comment'), + comment: `[unknown decision: ${decision || '(empty)'}]`, }, }; }🤖 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 `@packages/web/src/experiments/console/primitives/event.ts` around lines 228 - 246, The code currently treats any non-'rejected' decision (including '' or unexpected values) as 'approved'; update the logic around the decision variable (read via readString) to explicitly handle allowed values: check if decision === 'rejected' -> rejected branch, else if decision === 'approved' -> approved branch, and else -> an explicit fallback (e.g., return resolution with kind 'unknown' or log/warn and include the raw decision) so unexpected/empty values are not silently treated as approved; reference variables/functions: decision, readString, readStringOrNull, kind:'approval', resolution, raw.created_at.
🤖 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.
Nitpick comments:
In `@packages/web/src/experiments/console/primitives/event.ts`:
- Around line 228-246: The code currently treats any non-'rejected' decision
(including '' or unexpected values) as 'approved'; update the logic around the
decision variable (read via readString) to explicitly handle allowed values:
check if decision === 'rejected' -> rejected branch, else if decision ===
'approved' -> approved branch, and else -> an explicit fallback (e.g., return
resolution with kind 'unknown' or log/warn and include the raw decision) so
unexpected/empty values are not silently treated as approved; reference
variables/functions: decision, readString, readStringOrNull, kind:'approval',
resolution, raw.created_at.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33675beb-81e3-46e3-86e9-b850be9caa0d
📒 Files selected for processing (4)
packages/web/package.jsonpackages/web/src/App.tsxpackages/web/src/experiments/console/primitives/event.test.tspackages/web/src/experiments/console/primitives/event.ts
Collaborator
Author
PR Review Summary — multi-agent review7 specialized agents reviewed this PR (code, docs, tests, comments, errors, types, simplify). Three of them (code-reviewer, comment-analyzer, silent-failure-hunter) independently traced every key-mapping back to the actual server emit sites — and all the core fixes are correct:
Console reads from DB rows ( Critical Issues (0)None. No blocking issues; CI is green (ubuntu + windows). Important Issues (2) — recommended, neither blocks merge
Suggestions (6)
Pre-existing latent notes (not regressions, safe to defer)
Documentation
Strengths
Verdict: READY TO MERGENo critical or blocking issues; CI green; the core correctness + security fixes are verified against the source of truth. The two "important" items (I1 misclassification, I2 inaccurate comment) are quick, worthwhile polish — I1 hardens the normalizer against the same silent-mismatch class it fixes, and I2 is a one-line reword — but neither blocks. S1-S6 are optional. Recommended actions (all ~5-line changes)
🤖 Multi-agent review via Claude Code |
…ocs)
I1 approval_received now matches `decision` explicitly — an unknown/missing
decision stays unresolved (null) instead of silently rendering as approved
(the exact silent-mismatch class this normalizer set out to kill).
I2 reworded the approval comment to future tense and noted that nothing renders
approval events in the run stream today (paused gates use run.approval metadata).
S1 transition mapping → NODE_TRANSITION_BY_EVENT record lookup (flatter, shows
the whole map; node_failed kept explicit so its test guards the default).
S2 enrichment JSDoc: dropped the "per-node accordion (follow-up)" forward
reference; stated the completed-only null contract instead.
S6 App.tsx comment: SessionGate is a passthrough after a brief (cached)
auth-status check, not a literal no-op.
S4 added a node_failed test (guards the map vs the `?? 'skipped'` default).
S5 added error (error>message priority), workflow_*→system, and nodeName
fallback tests; plus an I1 guard test (unknown decision → unresolved).
docs narrowed the stale "CI does not guarantee these routes work" line in
experiments/README.md now that console primitives run in CI.
Skipped with rationale: S3 discriminated-union-on-transition refactor (defer to
the PR that renders the fields); argsSummary dead-read cleanup (pre-existing, no
consumer yet); a SessionGate automated test (needs jsdom/testing-library infra
not present in @archon/web — a future SessionGate unit test is the right target).
Collaborator
Author
|
Addressed in Fixed
Skipped (with rationale)
|
Wirasm
added a commit
that referenced
this pull request
Jun 5, 2026
…1890) * feat(web/console): settings core — assistant config + system panel Adds the console's first settings surface (/console/settings, global) — the parity floor before cutover. PR #4 of the console sequence (after #1878/#1881/#1885). - skills/settings.ts: getConfig/updateAssistantConfig/getHealth/getUpdateCheck plus the pure buildAssistantUpdate(form) transform (8 unit tests). skills/providers.ts: listProviders. Types from @/lib/api.generated (console isolation boundary). - store/keys.ts: config/health/providers/updateCheck keys (health reuses the literal 'health' so it shares lib/health's cache entry). - lib/health.ts: full HealthResponse + useHealth(); useIsDocker derives from it. - AssistantConfigPanel: default-assistant picker (registered providers) + free-text model per provider + codex reasoning/web-search; dirty-gated Save → PATCH /api/config/assistants → ~/.archon/config.yaml → invalidate(K.config) re-seeds. Model is free-text for every provider (Archon does not validate model strings). - SystemPanel: status/adapter/db/version, concurrency (active/maxConcurrent, coerced defensively — concurrency is an open record), running workflows, platform badges, update-check. - ConsoleApp: /console/settings route, gear header link, ',' global keybinding; shortcuts.ts catalogue entry. Honors the error-is-undefined cache contract throughout (the #1885 gotcha). Excludes the GitHub device-flow panel (PR #5) and env-var editing (project-scoped). Validation: web type-check / lint / format:check clean; 59 console tests pass (8 new). Verified end-to-end on an isolated server: read APIs return the expected shapes, save round-trips to config.yaml (binary paths preserved via the server deep-merge), and a browser smoke of /console/settings renders both panels (5 providers, codex effort/web-search, system grid, Save dirty-gated). * fix(web/console): address PR #1890 review — comments + update-check silent failure I1: correct the buildAssistantUpdate JSDoc. Verified the PATCH route does NOT safe-filter per field on the write path — it validates only provider ids and merges the body into config.yaml unfiltered (safe-filtering is read-path only). The real invariant is that this function only ever attaches codex-only fields to the codex entry; the comment now says that instead of the false "server safe-filters" claim. I2: rewrite the K.health note. This PR routes lib/health through K.health, so the old "lib/health already caches under this literal" premise is stale; the invariant is that both consumers read via useHealth() to share one cache entry. I3 (silent failure): SystemPanel showed "checking…" forever on a failed update-check (the error was destructured away). Surface updateError via an UpdateStatus helper → "update check unavailable". S1: SettingsSection children typed ReactNode (ReactElement|ReactElement[] fought the `cond && <el/>` pattern). S5: replaced the nested update-status ternary with the UpdateStatus helper + early returns for the health loading/error states. S6: extracted the shared SettingsSection card shell (PR #5 is the 3rd consumer), a SELECT_CLASS const for the two codex selects, and bound activePlatforms once. Docs: added /console/settings to the console README routes. Deferred (with rationale): - S2 (re-seed on providers identity): latent only — nothing invalidates K.providers, and a ref-snapshot "fix" introduces a config-vs-providers load-order race. Keep the simple [config, providers] effect. - S3 (literal-union effort/webSearch types): kept bare string so seedForm tolerates an out-of-enum value in config.yaml; the <select> is the write-side enforcement. - S4 (show both load errors): an error panel showing the first error is acceptable. Validation: web type-check / lint / format:check clean; 59 console tests pass.
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.
Summary
/consolethe default UI (replacing old/chat+ dashboard): (1)/console/*was reachable without login when web auth is enabled, and (2) the run-event normalizer read keys the server never writes, so node durations were always null, approvals never rendered, and resume-skipped nodes vanished./console/*inSessionGate; fixed theevent.tsnormalizer (duration→duration_ms,approval_requested/approval_receivedwithdecision/comment/reason, addednode_skipped_prior_success, carry node-completion enrichment); added the first console test + wiredsrc/experiments/console/into the web test script.UX Journey
Before
After
Architecture Diagram
Before / After
Connection inventory:
App.tsx/console/*SessionGateSessionGateConsoleAppevent.tstoRunEventworkflow_events.dataweb/package.jsontestsrc/experiments/console/Label Snapshot
risk: lowsize: Swebweb:consoleChange Metadata
bug(security + correctness)webLinked Issue
Validation Evidence (required)
event.test.ts(14 cases) asserts each corrected mapping against payload shapes taken from realworkflow_eventsrows in a live DB (duration_ms,node_output/cost_usd/stop_reason/num_turns,approval_receiveddecision/comment/reason,node_skipped_prior_successreason:'prior_success').bun run test— change is isolated to@archon/web; ran that package's full suite instead. CI runs the rest.Security Impact (required)
SessionGateis the same component already gating every other route; it no-ops when web auth is disabled, so solo/default installs are unaffected.Compatibility / Migration
Human Verification (required)
workflow_eventsrows (node_completed, node_skipped, node_skipped_prior_success, tool_called/completed) and against the emit sites for approval rows (dag-executor.ts,workflow-operations.ts). ConfirmedRunStream/NodeDivideralready consumedurationMs/skipReason, so the duration + skip fixes are immediately visible.node_startedhas null duration/enrichment; the never-writtenapproval_pendingtype correctly does NOT render as an approval (regression guard).RunStreamdoesn't yet renderkind:'approval'entries (the normalizer now classifies them correctly; the renderer is a follow-up).Side Effects / Blast Radius (required)
SessionGatepassthrough preserves current behavior when auth is off; normalizer changes only correct mismatched keys (the onlyNodeTransitionEventconstructor is the normalizer itself).Rollback Plan (required)
git revert <merge-commit>— 4 files, web-only./loginunexpectedly (would indicate an auth-status misconfig, not this change) or node durations regress to null.Risks and Mitigations
NodeTransitionEventliterals and breaks on the new required fields.RunStreamonly reads the type. Type-check passes.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation