Server-aware mutations: remove hardcoded slug identity (ref/rawGq)#10
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53a9a0de9a
ℹ️ 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".
| select_state: /sel/decision | ||
| select_column: slug |
There was a problem hiding this comment.
When this notebook is run with pnpm tui, this new select_state is never written: packages/tui/src/components/Table.tsx only reads columns, rows, and dense and renders static rows, unlike the web Table selection handler. That means the new open-issues action resolves decision: { $state: /sel/decision } to undefined in the terminal, so the required catalog mutation param is omitted and the advertised edge mutation cannot succeed there. Either implement Table selection in the TUI or avoid making the demo mutation depend on it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit de36120. Configure here.
1. Nested $row/$state params (resolve.ts): unify resolveExpr/resolveMutationExpr into one *recursive* resolver shared by read + mutation params, so markers nested in arrays/objects resolve — matching the validator's recursive detection. "marked dynamic but sent literal" is now unrepresentable. (Cursor + Greptile) 2. Optimistic patch-key race (runtime.ts + mutations.ts): tag each dispatch with a monotonic seq on OptimisticPatch; a settle only mutates the entry it still owns (cur.seq === patch.seq), so an older Approve settle can't clobber or delete a newer Reject's in-flight patch. Mirrors the cellRuns/generation guard. (Greptile) 3. TUI Table selection (tui/Table.tsx): implement focus-aware select_state / select_column (useFocus/useInput + the ink setState action + sliding viewport), mirroring the web Table — so the demo's "Raise to selected decision" edge button works in `pnpm tui` too. Restores renderer parity. (Codex) 4. Wrong-kind ref param noise (validate.ts): short-circuit — validate params only when the ref's kind matches, so a mismatched ref yields exactly one wrong_query_kind error, no spurious unknown/missing-param noise. (Cursor) Tests: +9 (recursive resolver nested-marker cases; Approve→Reject race regression). typecheck + 91 tests + build green; Fix 4 verified live against the local catalog (one wrong_query_kind, no param errors; demo still ok).
…tity
Replace the interim client-compiled `set_field` write path (which hardcoded
`where slug = $target_id` in translate.ts) with a write spec that mirrors the
read `query` shape: `ref` (server-owned catalog mutation) XOR `rawGq`
(author-written inline .gq), plus typed `params` and an optional `optimistic`
overlay. The client no longer constructs any write predicate — identity is a
typed param resolved from the clicked row (`$row`) or state (`$state`) — so
non-slug keys, cross-type selection, and edges all work.
- core/spec: MutationSpec = ref XOR rawGq (same refine as QuerySchema);
MutationParams = { spec, row, rowKey }; drop set_field/kind/target_id
- core/runtime: resolveMutationParams ($row/$state/literal); optimistic patches
key on (cellId, rowKey, field) from explicit optimistic.set; drop mutationKinds
- client: Client.invokeMutation (queries.invoke expectMutation:true); ServerSource
branches ref/rawGq like read; delete translate.ts (+ test)
- cli/validate: validate action ref mutations vs catalog; warn on rawGq; $row dynamic
- renderers: ActionList fireAction passes { spec, row, rowKey } (web + tui)
- demo: query-keyword node mutations + an edge mutation (insert Raises); notebook
rewired to invoke refs incl. a live "Raise to selected" edge button;
server-demo.sh re-applies stored queries on cluster reuse
Verified: typecheck clean (5 pkgs), 82 tests pass, build green, the 3 new .gq
lint clean against company.pg, and `notebook validate` returns ok against a
live cluster catalog.
A configured server bearer token (e.g. exported by an operator's keychain
setup) wins over --unauthenticated and boots the demo server in default-deny
mode, which blocks stored-query invokes (POST /queries/{name}) — every cell
404s and the Approve/Reject/Raise buttons can't write. env -u it for the boot.
Verified live: with the token stripped, the cluster serves reads + the new
node/edge mutations; the built notebook Client.invokeMutation drives a real
PolicyClause status change and a Raises edge insert end-to-end.
1. Nested $row/$state params (resolve.ts): unify resolveExpr/resolveMutationExpr into one *recursive* resolver shared by read + mutation params, so markers nested in arrays/objects resolve — matching the validator's recursive detection. "marked dynamic but sent literal" is now unrepresentable. (Cursor + Greptile) 2. Optimistic patch-key race (runtime.ts + mutations.ts): tag each dispatch with a monotonic seq on OptimisticPatch; a settle only mutates the entry it still owns (cur.seq === patch.seq), so an older Approve settle can't clobber or delete a newer Reject's in-flight patch. Mirrors the cellRuns/generation guard. (Greptile) 3. TUI Table selection (tui/Table.tsx): implement focus-aware select_state / select_column (useFocus/useInput + the ink setState action + sliding viewport), mirroring the web Table — so the demo's "Raise to selected decision" edge button works in `pnpm tui` too. Restores renderer parity. (Codex) 4. Wrong-kind ref param noise (validate.ts): short-circuit — validate params only when the ref's kind matches, so a mismatched ref yields exactly one wrong_query_kind error, no spurious unknown/missing-param noise. (Cursor) Tests: +9 (recursive resolver nested-marker cases; Approve→Reject race regression). typecheck + 91 tests + build green; Fix 4 verified live against the local catalog (one wrong_query_kind, no param errors; demo still ok).
aaltshuler
force-pushed
the
ragnorc/server-aware-mutations
branch
from
1a22f27 to
a4fe121
Compare
| if (cur?.seq === patch.seq) { | ||
| this.optimistic.set(patch.key, { ...cur, saving: false }); | ||
| } |
There was a problem hiding this comment.
This settle check only protects fields that a newer mutation overwrote. If one action sets optimistic { status: "approved", assignee: "alice" } and a second action on the same row sets only { status: "rejected" }, the second action replaces the status patch but leaves the first action's assignee patch in the map. When the first action settles, this branch marks that stale assignee patch as no longer saving, and applyOptimisticPatches still applies it because it does not filter settled patches. The row can show the newer rejected status with the older assignee value until the next re-read clears it.
Context Used: CLAUDE.md (source)
2 participants
Why
The write path was incorrect by design: the client compiled every mutation to
.gqand hardcoded the row predicate aswhere slug = $target_id(packages/client/src/translate.ts). That baked a per-type, slug-only, node-only identity assumption into the layer with the least information, breaking for non-slug keys, cross-type selection, and edges (which have noslugat all). Canondash-books-canon.md§4.1/§4.5 mandates deletingtranslate.tsand moving writes into the server-owned catalog.What
The
mutationblock now mirrors the readqueryshape exactly —ref(server-owned catalog mutation) XORrawGq(author-written inline.gq), via the samerefine, plus typedparamsand an optionaloptimisticoverlay. Nokinddiscriminator, notarget_id. The client constructs no write predicate: identity is a typed param resolved from the clicked row ({ $row: col }) or state ({ $state: ptr }). The server mutation owns itswhere, so non-slug keys, composite keys, and edges all work.MutationSpec = { ref?, rawGq?, name?, params?, optimistic? }+ exactly-one refine;MutationParams = { spec, row, rowKey }.set_fieldremoved.resolveMutationParams($row/$state/literal); optimistic patches key on(cellId, rowKey, field)from an explicitoptimistic.set;mutationKindsdropped (gateref→namedQueries,rawGq→rawGq, like reads).Client.invokeMutation(queries.invokeexpectMutation:true);ServerSource.mutatebranchesref/rawGqlikeread;translate.ts(+test) deleted.refmutations validated vs the live catalog (exists +mutation===true+ params);rawGqwarned;$rowtreated as dynamic.ActionListfireActions pass{ spec, row, rowKey }.query-keyword node mutations (approve/reject_policy_clause) + an edge mutation (raise_issue_to_decision→insert Raises { from, to }); notebook rewired toinvokerefs incl. a live "Raise to selected decision" edge button;server-demo.shre-applies stored queries on cluster reuse.Verification
pnpm -r typecheckclean (5 packages)pnpm -r test— 82 pass (core 40, client 24, web 17, tui 1)pnpm -r buildgreen (libs + CLI bundle + web SPA)omnigraph linton the 3 new.gq— 0 errors (confirmsquery-keyword mutations +insert Raises { from, to })notebook validatevs a live cluster catalog —ok:true, matched theapprove/reject/raisemutations, emitted only the$statewarning (not$row)Generality is tested, not just claimed:
source.test.tsincludes a non-slug-key (email) case, and the demo edge button targets by(from, to)slugs — neither expressible under the oldslug-only path.Note on the branch
Pushed as
ragnorc/server-aware-mutations(notragnorc/investigate-notebooks) because the remote branch of that name is a separate unmerged feature line; this commit is based cleanly on currentmain.Note
Medium Risk
Touches the full write path (spec, runtime param resolution, optimistic concurrency, and server invoke). Incorrect resolution or settle logic could send wrong params or show stale UI, but behavior is heavily tested and predicates move server-side.
Overview
ActionList writes now mirror read queries: each button uses
mutation.ref(catalog mutation viaexpectMutation: true) ormutation.rawGq, with typedparams({ $row: col },{ $state: ptr }) and optionaloptimistic.set. The oldset_field/target_idshape andpackages/client/src/translate.tsare removed —ServerSourceforwardsresolvedParamsonly.The runtime resolves mutation params recursively, passes
{ spec, row, rowKey, resolvedParams }to sources, and drives optimistic UI from explicitoptimistic.setkeys(cellId, rowKey, field)with a seq guard so stale settles cannot clobber newer clicks.mutationKindsis dropped; compatibility gatesref/rawGqlike reads.CLI validate checks action
mutation.refagainst the live catalog (mutation vs read, params);$rowis treated as reliably dynamic. Web/TUI ActionList dispatch the new payload; TUI Table adds row selection into state for cross-cell params (e.g. edgeraise_issue_to_decision).Demo: stored approve/reject node updates and an insert Raises edge mutation;
server-demo.shre-applies queries on reuse and stripsOMNIGRAPH_SERVER_BEARER_TOKENso unauthenticated invokes work.Reviewed by Cursor Bugbot for commit 1a22f27. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR moves mutations from client-built predicates to server-owned mutation calls. The main changes are:
mutation.refand inlinemutation.rawGqspecs.$rowand$stateparameter resolution.Confidence Score: 4/5
This is close, but the stale optimistic overlay should be fixed before merging.
packages/core/src/runtime/runtime.ts
Important Files Changed
Reviews (4): Last reviewed commit: "fix(notebook): address PR #10 review — 4..." | Re-trigger Greptile
Context used: