Overnight: v0.1 completion + v0.2 durable-run pipeline (umbrella) - #101
Closed
leon0399 wants to merge 24 commits into
Closed
Overnight: v0.1 completion + v0.2 durable-run pipeline (umbrella)#101leon0399 wants to merge 24 commits into
leon0399 wants to merge 24 commits into
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Short OpenClaw-style vision doc distinct from SPEC.md's detailed architecture: the why behind the core bets, current milestone focus, emerging directions not yet spec'd (personas, machine connector, Brain memory surface, calendar/email, n8n-style workflows), and guardrails on what we won't build yet. Linked from AGENTS.md's doc index. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
) OPENAI_BASE_URL and OPENAI_MODEL env vars let dev and the eval suite (#58) run the chat loop against any OpenAI-compatible endpoint (OpenRouter free tier, groq, local) instead of hardcoded api.openai.com. ModelsService resolves both from config; an explicit model argument still wins over the env default. Documented the OpenRouter free-tier setup in .env.example. v0.1 dev/eval stopgap by design — native OpenRouter provider, credential vault, and BYOK stay in v0.4 (#37/#82). Closes #88 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
When a chat's estimated live context exceeds COMPACTION_TOKEN_THRESHOLD (default 100k tokens), a post-turn model call summarizes the older turns into a first-class `compactions` row: `upto_seq` records exactly which messages it supersedes and `parent_id` chains to the compaction it absorbed — auditable, rewindable lineage (Hermes-style, SPEC §2.1); messages are never deleted or mutated. The next turn's context is summary + recent turns via ContextBuilder. Mechanics: the trigger fires after a completed turn (fire-and-forget — before the context limit, never delaying the user's response); the summarization call runs outside any DB transaction, with a staleness re-check so a concurrent compaction of the same chat wins and the stale one is discarded. Re-compaction feeds the prior summary into the new one, so lineage loses nothing. Security: compactions condense private conversation content, so the table ships with RLS ENABLE + hand-maintained FORCE (0004 precedent) and the same owner policy as messages; cross-tenant read and write denial proven in the RLS integration suite (rls-test.sh green). Unblocks the v0.1 eval set (#58) overflow case. Closes #57 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Three eval cases run the real loop over HTTP with a real provider: happy path, prompt-injection canary, and context-overflow/compaction (#57) coherence. Double-gated (RUN_MODEL_EVALS=1 + POSTGRES_URL) so rls-test.sh and CI never spend provider tokens; run via `pnpm --filter api test:evals`. Verified live against OpenAI: happy path and injection PASS. The overflow case initially stalled under the threshold because the model answered fillers too tersely — fillers now carry a fixed ~800-char inert payload so the window size is deterministic; that fix has NOT yet been re-verified live (session interruption), so this does not close #58. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Restores server-side chat titling lost in the #63 thin-client cutover (every chat stayed "New chat"). After the first completed turn, a cheap post-turn model call names the chat from the user's message — same fire-and-forget shape as compaction (#57), off the response path, and it rides into the worker with the loop (#50). The prompt and output sanitation follow the Vercel ai-chatbot reference (2–5 words, no prefixes/markdown/quotes, clamped to 80 chars). The model call is skipped unless the title is still the default, and the persist goes through an atomic WHERE title = 'New chat' guard, so a user rename mid-generation always wins. The e2e fake client tracks title calls separately from chat turns to keep turn-count assertions deterministic; the first-message e2e now proves the async title lands. Closes #78 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
All three eval cases now pass against a real model over the real loop: happy path, prompt-injection canary, and overflow — the last verified live after making the context-window sizing deterministic (fixed inert padding per filler turn instead of relying on model verbosity). The overflow case doubles as an end-to-end integration proof of the configurable provider (#88) + lineage compaction (#57): the chat compacts mid-conversation and a fact from the absorbed turns survives via the summary. This was the last v0.1 line item: ROADMAP drops the completed v0.1 section (forward-only) and moves budget enforcement (#91) under v0.2 where its GitHub milestone actually lives; CHANGELOG records the completion. Closes #58 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
…erface (#47) Stands up pg-boss v12 on the existing Postgres (own `pgboss` schema, own `pg` pool alongside Drizzle's postgres.js — two drivers to one database, SPEC §24.0.1) via @wavezync/nestjs-pgboss. No Redis, no separate scheduler service; scheduling is pg-boss cron, not the pg_cron extension (which runs SQL, not app code). Callers depend on a new Queue interface (QUEUE token: ensureQueue / enqueue / consume / stopConsumer / schedule / unschedule / cancel), so the engine can later swap to BullMQ or Temporal without touching call sites (SPEC §23.4). ensureQueue defaults every queue to retries with exponential backoff plus a `<queue>.dead` dead-letter queue — failed work stays inspectable and replayable, never silently dropped. QueueModule is deliberately NOT imported into AppModule: its consumers are the durable-run pipeline (#48) and the worker (#50); booting a live queue in the API request path today would deepen exactly the coupling the v0.2 worker split unwinds. Verified against real Postgres by a gated integration suite (same TEST_DATABASE_URL gate as the RLS spec): enqueue/consume roundtrip, retry-until-success, dead-letter routing with payload intact, cron schedule persistence/removal, and deferred (startAfter) delivery. Jest transforms the ESM-only pg-boss chain (pg-boss, serialize-error, non-error) via transformIgnorePatterns + allowJs; the production CJS build uses Node 20.19+'s require(esm). No dedup/singleton option is exposed yet — pg-boss v12 ties dedup to queue policy, and the verified semantics land with #48's idempotent enqueueing. Closes #47 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Lands the durability substrate of the run pipeline (SPEC §9.3–§9.4): - `runs` — one row per execution attempt (a retried message gets a fresh run), with the full SPEC §9.3 status enum so later milestones add no enum migrations. Created in the SAME transaction as the user message, so a message can never exist without its execution record. - `run_events` — append-only, replayable event log; `sequence` is a table-global identity (monotonic per run, not dense), indexed for the cursor reads the SSE replay (#49) will do. No update/delete methods exist on the repository — append-only by construction. - The streaming loop dual-writes the lifecycle (run.created → run.started → model.requested → model.completed → run.completed / run.failed / run.cancelled) as best-effort bookkeeping that can never break the live stream; when the loop moves into the worker (#50) this log becomes the authoritative execution record. Security: both tables ship RLS ENABLE + hand-maintained FORCE (0004 precedent); runs scope directly on user_id, run_events via their run. Cross-tenant read AND write denial proven in the RLS integration suite, and the e2e now asserts a completed turn leaves a completed run with a strictly ordered event log. Deliberately deferred within #48 (issue stays open): worker consumption from pg-boss, model.delta token events (lands with #49, its consumer), cancellation/heartbeat/timeout, and the per-chat single-flight unique index — unsafe before heartbeat exists, since a crashed in-flight run would deadlock its chat forever. Refs #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
/#49) Makes the durable run log complete and consumable — the API side of the refresh-safe replay (#49) and another #48 slice: - ModelStreamInput gains a narrow onTextDelta seam; the OpenAI client maps AI SDK onChunk text-deltas onto it. The loop coalesces deltas through a pure size-based delta-buffer (400 chars — one row per token would multiply writes) and appends model.delta events via a sequential promise chain, drained before the terminal events so the log always reads in stream order. - RunsController: GET /api/v1/runs/:id (egress-allowlisted RunResponse) and GET /api/v1/runs/:id/events?after_sequence=N — SSE per SPEC §9.4. Each frame's SSE id: is its event sequence, so Last-Event-ID-style resume falls out naturally; an in-flight run is polled (500ms) until terminal, a finished run streams its tail and closes with [DONE], a deleted-mid-stream run closes instead of spinning, and connections are capped at 5 minutes (clients reconnect with their cursor). Security: session-guarded; a cross-tenant run id 404s on both endpoints before any SSE headers are sent (no existence leak) — proven in e2e alongside full-replay ordering, mid-stream cursor resume, and the coalesced delta payload round-trip. Also fixes the e2e SSE parser, which dropped frames carrying an id: line before data: — it made a cursor assertion pass vacuously and hid the frames entirely. The apps/web resume-on-refresh client remains open in #49; worker consumption, cancellation, and heartbeat/timeout remain open in #48. Refs #48, #49 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
GitHub Actions workflow with two parallel jobs on every PR and pushes to master: - checks: pnpm install --frozen-lockfile → turbo run lint → turbo run build → api unit suite (DB-backed specs self-skip without a database; model evals stay opt-in behind RUN_MODEL_EVALS, so CI never spends provider tokens). - rls: apps/api/scripts/rls-test.sh — the FORCE-RLS cross-tenant proof plus the auth/chat HTTP e2e against a throwaway Postgres, exactly the script run locally (GitHub-hosted runners have Docker). Hardening: actions pinned to commit SHAs (tag in comment), top-level permissions: contents: read, persist-credentials: false, no untrusted interpolation. actionlint and zizmor both clean. Wiring root lint into CI surfaced that packages/ui's lint script had been silently broken forever — eslint was never in its devDependencies, so `turbo run lint` failed before reaching any code. Fixed the dependency and the three warnings it had been hiding (an unused shiki import and two declaration-merging interfaces the empty-object-type rule flags by design). Making CI required on master is a repo-settings step once the first run is green. Closes #70 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Splits the chat loop along the SPEC §9.5 seam: - RunExecutionService (new, transport-agnostic): context assembly at execution time (compaction summary + live window up to the triggering message), the model call, and every durable side effect — assistant turn persistence, run lifecycle + coalesced model.delta events, and post-turn compaction/titling. No HTTP types; the caller supplies the ModelClient, so the API keeps its 402-before-persistence contract and the worker will resolve credentials itself. - ChatLoopService keeps only the API-side steps: validate, store the user message, create the run (same transaction), hand off execution. Today the hand-off returns the live stream to the controller; the worker move (#50) replaces that one call with an enqueue while the worker drives the identical executeRun. The only observable change is that context assembly now reads in its own transaction after the message commit (read-committed makes this equivalent). Behavior-preserving by proof: the full e2e suite (event ordering, titling, abort/retry, replay, cross-tenant denial) and RLS integration pass unchanged, plus lint/build/unit suites. Refs #48, #50 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
/#50) With RUN_EXECUTION_MODE=worker the API side of a turn becomes exactly SPEC §9.5: validate → store message → create run → enqueue on pg-boss → respond. A co-located RunsWorkerService consumes the `runs` queue and drives the identical RunExecutionService extracted last commit; the HTTP response streams from the durable run-event log through a new bridge that translates run events into the AI SDK UI-message protocol (start/text-start/text-delta/text-end/finish/error + [DONE]), so the existing apps/web chat transport works unchanged. The durability win, proven by e2e: a client disconnect mid-run stops only the bridge — the worker finishes the model call, the assistant turn persists, and the run reaches `completed` with a fully ordered event log. The turn no longer lives or dies with the socket. Failure contract: run-level failures (model errors) are recorded durably by executeRun and the queue job succeeds — retrying would re-run a turn whose failure is already the source of truth; queue retries + dead-lettering apply to infrastructure failures (credential resolution, DB unavailability). The early 402 credential check in the API is preserved in both modes. Default stays `inline` pending soak — flipping it, cancellation, and heartbeat/timeout remain open in #48/#50. The e2e jest config gains the same pg-boss ESM transform allowlist as the unit config (AppModule now imports QueueModule, so every e2e suite loads pg-boss). Refs #48, #50 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Worker mode made runs survive client disconnects; this adds the deliberate way to stop one. Modeled as a resource PATCH (the house REST rule — no RPC verb handles): `cancelled` is the only client-writable transition, enforced by the DTO. Mechanics, layered for the coming worker-process split: - runs.cancel_requested_at (migration 0011) is the durable, cross-process signal, stamped atomically only on non-terminal, not-yet-requested runs (single-writer guard in the WHERE). - RunAbortRegistry is the in-process fast path: the worker registers an AbortController per executing run; the cancel endpoint aborts it directly while API and worker share a process. Mid-flight abort flows through the exact path a client abort used in inline mode. - A run cancelled while still queued is settled as `cancelled` at pickup — the model is never called. Semantics proven in worker-mode e2e: mid-flight cancel terminates the run as `cancelled` with no assistant answer; re-cancel is idempotent (200); a finished run answers 409; a cross-tenant run id answers 404 (no existence leak). Remaining on #48: heartbeat + run timeout, then the per-chat single-flight index they unblock. Refs #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
The last hard #48 criterion: a crashed or hung worker can no longer leave a run wedged in running_model forever. - The executing worker stamps runs.heartbeat_at (migration 0012) at start and on a configurable interval (RUN_HEARTBEAT_SECONDS, 15s). - Every enqueued run gets its own delayed deadman job on a `runs.timeouts` queue (pg-boss startAfter, RUN_TIMEOUT_SECONDS, 300s), enqueued API-side so it exists even if no worker ever picks the run up. At fire time, in the run owner's tenant context: a terminal run is left alone; a fresh heartbeat means a long turn — re-check later; a heartbeat older than RUN_HEARTBEAT_STALE_SECONDS (60s) means the executor died — the run is expired with a run.failed event. Deliberately NO cross-tenant reaper scan: the per-run job keeps every read inside the tenant boundary, so the RLS moat needs no system bypass. - Terminal statuses are now immutable at the repository level: markFinished excludes already-finished runs in its WHERE, so a late-finishing stream callback can never overwrite expired/cancelled (first writer wins) — this also hardens the cancellation race. Proven in worker-mode e2e: a hand-crafted zombie (running_model, stale heartbeat) expires with the event trail while a completed run's own deadman leaves it untouched; live runs under tight test thresholds keep completing because their heartbeats stay fresh. Remaining on #48: the per-chat single-flight index (now unblocked) and flipping RUN_EXECUTION_MODE's default. Refs #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
At most one non-terminal run per chat, enforced in the database — a partial unique index on runs(chat_id) WHERE status is non-terminal. This is the atomic guarantee against the concurrent double-model-call race deferred from #55 (#73): a second message racing an in-flight run loses at the index, its transaction rolls back whole (no orphan user message), and the client gets 409. Retry semantics preserved by supersession: a retry of the SAME message first cancels any non-terminal run for that message (same transaction, run.cancelled event, plus an in-process abort of the superseded execution after commit) — so a turn whose previous attempt died silently is always retryable, and at most one generation per message survives. Content equality of retries was already enforced, so nothing user-visible is lost. Resurrection guards: markStarted now refuses terminal runs (same immutability as markFinished) and worker pickup skips any run that went terminal while queued — a superseded run can never come back as running_model. The v0.1 "overlapping turns" e2e encoded the old concurrent contract and is rewritten to the serialized one: overlap → 409, first context never sees the rejected send, and the next turn sees the completed prior exchange. New e2e also proves the 409 rollback leaves nothing behind. Unique-violation detection walks drizzle's cause chain. Safe only now: heartbeat + deadman (previous commit) guarantee every run reaches a terminal status, so the single-flight slot always frees. This completes the #48 acceptance list (API-only request path in worker mode, ordered events, idempotency, cancellation, heartbeat + timeout, single-flight). Flipping RUN_EXECUTION_MODE's default remains a soak decision under #50. Closes #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
The three remaining items (single-flight shipped with #48): - DB-level in_reply_to integrity: a trigger (hand-authored migration 0014 — Drizzle can't express triggers; documented alongside 0004/0006 in the gotchas) rejects a reply that references a message in another chat or a non-user message, whichever code path writes it. Runs under the caller's RLS context. Proven with negative tests in the RLS integration suite; a composite-FK approach was rejected because partial SET NULL column lists aren't expressible through Drizzle and it can't cover the role check anyway. - Event-driven abort fidelity: the e2e fake model client now reacts to the abort EVENT mid-delay (racing the timer), not a post-sleep `.aborted` poll — the same interruption shape as the real AI SDK. A new e2e with an effectively infinite model delay proves the no-partial-write guarantee end-to-end: only the abort event can end the turn, onFinish never fires, the persisted assistant row is the empty aborted placeholder, and the run terminates cancelled (freeing the single-flight slot). - Unit fake onFinish timing: createFakeModelClient now fires callbacks on stream CONSUMPTION (pull-driven; `text` is a lazy getter, consumeStream drives the same once-only finish), awaited — instead of synchronously and discarded at call time. An unconsumed stream fires nothing, matching the real SDK. Closes #73 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
…, TRUST_PROXY (#68) First tranche of the #60 auth-surface hardening list: - Fail-closed by default: SessionAuthGuard registers as a global APP_GUARD; @public() is the explicit, reviewable opt-out (login, register, the liveness root). Per-route @UseGuards were REMOVED so the global guard is load-bearing — every existing 401 e2e assertion now proves the default-deny path, not a per-controller annotation. This kills the bug class behind the original /users exposure. - Atomic session validation: findActiveAndTouch validates and stamps last_seen_at in one UPDATE … RETURNING (no SELECT→check→UPDATE TOCTOU), with a read-only fast path inside a 60s debounce window that takes the per-request write off the hot path entirely. Stale rows are deleted as housekeeping on the miss path. - Session listing returns active (unexpired) sessions only, backed by a new (user_id, expires) index (migration 0015); the current-session lookup is a single WHERE user_id AND id query instead of list-then-find. - TRUST_PROXY (off by default — trusting proxy headers with no proxy lets clients spoof their IP): hop count or Express subnet spec, so session.ip records the real client behind a reverse proxy (SPEC §22.0 prerequisite for IP/subnet policies). Still open in #68: login/register rate limiting (@nestjs/throttler), the cross-site SameSite=None + CSRF posture, token-free responses for cookie clients, session rotation (vacuous until a change-password endpoint exists), and periodic expired-session cleanup. Refs #68 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
… cron (#68) - Rate limiting (@nestjs/throttler): instance-wide 300/min default with strict 10/min per-IP overrides on POST /auth/v1/login and /register — each attempt burns a bcrypt compare, an unbounded brute-force + DoS amplification surface. Guard order is deliberate: ThrottlerGuard registers BEFORE SessionAuthGuard, so a flood is rejected without paying the session lookup. Uses req.ip, so the TRUST_PROXY setting from tranche 1 feeds directly into per-client fairness. Proven by an e2e that rapid-fires logins into a 429 (placed last in the suite — it spends the instance's login budget). - Expired-session housekeeping on a pg-boss cron: SessionCleanupService schedules 'sessions.cleanup' hourly (off the :00 mark) and purges expired/idle rows — the #47 scheduler's first production consumer. Cross-user by design (sessions carry no RLS; they are consulted pre-authentication and expiry is a global fact, not tenant data); idempotent across concurrent instances. The purge SQL is proven against real Postgres in the integration suite: expired rows go, live rows stay. Remaining in #68: cross-site SameSite=None + CSRF posture, token-free responses for cookie clients, and session rotation (vacuous until a change-password endpoint exists). Refs #68 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
The API half of refresh-safe resume: GET /api/v1/chats/:id/stream returns the chat's active run as an AI SDK UI-message stream (the existing RunStreamBridgeService, reused wholesale — it replays the run's persisted events from the start and follows it live to completion), or 204 when there is nothing to resume. This matches the AI SDK v6 transport contract for reconnectToStream, so the apps/web hookup that closes #49 is a small custom-transport method + a resume flag. "The chat's active run" is well-defined because the per-chat single-flight index admits at most one non-terminal run — the new RunsRepository.findActiveByChatId leans on that invariant. Security: session-guarded like everything else (global guard); a cross-tenant or unknown chat id answers the same 204 as "no active run" — resuming leaks neither existence nor state. Proven in worker-mode e2e: disconnect mid-run, then GET the stream — the full ordered chunk sequence (start → text-start → deltas → text-end → finish) arrives with the complete answer; after completion the endpoint answers 204; another tenant gets 204. The apps/web resume client (transport reconnectToStream + resume flag + browser verification) remains to close #49. Refs #49 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
The client half of refresh-safe resume, matching the API endpoint from the previous commit: - The chat DefaultChatTransport gains prepareReconnectToStreamRequest, pointing the SDK's reconnect at GET /api/v1/chats/:id/stream (the default convention would have targeted .../messages/:id/stream). Shares the existing authAwareFetch + credentials wiring. - Persisted chat sessions mount with resume: true — on load, useChat probes for an active run and, when one exists (worker mode), replays its UI-message stream live from the durable event log. Draft chats (navigateOnFinish) skip the probe: they cannot have a server-side run yet. An idle chat's 204 resolves to null in the SDK — a no-op. Verification: new vitest unit for the URL preparation, web typecheck + lint + build clean, and the full 10-test Playwright browser suite passes against the live api+web stack — the resume probe on every persisted-chat mount regresses nothing in inline mode. Remaining to close #49: a browser test of an actual mid-run refresh, which needs the Playwright-launched API running RUN_EXECUTION_MODE= worker — the natural companion to the #80 chat-flow e2e and the worker-mode soak. Refs #49 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
…49, #80) The browser proof that closes the durable-runs story: - e2e/model-server.ts: a deterministic OpenAI-compatible mock speaking /chat/completions streaming SSE; the Playwright-launched api points OPENAI_BASE_URL at it (#88) — real loop, zero provider spend. A "SLOW" prompt drips tokens over ~4s so tests can reload mid-answer; title-generation calls get a distinct short answer so message and sidebar title are distinguishable. - The whole Playwright api now runs RUN_EXECUTION_MODE=worker: all 12 browser tests exercise the queue-executed pipeline — standing soak evidence for flipping the default (#50). - e2e/chat/chat-flow.spec.ts: create → stream → render (#80's scope), and the headline — reload the page mid-answer; the run survives in the worker, the persisted chat remounts with resume, and the FULL answer appears on screen from the durable event log. Fixes surfaced by building this: - Latent #88 bug: the model client used the provider default, which targets OpenAI's proprietary /responses endpoint — OpenAI-compatible providers (OpenRouter, groq, local; the whole point of OPENAI_BASE_URL) only implement /chat/completions. Now uses openai.chat(model), which works everywhere including OpenAI. - The strict 10/min per-IP auth throttle starved parallel browser workers logging in from one IP: the limit is now env-tunable (AUTH_RATE_LIMIT_PER_MINUTE, harness sets 1000); the production default stays 10/min. Verified: 12/12 Playwright browser tests green (auth + chat flows, all under worker mode), full api unit + RLS/e2e suites green after the /chat/completions change, builds clean. Closes #49 Closes #80 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Consolidation pass over the overnight branch before review: - chat-loop.service: `let run` in the single-flight path was implicitly any (typed now), and the unique-violation matcher used String() on unknown values (typeof-narrowed now). Both were real eslint ERRORS masked by the local lint wrapper's summary output — the CI workflow (#70) would have failed the branch on them. - runs-worker: at-least-once delivery could start a SECOND model call when pg-boss redelivered a job whose run was already executing (e.g. a retry after a mid-execution infrastructure error). The pickup gate now skips a running_model run with a fresh heartbeat — the live execution settles it — while a STALE running run still accepts the redelivery as the crash-recovery path, where markStarted refreshes the heartbeat. Interplay with the deadman is coherent in both orders: expired-first → terminal skip at pickup; restarted-first → the deadman sees a fresh heartbeat and reschedules. Verified: turbo lint green across all workspaces (--force, no cache), tsc clean, full unit + RLS/e2e suites green. Refs #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Two independent reviewers (security + concurrency lenses) audited the full overnight branch diff. Security: no high-severity findings (its one request — forged cross-tenant run_events INSERT coverage — already exists in the RLS suite). Concurrency confirmed three real races, all fixed here: 1. Lost cancellation in the pickup window: a PATCH cancel landing after the worker's skip-gate read but before abort registration stamped cancel_requested_at yet found no controller to abort — the run completed as if never cancelled. The worker now re-checks the flag immediately after registering; any later cancel hits the live controller, so the window is closed from both sides. 2. Execution without a claim: executeRun discarded markStarted's result, so a run that went terminal between enqueue and execution (superseded by a retry, cancelled, expired) still appended run.started/model.requested onto a terminal run and burned a full model call. Claiming is now mandatory and non-best-effort: a failed claim throws RunNotRunnableError — the worker treats it as settled (no queue retry), the inline path maps it to 409. 3. Permanent chat wedge: with no deadman in inline mode, a process crash left a run non-terminal forever and the single-flight index 409'd every subsequent message to that chat. The single-flight conflict handler now inspects the blocking run: stale heartbeat → expire it (event + terminal write) and take the slot, in the same transaction via a savepoint-wrapped insert (a unique violation no longer poisons the outer tx). A live run still 409s. Proven by a new e2e: a hand-staled zombie blocking a chat is expired and the new turn completes. Reviewer finding #1 (redelivery double-execution) was already closed by the previous commit's heartbeat pickup gate + the deadman fencing; finding #5 (deadman re-enqueue resilience) is accepted as-is — the queue retries the deadman job itself 3x, and the new unwedge path is the ultimate backstop for the failure it worried about. Refs #48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
Contributor
📝 WalkthroughWalkthroughThis PR introduces durable, worker-processed chat runs replacing inline streaming: new runs/run_events/compactions schema and migrations, a pg-boss-backed queue, RunExecutionService/RunsWorkerService, SSE stream resumption (API and web), context compaction, chat title generation, auth hardening (rate limiting, fail-closed sessions, public routes), and extensive e2e/integration test coverage. ChangesDurable Runs & Chat Execution Overhaul
Unrelated UI Package Tweaks
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant ChatLoopService
participant Queue
participant RunsWorkerService
participant RunExecutionService
participant RunEventsRepository
participant Client
ChatLoopService->>Queue: enqueue run job
Queue->>RunsWorkerService: deliver job
RunsWorkerService->>RunExecutionService: executeRun(runId, chatId, userMessage)
RunExecutionService->>RunEventsRepository: append run.started / model.delta events
RunExecutionService->>RunEventsRepository: append run.completed / run.failed
Client->>ChatLoopService: GET /chats/:id/stream (resume)
ChatLoopService->>RunEventsRepository: poll events by sequence cursor
RunEventsRepository-->>Client: SSE replay of run events
sequenceDiagram
participant Client
participant ThrottlerGuard
participant SessionAuthGuard
participant AuthService
participant SessionsRepository
Client->>ThrottlerGuard: HTTP request
ThrottlerGuard->>SessionAuthGuard: forward
SessionAuthGuard->>SessionAuthGuard: check `@Public`() metadata
alt public route
SessionAuthGuard-->>Client: allow
else protected route
SessionAuthGuard->>AuthService: validateToken(token)
AuthService->>SessionsRepository: findActiveAndTouch(tokenHash)
SessionsRepository-->>AuthService: session or undefined
AuthService-->>SessionAuthGuard: user or reject
end
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
e2e/chat/chat-flow.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. e2e/model-server.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. playwright.config.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86994b15df
ℹ️ 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".
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
ROADMAP.md-5-8 (1)
5-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep ROADMAP forward-only.
This status block still describes shipped v0.1 work. That belongs in
CHANGELOG.md;ROADMAP.mdshould stay strictly future-facing.🛠️ Suggested edit
-**Now:** v0.1 (minimal Q&A harness) has shipped — streaming single-model loop, configurable provider, context compaction, and a live-verified eval set (see [CHANGELOG.md](CHANGELOG.md)). **Next:** v0.2, durable runs — every message becomes a worker-processed run.Based on learnings: "Keep
ROADMAP.mdforward-only: it may list only work that is not yet done."🤖 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 `@ROADMAP.md` around lines 5 - 8, Update the status block in ROADMAP.md so it only describes future work: remove the shipped v0.1 summary and any “Now” language, and keep only upcoming items such as v0.2 and tracking references. Use the existing ROADMAP section content as the target, and move any completed-release wording to CHANGELOG.md instead.Source: Learnings
apps/web/lib/services/chat/transport.test.ts-1-23 (1)
1-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore
prepareSendMessagesRequestcoverage
This test file now only covers the URL helpers and reconnect request, soprepareSendMessagesRequestloses unit coverage even though it still powers the chat send path inapps/web/lib/services/chat/transport.ts.🤖 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 `@apps/web/lib/services/chat/transport.test.ts` around lines 1 - 23, The chat transport test file no longer exercises prepareSendMessagesRequest, leaving the send-path helper in transport.ts untested. Add back a unit test alongside buildChatMessagesUrl, buildChatStreamUrl, and prepareReconnectToStreamRequest that calls prepareSendMessagesRequest with a chat id and verifies the generated request targets the chat messages endpoint. Keep the coverage in the existing chat transport describe block so the helper is easy to locate if symbols change.apps/api/src/queue/pgboss-queue.service.ts-118-136 (1)
118-136: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a direct integration test for
cancel()The spec only exercisesstopConsumer()in teardown, where errors are swallowed, and never assertscancel(); add a job-cancellation case here.🤖 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 `@apps/api/src/queue/pgboss-queue.service.ts` around lines 118 - 136, Add a direct integration test for pgboss-queue.service’s cancel() path, since the current spec only hits stopConsumer() in teardown and won’t fail on cancel regressions. Update the existing queue service test suite to explicitly call PgbossQueueService.cancel(queue, jobId) and assert the job is removed/cancelled in the database or via the pg-boss API, using the cancel method name as the anchor. Keep the existing stopConsumer() teardown helper unchanged and add a separate job-cancellation case that verifies cancel() behavior directly.apps/api/test/worker-mode.e2e-spec.ts-290-300 (1)
290-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWait for the run to be executing before testing mid-flight cancel.
latestRun(chatId)can return a queued run, so this test can pass through the “cancelled before start” path instead of proving the abort path works. Poll forstatus === 'running_model'before PATCHing.🤖 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 `@apps/api/test/worker-mode.e2e-spec.ts` around lines 290 - 300, The mid-flight cancel test is using latestRun(chatId), which can return a queued run and accidentally validate the pre-start cancel path instead of the abort path. In the worker-mode e2e spec, update the wait logic around latestRun/chatId to poll until the run status is running_model before issuing the PATCH to /api/v1/runs/:id, so the test only cancels an executing run.apps/api/openapi.json-626-756 (1)
626-756: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd security requirements for the new run endpoints.
The run routes document
401but omit thesecurityblocks present on/api/v1/chats/{id}/stream. Generated clients may treat these authenticated endpoints as public. Prefer fixing the source Swagger decorators and regenerating this file.🤖 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 `@apps/api/openapi.json` around lines 626 - 756, The new run endpoints are missing explicit auth metadata even though they return 401, so generated clients may treat them as public. Add the same security requirements used by the authenticated chat stream route to the Swagger decorators for RunsController_getRun, RunsController_updateRun, and RunsController_streamRunEvents, then regenerate the openapi spec so the /api/v1/runs/{id} and /api/v1/runs/{id}/events entries include the security blocks.apps/api/test/qa-evals.e2e-spec.ts-98-127 (1)
98-127: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore
COMPACTION_TOKEN_THRESHOLDafter evals.This suite forces a low threshold before app bootstrap but leaves it in
process.env, which can change behavior for later tests in the same process.Suggested fix
const tag = Date.now(); let cookie = ''; let userId = ''; + let previousCompactionTokenThreshold: string | undefined; beforeAll(async () => { + previousCompactionTokenThreshold = process.env.COMPACTION_TOKEN_THRESHOLD; // Force an aggressive compaction threshold BEFORE the app reads config, so the @@ afterAll(async () => { await app?.close(); + if (previousCompactionTokenThreshold === undefined) { + delete process.env.COMPACTION_TOKEN_THRESHOLD; + } else { + process.env.COMPACTION_TOKEN_THRESHOLD = + previousCompactionTokenThreshold; + } });🤖 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 `@apps/api/test/qa-evals.e2e-spec.ts` around lines 98 - 127, The eval setup in qa-evals.e2e-spec.ts sets COMPACTION_TOKEN_THRESHOLD in beforeAll but never restores it, so later tests can inherit the override. Update the qa-evals suite to save the previous process.env.COMPACTION_TOKEN_THRESHOLD value before assigning '300', and restore or delete it in afterAll after app.close(); use the beforeAll/afterAll hooks in the qa-evals.e2e-spec.ts setup to keep the override scoped to this suite.apps/api/test/worker-mode.e2e-spec.ts-172-209 (1)
172-209: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore previous env values after the suite.
Deleting these variables can pollute later tests in the same Jest process when the caller had them set already.
Suggested fix
let userId = ''; + let previousEnv: Partial<Record<string, string | undefined>>; beforeAll(async () => { + previousEnv = { + RUN_EXECUTION_MODE: process.env.RUN_EXECUTION_MODE, + RUN_TIMEOUT_SECONDS: process.env.RUN_TIMEOUT_SECONDS, + RUN_HEARTBEAT_STALE_SECONDS: + process.env.RUN_HEARTBEAT_STALE_SECONDS, + RUN_HEARTBEAT_SECONDS: process.env.RUN_HEARTBEAT_SECONDS, + }; process.env.RUN_EXECUTION_MODE = 'worker'; @@ afterAll(async () => { - delete process.env.RUN_EXECUTION_MODE; - delete process.env.RUN_TIMEOUT_SECONDS; - delete process.env.RUN_HEARTBEAT_STALE_SECONDS; - delete process.env.RUN_HEARTBEAT_SECONDS; await app?.close(); + for (const [key, value] of Object.entries(previousEnv)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } });🤖 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 `@apps/api/test/worker-mode.e2e-spec.ts` around lines 172 - 209, The worker-mode e2e setup is mutating RUN_EXECUTION_MODE, RUN_TIMEOUT_SECONDS, RUN_HEARTBEAT_STALE_SECONDS, and RUN_HEARTBEAT_SECONDS and then deleting them unconditionally, which can clobber pre-existing test env state. In worker-mode.e2e-spec.ts, update the beforeAll/afterAll block to capture the original values before setting them and restore those exact values after the suite instead of deleting them. Use the existing beforeAll/afterAll hooks and the process.env assignments in this spec as the place to implement the save/restore logic.apps/api/src/auth/auth.service.ts-112-117 (1)
112-117: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep stale-session cleanup best-effort.
This cleanup is not part of the authorization decision, but any delete failure currently rejects
validateTokeninstead of returningundefinedfor the invalid token. Swallow or log cleanup failures so expired/unknown tokens still produce the intended unauthenticated result.Proposed fix
if (!session?.userId?.trim()) { // Housekeeping, not authorization: an expired/idle row serves no one. - await this.sessionsRepository.deleteStaleByTokenHash( - tokenHash, - SESSION_IDLE_TTL_MS, - ); + try { + await this.sessionsRepository.deleteStaleByTokenHash( + tokenHash, + SESSION_IDLE_TTL_MS, + ); + } catch { + // Best-effort cleanup must not change the invalid-token auth result. + } return undefined; }🤖 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 `@apps/api/src/auth/auth.service.ts` around lines 112 - 117, In validateToken, the stale-session cleanup path under the !session?.userId?.trim() check should be best-effort and must not fail the auth flow. Wrap the this.sessionsRepository.deleteStaleByTokenHash call in a try/catch (or equivalent) and swallow or log any cleanup error so the method still returns undefined for expired/unknown tokens. Keep the change scoped to validateToken and the deleteStaleByTokenHash cleanup branch.apps/api/src/auth/constants.ts-17-19 (1)
17-19: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequire an integer throttle limit.
AUTH_RATE_LIMIT_PER_MINUTE=0.5currently passes validation even though request limits are counts. Reject non-integers to avoid surprising auth endpoint lockouts or ineffective configuration.Proposed fix
export const AUTH_RATE_LIMIT_PER_MINUTE = (() => { const raw = Number(process.env.AUTH_RATE_LIMIT_PER_MINUTE); - return Number.isFinite(raw) && raw > 0 ? raw : 10; + return Number.isInteger(raw) && raw > 0 ? raw : 10; })();🤖 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 `@apps/api/src/auth/constants.ts` around lines 17 - 19, The AUTH_RATE_LIMIT_PER_MINUTE validation in the AUTH_RATE_LIMIT_PER_MINUTE initializer currently accepts fractional values because it only checks Number.isFinite and > 0. Update this logic to require a whole number by also validating that the parsed value is an integer, and keep the existing fallback behavior for invalid input; use the AUTH_RATE_LIMIT_PER_MINUTE constant block as the place to apply the fix.apps/api/src/auth/sessions.repository.ts-106-112 (1)
106-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter idle sessions out of session listings.
findActiveAndTouchrejects idle sessions, butlistForUseronly filters byexpires, so/auth/v1/sessionscan show sessions that are no longer usable until hourly cleanup runs. Pass the idle TTL into this query and filterlastSeenAtconsistently.Proposed fix
- async listForUser(userId: string): Promise<SessionRecord[]> { + async listForUser( + userId: string, + idleTtlMs: number, + ): Promise<SessionRecord[]> { + const now = Date.now(); return this.db .select() .from(sessions) - .where(and(eq(sessions.userId, userId), gt(sessions.expires, new Date()))) + .where( + and( + eq(sessions.userId, userId), + gt(sessions.expires, new Date(now)), + gt(sessions.lastSeenAt, new Date(now - idleTtlMs)), + ), + ) .orderBy(desc(sessions.createdAt)); }🤖 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 `@apps/api/src/auth/sessions.repository.ts` around lines 106 - 112, The listForUser query in SessionsRepository only filters by expires, so it can return idle sessions that findActiveAndTouch would reject. Update listForUser to also apply the same lastSeenAt idle-TTL cutoff used for active session validation, passing the idle TTL into this method and filtering sessions consistently alongside the existing expires check in the sessions repository logic.
🧹 Nitpick comments (5)
apps/api/src/queue/queue.integration.spec.ts (1)
47-213: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
Queue.cancel().The suite covers enqueue/consume/retry/DLQ/cron/deferred delivery but never exercises
cancel(), which is part of the publicQueuecontract and (per the PR's run-cancellation work) is on a critical path.✅ Suggested additional test
it('cancels a queued job before it is consumed', async () => { const name = `${tag}-cancel`; await queue.ensureQueue(name); const received: unknown[] = []; const jobId = await queue.enqueue(name, { should: 'not-run' }, { startAfter: 5 }); await queue.cancel(name, jobId as string); await consume(name, (data) => { received.push(data); return Promise.resolve(); }); await new Promise((resolve) => setTimeout(resolve, 6_000)); expect(received).toHaveLength(0); });🤖 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 `@apps/api/src/queue/queue.integration.spec.ts` around lines 47 - 213, Add integration coverage for Queue.cancel() in the existing queue test suite so the public contract is exercised alongside enqueue/consume/retry/DLQ/cron/deferred flows. Create a new test near the other cases in the describe block that enqueues a delayed job, captures the returned job id from queue.enqueue, calls queue.cancel(name, jobId), then sets up a consumer and verifies the payload is never delivered. Use the existing tag, consume helper, and Queue interface methods to keep the test consistent and easy to locate.apps/api/src/chats/run-stream-bridge.ts (1)
166-176: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExtra
findByIdquery on every idle poll cycle.While waiting for the next event (the common steady-state case), each ~200ms tick issues both
listByRunIdand a follow-upfindByIdjust to check the run still exists. Over a multi-minute stream this doubles DB round-trips for a check that only needs to catch the rare "terminal without a terminal event" edge case.♻️ Proposed fix: only re-check run existence every N empty polls
+ let emptyPolls = 0; if (events.length === 0) { - const run = await tenantDb.runAs(input.userId, (tx) => - new RunsRepository(tx).findById(input.runId, input.userId), - ); - if (!run) { - break; - } + emptyPolls += 1; + if (emptyPolls % 25 === 0) { + const run = await tenantDb.runAs(input.userId, (tx) => + new RunsRepository(tx).findById(input.runId, input.userId), + ); + if (!run) { + break; + } + } await sleep(POLL_MS); }🤖 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 `@apps/api/src/chats/run-stream-bridge.ts` around lines 166 - 176, The idle polling path in run-stream-bridge is doing an extra RunsRepository.findById lookup on every empty events cycle, which doubles DB traffic in the common steady state. Update the polling loop in run-stream-bridge to only re-check run existence after N consecutive empty polls (or another throttled cadence) while keeping the existing terminal-without-event safeguard, and reuse the existing listByRunId/events.length logic to avoid redundant tenantDb.runAs lookups.apps/api/src/chats/chats.module.ts (1)
20-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated
RunsModuleper the one-module-per-feature guideline.
RunsControllerplus the run/compaction/title providers are all bundled intoChatsModule. As per path instructions, "Use one NestJS module per feature, wire providers/controllers through dependency injection, and register feature modules insrc/app.module.ts." Splitting Runs (and possibly Compaction/Title) into their own module(s) registered inapp.module.tswould better match this convention, though the current grouping is defensible given how tightly runs are coupled to chats.🤖 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 `@apps/api/src/chats/chats.module.ts` around lines 20 - 33, The ChatsModule currently bundles the RunsController and several run-related providers, which conflicts with the one-module-per-feature guideline. Extract the run-specific pieces into a dedicated RunsModule (including RunsController, RunExecutionService, RunStreamBridgeService, RunsWorkerService, RunAbortRegistry, and any related compaction/title services if appropriate), then import that feature module from the app-level module instead of wiring them through ChatsModule.Source: Path instructions
apps/api/src/db/migrations/0013_round_expediter.sql (1)
1-1: 🚀 Performance & Scalability | 🔵 TrivialConsider
CONCURRENTLYfor this index ifrunssees production traffic.A plain
CREATE UNIQUE INDEXtakes a table lock that blocks writes torunsfor the duration of the build. If this migration runs against a populated table, preferCREATE UNIQUE INDEX CONCURRENTLY(requires running outside a transaction block).🤖 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 `@apps/api/src/db/migrations/0013_round_expediter.sql` at line 1, The new unique index on runs should be built without blocking writes in production. Update the migration to create the runs_chat_inflight_unique index using CONCURRENTLY, and ensure the migration is structured to run outside a transaction block since concurrent index creation is not allowed inside one. Refer to the index definition in the migration itself when making the change.apps/api/src/db/migrations/meta/0011_snapshot.json (1)
799-1075: 🔒 Security & Privacy | 🔵 TrivialRun the RLS test suite for these new tables.
This snapshot introduces RLS-enabled
runsandrun_eventstables with owner-scoping policies keyed onuser_id. Based on learnings, runscripts/rls-test.shafter touching the schema, RLS,TenantDbService, or the auth/HTTP surface — please confirm it has been run (or is wired into CI) for this new surface.🤖 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 `@apps/api/src/db/migrations/meta/0011_snapshot.json` around lines 799 - 1075, Add RLS coverage for the new runs and run_events snapshot by running scripts/rls-test.sh and ensuring the owner-scoping policies in runs_owner and run_events_owner are exercised. If this test is not already part of CI for schema/RLS changes, wire it in so future updates to this surface automatically validate the current user_id-based access rules.Source: Learnings
🤖 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 `@apps/api/src/app.setup.ts`:
- Around line 34-43: Reject non-integer TRUST_PROXY values in
getTrustProxySetting, since Number.isFinite() currently allows decimals and
scientific notation that Express will treat as hop counts. Update the
parsing/validation in getTrustProxySetting so only plain non-negative integers
are accepted, and otherwise return undefined or fail startup on invalid
configuration; keep the behavior localized around the TRUST_PROXY handling in
app.setup.ts.
In `@apps/api/src/chats/compaction.service.ts`:
- Around line 130-153: Serialize compactions per chat in CompactionService so
overlapping fire-and-forget runs cannot both create rows. The current staleness
check in the runAs block only rejects after another compaction has already
committed, so add per-chat locking/serialization around the compaction flow
(around the latest lookup and create in the compaction service) or enforce a
unique/locking constraint in CompactionsRepository/compactions to guarantee only
one active compaction per chat.
In `@apps/api/src/chats/run-execution.service.ts`:
- Around line 248-263: The fire-and-forget post-turn tasks in
run-execution.service.ts are missing rejection handling, so failed compaction or
title generation can surface as unhandled promise rejections. Update the
telemetry.status === 'completed' block in RunExecutionService to keep the
non-blocking behavior but attach explicit .catch() handlers (or equivalent
guarded logging) to both this.compaction.maybeCompact and
this.titles.maybeGenerateTitle so failures are swallowed and recorded without
affecting the turn or worker.
- Around line 206-246: The `onFinish` handler in `run-execution.service.ts` maps
every non-completed turn to `cancelled`, which hides model errors that arrive
with `finishReason === 'error'`. Update the status derivation in `onFinish` to
mirror the `onError` mapping: keep `completed` as-is, treat `aborted` as
`cancelled`, and map error finishes to `failed`. Then ensure the recorded
terminal event and `RunsRepository.markFinished` use the corrected status so
`run.failed` is emitted and the UI can surface the error.
In `@apps/api/src/chats/run-stream-bridge.ts`:
- Around line 138-180: The timeout path in runStreamBridge currently breaks out
of the loop and sends [DONE] without a terminal translator chunk, which can
leave the UI streaming state open. Update the bridge flow in runStreamBridge so
that when the MAX_STREAM_MS limit is hit it emits a terminal chunk through
translator.translate or otherwise finalizes the stream with the same
text-end/finish sequence used on normal completion before
controller.enqueue('[DONE]'). Keep the change localized to the runStreamBridge
loop and translator.finished handling.
In `@apps/api/src/chats/runs-worker.service.ts`:
- Around line 132-145: The terminal event is being appended before the guarded
terminal-state update wins, which can leave stale run.failed/run.cancelled
events in replay. In runs-worker.service.ts, update the run via
markFinished(...) first in the affected branches, then only call
RunEventsRepository.append(...) if the transition succeeds, following the same
“transition first, append only if updated” pattern used around RunsRepository
and RunEventsRepository. Apply this to the stale-heartbeat expiration path and
the other cancellation paths referenced by the review so that only the executor
that actually wins the terminal transition writes the terminal event.
In `@apps/api/src/chats/runs.controller.ts`:
- Around line 174-193: The streaming loop in RunsController is sending the
terminal sentinel even when it exits due to the max stream timeout, which can
stop clients from reconnecting with their cursor. Update the logic around the
timeout break and the final response.write('data: [DONE]\n\n') so that [DONE] is
only sent when a run is truly terminal or deleted, and not when the stream is
capped but still active. Use the existing flags and symbols in this method,
especially terminalSeen, MAX_STREAM_MS, and the current run lookup via
RunsRepository.findById, to decide whether to close with [DONE] or simply end
the capped stream.
In `@apps/api/src/db/migrations/0014_reply_integrity_trigger.sql`:
- Around line 27-29: The reply integrity migration only validates rows being
inserted or updated as replies, but it does not protect the target message later
from changing in a way that breaks existing references. Update the migration
around assert_message_reply_integrity and messages_reply_integrity to add a new
trigger/function pair that checks any message being updated when it is
referenced by child rows via in_reply_to, and reject changes to role or chat_id
unless the message stays a user message in the same chat. Use a new function
like assert_message_reply_target_integrity and a BEFORE UPDATE OF role, chat_id
trigger on messages to enforce this invariant.
In `@apps/api/src/db/schema/chats.ts`:
- Around line 249-279: The `runEvents` table in `chats.ts` is only protected by
the `run_events_owner` RLS policy, but that still allows future writers with
table privileges to update or delete owned rows. Add a DB-level append-only
guard for `run_events` in the schema, using either command-specific RLS policies
or a `BEFORE UPDATE OR DELETE` trigger that raises on mutation, and keep
`runEvents` as the symbol to update. After changing the schema, regenerate the
migration with `db:generate`.
- Around line 139-142: The compaction lineage on parentId only checks that a
parent compaction exists, not that it belongs to the same chat as the child, so
update the chats schema around compactions.parentId to enforce same-chat
ancestry. Add a database-level constraint in the compactions/chatId lineage path
that verifies parent.chat_id matches NEW.chat_id, using the appropriate
composite foreign key or a trigger/check mechanism if the relation cannot be
expressed directly in the schema. Regenerate migrations after updating the
schema definition so the new isolation rule is applied consistently.
- Around line 203-207: The runs schema currently only validates that chatId
exists, so add a database-level constraint or trigger in the runs/chats schema
to enforce that runs.userId always matches chats.ownerUserId for the referenced
chat. Also enforce that any messageId on a run belongs to the same chatId, using
the existing runs table fields and chats/users ownership symbols so the
invariant is enforced even if an internal writer regresses. Update the schema
truth in src/db/schema and regenerate the migration after adding the
constraint/trigger.
In `@apps/api/src/models/fake-model-client.ts`:
- Around line 60-95: The fake stream client eagerly creates textStream in
createFakeStreamTextResult, which can trigger finish() before the stream is
actually consumed. Make textStream lazy like text by deferring
createTextStream(...) until the textStream property is accessed, preserving the
consumption-driven timing in createFakeStreamTextResult/createTextStream.
---
Minor comments:
In `@apps/api/openapi.json`:
- Around line 626-756: The new run endpoints are missing explicit auth metadata
even though they return 401, so generated clients may treat them as public. Add
the same security requirements used by the authenticated chat stream route to
the Swagger decorators for RunsController_getRun, RunsController_updateRun, and
RunsController_streamRunEvents, then regenerate the openapi spec so the
/api/v1/runs/{id} and /api/v1/runs/{id}/events entries include the security
blocks.
In `@apps/api/src/auth/auth.service.ts`:
- Around line 112-117: In validateToken, the stale-session cleanup path under
the !session?.userId?.trim() check should be best-effort and must not fail the
auth flow. Wrap the this.sessionsRepository.deleteStaleByTokenHash call in a
try/catch (or equivalent) and swallow or log any cleanup error so the method
still returns undefined for expired/unknown tokens. Keep the change scoped to
validateToken and the deleteStaleByTokenHash cleanup branch.
In `@apps/api/src/auth/constants.ts`:
- Around line 17-19: The AUTH_RATE_LIMIT_PER_MINUTE validation in the
AUTH_RATE_LIMIT_PER_MINUTE initializer currently accepts fractional values
because it only checks Number.isFinite and > 0. Update this logic to require a
whole number by also validating that the parsed value is an integer, and keep
the existing fallback behavior for invalid input; use the
AUTH_RATE_LIMIT_PER_MINUTE constant block as the place to apply the fix.
In `@apps/api/src/auth/sessions.repository.ts`:
- Around line 106-112: The listForUser query in SessionsRepository only filters
by expires, so it can return idle sessions that findActiveAndTouch would reject.
Update listForUser to also apply the same lastSeenAt idle-TTL cutoff used for
active session validation, passing the idle TTL into this method and filtering
sessions consistently alongside the existing expires check in the sessions
repository logic.
In `@apps/api/src/queue/pgboss-queue.service.ts`:
- Around line 118-136: Add a direct integration test for pgboss-queue.service’s
cancel() path, since the current spec only hits stopConsumer() in teardown and
won’t fail on cancel regressions. Update the existing queue service test suite
to explicitly call PgbossQueueService.cancel(queue, jobId) and assert the job is
removed/cancelled in the database or via the pg-boss API, using the cancel
method name as the anchor. Keep the existing stopConsumer() teardown helper
unchanged and add a separate job-cancellation case that verifies cancel()
behavior directly.
In `@apps/api/test/qa-evals.e2e-spec.ts`:
- Around line 98-127: The eval setup in qa-evals.e2e-spec.ts sets
COMPACTION_TOKEN_THRESHOLD in beforeAll but never restores it, so later tests
can inherit the override. Update the qa-evals suite to save the previous
process.env.COMPACTION_TOKEN_THRESHOLD value before assigning '300', and restore
or delete it in afterAll after app.close(); use the beforeAll/afterAll hooks in
the qa-evals.e2e-spec.ts setup to keep the override scoped to this suite.
In `@apps/api/test/worker-mode.e2e-spec.ts`:
- Around line 290-300: The mid-flight cancel test is using latestRun(chatId),
which can return a queued run and accidentally validate the pre-start cancel
path instead of the abort path. In the worker-mode e2e spec, update the wait
logic around latestRun/chatId to poll until the run status is running_model
before issuing the PATCH to /api/v1/runs/:id, so the test only cancels an
executing run.
- Around line 172-209: The worker-mode e2e setup is mutating RUN_EXECUTION_MODE,
RUN_TIMEOUT_SECONDS, RUN_HEARTBEAT_STALE_SECONDS, and RUN_HEARTBEAT_SECONDS and
then deleting them unconditionally, which can clobber pre-existing test env
state. In worker-mode.e2e-spec.ts, update the beforeAll/afterAll block to
capture the original values before setting them and restore those exact values
after the suite instead of deleting them. Use the existing beforeAll/afterAll
hooks and the process.env assignments in this spec as the place to implement the
save/restore logic.
In `@apps/web/lib/services/chat/transport.test.ts`:
- Around line 1-23: The chat transport test file no longer exercises
prepareSendMessagesRequest, leaving the send-path helper in transport.ts
untested. Add back a unit test alongside buildChatMessagesUrl,
buildChatStreamUrl, and prepareReconnectToStreamRequest that calls
prepareSendMessagesRequest with a chat id and verifies the generated request
targets the chat messages endpoint. Keep the coverage in the existing chat
transport describe block so the helper is easy to locate if symbols change.
In `@ROADMAP.md`:
- Around line 5-8: Update the status block in ROADMAP.md so it only describes
future work: remove the shipped v0.1 summary and any “Now” language, and keep
only upcoming items such as v0.2 and tracking references. Use the existing
ROADMAP section content as the target, and move any completed-release wording to
CHANGELOG.md instead.
---
Nitpick comments:
In `@apps/api/src/chats/chats.module.ts`:
- Around line 20-33: The ChatsModule currently bundles the RunsController and
several run-related providers, which conflicts with the one-module-per-feature
guideline. Extract the run-specific pieces into a dedicated RunsModule
(including RunsController, RunExecutionService, RunStreamBridgeService,
RunsWorkerService, RunAbortRegistry, and any related compaction/title services
if appropriate), then import that feature module from the app-level module
instead of wiring them through ChatsModule.
In `@apps/api/src/chats/run-stream-bridge.ts`:
- Around line 166-176: The idle polling path in run-stream-bridge is doing an
extra RunsRepository.findById lookup on every empty events cycle, which doubles
DB traffic in the common steady state. Update the polling loop in
run-stream-bridge to only re-check run existence after N consecutive empty polls
(or another throttled cadence) while keeping the existing terminal-without-event
safeguard, and reuse the existing listByRunId/events.length logic to avoid
redundant tenantDb.runAs lookups.
In `@apps/api/src/db/migrations/0013_round_expediter.sql`:
- Line 1: The new unique index on runs should be built without blocking writes
in production. Update the migration to create the runs_chat_inflight_unique
index using CONCURRENTLY, and ensure the migration is structured to run outside
a transaction block since concurrent index creation is not allowed inside one.
Refer to the index definition in the migration itself when making the change.
In `@apps/api/src/db/migrations/meta/0011_snapshot.json`:
- Around line 799-1075: Add RLS coverage for the new runs and run_events
snapshot by running scripts/rls-test.sh and ensuring the owner-scoping policies
in runs_owner and run_events_owner are exercised. If this test is not already
part of CI for schema/RLS changes, wire it in so future updates to this surface
automatically validate the current user_id-based access rules.
In `@apps/api/src/queue/queue.integration.spec.ts`:
- Around line 47-213: Add integration coverage for Queue.cancel() in the
existing queue test suite so the public contract is exercised alongside
enqueue/consume/retry/DLQ/cron/deferred flows. Create a new test near the other
cases in the describe block that enqueues a delayed job, captures the returned
job id from queue.enqueue, calls queue.cancel(name, jobId), then sets up a
consumer and verifies the payload is never delivered. Use the existing tag,
consume helper, and Queue interface methods to keep the test consistent and easy
to locate.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d9ad8d2-974d-482b-b1c3-586c741a7cfd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (88)
.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdROADMAP.mdVISION.mdapps/api/.env.exampleapps/api/AGENTS.mdapps/api/openapi.jsonapps/api/package.jsonapps/api/src/app.controller.tsapps/api/src/app.module.tsapps/api/src/app.setup.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/auth.service.spec.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/constants.tsapps/api/src/auth/public.decorator.tsapps/api/src/auth/session-auth.guard.spec.tsapps/api/src/auth/session-auth.guard.tsapps/api/src/auth/session-cleanup.service.tsapps/api/src/auth/sessions.repository.tsapps/api/src/chats/chat-loop.service.tsapps/api/src/chats/chats-repository.spec.tsapps/api/src/chats/chats-repository.tsapps/api/src/chats/chats-rls.integration.spec.tsapps/api/src/chats/chats.controller.spec.tsapps/api/src/chats/chats.controller.tsapps/api/src/chats/chats.module.tsapps/api/src/chats/compaction.service.tsapps/api/src/chats/compaction.spec.tsapps/api/src/chats/compaction.tsapps/api/src/chats/context-builder.spec.tsapps/api/src/chats/context-builder.tsapps/api/src/chats/delta-buffer.spec.tsapps/api/src/chats/delta-buffer.tsapps/api/src/chats/dto/runs.dto.tsapps/api/src/chats/run-abort-registry.tsapps/api/src/chats/run-execution.service.tsapps/api/src/chats/run-stream-bridge.spec.tsapps/api/src/chats/run-stream-bridge.tsapps/api/src/chats/runs-repository.tsapps/api/src/chats/runs-worker.service.tsapps/api/src/chats/runs.controller.tsapps/api/src/chats/title.service.tsapps/api/src/chats/title.spec.tsapps/api/src/chats/title.tsapps/api/src/db/migrations/0009_cool_lenny_balinger.sqlapps/api/src/db/migrations/0010_cuddly_reavers.sqlapps/api/src/db/migrations/0011_puzzling_ricochet.sqlapps/api/src/db/migrations/0012_nebulous_lila_cheney.sqlapps/api/src/db/migrations/0013_round_expediter.sqlapps/api/src/db/migrations/0014_reply_integrity_trigger.sqlapps/api/src/db/migrations/0015_clean_human_fly.sqlapps/api/src/db/migrations/meta/0009_snapshot.jsonapps/api/src/db/migrations/meta/0010_snapshot.jsonapps/api/src/db/migrations/meta/0011_snapshot.jsonapps/api/src/db/migrations/meta/0012_snapshot.jsonapps/api/src/db/migrations/meta/0013_snapshot.jsonapps/api/src/db/migrations/meta/0014_snapshot.jsonapps/api/src/db/migrations/meta/0015_snapshot.jsonapps/api/src/db/migrations/meta/_journal.jsonapps/api/src/db/schema/auth.tsapps/api/src/db/schema/chats.tsapps/api/src/models/fake-model-client.tsapps/api/src/models/model-client.spec.tsapps/api/src/models/model-client.tsapps/api/src/models/models.service.spec.tsapps/api/src/models/models.service.tsapps/api/src/models/openai-model-client.tsapps/api/src/queue/pgboss-queue.service.tsapps/api/src/queue/queue.integration.spec.tsapps/api/src/queue/queue.module.tsapps/api/src/queue/queue.tsapps/api/test/auth.e2e-spec.tsapps/api/test/chats-messages.e2e-spec.tsapps/api/test/jest-e2e.jsonapps/api/test/qa-evals.e2e-spec.tsapps/api/test/worker-mode.e2e-spec.tsapps/web/app/(chat)/components/chat-page.tsxapps/web/lib/services/chat/transport.test.tsapps/web/lib/services/chat/transport.tse2e/chat/chat-flow.spec.tse2e/model-server.tspackages/ui/package.jsonpackages/ui/src/components/code-block.tsxpackages/ui/types/ua-client-hints.d.tsplaywright.config.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
16 issues found across 89 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This was referenced Jul 2, 2026
leon0399
added a commit
that referenced
this pull request
Jul 3, 2026
Review findings on the rebased #105: - ensureQueue assumed pg-boss createQueue() upserts, but in v12 it is INSERT ... ON CONFLICT DO NOTHING — a redeploy with changed QueueOptions silently kept the old policy forever. ensureQueue now follows createQueue with updateQueue (COALESCE per passed field), so it is a real idempotent policy apply on every boot; proven by a new integration case (re-ensure with a different retryLimit reflects in the stored queue). - cancel() had no direct coverage (CodeRabbit, umbrella #101): a new case enqueues a deferred job, cancels it, and asserts the stored state is 'cancelled' before delivery was ever possible. - The spec's private waitFor poll helper moved to test/support.ts as the shared copy (4th reimplementation in the tree; the other three suites migrate separately). Integration suite: 7/7 against a throwaway Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
leon0399
added a commit
that referenced
this pull request
Jul 3, 2026
/code-review findings on the rebased #106, plus surviving items from the #106/#101 review threads: - extracted finalizeRun with a first-writer-wins gate: onError and onFinish can in principle both fire for one stream; markFinished's finished_at guard protects the runs ROW but the append-only event log had no guard — two contradictory terminal events were possible. Also dedupes the drain-then-finalize sequence the two callbacks had copy-pasted. - a synchronous throw from client.streamText (provider/config validation before any callback fires) stranded the run at 'running_model' forever; the call is now wrapped and finalizes the run as failed before rethrowing. - runs.controller wraps the post-headers SSE loop in try/catch: DB errors mid-poll can't reach the exception filter on a flushed stream (from #101 review). - CHANGELOG: removed merge-conflict markers committed by the previous rebase pass (duplicate/contradictory bullets deduped, the #48/#49 replay bullet moved to the ship-date section). - documented the concurrent-duplicate-run gap at the creation site: the real fix is per-chat single-flight, deliberately deferred to the heartbeat slice of #48 (without heartbeat a crashed run deadlocks its chat). Deferred with rationale (not fixed here): lost terminal dual-writes leave a run non-terminal until the deadman sweep lands (later #48 slice) — recordRunProgress swallowing failures is by design so the live stream never breaks. Verified: tsgo + oxlint clean, unit 117, full rls-test.sh green (RLS 11, queue 7, e2e 27). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
leon0399
added a commit
that referenced
this pull request
Jul 3, 2026
…tence /code-review (high) + PR #107/#101 thread triage on the rebased branch: - markStarted is now a real check-and-claim: it refuses a run already at running_model unless the caller opts into crash recovery (reclaimStaleMs) AND the last sign of life is older than that window — the claim stamps a fresh heartbeat in the same UPDATE, so two consumers racing to reclaim a stale run can never both win. Prevents a duplicate concurrent model call (double provider spend) when a heartbeat write merely lagged. Worker redelivery passes the window; inline mode never reclaims. - assistant persistence is decoupled from run bookkeeping: finishRun returns a tri-state (won / lost+finalStatus / errored) — a transient DB error in bookkeeping no longer drops a fully streamed reply from history, and losing the terminal claim to the deadman ('expired', a liveness misjudgment) persists the turn anyway; only an intentional terminal state written by someone else (cancelled = user stop or supersede, or a dual-fire that already recorded it) suppresses it. - both expiry paths (deadman sweep, single-flight unwedge) now append 'run.expired' — the RunEventType invariant's own type, previously unused while the log said run.failed — and the stream-bridge translator surfaces it; all terminal writers outside finishRun now markFinished FIRST and append the event only when they won the transition (deadman, pickup-gate cancel, cancelled-meanwhile, unwedge), so a racing finish can't leave a contradictory terminal event. - migration 0012's backfill now appends matching run.cancelled events for the runs it cancels (CTE UPDATE…RETURNING → INSERT) — the invariant holds for historical rows too. - the run job and its deadman timeout job enqueue in parallel (no ordering dependency; a timeout job for a failed run-enqueue just expires the orphan sooner); the enqueue-not-transactional design gap and its self-heal story are documented at the site (#48 constraint 1). Rejected with rationale: making the run claim best-effort again — it is now load-bearing for single-execution exclusivity and deliberately fails the turn instead of double-running it. Verified: tsgo + oxlint + prettier clean, unit 125, nest build + openapi, full rls-test.sh green (RLS 11, queue 7, e2e 34). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
leon0399
added a commit
that referenced
this pull request
Jul 3, 2026
…tence /code-review (high) + PR #107/#101 thread triage on the rebased branch: - markStarted is now a real check-and-claim: it refuses a run already at running_model unless the caller opts into crash recovery (reclaimStaleMs) AND the last sign of life is older than that window — the claim stamps a fresh heartbeat in the same UPDATE, so two consumers racing to reclaim a stale run can never both win. Prevents a duplicate concurrent model call (double provider spend) when a heartbeat write merely lagged. Worker redelivery passes the window; inline mode never reclaims. - assistant persistence is decoupled from run bookkeeping: finishRun returns a tri-state (won / lost+finalStatus / errored) — a transient DB error in bookkeeping no longer drops a fully streamed reply from history, and losing the terminal claim to the deadman ('expired', a liveness misjudgment) persists the turn anyway; only an intentional terminal state written by someone else (cancelled = user stop or supersede, or a dual-fire that already recorded it) suppresses it. - both expiry paths (deadman sweep, single-flight unwedge) now append 'run.expired' — the RunEventType invariant's own type, previously unused while the log said run.failed — and the stream-bridge translator surfaces it; all terminal writers outside finishRun now markFinished FIRST and append the event only when they won the transition (deadman, pickup-gate cancel, cancelled-meanwhile, unwedge), so a racing finish can't leave a contradictory terminal event. - migration 0012's backfill now appends matching run.cancelled events for the runs it cancels (CTE UPDATE…RETURNING → INSERT) — the invariant holds for historical rows too. - the run job and its deadman timeout job enqueue in parallel (no ordering dependency; a timeout job for a failed run-enqueue just expires the orphan sooner); the enqueue-not-transactional design gap and its self-heal story are documented at the site (#48 constraint 1). Rejected with rationale: making the run claim best-effort again — it is now load-bearing for single-execution exclusivity and deliberately fails the turn instead of double-running it. Verified: tsgo + oxlint + prettier clean, unit 125, nest build + openapi, full rls-test.sh green (RLS 11, queue 7, e2e 34). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
leon0399
added a commit
that referenced
this pull request
Jul 4, 2026
…tence /code-review (high) + PR #107/#101 thread triage on the rebased branch: - markStarted is now a real check-and-claim: it refuses a run already at running_model unless the caller opts into crash recovery (reclaimStaleMs) AND the last sign of life is older than that window — the claim stamps a fresh heartbeat in the same UPDATE, so two consumers racing to reclaim a stale run can never both win. Prevents a duplicate concurrent model call (double provider spend) when a heartbeat write merely lagged. Worker redelivery passes the window; inline mode never reclaims. - assistant persistence is decoupled from run bookkeeping: finishRun returns a tri-state (won / lost+finalStatus / errored) — a transient DB error in bookkeeping no longer drops a fully streamed reply from history, and losing the terminal claim to the deadman ('expired', a liveness misjudgment) persists the turn anyway; only an intentional terminal state written by someone else (cancelled = user stop or supersede, or a dual-fire that already recorded it) suppresses it. - both expiry paths (deadman sweep, single-flight unwedge) now append 'run.expired' — the RunEventType invariant's own type, previously unused while the log said run.failed — and the stream-bridge translator surfaces it; all terminal writers outside finishRun now markFinished FIRST and append the event only when they won the transition (deadman, pickup-gate cancel, cancelled-meanwhile, unwedge), so a racing finish can't leave a contradictory terminal event. - migration 0012's backfill now appends matching run.cancelled events for the runs it cancels (CTE UPDATE…RETURNING → INSERT) — the invariant holds for historical rows too. - the run job and its deadman timeout job enqueue in parallel (no ordering dependency; a timeout job for a failed run-enqueue just expires the orphan sooner); the enqueue-not-transactional design gap and its self-heal story are documented at the site (#48 constraint 1). Rejected with rationale: making the run claim best-effort again — it is now load-bearing for single-execution exclusivity and deliberately fails the turn instead of double-running it. Verified: tsgo + oxlint + prettier clean, unit 125, nest build + openapi, full rls-test.sh green (RLS 11, queue 7, e2e 34). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
This was referenced Jul 4, 2026
Owner
Author
|
Superseded and fully absorbed by the replacement stack — every change in this umbrella reached master through the reviewed slices: #104 (v0.1 completion), #105 (pg-boss queue substrate, #47), #106 (durable-run substrate, #48 part 1), #107 (worker run execution, #48/#50), #108 (streaming-loop hardening, #73), #109 (auth hardening, #68), #110 (chat stream resume + browser e2e, #49/#80). All review threads here were individually triaged: fixed in the slices, refuted with evidence, or tracked in #125. Closing without merge. |
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.
Overnight experiment: v0.1 completion + v0.2 durable-run pipeline
One night of autonomous 30-minute iterations on
experiment/overnight-loop. 29 commits, ten issues addressed with closing keywords, every commit individually verified (unit + RLS integration + HTTP e2e + browser e2e where applicable).v0.1 — completed and eval-verified
OPENAI_BASE_URL/OPENAI_MODEL); later hardened to use/chat/completions(the provider default targets OpenAI-proprietary/responses, which compatibles don't implement).compactionsrows record what they supersede (upto_seq,parent_id); messages are never deleted. Post-turn trigger, staleness-guarded, RLSENABLE+FORCE.RUN_MODEL_EVALS=1gate): happy path, prompt-injection canary, overflow/compaction coherence — all green; the overflow case doubles as an integration proof of feat(api): configurable OpenAI-compatible provider (base URL + model) — cheap/free providers for dev + evals #88+feat(api): conversation context compaction #57.v0.2 — durable runs, end to end
Queueinterface; retry/backoff + dead-letter defaults; integration-proven (roundtrip, retries, DLQ, cron, deferred delivery).runs+ append-onlyrun_events(RLS-forced, cross-tenant-denied), coalescedmodel.deltapersistence, flag-gated worker execution (RUN_EXECUTION_MODE=worker) with a UI-message stream bridge (existing web client unchanged), cancellation (PATCH /runs/:id), heartbeat + per-run deadman timeout (no cross-tenant reaper — the RLS moat needs no bypass), and per-chat single-flight (partial unique index; retry supersedes prior attempt).GET /runs/:id/events), the resume endpoint (GET /chats/:id/stream, AI SDKreconnectToStreamcontract), web transport wiring — proven in a real browser: reload mid-answer, the run survives and completes on screen.Hardening
rls-test.shon every PR; SHA-pinned, zizmor/actionlint clean. (Surfaced thatpackages/uilint had been broken forever — fixed.)in_reply_tointegrity trigger, event-driven abort test fidelity, consumption-driven fake timing.@Public(), atomic session validation (TOCTOU closed, debounced hot path), login/register rate limiting (env-tunable),TRUST_PROXY, session cleanup on a pg-boss cron. Remaining items are deploy-shape-dependent (CSRF posture, token-free cookie responses).Decisions left deliberately open
RUN_EXECUTION_MODEdefault toworker(refactor(api): move the single-model loop into the worker #50) — browser suite already soaks it.masteronce the first run is green.apps/webhas two pre-existing unused-import warnings (untouched files) — flagged, not fixed.Review pass
The full branch diff was adversarially reviewed by two independent reviewers before this PR:
5ba547a,86994b1): redelivery double-execution (heartbeat pickup gate), lost cancellation in the pickup window (post-registration re-check), execution without a claim (mandatorymarkStartedclaim; failed claim = no spend, no events), and the permanent chat wedge (stale-zombie supersede on single-flight conflict — a chat can no longer be wedged in any mode). One MEDIUM (deadman re-enqueue resilience) accepted with rationale: queue-level retries plus the unwedge backstop cover its failure mode.Summary by cubic
Completes v0.1 and introduces the v0.2 durable-run pipeline: chats now execute as durable runs that survive refresh/disconnects, with context compaction and server-side titles. Adds CI and auth hardening.
New Features
OPENAI_BASE_URL/OPENAI_MODEL(defaults updated); targets/chat/completions.runs/run_events,Queueinterface onpg-boss(@wavezync/nestjs-pgboss), worker mode (RUN_EXECUTION_MODE=worker) with SSE replay (/runs/:id,/runs/:id/events,/chats/:id/stream); cancellation, heartbeat/timeout, per-chat single-flight.Hardening
@Public(), atomic validate+touch (debounced), login/register rate limits via@nestjs/throttler,TRUST_PROXY, hourly expired-session cleanup viapg-boss./chat/completions.Written for commit 86994b1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores