feat(api): agent + chat todos — durable chat-scoped task tracking (write gated) - #148
Draft
leon0399 wants to merge 19 commits into
Draft
feat(api): agent + chat todos — durable chat-scoped task tracking (write gated)#148leon0399 wants to merge 19 commits into
leon0399 wants to merge 19 commits into
Conversation
v0.3 opens: the foundational identity/org model everything downstream (policy engine #45, config resolver #46, projects) checks against. - org_units: arbitrary nesting with an ID-based materialized path (deviation from the issue's slug example, deliberately: ids are rename-stable, so only moves rewrite paths). The ancestor set is embedded in the path itself — every RLS policy resolves "role on unit or any ancestor" with ONE memberships scan via string_to_array(path), avoiding the self-referential org_units join Postgres rejects as infinite policy recursion. - memberships: full SPEC §7.3 role enum per (user, unit), unique pair. Inherited roles are COMPUTED along the ancestor path (nearest node wins — a subtree can demote as well as promote), not materialized (no inherited_from_id fan-out to maintain on moves). - external_identities: canonical (provider, external_subject) → user map for channels (SPEC §19.2). Research across open-webui / opencode / hermes-agent found no OSS precedent for nested groups with per-membership roles or cross-channel identity — original design, closest in spirit to open-webui's group_member join table. - RLS ENABLE + FORCE on all three (migration 0015; FORCE hand-appended like 0004/0009/0011 — gotcha list updated). Creator-bootstrap policy solves the fresh-root chicken-egg (root creator self-grants owner in the same tx). Fail-closed everywhere: memberships are own-rows-only, member lists arrive with the admin surface. - IdentityService (providers-only module — zero reachable HTTP surface until the admin API slice): root/child creation, move, effective-role resolution. Two landmines found by the integration suite, fixed + documented: - Postgres applies SELECT policies to INSERT…RETURNING rows: a granted membership belongs to the GRANTEE, invisible to the granter under the fail-closed select policy — grants no longer RETURNING. - substring(x from $n) with an untyped bind parameter resolves to the POSIX-REGEX form and silently yields NULL; the RLS WITH CHECK rejected the corrupted row before NOT NULL ever saw it. Moves use substr(text, int). Verified: 16 unit tests (path utils, nearest-wins resolution), full rls-test.sh harness green — RLS integration (FORCE proofs, cross-tenant denial, escalation denial, forged-path denial, subtree visibility after move) + queue + auth e2e on a throwaway non-superuser-owner Postgres. Lint, tsc, build green. Closes #44 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Multi-user governance (nested org units, memberships, roles) was fully built
at the service + FORCE-RLS layer but had ZERO HTTP surface — an operator
couldn't create an org or add a member except by hand-editing rows.
New IdentityController: POST /org-units (create root — creator becomes owner
in one tx), POST /org-units/:id/children (child; RLS owner/admin on an
ancestor), GET /org-units (the caller's visible units), POST
/org-units/:id/memberships (grant). Owner-scoped by the authenticated
identity + the existing FORCE-RLS policies.
Escalation is closed at the DTO: the grant role enum is { admin, member }
only — owner is assigned solely at creation, so the API can never mint or
escalate to owner; a re-grant conflicts → 409, a garbage userId (FK) → 404.
A single-reviewer round verified all five security claims and caught a real
P1 (CreateOrgUnitDto.type needed @IsOptional(), else omitting it 400s).
Member REVOKE + ROSTER are deferred together: the harness proved an admin
can't remove/see ANOTHER member's row (Postgres applies the own-rows SELECT
policy to a DELETE's targets), so both need the same recursion-safe SECURITY
DEFINER member-visibility change — a coherent follow-up.
Verified: 32 identity specs pass (RLS integration: create → creator-is-owner
+ visible, non-admin can't grant, cross-tenant grant denied, 409; DTO
escalation-guard units), tsc/lint/build clean; api-only.
Closes #44
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Layered effective-config resolution (SPEC §6.3-§6.4) with per-leaf provenance, snapshotted onto every run: - `configs` table: one versioned document per scope (org_unit / user / chat), unique per (scope_type, scope_id), version bumped on every write. RLS ENABLE+FORCE (migration 0016): own user scope, owned chat scope, org members read / owner-admin write via the #44 path trick. The instance layer stays env-derived (no row nobody could write); org_units.settings (added in #44, unreleased, no consumer) is dropped in favor of the uniform table — one source of truth (SPEC §7.2/§25.1 updated to match). - Merge engine: inheritance order instance → user → chat (org-unit layers slot in once chats/projects attach to org units, v0.5), later wins, objects deep-merge, arrays REPLACE whole — fail-closed by design (additive arrays are how a lower scope could smuggle a capability past a higher one; reference platforms concat+dedupe, we deliberately do not). Every resolution records per-leaf provenance (scope + version) plus the full consulted-layer chain (SPEC §6.4 config_version_ids). - Run snapshot: runs.config_snapshot is resolved in the SAME transaction that persists the message and run; execution reads only the snapshot, never live config, so a mid-flight config change can never re-configure a live run. Downstream consumers (the run-token budget, the compaction threshold) read the snapshot via snapshotMaxOutputTokens/snapshotCompactionThreshold — wiring the actual enforcement into run-execution.service.ts is those slices' job, not this one's. - Explain endpoint: GET /api/v1/chats/:id/effective-config returns effective + provenance + layers (404 cross-tenant, no existence leak). Reference research: claude-code's getSettingsWithSources() is the same idea; no comp persists a resolved snapshot at all — the durable per-run snapshot is deliberately novel (SPEC §6.4). - `configs_select`'s org-unit read arm ships with the ancestor- governance fix from the start (a config binds you whether it sits on your own unit's ancestor path OR on a unit whose subtree you belong to) — a too-narrow version of this exact policy shape shipped once elsewhere in this scope model and silently failed to bind descendant members to an ancestor-scoped row; proven here by a dedicated RLS integration test before it could recur. - Policy deny-stripping composes when #45 lands (the issue's third design step) — resolver and snapshot shapes already accommodate it. Verified on the isolated throwaway-Postgres harness (rls-test.sh): 5 integration suites incl. 6 new configs-RLS tests (scope isolation, cross-tenant denial, ancestor-governance read, write-denied-to-members, version increments) + 5 e2e suites incl. two new tests — instance-layer snapshot provenance, and a user-scope config overriding the instance env layer with provenance on the run row, the explain endpoint, and its 404 cross-tenant. 159 unit tests, lint, tsc green. Refs #46 — the deny-stripping acceptance step arrives with #45; the run-token-budget and compaction-threshold *consumers* of the snapshot are separate slices, not part of this one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
v0.3's governance spine, completing the milestone's core alongside #44 and #46: - `policies` rows (SPEC §7.4): effect (allow|deny), dotted action with '*' wildcard support, optional resource_type/resource_id, jsonb conditions (equality vs check context — ALL must hold, absent keys fail closed), and a SPEC §7.5 approval level on allows. `version` bumps on every write. `never_allowed` is deliberately not an approval value — that is what a deny IS. - Pure evaluator (policy-eval.ts): DEFAULT DENY ("policy before capability"); DENY OVERRIDES ALLOW regardless of specificity; approval = the STRICTEST level demanded by any matching allow — a deeper auto_allow deliberately cannot soften an org's always_ask (the approval analogue of deny-overrides-allow). Reference research: Claude Code's strict deny>ask>allow type precedence is the model; opencode's last-match-wins is the documented multi-author footgun we avoid; open-webui is allow-only and cannot express exclusions. `requiresHumanApproval` is the single source of truth for the approval-demanded threshold (auto_allow_* never asks; ask_*/ always_ask/admin_only do) — exported for the tool-calling loop's gate to consume, not wired into it here. - PolicyService.check(): collects policies across the org-path → user → chat scope chain and appends a `policy_decisions` audit row IN THE SAME TRANSACTION — an unauditable decision does not happen. The decision records effect, approval, and every matched policy id + version (deterministic and auditable after policies change). - Providers-only module, no HTTP surface. Consumers: the config resolver's deny-stripping step (#46) and the tool/connector/model gates of v0.4+ — neither wired in this slice. `policies_select`'s org-unit read arm ships with the same both-ways ancestor-governance shape as `configs_select` (#46) from the start — a member of a CHILD unit can read a policy set on an ANCESTOR unit, proven by a dedicated RLS regression test — rather than replaying the narrower-then-fixed history the original implementation went through. Verified: 213 api unit tests (23 in policy-eval.spec.ts, incl. new requiresHumanApproval coverage), full isolated rls-test.sh harness green (6 integration suites / 51 tests incl. policies-rls's 8, queue substrate, auth e2e), lint/tsc green. `drizzle-kit check` passes. Closes #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BYOK foundation (SPEC §14.1-§14.3), carved from experiment/overnight-loop-3 onto the identity + config-resolver + policy-engine stack: - provider_accounts: owner scope (user now, org_unit enum-ready), provider_type (openai_compatible first; rest of the adapter vocabulary enum-reserved so a new adapter is never an enum migration), auth_mode, base_url, default_model, models_cache (null until catalog sync #84), enabled. RLS FORCE with the #45 read/write split (subtree members read org accounts; owner/admin write). - credentials: AES-256-GCM envelopes under a VERSIONED master-key ring (CREDENTIAL_MASTER_KEYS="1:<b64>,2:<b64>"). New secrets seal under the highest version; decryption selects by the row's key_version — rotation is append-and-re-encrypt-lazily, never a flag day. The GCM AAD binds each ciphertext to (provider_account_id, secret_type): a payload copied onto another row fails authentication even with raw DB write access. Credentials are never more visible than their parent account (RLS via the FK), and secret material never leaves apps/api. - SecretString (lifted from opencode's Redacted<T>): decrypted keys redact through String()/JSON/inspect/template literals; only an explicit .reveal() at the model-client factory exposes the value. - Resolution (SPEC §14.3, wired into ModelsService): user BYOK account first, instance env key second, 402 otherwise. The instance boots with no provider AND with no vault key ring (BYOK then fails closed with a clear 400). - HTTP: GET/POST/DELETE /api/v1/provider-accounts — the API key is write-only (hidden from OpenAPI, absent from every response), global auth guard, cross-tenant 404. Review fix on top of the carved commit: resolveUserCredential's own comment promised "fail closed and logged" for a tampered/corrupted credential, but the decrypt call had no try/catch — a CredentialCryptoError would propagate unhandled (500 today; a permanently wedged worker job retrying forever against the same corrupt row once execution moves fully to the worker). Wrapped it: a decrypt failure now logs a warning and returns null so resolution falls through to the instance env key, with a new integration test proving the fail-closed path end-to-end. Also dropped a dead RUN_EXECUTION_MODE=inline env toggle from the e2e spec — inline execution mode no longer exists on this base. Reference comparison (opencode, open-webui): opencode's Redacted<T> is a type-level redaction wrapper only, no encryption at rest — confirmed no cipher/encrypt call anywhere in packages/llm/src/route/auth.ts. Open-webui encrypts with Fernet only for OAuth sessions and tool "valves" (single static key, no versioning — confirmed in utils/valves.py/models/oauth_sessions.py); actual model-provider keys (OPENAI_API_KEYS) are an instance-wide, unencrypted, semicolon-joined env list, and even per-user API auth keys are stored as plaintext columns (models/users.py). Neither comp does per-user BYOK with envelope encryption or key rotation — this is ahead of both on that axis. Migration 0018 regenerated fresh via drizzle-kit against this base (FORCE hand-appended per the established gotcha pattern; AGENTS.md updated). Closes #18 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… selection (#82, #76) OpenRouter becomes a first-class provider type dispatching through the native @openrouter/ai-sdk-provider package (createOpenRouter -> .chat) rather than the OpenAI-compatible base_url shim, per the issue's explicit mandate. ModelsService.createModelClient dispatches on the resolved credential's providerType and fails closed (UnsupportedProviderTypeError) for any adapter-less type; the chat loop checks dispatch eagerly at enqueue time so a misconfigured BYOK account rejects immediately instead of failing a run at worker pickup. GET /api/v1/models returns the caller's available set (each enabled provider account's default model plus the instance-env model, deduped and scoped to the caller). The chat loop validates a selected model against that set before any provider invocation (422 on an unknown or unauthorized id, re-validated worker-side so a job whose account was disabled after enqueue fails closed). A selected BYOK model resolves its owning account's credential via a shared resolveFromAccount helper, preserving the existing fail-closed tampered-payload handling in one place. Additive provider_type enum migration (0019), regenerated against this branch's actual schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Settings gets a Model providers section: lists the user's provider accounts (name, type badge, default model, base URL, disabled state), adds one via a dialog restricted to the two provider types with a working adapter (openrouter, openai_compatible) so the UI cannot create an account that would fail closed at chat time, and deletes with a confirmation. A thin TanStack Query client over the existing vault API through the shared ky client -- web never sees a secret (write-only on create, absent from every response) and surfaces the api's "BYOK disabled" 400 as a clear inline message. useModelsQuery now fetches GET /api/v1/models instead of returning a static list, merging static display metadata (icons, pricing) by id. The chat composer forwards the selected model only when it is in the live available set; prepareSendMessagesRequest carries it through the request body, and an id outside that set is omitted so the api resolves the caller default instead of rejecting the send. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#18, #82, #76) A new Playwright spec drives the real UI end to end: register, add an openai_compatible provider in Settings (key sealed into the vault, base URL pointed at the mock model server), the catalog lists it, the picker shows the live set, select it, send, the answer streams. Assertions the unit/HTTP suites structurally cannot make: the mock model server now records the credential + model id it actually receives on real chat calls (append-only, per-worker key so parallel workers never race), and the test asserts it saw THIS user's decrypted BYOK key under THIS selected model, and that the static default model entry is gone from the picker once the live set loads. The Playwright api now enables the vault (CREDENTIAL_MASTER_KEYS, test-only key). Not executed as part of this change (CI-gated per repo policy) but included for review and CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prior commit's prettier run reformatted the whole file (long single-line imports/JSX wrapped) even though only a 4-line addition (the ProviderAccountsSection import + mount, and the flex gap) was needed. apps/web has no prettier pre-commit gate (oxlint only; the root format:check only covers apps/api), so nothing required the reformat, and it needlessly widens the conflict surface on a file four other sibling stack PRs also touch (memory, custom-instructions, usage dashboard, prompt library settings sections). Restored the original formatting; the only functional change is the provider section mount. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The unit dispatch/catalog specs mock ProvidersService, and chats-messages.e2e-spec.ts overrides ModelsService with a fake, so nothing exercised the new ModelsController's per-user scoping against a real ProvidersService + RLS-scoped Postgres query. Added to the existing two-user provider-accounts HTTP e2e (which already boots the real AppModule against a real vault key ring): user A's own BYOK model appears in their GET /api/v1/models, and it never appears in user B's — the same isolation guarantee provider_accounts_select already proves for the underlying table, now proven at the HTTP surface this PR adds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…45, #91) Turns the answer-only chat into a bounded, pre-filtered tool-calling loop: a code-owned BuiltinTool registry (types, risk taxonomy, structured ToolResult), a single MVP tool (get_current_time, read-only), and the RunExecutionService wiring that builds each turn's tool set from the fail-closed safe allowlist, wraps every built-in in an AI SDK tool() whose execute emits tool.call/tool.result, and forwards {tools, stopWhen: stepCountIs(maxSteps)} to both provider clients. - Tool events go through the SAME serialized event chain as model.delta (enqueueEvent), and the wrapper flushes the delta buffer before emitting a tool event, so a partial in-flight model.delta can't lose the insert race and corrupt run_events order. - Hard step cap comes from the run's config snapshot (RUN_MAX_STEPS -> snapshotMaxSteps, default DEFAULT_MAX_STEPS = 4) — the resolver's instance-layer pattern extended with a `run.maxSteps` reader alongside the existing maxOutputTokens/compaction-threshold accessors. - Answer-only system prompt replaced to permit tool use, without claiming tool use that didn't happen. Verified with a MockLanguageModelV3 mechanism test that drives the real AI SDK loop (auto-execute + model re-call, ordered tool events, stopWhen hard-caps a runaway caller) and an executeRun -> run_events persistence integration test proving the P0 ordering fix end to end against Postgres. Refs #45, #91 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wires the tool pre-filter into the v0.3 policy engine, making roadmap principle #3 real for tools: "a tool is available only if effective policy allows it; deny overrides allow." - resolveAvailableTools becomes a 3-way gate: 'deny' excludes (deny overrides EVERYTHING, incl. the safe allowlist — an admin can revoke a read-only built-in); 'allow' admits even a non-safe tool (explicit grant); 'unset' (no policy matched) -> the safe allowlist (today's behavior, preserved when no policies exist). - RunExecutionService resolves each built-in's verdict ONCE before the stream via PolicyService.checkWithin in one transaction — each decision audited, no mid-stream policy DB work. - Approval tiers honored: auto_allow_readonly/auto_allow_low_risk are allows that never ask (admitted); ask_*/always_ask/admin_only demand human approval and are excluded (no approval flow yet — fail closed). requiresHumanApproval is the single source of truth for the threshold. - Fail closed on error: a policy-resolution failure denies every tool that turn (empty set, answer-only) — never falls back to 'unset', which would silently un-revoke a deny on a safe tool. Adds TOOLS_ENABLED, an instance-level, operator-only env switch that composes with the policy gate rather than bypassing it: it upgrades only the 'unset' verdict (a policy deny still wins; an explicit allow is untouched), and is read directly from env — never the user-mergeable config snapshot, so a user can't self-grant via their own config row. Restricted to low/own-scope risk classes; a write_external/destructive tool can never be env-enabled, only an explicit policy allow can grant it. No built-in tool shipped so far needs enabling (both are safe-listed); this is the control surface for the write-capable tools landing next. Verified: unit tests for the 3-way matrix (deny-overrides, fail-closed all-deny) and the enablement composition rules, plus 3 integration cases against the real policy engine + Postgres (deny revokes a safe tool, auto_allow_low_risk admits a non-safe tool, always_ask stays excluded). Refs #45 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the first tool with real value and the pattern every future
data/memory/knowledge tool reuses: the agent searches the authenticated
user's OWN past messages across their chats (memory beyond the live
context window / what compaction summarized away).
Security property, validated against opencode/openclaw/hermes — all
inject identity, none take it from model args:
- BuiltinTool.execute gains an optional `context` param (ToolContext =
{ userId, chatId, tenantDb }) built by the run loop from the RUN's
trusted fields, NEVER from model input. The tool's input schema is
{query, limit} only, so the model cannot widen the scope. RLS is the
primary guarantee; an explicit ownerUserId filter is defense-in-depth
(matching findByChatId's convention).
- get_current_time ignores context (pure); the optional param keeps its
single-arg call sites compiling.
Search correctness:
- Matches text VALUES via jsonb_array_elements -> e->>'text' ILIKE, NOT
parts::text (which would match the JSON keys "type"/"text" in every
row).
- Single-pass wildcard escaping; recent-first ~200-char snippets.
- First slow, unindexed, data-dependent query in the hot path, holding
the process's single Postgres connection (max:1). Runs under
SET LOCAL statement_timeout = 3000 and returns a structured
search_failed result on failure rather than starving every other
concurrent run/request. A GIN/FTS index is the real fix (deferred).
Verified: unit tests (schema/scope/snippet/fail-closed-without-context)
and an RLS cross-tenant integration test proving a search run as user A
NEVER returns user B's messages, plus the jsonb-value-match and
wildcard-escaping properties, against a live Postgres.
Refs #45, #57
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the tool-calling loop server -> UI: the agent's tool use was invisible (the bridge ignored tool events; only the final answer showed). Now it renders. - The run-execution tool wrapper captures the SDK-supplied toolCallId and records it (plus the structured output) on the tool.call/ tool.result run events, so the call and its result correlate into one UI part. - run-stream-bridge translates tool.call -> tool-input-available and tool.result -> tool-output-available (AI SDK v6 UI-message chunks -> `dynamic-tool` parts). A tool part CLOSES the open text part first, so the UI shows ordered parts (text -> tool -> text) instead of merging text across the tool boundary; each contiguous text run is its own part (text-1, text-2, ...). - web: a new collapsed ToolCallPart renders `dynamic-tool` parts — a chip with the tool name and running/done state, expanding to the input arguments and the result. Wired into the chat-page part switch. - e2e mock model server gained the ability to emit tool calls, so the full tool loop (call -> execute -> re-call -> answer) can be proven end to end in a real browser, not just at the unit/integration level. Verified: bridge translator unit tests (ordered text->tool->text, tool-before-text with no dangling text-end, dropping events missing the correlation id), the executeRun -> run_events persistence integration test (tool payloads carry toolCallId/output), api/web unit suites, and a browser e2e spec driving the tool loop through the mock model server. Refs #48, #50 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TOOLS_ENABLED (#45, introduced in 4e09d27) was read via ConfigService.get but never documented alongside the other run.* instance-layer env vars, breaking the pattern every other env var in this file follows (a comment explaining what it does, its scope, and its default). Adds the missing block: what it composes with (the policy gate — env only upgrades an 'unset' verdict, never overrides an explicit deny), which risk classes it can and cannot enable, and that it's currently a no-op since both shipped tools are already safe-listed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A v0.5 control primitive (principle #2, "todos are structured data"): a `todos` table (chat-owned, RLS ENABLE+FORCE, todo_status enum, unique (chat_id, source, position), DB content CHECK) + two tools. write_todos (fullList) replaces the AGENT's plan — the replace-all pattern Claude Code + opencode converge on (delete-then-reinsert with position, atomic in one runAs transaction, statement_timeout-bounded); list_todos() reads the full list. write_todos is a WRITE and default-DENY, enabled by the operator (TOOLS_ENABLED=write_todos) or a policy allow — consistent with the existing "agent writes are operator-opt-in" posture: replace-all (delete-all-then-reinsert; an omitted item is deleted, an empty call clears the plan) is STRICTLY more destructive than an append-only write, so it gets the same gate. list_todos (read-only) is default-available. A `source` ('user'|'agent') column is included from the start so a later user-facing panel can add its own todos without an agent plan-write ever wiping them — the unique index is partitioned by source and write_todos's replace-all touches only source='agent' rows. Scope from the injected chatId (never a model arg); RLS scopes every op to the chat's owner. Verified: unit tests for both tools (write-gating, input caps, fail-closed without context), RLS integration coverage (FORCE enabled, cross-tenant denial, atomic replace-all, the content CHECK, position ordering), and the policy-gated-tool-availability suite extended to prove write_todos is excluded by default and admitted only by an explicit policy allow. Full RLS harness (migrations + RLS + policy-gate + auth e2e) green. Refs #38 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface the built-but-invisible todos primitive (the `todos` table +
`write_todos`/`list_todos` agent tools had no UI) as a user-manageable
task panel, and fold in copy-to-clipboard on messages.
- `GET/POST/PATCH/DELETE /api/v1/chats/:id/todos` + `ChatTodosController`
+ `TodosRepository` CRUD, chat-scoped and RLS-guarded (`todos_owner`).
- Sidebar-of-composer `ChatTodos` panel: the user adds / toggles /
deletes their own todos; the assistant's plan is shown read-through
with a badge. Rendered only once a chat is no longer a draft
(`!navigateOnFinish`), so it never POSTs to a not-yet-created chat.
- Copy button on every message: copies only text parts (skips
reasoning/tool internals), secure-context-safe (execCommand fallback
for self-hosted HTTP; never throws).
Write-coordination: the previous commit's `source` ('user'|'agent')
column means the agent's replace-all touches ONLY `source='agent'`
todos — the user's list is never wiped; the user-facing update/delete
endpoints are in turn scoped to `source='user'`, so this panel can never
mutate the agent's plan (it is shown read-through only). The unique
index is partitioned by source so the two lists never collide; list
shows the agent's plan first, then the user's todos. A concurrent
same-tick add races on position → the repo retries once instead of
surfacing a spurious 409.
Also extends `chatQueryKeys` (the established web query-key factory)
with `todos(chatId)` rather than a one-off key.
Verified: RLS integration cases for the new CRUD paths (add/toggle/
delete, the AGENT-replace-preserves-USER-todos boundary, cross-tenant
denial, and that the user-facing endpoints cannot mutate an agent todo)
+ 5 clipboard unit cases + api/web typecheck, lint, and test suites
green. Full RLS harness (migrations + RLS + policy-gate + auth e2e)
green end to end.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces durable, chat-scoped task tracking (todos) for both the agent and the user, including a PostgreSQL schema with Row Level Security, agent tools (write_todos and list_todos), a REST API, and a collapsible task panel in the web UI. It also adds a secure-context-safe message copy-to-clipboard button. The review feedback highlights two important issues: first, transaction poisoning in PostgreSQL during unique key violation retries within a transaction block in TodosRepository.add, which requires nested transactions (savepoints) to resolve; second, a potential duplicate submission bug in the UI's onAdd handler that should be guarded with add.isPending.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Two real findings from PR #148 review (gemini-code-assist), both verified before fixing: - TodosRepository.add() ran its retry-on-position-conflict loop directly against `this.db`, which is already the runAs transaction. Confirmed via drizzle-orm's postgres-js session.ts: a bare insert that throws (a real 23505 unique violation) poisons that transaction — any later statement on it errors with "current transaction is aborted" — so the retry attempt could never actually succeed, only ever hit the same abort. Wrapped each attempt in its own `this.db.transaction(...)`, which drizzle-orm compiles to a SAVEPOINT when called on an already-open transaction (verified in the same driver source): a losing attempt rolls back to the savepoint, not the whole transaction, so the retry runs clean and the outer transaction (and RLS's app.current_user_id) survives for it. Added a concurrent-add integration test (5 parallel add() calls racing the same MAX(position)+1 subquery) that all succeed with distinct positions — this fails deterministically without the fix. - ChatTodos' onAdd() guarded the Add button's `disabled` state on add.isPending but not the Enter-key path, so rapid Enter presses could fire multiple concurrent add.mutate() calls before the first settled. Added the same isPending check to onAdd() itself. Verified: api/web typecheck + lint clean, full jest suite green (245 unit + 79 DB-gated, incl. the new concurrency test), web vitest green, full RLS harness (migrations + RLS + policy-gate + auth e2e) green end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leon0399
force-pushed
the
stack/split-tool-loop
branch
from
July 10, 2026 21:53
fbae6dd to
35747fc
Compare
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.
What
Ships the todos system in two commits, stacked on
stack/split-tool-loop(#142):todostable (chat-owned, RLSENABLE+FORCE,todo_statusenum, unique(chat_id, source, position), DB content-length CHECK) plus two agent tools:write_todosreplaces the agent's plan (delete-then-reinsert, atomic in onerunAstransaction, statement-timeout-bounded — the pattern Claude Code and opencode converge on),list_todosreads it.write_todosis a WRITE and default-deny (TOOLS_ENABLED=write_todosor an explicit policy allow);list_todosis default-available. Thesource(user/agent) column ships from the start of this migration so a same-table user surface can be added later without a follow-upALTER TABLE.GET/POST/PATCH/DELETE /api/v1/chats/:id/todos+ChatTodosController+TodosRepositoryCRUD lets the user manage their own todos (add/toggle/delete) alongside the agent's plan, shown read-through. A copy-to-clipboard button on every message (text parts only,execCommandfallback for non-secure-context self-hosted HTTP).Both original commits (
99b0745,1dbd0db) carried a second, later migration each (ALTER TABLE ... ADD COLUMN source) in the upstream lineage. Neither is released in this stacked lineage, so I collapsed each pair into one clean migration per commit — commit 1 shipstodoswithsourcealready present (migration0020), commit 2 is pure application code (controller/DTO/UI) with no further schema change.Refs #38 (durable, chat-scoped task tracking; no dedicated tracking issue for the UI panel in the original branch).
Security & tenancy
ENABLE+FORCEontodos, policytodos_ownerscoped to chat ownership (EXISTS (... c.owner_user_id = current_setting('app.current_user_id'))), same pattern asmessages.todos-rls.integration.spec.ts, run against a live non-superuser-owner Postgres viarls-test.sh): cross-tenant list/replace denied for the agent-tool path; cross-tenant add/toggle/delete denied for the user-facing CRUD path; FORCE is actually enabled (relforcerowsecurity); the content-length CHECK holds.policy-gated-tools.integration.spec.tsnow assertswrite_todosis excluded with no policy at all, and admitted only after an explicitPoliciesRepositoryallow — using the real tool, not a stand-in.updateStatus/deleteByIdcannot mutate an agent-authored todo even for its rightful owner — the repository methods filter onsource='user', not just(chatId, id).CurrentUser()(session-derived) /context.userId(run-loop-injected), never client input; the todo id path param is always paired with achatId+sourcefilter, never trusted alone.Review findings & refactors
chat-todos.controller.ts's docstring claimed "the agent's plan is read-through in the panel, never mutated from there," but the underlyingTodosRepository.updateStatus/deleteByIdonly filtered on(chatId, id)— a user could PATCH/DELETE an agent-authored todo via a direct API call (same owner, so not a tenancy bug, but a real violation of the stated invariant, and it would make an agent'swrite_todos-driven UI state inconsistent with what the tool itself last wrote). Addedsource: 'user'to both queries'WHEREand added the negative test proving it (updateStatus/deleteById cannot mutate an AGENT todo (source-scoped)).0025(base table) then, ~unrelated commits later,0027(ADD COLUMN source) — both unreleased here, sodrizzle-kit generateagainst this base produced one clean0020migration withsourcebaked in; no split-then-alter needed.apps/web/lib/services/chat/todos.tsdefined its own standalonetodosQueryKey;apps/web/AGENTS.mddocumentschatQueryKeysinqueries.tsas the one feature key factory. Extended it withchatQueryKeys.todos(chatId)and had the todos service import that, instead of introducing a second key scheme.enabledgate. The original<ChatTodos enabled={displayMessages.length > 0}>heuristic would flip totruethe instant a brand-new chat's first (optimistic) user message renders — before the create-or-append POST that actually persists the chat completes — which is exactly the premature-POST-to-a-nonexistent-chat bug the design doc says it avoids. This branch's base (stack/split-tool-loop) already threads a precise signal for this (navigateOnFinish, true for the entire lifetime of a not-yet-persisted draft session), so I usedenabled={!navigateOnFinish}instead.apps/api/src/chats/tools/registry.ts,chats.module.ts,tools.spec.ts, andpolicy-gated-tools.integration.spec.tsagainst this branch's actual base rather than taking the upstream diffs verbatim — the upstream versions of these files were edited in a lineage that already had the sibling memory feature (remember/recall) landed, which this stack does not include yet. Verified norecall/rememberreferences leaked in.apps/web/lib/clipboard.ts(module-levelcopyText+messageText) is new to this lineage; PR feat: public read-only chat sharing (activates chats.visibility) #130 (chat sharing, offmaster) independently created the same path withcopyTextonly. The trees will collide textually when they meet (this stack goes tool-loop → BYOK → … → master); resolution is trivial (identicalcopyText, this branch's version is a strict superset withmessageText).Reference comparison
TodoWriteTool(~/.cache/checkouts/github.com/codeaashu/claude-code/src/tools/TodoWriteTool/TodoWriteTool.ts): confirmed replace-all semantics (whole list every call) — matches. It is session-ephemeral, not durable (context.getAppState().todos[...], in-memory, never persisted, single writer, no per-item ownership concept). llame'ssourcecolumn is a justified deviation: Claude Code's tool has no user-editable counterpart to protect, ours does (the panel), so the coordination problem it solves doesn't exist upstream.notes.py). Pure contrast case, nothing to reconcile against.Verification
pnpm --filter api typecheck/pnpm --filter web typecheck— clean.pnpm --filter api lint(oxlint --fix) /pnpm --filter web lint(oxlint --deny-warnings) — clean.pnpm --filter api exec jest --maxWorkers=1(full unit suite) — 245 passed, 0 failed, 78 skipped (DB-gated integration specs, covered below).pnpm --filter web test(vitest) — 34 passed, 0 failed, including 5 new clipboard cases.RLS_TEST_PORT=<port> bash apps/api/scripts/rls-test.sh(full harness: migrations as non-superuser owner + all.integration.spec.ts+ queue integration + auth e2e) — green end to end: 11 integration suites / 78 tests passed (includes the new 9-testtodos-rls.integration.spec.ts), queue suite passed, auth e2e (6 suites / 48 tests) passed.pnpm exec drizzle-kit check— migration chain (0000…0020) verified consistent.pnpm --filter api build— regeneratedapps/api/openapi.json(adds the two new todo paths); not hand-merged.Summary by cubic
Adds durable, chat‑scoped todos for agents and users: a RLS‑guarded
todostable with readlist_todos, gated writewrite_todos, a chat task panel, and a message copy button. Also fixes todo add retry logic and prevents duplicate adds; aligns with #38 (durable, chat‑scoped task tracking).New Features
todostable with FORCE RLS andtodo_status/todo_source;GET/POST/PATCH/DELETE /api/v1/chats/:id/todos; OpenAPI updated; tool registry addslist_todosandwrite_todos.write_todosreplaces the agent plan atomically;list_todosreads the full list.chatQueryKeys.todos(chatId).source='user'; agent writes touch onlysource='agent'.write_todosis behindTOOLS_ENABLED=write_todosor a policy allow;list_todosis default‑available.Bug Fixes
TodosRepository.add()wraps each retry in a transaction SAVEPOINT to avoid aborting the outer transaction; concurrent adds now resolve to distinct positions.add.isPendingto prevent duplicate add calls.Written for commit f7f1c65. Summary will update on new commits.