Repository navigation
release: v3.4.0 — develop → stage - #2405
Merged
Merged
Conversation
…2298) * docs(agents): restore the user task battery beside the agent docs it grades The 256-item acceptance sheet for the ROCKETRIDE_* context docs was dropped by the docs revamp (#2139). Bring it back at docs/agents/task-battery.md, outside context/ so the /client/docs bundle does not ship it, point its scoring-pass note at the post-revamp paths, and describe it in the README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs: surface the Drizzle ORM driver in the agent bundle and SDK references The Drizzle-over-pipes driver (#1908) updated the public guide pages but not the agent-facing context, the pipeline-skill doc map, or the SDK reference tables, which still steered readers to the deprecated Sequelize binding. - ROCKETRIDE_typescript_API.md: add a `drizzle()` subsection, mark `sequelize()` deprecated, document `rowMode` on `query()` - ROCKETRIDE_DOC_MAP.md: link the Drizzle page - typescript/reference.md: add `drizzle` row and types, flag Sequelize as deprecated, add `rowMode` to the `query` signature - python/reference.md: point the cross-reference at Drizzle Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs: address review on the Drizzle agent-doc surfacing - Point the agent bundle at the absolute docs-site URL for the Drizzle guide; the bundle ships into user workspaces where repo paths do not resolve - `debug` is an npm package, not a Node built-in: name `util` and the `tty`/`util` pulled in via `debug` as the actual browser-bundle blockers (agent doc and public Sequelize guide) - Note the `unknown[][]` return shape under `rowMode: 'array'` and that `rowMode` is TypeScript-only Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
After a Python lane handler returns normally, the engine also forwards the handler's input on the same lane. Only `preventDefault()` stops that. Four nodes returned normally, so downstream received the raw input next to the node's own output. - `context_optimizer`: both exits of `writeQuestions` now end in `return self.preventDefault()`. Downstream no longer gets the untrimmed original next to the trimmed question. - `summarization`: `writeText` and `writeTable` collect the text, then suppress the forward. Only the summary from `closing()` goes on. - `tool_guild`: `writeText`, `writeQuestions` and `writeDocuments` collect, then suppress. The docstring claiming "overriding suppresses raw passthrough" is corrected. - `tool_n8n`: the same for `writeText`, `writeQuestions`, `writeDocuments`, and every `writeImage` / `writeAudio` / `writeVideo` chunk. - Missing docstrings added in the four node files. Tests: - `test_lane_forward_once_nodes.py`: delivery-count cases for every changed handler. - New `summarization/test_summarization.py`: the node had no unit tests. - `context_optimizer/test_all.py` and `test_tool_n8n.py`: direct handler calls now require the `PreventDefault` exception.
…rs (#1682) * fix(memory_persistent): prevent double-forward of questions and answers * test(memory_persistent): assert preventDefault is called on explicit forwards * docs(memory_persistent): note preventDefault() unreachability and test stub caveat * fix(memory_persistent): cover all six exits with delivery-count tests - Every exit of `writeQuestions` and `writeAnswers` now ends in `return self.preventDefault()`. The separate `return` after `preventDefault()` could never run, because `preventDefault()` raises. - `test_node.py`: two tests for the invalid `session_id` paths, which no test reached before. Removing `preventDefault()` from either path now fails a test. - `test_lane_forward_once_nodes.py`: six delivery-count cases, one per exit (no store, invalid session, normal path), under the engine's forward rule. On the normal path, only the enriched or stored copy reaches downstream, never the original. - Docstrings in `IInstance.py` now document their arguments. --------- Co-authored-by: Alexandru Sclearuc <asclearuc@gmail.com>
#2306) `stop()` queues its own `None` sentinel when it starts waiting before the audio callback has reached the decoder's. The callback stops at the first `None`, so the second one stays in the queue. `test_begin_write_end_still_drains_normally` still required an empty queue, so it failed whenever `stop()` won that race, which happens on a loaded CI runner. - The drain test now accepts `[]` or `[None]` as the leftover. Unplayed audio still fails it. - New `test_stop_waiting_before_the_callback_leaves_only_its_own_sentinel` forces the `stop()`-first order. It requires exactly `[None]` to be left, all audio played, and `stop()` returned. - The shared setup moved into `_player_with_queued_audio()` and `_start_simulated_hardware()`.
Mint a release-bot GitHub App token and pass it to create-pull-request so the weekly Sync LLM Models PR triggers CI on its own instead of waiting for a manual approve. Falls back to github.token if the token can't be minted. release-bot was granted Pull requests: write on the org install. Fixes #2311.
…work (#2299) * feat(ai): break the profiler report down by thread stop() now also captures the session per yappi context, next to the flat capture and before clear_stats(). threads() lists them busiest first with their counts, and report_tree() takes an optional thread id; without one it behaves as before. DAP: new rrext_cprofile_threads, plus an optional thread argument on rrext_cprofile_report_tree, in the engine subprocess and the task-server proxy. Two yappi traps shape the capture: - get_func_stats(ctx_id=...) tests the id for truthiness, so thread 0 would silently get every thread's functions. The filter goes in as a dict. - The default context name is the thread's class, so workers read 'Thread' and engine workers '_DummyThread'. start() installs a callback reading Thread.name from threading._active; current_thread() would deadlock when the profiled call is _active_limbo_lock.acquire() (yappi #48). yappi keeps the first name it gets, so the thread ident is reported as well. Cost on a parse -> preprocessor pipeline over 12 and 36 documents: the per-thread copy holds 2.1-3.5 MB against 0.5 MB flat (13-25 threads), and the capture under _lock grows from ~117 ms to ~250-275 ms. * perf(ai): cache _relativize_path stop() calls it under _lock for every entry and callee of each capture: about 5,900 calls per stop on a parse pipeline over 12 documents, since the per-thread capture repeats the flat one's paths, over a few hundred distinct paths. The result depends only on the path, so cache it. On the same pipeline the capture under _lock drops from ~250-275 ms to ~160-170 ms, against ~117 ms before the per-thread capture existed. Memory drops only ~10%: every entry of a path now shares one string, but the per-entry dicts are most of the size. * fix(ai): stop the call tree from pruning away real work report_tree() dropped every node whose own time fell under min_pct, and with it the whole subtree. Two kinds of node read light while real work hangs below them: frames still running at stop() (thread entry points, executor worker loops) read ~0 because yappi books time on return, and the event loop step that resumes a coroutine is short while the coroutine is booked its whole lifetime. On a parse pipeline this emptied the merged tree, the event-loop thread and every executor thread started mid-session; threads that predated the session survived only because their zero-time frames sat at depths 0 and 1, which are never pruned. A node is now pruned only when nothing reachable below it reaches the threshold either. The threshold was also inflated: total_time summed ttot, counting nested calls again (a 5 s session summed to 412 s). It is now the time the threads ran, as yappi measures it per thread - the selected one, or all summed - falling back to the sum of self times without thread data. Same session after the fix: project code shows in every view at the default threshold, and trees stay at 4-168 nodes, built in under 60 ms. * feat(client): list profiled threads and fetch one thread's tree Python: cprofile_threads() and a thread parameter on cprofile_report_tree(). TypeScript: cprofileThreads() and a trailing thread parameter on cprofileReportTree(), with CProfileThreadInfo / CProfileThreadsResponse. Thread 0 is a real id, so both clients test for absence, not truthiness. The shell re-exports the SDK's types, so the two new ones are new shell-api names and shell:check fails until they are frozen: this freezes shell-api v4. Its delta over v3 is exactly these types and the two method signatures. The contract test runs data through a task's pipeline and checks that no thread carries yappi's class-based names, that one thread's tree totals that thread's calls, and that an unknown id is refused, which proves the argument reached the engine. test-ui's API sweep and the SDK references list the new method; the Python reference also claimed include_system defaults to False, which it never did. * feat(profiler-ui): pick a thread to visualise A Thread dropdown heads the viz controls: all threads (the old view) or one, labelled with name, thread id and time, since names repeat. The views get the same tree shape either way. The list is refetched with the report, a selection missing from the new list falls back to all threads, and a server without rrext_cprofile_threads just hides the dropdown. Start clears the list, since ids are per session. Bumps the app to 1.3.0: an OSS server reseeds a built-in app only when its apps.json version changes, so an upgraded server would keep serving the old profiler otherwise. * fix(profiler-ui): prune the charts by subtree weight The client-side cutoff compared a child's own cumtime with its parent's, so a zero-time frame the server now keeps was dropped again at the root, taking the work under it along. It now compares the largest cumtime in each subtree, and runs before the depth limit so that weight covers the whole subtree. * fix(profiler-ui): clear the two type errors tsc reports initConnectionStore took the shell's settings as Record<string, string>, but they are Record<string, SettingValue>, and a port setting may well be a number; the seeded host and port are now coerced to strings. The sunburst filtered on x0/x1 before casting to HierarchyRectangularNode, which the flame graph already does first. Neither changes behaviour: the app build does not type-check, which is how both went unnoticed. * fix: CodeRabbit
…, engine URI, tools) (#2315) * fix(mcp): refuse query-string credentials on /mcp and never fall back to the shared client for authenticated callers AuthMiddleware falls back to `?auth=` when no Authorization header is sent, but auth.authorize() only read the header. `POST /mcp/?auth=<key>` therefore passed the middleware, skipped every /mcp check (tk_/pk_/cd_ refusal, JWT signature, issuer, audience) and ran tools on the server's shared engine client. - authorize() now refuses any request carrying an `auth` query parameter (parsed as Starlette parses query_params), per the MCP authorization spec. - Defence in depth: a request the middleware authenticated (scope['state']['account']) that carries no header credential is refused instead of served by the shared client. The credential-less dev bypass (public route, no account) is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): reserve /mcp for the MCP API — move the shell page to /mcp-server and route bare /mcp to the handler The shell registered a public GET page at /mcp. The MCP endpoint is a Starlette Mount, which only matches /mcp/..., so the page won the bare path: POST /mcp answered 405 with no WWW-Authenticate challenge (spec clients POST to the advertised resource .../mcp and never saw the OAuth challenge), and AuthMiddleware treated every /mcp request as public. add_route's duplicate check never saw the clash because the Mount goes straight onto the router. - shell: the MCP marketing page moves to /mcp-server (still unlisted). - mcp: an exact, method-agnostic Route serves bare /mcp alongside the Mount, so /mcp and /mcp/ reach handle_mcp identically (no 405/307). - mcp: initModule refuses to start if any route already claims /mcp or /mcp/*, or a public pattern matches the endpoint. - mcp: MCP_DEV_NO_AUTH now opens exactly /mcp and /mcp/ (loopback only). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(mcp): default the engine URI to the public MCP origin, or the local address on loopback binds ROCKETRIDE_URI had no default, so on staging/prod every MCP tool call raised "Missing engine URI". The URI is now resolved once in initModule and injected into the config every engine client is built from, and it also drives the widget CSP origin and the upload/dropper links: 1. explicit: config rocketride_uri / env ROCKETRIDE_URI, used as-is 2. loopback default: ws://127.0.0.1:<port> (ws://[::1]:<port>) so a laptop engine never silently calls RocketRide's cloud 3. public default: the MCP_RESOURCE_IDENTIFIER origin, https->wss / http->ws (default wss://api.rocketride.ai) One startup log line names the resolved URI (userinfo/query stripped) and the rule that chose it. ROCKETRIDE_AUTH/APIKEY handling is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): only offer send_files on loopback-bound engines — it reads paths on the engine host The SDK's send_files checks and opens each caller-supplied path on the machine running the MCP module. On a deployed engine that is our server, so any authenticated caller could probe file existence ("File not found") and upload server files into their own pipeline. initModule now decides local_engine = is_loopback_bind(bind_host) once and passes it to build_mcp_server -> ToolRegistry(local_engine=...). Tools registered local_engine_only (send_files) are neither listed nor dispatched on a deployed engine: a call by name gets an Unavailable refusal that ignores the client and arguments. The registry defaults to the deployed surface. Loopback binds are unchanged. Also corrects the send_files description (engine-host paths, not store-relative), drops the stale "or filepath" from run_pipeline, and updates the MCP docs and agent skill contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): send no SDK env from deployed engines so caller secrets keep precedence WsEngineClient built RocketRideClient(uri, auth) with no env, so the SDK loaded os.environ plus ./.env and use() forwarded every ROCKETRIDE_* var as the run's env. The engine applies caller env last, over org/team/user secrets, so on a deployed engine the server's own ROCKETRIDE_* vars overrode a customer's same-named secrets for MCP-started runs. initModule injects local_engine (the same loopback decision that gates send_files) into config alongside the resolved URI; make_engine_client builds the client with env={} unless it is set. Loopback binds keep the SDK default. Explicit uri/auth are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): rewrite deploy tools against the current SDK deploy API The MCP deploy tools (#1880) were written against a Python SDK deploy API that deploy-2 (#1764, 0dfdc8b) had already removed. In production every call failed: deploy_add passed a `schedule` kwarg add() no longer takes, deploy_status/deploy_update called methods that no longer exist, deploy_remove omitted the now-required team_id, and deploy_list counted the keys of the {rows,total,page,pageSize} envelope. The suite stayed green because FakeEngine and the seam tests mirrored the old API. The seam (WsEngineClient) is now one keyword-argument passthrough per DeployApi method, and the tools are thin wrappers in the teams-as-environments model: - deploy_add -> add (pipeline, comment, deploy_to) - deploy_list -> list (team_id, paging, search, filters, sort); returns deployments + count/total/page/pageSize - deploy_status -> get (project_id, team_id) - deploy_versions -> versions (project_id, paging) - deploy_to_team -> deploy (project_id, version, team_id) - deploy_set_schedule -> set_schedule (project_id, source_id, schedule, team_id, ttl) - deploy_enable / deploy_disable / deploy_remove -> enable / disable / remove (project_id, team_id) deploy_update is gone; its operations are deploy_add + deploy_to_team and deploy_set_schedule. The rocketride://pipelines resource returns the unwrapped envelope. Tool count: 33 (32 on a deployed engine). Drift guard: test_engine.py calls every deploy_* seam method against a recording fake and binds each recorded call to the real rocketride.deploy.DeployApi signature (method must exist, arguments must bind, every seam argument must arrive under the same name). It fails on the old seam code. A second test pins the EngineClient protocol and the conftest FakeEngineClient to the seam's deploy signatures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(task_http): don't re-send engine env as caller env from task_Execute on deployed engines task_Execute built RocketRideClient(uri, auth) with no env, so the SDK snapshotted os.environ plus ./.env and use() forwarded every ROCKETRIDE_* var as the run's caller env. The engine applies caller env last, over the org/team/user secrets from get_merged_env, so on a deployed engine the server's own ROCKETRIDE_* vars overrode a user's same-named secrets for runs started via POST /task. Apply the rule MCP already uses (7670865): the client is built with env={} unless the server's configured bind host is loopback (ai.modules.mcp.auth.is_loopback_bind, falling back to the same CONST_DEFAULT_WEB_HOST WebServer binds with). Loopback binds keep the SDK default so a local developer's .env still applies. The MCP import is deferred so loading task_http doesn't pull in the MCP package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): close the credential-less auth hole and harden the MCP boundary (#2315) Addresses the four review findings on PR #2315. 1. Log the resolved bind mode at startup. `is_loopback_bind` judges the CONFIGURED host and `CONST_DEFAULT_WEB_HOST` is 'localhost', so a deployment that never sets `host` silently runs as a local engine. `is_loopback_bind` is unchanged (as the reviewer asked); instead the first lines of the log now state the mode, the host it was judged from, and what that grants (dev-bypass eligibility, `send_files`, caller-env forwarding), with a WARNING when the bind is loopback only by fallback. 2. Refuse credential-less MCP requests. A public route pattern matching an MCP descendant (`/mcp/{path}`) makes AuthMiddleware skip the request while the Mount still forwards it to `handle_mcp`, where `authorize()` used to return None and run tools on the shared, server-identity engine client. Both ends are fixed: `authorize()` now refuses a request with no credential unless the loopback dev bypass is genuinely active, and the startup guard probes representative descendant paths through `is_public_route`, not just `/mcp` and `/mcp/`. The bypass rule itself lives in one place -- `auth.dev_bypass_active` -- used by both `initModule` and `authorize`. 3. Require encrypted engine transport on a non-loopback bind. `handle_mcp` hands the caller's credential to `WsEngineClient`, which sends it in the first DAP `auth` message, so a `ws://`/`http://` engine URI (explicit or derived from the MCP resource identifier) now fails boot with a message naming the value and the variable it came from. Loopback binds keep cleartext; the https staging/prod defaults are unaffected. 4. Reconcile MCP_TOOL_CONTRACT.md with the registrations. The tables listed ~23 of the 33 registered tools: `resolve_config` and `scaffold_node` were undocumented and grouped capability rows hid individual tools. Every tool now has its own row derived from its real schema and result, and the stale `origin/develop @ eb67dde` pin is replaced with what is accurate now. Test-first: 27 new tests fail against the previous implementation. Three existing tests encoded behaviour findings 2 and 3 deliberately change and were updated; the WebServer-backed route tests now clear ROCKETRIDE_URI / MCP_RESOURCE_IDENTIFIER after construction so a developer's dist/server/.env no longer decides what they resolve. 717 passed, 0 skipped (was 683). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): key the startup guards on patterns and targets, not probes and binds (#2315) Three follow-ups to the review findings on PR #2315. 1. The startup guard now inspects the registered public PATTERN STRINGS, not just probe literals. Public entries are compiled with `compile_path`, so a converter-typed pattern (`/mcp/{id:int}`, `/mcp/{x:uuid}`) narrows the regex to values no fixed probe satisfies -- the guard missed it while AuthMiddleware still skipped auth for every `/mcp/<digits>` the Mount forwards. `_registered_public_patterns` reads `_public_paths` (real WebServer) and `public` (test double), the same pair the dev bypass already writes to, and degrades to nothing when neither is present. `_claims_mcp_pattern` normalises (whitespace, trailing slashes) and refuses only a whole-segment match, so `/mcp`, `/mcp/` and `/mcp/{...}` trip while `/mcpx` and `/mcp-server` do not. Message shape and per-claimant naming are unchanged. 2. The cleartext refusal now keys on the engine URI's TARGET HOST rather than the bind. A credential sent to `ws://127.0.0.1:5565` never reaches a wire anyone can tap, and the bind-keyed rule was a deployment hazard: `WebServer.__init__` loads `dist/server/.env`, `.env.template` ships `ROCKETRIDE_URI=http://localhost:5565`, and `docker/Dockerfile.engine` copies `dist/server/` into the image, so a pod carrying that file would have failed to boot. `auth.is_loopback_bind` judges a configured bind string against exact literals and does not fit a URI netloc (userinfo, port, IPv6 brackets, the whole 127.0.0.0/8 range), so a small `_targets_loopback_host` helper was added next to the rule instead of bending it; it still reuses `auth.LOOPBACK_HOSTS` for the by-name allowlist. Non-loopback targets fail boot as before, with the same redaction. 3. `MCP_TOOL_CONTRACT.md`'s `scaffold_node` row described only `local_nodes/<name>/...`, so a contract-following client omitted the `local_nodes/__init__.py` package marker `_scaffold_node` also returns and the engine could not import `local_nodes.<name>`. The row now names the marker and says to write every returned path; `docs/public/mcp/http/tools.md` carried the same omission and is corrected too. The engine-module doc now states the target-keyed transport rule. Test-first for 1 and 2: 46 new tests fail against the previous implementation. No existing test encoded the old bind-keyed rule (every refusal case already used a non-loopback target), so none needed updating. 763 passed, 0 skipped (was 717). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): refuse a cleartext engine URI on every bind, not just a public one (#2315) Closes the gap the previous commit left in `_resolve_engine_uri`. That commit moved the cleartext refusal onto the engine's TARGET host (`_targets_loopback_host`), but the explicit branch still guarded the call with the BIND: if explicit: if not loopback: _refuse_cleartext_engine_uri(...) so a loopback-bound MCP server with `ROCKETRIDE_URI=ws://engine.remote:5565` skipped the check entirely and sent the CALLER's credential over the network in the clear -- `handle_mcp` binds it to the per-request engine client and `WsEngineClient` puts it in the first DAP `auth` frame. The bind says nothing about whether the wire is exposed; only the target does. `_refuse_cleartext_engine_uri` now runs for every explicit URI, on any bind, with the same `rocketride_uri` / `ROCKETRIDE_URI` source naming and the same redaction. Loopback TARGETS stay allowed inside the helper, so the shipped `dist/server/.env` (`ROCKETRIDE_URI=http://localhost:5565`) still boots. The local `loopback` variable is gone: `auth.is_loopback_bind` is now read inline where it genuinely selects the DEFAULT URI, so no bind-keyed condition is left anywhere near the encryption rule. The `loopback default` branch resolves `ws://127.0.0.1:<port>`, a loopback target, and the `public default` branch already called the helper. Docs and docstrings that stated the old bind-keyed rule now say the check keys on the target host: `_resolve_engine_uri`, `initModule`, `_refuse_cleartext_engine_uri`, and `docs/development/engine/mcp-module.md`. `docs/public/mcp/http/self-hosting.md` never stated the rule at all even though it fails an operator's boot, so its guard list now carries it. Test-first: 8 new cases fail against the previous implementation (explicit `ws://` and `http://` remote targets on `127.0.0.1` / `localhost` / `::1` binds, from env and from config, redaction included), plus explicit `wss://` accepted on any bind and the loopback-target cases extended over loopback binds. Two existing tests used an explicit remote `ws://` URI incidentally -- one for dropper-link origins, one for `engine_origin` threading -- and now use `wss://`; neither was asserting the transport rule. 794 passed, 0 skipped (was 763). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…l sends (#2346) * fix(ai): accept the single-component validation payload the node panel sends The node config panel validates one component on save and sends `{version, component}` — the `IComponentValidatePayload` shape the shell contract declares this endpoint accepts (shell/src/types/project.ts:308). The engine validates pipelines and requires `components`, so every save answered "'pipeline.components' must be an array", whatever the node. The component now travels as a one-item `components` list, built before the source is inferred so `resolve_implied_source` sees it. A config that already carries `components` is untouched. Reachable since #1573: until then `rrext_validate` failed on every call with "'pipeline' is missing or invalid", so this payload never reached the validator. The panel has sent this shape since #1843 (7 Aug). Removing the four lines fails the two new tests, and the engine itself answers the reported message: validatePipeline({'pipeline': {'version': 1, 'component': c}}) -> "'pipeline.components' must be an array" validatePipeline({'pipeline': {'version': 1, 'components': [c]}}) -> 0 errors * docs(clients,ai): document the single-component validate payload The endpoint now accepts {version, component} as well as a full pipeline, which is a public-contract change: both SDK references say so, and on_rrext_validate's Args: block does too — the inline comment explained it, but the docstring is what a reader hits first. Also states that a payload carrying both keys keeps 'components' and drops the stray 'component', so the next reader does not have to derive that it is deliberate.
Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* fix(notion): accept search pagination cursors * test(notion): cover empty search cursors --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
…sh_schema tool (#2283) * fix(ai,nodes): surface database error text from execute and add refresh_schema tool Two gaps in the shared database node base, both scoped to stay out of the way of the pull requests already open against this file. _executeRawQuery caught every SQLAlchemyError, logged it, and returned None, so execute() answered with the same 'check server logs for details' string whether the statement had a typo, named a missing table, or was refused for permissions. Its except clause now raises with the driver's own message through IGlobal._format_db_error. Only that clause changes: execute()'s `if result is None` branch becomes unreachable but is left in place, with a comment saying so, because an open PR is rewriting that region. Session-bound execute already re-raised the real exception, so the two paths finally agree, and the max_execute_rows rollback is untouched. IGlobal.db_schema is assigned exactly once, in beginGlobal, so get_schema serves a task-start snapshot for the life of the task and a table created through execute stays invisible until the pipeline restarts. The new refresh_schema tool re-reflects and replaces it, returning the get_schema shape plus a refreshed_at UTC timestamp so a caller can show how current the schema it holds is. Reflection and publication run under a module-level lock: concurrent callers would otherwise repeat the same full table walk and race to publish the result. The tool is additive and lives entirely in db_instance_base.py, since another open PR is adding to db_global_base.py. It also fixes the natural-language path, which describes that same dict to the LLM. get_schema's nested table formatter is lifted to a shared private method so both tools render one shape. Tests are a new module rather than an addition to test_db_base.py, which two open PRs are both appending to. Eleven cases over an in-memory SQLite engine cover the driver text reaching the caller, the gate still refusing first, re-reflection seeing created and dropped tables, the timestamp format, and four concurrent refreshes all returning. Tool tables in the four node READMEs that inherit this surface list refresh_schema and say in one sentence what get_schema's snapshot does not show. services.json declares no tool list, so nothing to mirror there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd * test(ai): give the execute-session IGlobal stub a real _format_db_error execute()'s stateless failure path now formats the driver error through IGlobal, but this module builds IGlobal as a SimpleNamespace carrying only the four fields the transaction tests needed. Nothing failed, because no test here drove a bad statement through that path — the next one to try would have died with AttributeError instead of the RuntimeError it was asserting. The stub borrows the real DatabaseGlobalBase implementation rather than a lambda, so the message a test sees is the message a caller gets. A test for the stateless failure path comes with it, so the fixture stays honest: removing the one line makes it fail with AttributeError. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd * refactor(ai): shrink the refresh_schema diff in db_instance_base No behaviour change; this only narrows what the previous commit touched in a file three open pull requests are editing. get_schema's body is back to exactly what develop has, docstring aside: the nested _format_table closure returns, and the _format_table static method and _schemaPayload helper it had been lifted into are gone. Sharing them was tidier, but it rewrote a method none of the open PRs needs to change, and a few duplicated formatting lines cost less than that. That hunk is now five added docstring lines instead of a 31-line rewrite. refresh_schema moves to the end of the tool section, below dialect, so the whole tool arrives as one appended block past the execute body PR #1908 rewrites rather than as an insertion ahead of it. It formats its own tables inline, and the lock is now _REFLECT_LOCK. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd * docs(nodes): complete the agent tool contract in the db READMEs Two review findings on the READMEs this PR touched, both in the section the node README schema calls the agent-facing contract and holds to the highest accuracy bar. db_postgres listed four tool functions in the summary and in "As a tool", then a separate table of three transaction tools — seven of the nine @tool_function methods an agent actually sees on DatabaseInstanceBase. `execute` appeared only as prose inside the transaction paragraph and `dialect` only as the QuestionType.DIALECT lane behaviour, so an agent author reading the inventory would not know either exists as a tool. The count is now nine in both places, `dialect` joins the open table, and `execute` joins what was the Transactions table, renamed "Raw SQL and transactions" because a statement without a session_id is neither. That table's gate sentence also said requests are "silently dropped" when allow_execute is off; the four tools raise (db_instance_base.py:301, 339, 371), and it is the questions lane that logs and drops — the two behaviours are now stated apart. The `execute` row records the driver message and the rollback-on-overflow that this PR's own change produced, rather than implying a truncated result set. rocketride_sql named refresh_schema in its tool table but left its contract out of the prose beneath, where that README already documents every other non-obvious argument and return shape. It takes no arguments (input_schema declares no properties) and returns {database, tables} exactly as get_schema does, plus a refreshed_at UTC ISO-8601 timestamp from datetime.now(timezone.utc).isoformat(). Docs only; no behaviour change. Both nodes still pass scripts/validate-node-readme.py, and so does the whole corpus under ./builder docs:validate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ai): keep SQL statements and bind parameters out of execute error text `_format_db_error` only produced a clean `Error <code>: <msg>` when the driver put an integer first in `orig.args` — the pymysql and clickhouse-driver shape. psycopg2 and sqlite3 put a string there, so both fell through to `str(exc)`, which for a SQLAlchemy StatementError is the full repr: the executed statement in `[SQL: ...]` and the values bound into it in `[parameters: ...]`. That string is no longer log-only. `_executeRawQuery` re-raises it as a RuntimeError that the `execute` tool returns to its caller, and #2282 wires a user-facing Refresh/Run UI onto exactly that path, so a failed INSERT would hand the caller back the row it tried to write. Unwrap each driver shape explicitly and keep only the primary message: the numeric-code branch as before; psycopg2's `diag.message_primary` (its `str()` also carries a `LINE n:` echo of the statement, which psycopg2 has already interpolated the bind values into); and `args[0]` for sqlite3 and the generic DBAPI shape. Every branch then runs through `_strip_statement_detail`, which truncates at the first `[SQL:`, `[parameters:`, `LINE n:`, `DETAIL:`/`HINT:`/`CONTEXT:` or sqlalche.me marker, so a driver shape not enumerated here still cannot leak the tail. DETAIL is dropped along with the rest because PostgreSQL uses it to restate the offending key values. The helper is module-level rather than a method so `_format_db_error` keeps working when tests bind it onto a stub IGlobal. Tests cover the real sqlite3 driver end-to-end through `execute`, plus the psycopg2 (with and without `diag`), sqlite3 and pymysql shapes and a bare StatementError repr. `test_format_db_error_falls_back_to_str_when_args_not_int_first` pinned the old leaking fallback, so it is replaced by an assertion that the driver message — not `str(exc)` — is what comes back. `_strip_statement_detail`'s degenerate case gets the same treatment. A message that is nothing but detail markers left no primary sentence, and the old fallback returned the first line — which on that input is the `[SQL: ...]` echo the function exists to remove. It now returns a neutral constant, so there is no input on which the helper hands back the statement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ai): refresh the answers-lane column map in refresh_schema `refresh_schema` replaced `IGlobal.db_schema`, the database-wide reflection the natural-language path describes to the LLM, and stopped there. `IGlobal.schema` — the configured table's column map that `_insertData` iterates to build every answers-lane INSERT — is a second start-up snapshot, and it was left untouched, rebuilt only lazily when falsy. So the node was current on one path and stale on the other: after running `ALTER TABLE ... ADD COLUMN` and calling the refresh tool, the LLM path saw the new column while the insert lane kept writing the start-up column set and silently dropped it. The four db node READMEs document both the insert lane and "re-reads the schema from the database", so the documented contract overstated the code. Invalidate the map inside the same reflection lock. Emptying it re-arms the lazy rebuild already at the top of `_insertData`, which reflects through the same `_getTableSchema` call `beginGlobal` uses, so the next insert sees what a freshly started node would. Emptying rather than re-reflecting here keeps the call cheap for a node with no answers lane wired, and makes the write a single atomic rebind: a concurrent `_insertData` either reads the old map or finds it falsy and rebuilds, never a half-built dict. Invalidating the map is only safe once `_insertData` stops binding a generated primary key, so that is fixed in the same commit. `_createTableFromData` curates `IGlobal.schema` down to the data columns for an auto-created table, because the `id` it prepends is generated by the database; a map built by reflection carries `id` instead. Since `_insertData` binds NULL for any schema column the rows do not supply, swapping the curated map for a reflected one would have started binding `id=None` on every later insert — tolerated by SQLite's rowid alias, a not-null violation against the `id SERIAL NOT NULL` Postgres renders for the same column. `_insertData` now drops primary-key columns that no row in the batch supplies, decided once per batch so every mapping handed to executemany keeps an identical key set. The two maps therefore produce the same INSERT and refreshing cannot change answers-lane behaviour mid-task. It also fixes the same NULL bind on the path that always reflected: a table that already existed when `beginGlobal` ran. Tests cover the refreshed insert picking up a new column, the pre-existing stale behaviour it fixes, a dropped configured table leaving a falsy map, and — by asserting on the compiled INSERT rather than on what SQLite tolerates — that an auto-created table's insert still carries the data columns only after a refresh, that a reflected primary key the rows do not supply is omitted, that one they do supply is still bound, and that a primary-key-only table falls back to binding it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(nodes): describe the raw-SQL gate the db nodes actually have The `Raw SQL and transactions` section said `allow_execute` is "the same gate as `QuestionType.EXECUTE`" and contrasted the tools with "the `questions` lane, which logs and drops the request". No SQL database node implements that branch: `DatabaseInstanceBase.writeQuestions` does not look at `question.type` at all, and `QuestionType` appears in db_instance_base.py only at the import and in one unrelated `Question(type=QuestionType.QUESTION, ...)` construction. The dispatch-on-type behaviour belongs to the graph node (`graph_instance_base.py` handles `DIALECT` and `EXECUTE`), and the wording was carried across without being checked against this file. Correct it wherever it appears in prose: - db_postgres: the "Two special question types are handled on the questions lane" bullets are removed — the lane has one behaviour, translate-then-execute, and `dialect` / `execute` are the tool functions that reach those behaviours. The gate sentence now says what `allow_execute` gates (the four raw-SQL tools) and that it does not change the questions lane, which never runs raw SQL. - rocketride_sql: same two claims, in the config and Notes sections. Also make the refresh_schema wording match the code now that the tool invalidates the answers-lane column map as well as the database-wide schema, so "re-reads the schema from the database" is true end to end rather than only for the LLM path. The same claim lived in the generated config tables, whose source of truth is the `allow_execute` field description in each node's services*.json. Those five descriptions now name the four tool functions the flag actually gates, and the generated regions are regenerated from them, so db_postgres/README.md no longer contradicts itself between its prose and its parameter table. Only the SQL nodes are touched: the graph nodes do dispatch on `QuestionType.EXECUTE`, so their copy is correct as it stands. Two smaller instances of the same transposition: the new db_postgres sentence named `db_neo4j`, a retired path that redirects to `graph_neo4j`, and db_clickhouse/IInstance.py said its dialect string is "surfaced via QuestionType.DIALECT" when only the `dialect` tool reaches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ai): apply the same execute error contract to the session path A raw statement run with a session_id went through TransactionRegistry.execute, which has no try/except, and the existing `except Exception` arm in execute() re-raised the SQLAlchemy exception unchanged. str() of that exception ends in `[SQL: ...]` and `[parameters: ...]`, so the same tool behind the same allow_execute gate returned the statement and its bound values to the caller purely because a session id was passed, while the sessionless half stripped them. Add a `SQLAlchemyError` arm above the existing `except Exception` (the max-rows RuntimeError is not a SQLAlchemyError, so its wording and its rollback are untouched): log the full exception, release the session, and raise `RuntimeError('SQL execution failed: ' + _format_db_error(e))`. The fix lives in execute() rather than tx_registry because the registry has no IGlobal and is shared with other callers. Session release moves into _releaseFailedSession, shared by both arms. A rollback that itself fails is logged and swallowed: previously an exception raised inside the cleanup replaced the error the caller was being told about (a monkeypatched rollback surfaced as 'boom'). Also drop the dead `result is None` branch in execute() and correct _executeRawQuery's annotation to `-> dict`: the method has returned the shaped dict or raised since it started carrying the driver's message, so the branch could only ever have produced the opaque 'check server logs for details' string this work exists to replace, and the annotation contradicted the comment above the branch. execute() is its only caller. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * refactor(ai): share one table formatter between get_schema and refresh_schema refresh_schema rebuilt get_schema's nested _format_table line for line, with `result` renamed to `entry`. Both tools promise callers the same per-table shape -- their tool descriptions say so, and an LLM picks between them on that sentence -- but the equality test compared the two copies to each other, so it would have stayed green while a later change added a key (an index list, a column comment, a nullable flag) to one copy only. Lift the formatter to a module-level _format_table and call it from both; get_schema's body loses the nested definition, refresh_schema loses the loop. The wire format is byte-identical, so the existing shape tests pass untouched. The docstring paragraph calling the duplication deliberate goes with it. test_refresh_schema_matches_get_schema_shape now reflects a table with both a primary key and a foreign key, so the two optional entries are actually exercised on both sides, and _format_table gains a direct unit test for the optional-key rules an equality check cannot see. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): make the lazy schema rebuild safe for concurrent inserts refresh_schema empties IGlobal.schema to re-arm the rebuild at the top of _insertData, which is what makes this reachable at runtime: a second insert can now arrive while the first is reflecting. _getTableSchema published `self.schema = {}` and then grew it column by column, and _insertData checked, rebuilt and then took a reference to that same dict without a lock. A concurrent insert could therefore see the empty map (falling into the "no schema cached" branch, which binds the item's raw keys and skips the generated-key handling), or a truthy but half-built one (silently dropping columns the caller had supplied), or a dict changing size while the per-row loop iterated it. A reflection that failed midway left the half-built map published for the next insert to believe. _getTableSchema now builds into a local dict and publishes it with one assignment, so a failure leaves the previous map untouched. _insertData holds _REFLECT_LOCK across the falsy check, the rebuild and a snapshot copy, and builds the batch from the snapshot; the lock is released before the Table reflection and before the INSERT, neither of which reads the map. The comment above refresh_schema's invalidation no longer claims the single rebind makes the rebuild atomic. Tests hold a rebuild open inside the real column walk rather than simulating the window: without the lock the second insert stored NULL in a column it had supplied, an overlapping refresh left a one-column map, and a failed reflection published a partial one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): decide generated primary keys per row, not per batch The answers lane worked out which primary-key columns to leave unbound once per batch, from the union of every row's keys. One row carrying `id` therefore put `id` into every mapping, and the rows that omitted it bound NULL into a key the database was supposed to generate. SQLite tolerates that for a rowid alias; Postgres rejects the whole batch for an `id SERIAL NOT NULL` column. The sole-primary-key table had the same problem by construction: the `omit_columns = set()` fallback bound NULL because omitting everything would have left an empty mapping. Which primary keys the database generates is now read from the reflected Table -- `autoincrement_column` (SQLAlchemy's own resolution, covering a reflected `autoincrement=True` and the `'auto'` rule that recognises SQLite's rowid alias), plus any primary-key column with a server_default, an Identity, or an explicit autoincrement -- rather than assumed of every primary key. Per row: a generated key the row omits is left out of that row's mapping, a supplied value is bound, and an explicitly supplied None is bound as NULL, so the three cases stay distinguishable. A primary key the database does NOT generate and the row does not carry is a ValueError naming the table, the columns and the row position, raised before the transaction opens. Rows are then executed as contiguous runs sharing a key set, all inside one engine.begin(): contiguous rather than gathered, because generated ids follow insertion order and regrouping would hand back ids in an order the caller's rows never had, and one transaction because a batch must stay all-or-nothing as a single executemany was. A run whose mapping is empty gets a values-less `insert(table)` per row, which SQLAlchemy renders as `DEFAULT VALUES` on PostgreSQL and SQLite and `() VALUES ()` on MySQL. test_insert_binds_the_primary_key_when_it_is_the_only_column pinned the old fallback and is replaced by the default-values test, plus a compile-only check of all three dialect renderings (no live PostgreSQL or MySQL in this suite). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(ai): pin the caller-facing error policy for Postgres and MySQL CodeRabbit's security finding asks for a generic message or a stable category instead of the driver's own words. That is not adopted: the maintainer's review endorses the primary-message contract precisely because "no such column: foo" is what lets a caller fix their statement. What was missing was the policy written down and held in place by tests. _format_db_error's docstring now states the residual it accepts: a driver's primary sentence may quote a value from the statement being reported (Postgres `invalid input syntax for type integer: "abc"`, MySQL `Duplicate entry 'x' for key 'users.email'`). That value was submitted by the caller now reading the message, who also holds allow_execute; values belonging to other rows appear only in DETAIL / HINT / CONTEXT, which are stripped. Blanking quoted tokens generically is rejected because MySQL quotes identifiers with single quotes too, so the redaction would delete "which column" from the message that most needs it. The tests build real DBAPIError wrappers via DBAPIError.instance, so str(exc) genuinely carries the `[SQL: ...]` / `[parameters: ...]` tail and the assertions bite: a Postgres duplicate-key whose DETAIL restates the email, the same without .diag so the LINE echo is exercised, the MySQL duplicate entry that documents the accepted residual, and the MySQL unknown-column that documents why redaction is refused. One more drives both shapes through BOTH execute paths and asserts the two messages are identical -- against the pre-fix source the session-bound half raised the raw DBAPIError with the bound 'ada@example.com' attached. Mock-based by necessity: no live PostgreSQL or MySQL in this suite, so the driver exception shapes are reproduced rather than provoked. No behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(nodes): correct the lane, cache and error prose in the four SQL READMEs All four checked against the node code and services.json rather than against each other. db_clickhouse declares only a `questions` lane (services.json `lanes: {questions: [...]}`), so the sentence about schema refresh reaching the `answers` insert lane described a lane the node does not have; it is removed. db_mysql, db_postgres and rocketride_sql do declare `answers`, so theirs stays — reworded, because refresh_schema invalidates that column map rather than rebuilding it, and the rebuild happens on the next insert. "Return the schema reflected when the node started" was wrong in all four after refresh_schema shipped: get_schema serves whatever snapshot the node currently holds, which is the start-up reflection until a refresh replaces it. db_postgres said every question on the `questions` lane is translated to SQL and executed. When the LLM reports the question is not a database query (`isValid: false`), writeQuestions emits its prose answer and nothing runs against the database, so the lane is described as the natural-language path with only valid generated SQL executed. Each README now states the execute error contract the code enforces: a failure raises `SQL execution failed:` plus the driver's primary message, identically with and without a session_id, with the statement and bound parameters kept to the server log — and with the residual named, since a driver's primary sentence can quote a value the caller itself submitted. scripts/validate-node-readme.py passes for all four nodes; no GENERATED block touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): publish the auto-created table's column map in one assignment _createTableFromData is the second writer of IGlobal.schema and still had the publish-then-fill shape B4 removed from _getTableSchema: it created the table, assigned `self.schema = {}`, then added one key per column, all outside _REFLECT_LOCK. By the time that loop runs the table exists, so a second first-batch can already be inside _insertData; it takes the lock, finds a truthy one-column map, snapshots it and silently drops every other column its row carried. Reproduced by pausing the real fill loop: the second row landed as {'label': 'b', 'size': None, 'colour': None} for input that supplied all three. The map is now built in a local dict and published in a single assignment. The lock is NOT widened over table creation, so the pre-existing double-create race is untouched and still out of scope. Also seeds a stale map into a direct _getTableSchema failure test: the existing rebuild-failure test starts from {}, which cannot tell "previous map left untouched" from "map reset to {}". Against the pre-B4 source that new test fails with the stale entry replaced by a partial one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): cut the clickhouse stack trace and position echo from execute errors The formatter's docstring claimed "pymysql / clickhouse-driver put (errno, message) in .orig.args". Only pymysql does. clickhouse-driver's DBAPI layer raises OperationalError(ServerException), so .orig.args holds one exception object, every branch falls through to str(orig), and what reached the caller was the driver's full string form: "Code: N.\n DB::Exception: <message>. Stack trace:\n\n0. DB::Exception::Exception(...) @ 0x..." — server internals the caller cannot act on — plus, for a syntax error, the failing statement fragment quoted after "failed at position N ('...')", which is the same class of statement echo as psycopg2's "LINE n:". _DB_ERROR_DETAIL gains "Stack trace:" and "failed at position" as markers, so both are cut by the stripper that already handles the other drivers rather than by a new branch. Neither phrase can misfire on the shapes the other drivers produce. The docstring now states the real clickhouse-driver shape instead of claiming the pymysql one. Two constructed tests cover it, labelled in their docstrings as reproduced from the driver's documented behaviour: clickhouse-driver is not installed in this engine, so the shape cannot be provoked here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ai): state the real execute error contract and schema-cache wording Three corrections to text, no behaviour change. The error-contract paragraph overclaimed. It said values belonging to other rows appear only in DETAIL / HINT / CONTEXT, which are stripped. Both halves of that are wrong: a real SQLite run returns `SQL execution failed: near "'hunter2'": syntax error` — the statement fragment the parser stopped on is in the primary sentence — and PostgreSQL's `invalid input syntax for type integer: "<value>"` names a row value an INSERT ... SELECT or a CAST merely touched, which can come from another table. pymysql and psycopg2 interpolate binds client-side, so such a fragment can carry a bound value too. The docstring now lists what is removed (the SQLAlchemy tail, LINE n:, DETAIL/HINT/CONTEXT/ QUERY/STATEMENT, the ClickHouse stack trace and position echo) and what is passed through, and rests the policy on the argument that actually holds: the recipient has allow_execute and can already read the same data with a SELECT, so the error channel does not widen read access. Two tests (one per execute path) pin the syntax-error shape on a real SQLite run. get_schema's docstring said the schema is "reflected once in beginGlobal" and refresh_schema's tool description — the sentence an LLM reads when choosing between the two tools — said get_schema serves the snapshot reflected when the node started. Both became false the moment refresh_schema shipped, and both contradicted the READMEs corrected for exactly that in c258211d. _generated_primary_keys gains a paragraph recording the SQLite-only residuals of SQLAlchemy's 'auto' rule: INT/BIGINT/INTEGER-DESC primary keys are treated as generated although SQLite does not alias them to the rowid, and an INTEGER PRIMARY KEY that is also a foreign key is treated as required although it is an alias. PostgreSQL and MySQL reflection set autoincrement explicitly, so production nodes resolve exactly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(nodes): correct the error-contract and remaining schema wording Two corrections, both to sentences this PR itself introduced or left behind. The error sentence in all four READMEs promised that "the statement text and the bound parameter values are never part of it". That is not what the code does: a real run returns `SQL execution failed: near "'hunter2'": syntax error`, and PostgreSQL's `invalid input syntax for type integer: "<value>"` names a value an INSERT ... SELECT or a CAST read from another table. Each README now says what is actually removed (SQLAlchemy's `[SQL:]`/`[parameters:]` echo, and per engine: PostgreSQL's LINE n:/DETAIL/HINT/CONTEXT, ClickHouse's server stack trace and its `failed at position` echo) and that the primary sentence is passed through as the database wrote it, so it can name a value the statement carried or touched — resting the policy on the argument that holds, that reaching this tool requires Allow direct query execution and such a caller can read the same data with a SELECT. The full text stays in the server log. db_postgres moves that contract out of the already-long `execute` table cell into prose below the table, as the other three do. The intro sentences in db_clickhouse and db_mysql still said "startup-reflected schema", the wording corrected everywhere else in c258211d; both now describe the cached snapshot and name refresh_schema. scripts/validate-node-readme.py passes for all four; no GENERATED block touched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): leave every database-owned column to the database on the insert lane Two ways an explicit NULL still reached a column the database was meant to fill, both silent because writeAnswers only logs, and one comment that claimed neither could happen. _generated_primary_keys skipped primary-key columns only, so any other newly reflected column was bound NULL — and an explicit NULL overrides a server default. The sequence the refresh tool invites: answers is auto-created as (id, q, a) with the curated map {q, a}; an agent runs ALTER TABLE answers ADD COLUMN created_at ... NOT NULL DEFAULT now(); the tool description tells it to call refresh_schema, so the next batch rebuilds the map by reflection, binds created_at = NULL, and the batch that worked a moment earlier is dropped with one log line. Reproduced on SQLite: NOT NULL constraint failed: answers.created_at, parameters ('how', 'like this', None). _insertData now also reads generated_defaults — every column with a server_default or an identity — off the reflected table and omits those the row does not supply. An explicit null on a generated key was bound as NULL. On the execute tool that is right: a person wrote the statement. On the insert lane the caller is an upstream node, and an LLM node or a JSON mapper emits every schema key, writing null for the ones it has no value for — the same intent as omitting it. Postgres rejects that NULL for a SERIAL column; MySQL and SQLite generate a value, so the failure is dialect-dependent as well as silent. A supplied null on a generated primary key is now treated as omitted; everywhere else it is still bound as given. The comments at refresh_schema's invalidation and above the binding loop said the invalidation cannot change answers-lane behaviour. This PR's own test shows it can, and that half is the point of the tool. They now state the guarantee that actually holds: the columns present only in the reflected map are omitted rather than bound NULL, so the database still fills in what it owns. Also drops the unreachable "no schema cached" branch: after the locked block IGlobal.schema is populated or the method has raised, so the snapshot is always truthy. The empty-map half of the lock rationale went with it; the half-built-map half, which the concurrency test proves, stays. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): format driver errors the dialect did not wrap in a SQLAlchemy exception The error contract this PR documents was not delivered on the ClickHouse node at all. db_clickhouse connects with `clickhouse+native://`, which is clickhouse-sqlalchemy's own connector; its Cursor.execute raises `clickhouse_sqlalchemy.exceptions.DatabaseException`, a plain Exception subclass carrying the driver's ServerException in `.orig`. SQLAlchemy does not wrap a non-DBAPI exception, so `except SQLAlchemyError` never fired on EITHER execute path, `_format_db_error` never ran, and the caller got `Orig exception: Code: 62.\nSyntax error: failed at position 21 ('hunter2') ... Stack trace:\n\n0. DB::Exception::Exception(...)` — raw, on both paths. The markers added in 27a04f68 were verified against clickhouse-driver's own DBAPI layer, which this node does not use, so they were unreachable for the node they were added for. Both paths now handle a non-SQLAlchemy exception that carries a driver `.orig`: log it and raise the same `SQL execution failed: <formatted>` RuntimeError, releasing the session on the session path as before. Duck -typed on `.orig` rather than importing clickhouse modules into the shared base — a driver error carrying its original is precisely the shape being handled, and the base must not depend on one node's driver. Anything without `.orig` is re-raised untouched, so the max_execute_rows RuntimeError keeps its wording and rollback and a programming error still surfaces as itself. _format_db_error also reads `.code` and `.message` off the original when they are an int and a string: clickhouse-driver's ServerException puts the code there rather than in args, so the numeric branch missed it and the caller lost the code. ClickHouse now reads like pymysql — `Error 60: Table default.widgets doesn't exist` — and the message still runs through the stripper, so the stack trace and the `failed at position` fragment inside it are cut. The two existing clickhouse formatter tests are relabelled: they cover clickhouse_driver.dbapi's shape, which this node never produces. New tests cover the connector's real shape at the formatter and drive it through both execute paths, asserting no stack trace, no statement fragment, and a released session; two more assert that an exception without `.orig` is not swallowed. All labelled as constructed shapes — clickhouse-sqlalchemy is not installed in this engine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): reject a malformed params argument instead of misreporting it `to_sqlalchemy_text` turns `$n` into a bind by indexing `params[n - 1]`, so a JSON object for `params` raised `KeyError: 0` and a short list an `IndexError`, both from inside the rewriter. The sessionless path leaked those bare; worse, on the session path the `KeyError` was caught by the arm that maps a missing session id, so `execute` reported "unknown or expired transaction session: <sid>" for a session that was still open and still usable — the caller was told to abandon a live transaction because they passed the wrong JSON type. `execute` now validates before dispatching anything: `params` must be absent or an array, and every `$n` the statement references must have a value, with the offending placeholder named. tx_registry.py is untouched — the check lives in `execute` and imports the placeholder pattern from the module that owns it so it cannot drift from the rewriter it guards. An empty list keeps its current meaning of "no binds", the same as omitting `params`, because a statement carrying a literal `$n` inside a string body relies on the rewriter leaving it alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(nodes): document the insert-lane null rule and the clickhouse error layer The three nodes that declare an `answers` lane now state, beside the existing insert prose, what the code does with a value the row does not carry: a column with a generated key or a DEFAULT is left out of the statement so the database supplies it, rather than bound as an explicit NULL that would override the default; a generated primary key supplied as `null` counts as not carried, because the sender on this lane is an upstream node that may emit every schema key, while a `null` anywhere else is inserted as given. db_postgres also records the rejection of a primary key it cannot generate. db_clickhouse declares no `answers` lane, so none of this applies there. db_clickhouse's error sentence now describes the layer that actually raises: `clickhouse+native://` does not produce a DBAPI error, so the node unwraps the driver error it carries, and the message is prefixed with the server's own `Error <code>:`. The comment above refresh_schema's invalidation also drops the last sentence that was still too strong. It said the database fills in what it owns either way, which holds only for a column the row omits. For a supplied primary key the two maps genuinely differ: on an auto-created table the curated map has no `id`, so a row carrying one has it silently dropped and the database generates a different value, and after a refresh the reflected map binds it and the row's own value is kept. The comment now says that, and notes that curating the key into the auto-created map would be a separate decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ai,nodes): tighten the refresh comment and two README sentences Comments and prose only; no behaviour change. The refresh_schema comment said _insertData "leaves out any primary-key or server-default column the rows do NOT supply, so a column present only in the reflected map is omitted rather than bound NULL". Two clauses overreached: a primary key the database cannot generate is rejected with a ValueError when the rows omit it, not left out; and a plain nullable column with no default that appears only in the reflected map IS bound NULL. The guarantee named in the sentence before it still holds -- the stored value is the NULL the database would have used anyway -- so the sentence now says that instead of describing a mechanism the code does not have. rocketride_sql's insert-lane rule had been spliced onto the end of the "Table name and database description" paragraph, running the two subjects together; it now ends a paragraph of its own. db_clickhouse's "unwraps its exception by the driver error it carries" reads as intended: "unwraps it to the driver error it carries". Two directories: 194 passed. Validator passes for both edited nodes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(nodes): regenerate the DB node parameter tables dylan-savage (review 5215826786, nit 5) flagged that the ROCKETRIDE:GENERATED:PARAMS regions of the four DB node READMEs had been hand-edited, which AGENTS.md:87-88 forbids, and that rocketride_sql/README.md carried an EMPTY region even though this PR changed its services.json. Develop has since filled that region: e035c26c3 (#2285) regenerated it from the pre-PR wording, so merging develop reinstated "Permit QuestionType.EXECUTE callers to run raw SQL..." — the exact claim this PR corrected at the source (services.json:125 now says "Permit the execute, begin, commit, and rollback tool functions..."). Regenerated with the repo generator rather than by hand: a scratch `git archive HEAD | tar -x` copy with a fresh `git init` (so the branch-name gate at nodes/scripts/gen-node-tables.mjs:211 passes and the source links resolve to the upstream fallbacks rocketride-org/rocketride-server @ develop, not this fork's origin), then `node nodes/scripts/gen-node-tables.mjs db_mysql db_postgres db_clickhouse rocketride_sql`. Only rocketride_sql/README.md changed; the generator reproduced the other three regions byte-for-byte, which also settles dylan's hand-edit concern for them — they already match generator output. Verified: python3 scripts/validate-node-readme.py PASS on all four nodes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): anchor the PostgreSQL block markers and stop stripping diag.message_primary dylan-savage (review 5215826786, item 4) showed that _DB_ERROR_DETAIL matched DETAIL:/HINT:/CONTEXT:/QUERY:/STATEMENT: as bare substrings with .search(), so the stripper truncated messages that merely contain one of those words, and that every branch — psycopg2's already-clean diag.message_primary included — ran through it. All three of his shapes reproduced on the PR head: RAISE EXCEPTION 'Order rejected. HINT: contact billing' -> 'Order rejected.' pymysql (1644, 'Invalid QUERY: missing tenant filter') -> 'Error 1644: Invalid' a message whose first token is 'HINT:' -> 'Database error' The five PostgreSQL blocks are separate LINES of the server message, so they are now matched only after a newline (\n[ \t]*), while the tails a library appends — [SQL:, [parameters:, [cached since, [generated in, (Background on this error, LINE n:, Stack trace:, failed at position — stay unanchored, because those genuinely appear mid-string and anchoring them would reopen the leak this PR exists to close. The anchor is \n rather than (?m)^ on purpose: ^ also matches at position 0, which is what turned a message beginning with 'HINT:' into all-detail and degraded it to the fallback constant. _format_db_error now returns diag.message_primary without the stripper. psycopg2 splits DETAIL/HINT/CONTEXT/QUERY/STATEMENT and the LINE n: echo onto their own diag fields, so that string carries no tail to cut and a marker firing in it could only be truncating the application's own wording. Tests (tests/ai/common/database/test_db_base.py): the three shapes above plus a message whose inline HINT: is followed by a real DETAIL: line (only the line goes), and test_format_db_error_still_cuts_a_real_postgres_detail_block, which pins that the anchoring does not let a genuine DETAIL block through on the no-diag path. Against the pre-fix code the four new assertions failed: "4 failed, 1 passed, 65 deselected in 0.52s". No live PostgreSQL or MySQL is available here, so each case is a constructed exception shape reproducing what the driver hands SQLAlchemy; every new test's docstring says so. Verified: 243 passed (tests/database + tests/ai/common/database), ruff check + format clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): cut ClickHouse's in-message statement echoes dylan-savage (review 5215826786, item 3) pointed out that _DB_ERROR_DETAIL knew only ClickHouse's `Stack trace:` and `failed at position` tails, and that ClickHouse does not always append its statement echo — code 47 (UNKNOWN_IDENTIFIER) splices the whole query into the MIDDLE of the sentence. Reproduced on the PR head by running the module's own regex: Code: 47. DB::Exception: Missing columns: 'secret' while processing query: 'SELECT secret FROM users WHERE token = ''hunter2''', required columns: 'secret'. Stack trace:… was cut only at `Stack trace:`, so the statement — with the bound value already inlined — reached the caller. The analyzer's `in scope <sql>` form leaked the same way. Three markers added: `while processing query:`, `, required columns:`, and `in scope `. The first two are distinctive enough as bare substrings; `in scope ` carries a lookahead for an uppercase statement keyword, because ClickHouse formats the echoed query back out of its AST (so the phrase is always followed by SELECT/INSERT/… there) while in prose — "not in scope for this trigger" — it is not. That is the same lesson as the DETAIL/HINT anchoring in the previous commit: a marker that can fire inside an application's own sentence will. This dialect is the one where the echo genuinely carries user data: clickhouse-driver sends no server-side parameters by default, so it interpolates binds client-side and the repeated statement contains the caller's literal rather than a placeholder. The docstring is reconciled accordingly — its opening line no longer claims "never the statement or its parameters" flatly but points at the REMOVED / PASSED THROUGH section, the ClickHouse bullet now names the two in-message echoes and the client-side interpolation, and the "every branch runs through the stripper" sentence now excepts the diag.message_primary branch the previous commit took off it. Tests (tests/ai/common/database/test_db_base.py): the code-47 shape on both connectors — clickhouse_driver.dbapi's OperationalError( ServerException) and clickhouse-sqlalchemy's DatabaseException, which is what the db_clickhouse node's `clickhouse+native://` URL actually raises — plus a lone `, required columns:` tail and a guard that prose containing "in scope" is not truncated. Against the pre-fix code: "3 failed, 1 passed, 70 deselected in 0.57s" (the fourth is a guard and passed before too). No live ClickHouse server is available here and neither clickhouse-driver nor clickhouse-sqlalchemy is installed, so both connector shapes are constructed from the message forms the server documents; each test docstring says so. Verified: 247 passed (tests/database + tests/ai/common/database), ruff check + format clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): format _validateQuery's EXPLAIN error before returning it dylan-savage (review 5215826786, nit 1) noted that _validateQuery's `except Exception as e: return False, str(e)` made the PR description's security claim false. The EXPLAIN failure does not stop at the retry loop: _buildSQLQuery carries it into `result['error']`, get_sql returns that as its `error` field, and get_data returns get_sql's dict unchanged. Neither tool consults IGlobal.allow_execute — the gate exists only in execute() — so `str()` of a SQLAlchemy StatementError, tail and all, reached callers who were never granted raw SQL. On the pre-fix code the string was: (sqlite3.OperationalError) no such table: no_such_table [SQL: EXPLAIN SELECT secret FROM no_such_table WHERE token = 'hunter2'] (Background on this error at: https://sqlalche.me/e/20/e3q8) Now `self._format_db_error(e)`, the same formatter execute() uses, and the full exception is logged with warning() first so the statement stays in the server log where the retry loop's own log line used to keep it. Tests (tests/ai/common/database/test_db_execute_errors.py): the helper directly, then both ungated lanes — get_sql and get_data with allow_execute switched OFF and a stubbed _buildSQLQueryOnce that keeps proposing a query against a missing table — asserting the driver's sentence survives while '[SQL:', '[parameters:' and the bound literal do not. Plus the success half, `(True, '')`, which is unchanged. Against the pre-fix code: "3 failed, 1 passed, 40 deselected in 0.55s". Verified: 251 passed (tests/database + tests/ai/common/database), ruff check + format clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): read an explicit null on any database-owned column as omitted dylan-savage (review 5215826786, item 7) pointed out the asymmetry the previous round left behind: the OMITTED branch skips a generated key AND anything carrying a server default or identity, while the EXPLICIT-null branch only skipped generated keys. So `{'created_at': None}` against `created_at timestamptz NOT NULL DEFAULT now()` fell through to `prepare_value(None)` and bound NULL. Reproduced on SQLite, which raises the same class of error: sqlite3.IntegrityError: NOT NULL constraint failed: notes.created_at [SQL: INSERT INTO notes (label, created_at) VALUES (?, ?)] [parameters: ('a', None)] and on the answers lane writeAnswers only logs, so the batch vanished with one line — the exact failure mode the generated-key rule was added to prevent. The condition now reads `lowered in generated_pk_columns or lowered in generated_defaults`, i.e. the same column set as the omitted branch. Splitting the rule by nullability or by key-ness would make one emitted row land differently depending on a column attribute its sender — an LLM node or a JSON mapper that emits every schema key — cannot see. The three insert-lane README paragraphs said "a null on any other column is inserted as NULL as given"; they now say a null on any column the database fills in counts as not carried, and a null on a column with nothing behind it is inserted as given. Hand-written prose only; the GENERATED:PARAMS regions are untouched (validate-node-readme passes on all four DB nodes). This is a behaviour change for a pipeline that deliberately writes NULL into a column that happens to carry a DEFAULT — it now gets the default instead. The `execute` tool is a different contract and is unaffected. Tests (tests/ai/common/database/test_db_execute_errors.py): the NOT NULL DEFAULT case dylan named, the nullable-default case, a column with nothing behind it still binding NULL, and one showing the run-splitting this collapses — a batch alternating an explicit null and an omission is now a single executemany instead of one run per row. The narrow-rule pin test_insert_still_binds_an_explicit_null_on_a_non_key_column is removed: it asserted the behaviour this item reverses, and the two new cases cover both halves of what replaced it. Against the pre-fix code: "3 failed, 3 passed, 42 deselected in 0.78s". Verified: 254 passed (tests/database + tests/ai/common/database), ruff check + format clean, validate-node-readme PASS on db_postgres, db_mysql, db_clickhouse and rocketride_sql. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): guard refresh_schema's reflection and keep both caches on failure dylan-savage (review 5215826786, item 5) pointed out that `self.IGlobal.db_schema = self.IGlobal._getDatabaseSchema()` ran unguarded inside the locked block. _getDatabaseSchema has no per-table try/except — one `inspector.get_columns(table)` the database refuses (a revoked grant, a lock timeout, a table dropped mid-walk) propagates out of the whole walk — so refresh_schema was the one tool in this file whose failure still reached the caller as the SQLAlchemy repr, tail included, and this tool has no allow_execute gate in front of it. The reflection now runs in a try/except: the full exception goes to the log with error(), and the caller gets `RuntimeError('Schema refresh failed: ' + _format_db_error(e))` with `from None`, matching execute()'s shape. Publication is all-or-nothing, and the docstring now says so and why. The result is assigned only after the walk returns, so a failure leaves db_schema AND IGlobal.schema exactly as they were — the node keeps serving the schema it has instead of publishing a half-walked database, and IGlobal.schema is not emptied, which would otherwise re-arm the insert lane's lazy rebuild against a database that just refused to be reflected. Skipping the offending table and continuing (what _getTableSchema does for a single table) is rejected explicitly: the LLM path cannot tell a table that vanished from one that could not be read, and would write queries as if it were gone. Tests (tests/ai/common/database/test_db_execute_errors.py): a patched inspector whose get_columns raises a wrapped driver error for one table — injected, since no PostgreSQL is available here to revoke a grant on, and injected after get_table_names so the walk fails part-way as a real one would. Three cases: the message carries the driver's sentence and neither '[SQL:' nor the reflection statement; both caches are byte-equal to their pre-refresh values and a table created in between does not appear; and _REFLECT_LOCK — process-wide, so a leak would wedge every node in the engine — is free afterwards and the next refresh succeeds. Against the pre-fix code: "3 failed, 1 passed, 46 deselected in 0.65s". Verified: 257 passed (tests/database + tests/ai/common/database), ruff check + format clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): format an ODBC (sqlstate, message) pair instead of returning the code dylan-savage (review 5215826786, nit 6) noticed that the generic `args[0]` branch returns the SQLSTATE alone for a driver that reports `('42S02', 'Base table or view not found')` — pyodbc and the other ODBC-backed drivers put a five-character SQLSTATE where pymysql puts an integer errno, so the integer branch misses them and the caller gets the half of the pair they cannot act on. The str(exc) fallback test this PR removed used to cover the shape incidentally. A branch before the generic one formats it as `Error <sqlstate>: <message>`, matched strictly against `\A[0-9A-Z]{5}\Z` and requiring a non-empty string second entry, so a driver that merely happens to put two strings in `args` keeps the old behaviour. The result still runs through the stripper. dylan notes correctly that no node in this repo uses such a driver today (there is no pyodbc dialect under nodes/src/nodes), so this is hardening for a shape we do not ship rather than a live bug; the docstring bullet says as much. Tests (tests/ai/common/database/test_db_base.py): the pair formatted, the stripper still cutting a STATEMENT block inside it, a two-string `args` that is not a SQLSTATE left alone, and the single-argument sqlite3 shape unchanged. Constructed shapes — no ODBC driver is installed here. Against the pre-fix code: "2 failed, 2 passed, 74 deselected in 0.46s" (the two guard cases passed before, which is the point of them). Verified: 261 passed (tests/database + tests/ai/common/database), ruff check + format clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): give each database node its own reflection lock dylan-savage (review 5215826786, item 8): `_REFLECT_LOCK` was module-level and held across `refresh_schema`'s full-database inspector walk, which runs on an engine with no statement timeout. `_insertData` took the same lock unconditionally on every batch, just to evaluate `if not IGlobal.schema`. So one node refreshing a wide database stalled the answers lane of every other DB node in the same engine process -- cross-node blocking the pre-PR code never had, since two nodes share no cache and have nothing to race on. The lock is now per-IGlobal, created in `beginGlobal` (`reflect_lock`, declared `None` on the class so a global that never ran `beginGlobal` fails loudly instead of silently sharing class state). Both writers rebind `schema` / `db_schema` wholesale and neither reads the other node's, so the state the lock guards is exactly one node's. `_insertData` still takes the lock on every batch, and the comment now says why that is the deliberate choice: an uncontended acquire is nanoseconds against an insert about to do network I/O, while a check-then-lock fast path needs the map read once into a local and snapshotted from THAT -- read twice, a `refresh_schema` landing between the two hands the batch an empty map -- which is a second path no test pins. What made the unconditional acquire expensive was its process-wide scope, and that is what changed. Also item nit 4 (DS-N4), first half: `refresh_schema` rendered the tool payload inside the lock. It is pure formatting, so it now runs after the release, over the local `refreshed` map rather than `IGlobal.db_schema` -- a refresh that overtook this one has already published its own map, and this call still owes its caller the schema it reflected. The second half of that nit (rebuilding `IGlobal.schema` from `db_schema[table]['columns']` instead of emptying it) is left alone: it changes the map's value shape and the table-is-gone case that `test_refresh_schema_empties_the_column_map_when_the_table_is_gone` pins, so it is not a nit-sized change. Test: `test_a_refresh_does_not_block_another_nodes_insert` builds two globals over two separate SQLite files, pauses one node's refresh inside the column walk, and asserts the other node's insert completes while it is held. Against the pre-fix code it fails with "the second node's insert waited for the first node's refresh" (1 failed, 50 deselected in 5.63s). The five existing deterministic concurrency tests are unchanged in substance; the two that assert the lock is not stranded now acquire the node's own `reflect_lock`, and the fixture builds one per global the way `beginGlobal` does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(ai): pin the post-refresh supplied-id flip on an auto-created table dylan-savage (review 5215826786, item 9): the comment in `refresh_schema` concedes that on an auto-created table the curated column map has no `id`, so a row carrying one has it dropped, while after a refresh the reflected map binds it -- and asks for that flip to be either removed or pinned by a test. Nothing exercised it: `test_insert_after_refresh_does_not_bind_the_generated_primary_key` only inserts rows that OMIT `id`. `test_a_supplied_id_is_dropped_before_a_refresh_and_bound_after` auto-creates through the real path, asserts the compiled column list of an insert that supplies `id` before the refresh ({q, a}) and after it ({id, q, a}), and checks the stored rows. Its docstring names the consequence the SQLite fixtures cannot show: PostgreSQL advances a SERIAL sequence only for rows that take the DEFAULT, so a bound `id` of 501 leaves the sequence at 3 and the next generated row collides on the primary key, which `writeAnswers` logs and drops. No behaviour change -- this is characterisation only. Curating `id` into `_createTableFromData`'s map so both maps agree is the alternative dylan offers; it is a product decision about what an auto-created table promises and is not taken here. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(nodes): name the trigger-populated primary key as a known limitation dylan-savage (review 5215826786, item 6): `_insertData` rejects a row that omits a primary key with no reflected `server_default` / identity / autoincrement. A `CHAR(36)` key filled by a `BEFORE INSERT` trigger -- the standard MySQL UUID idiom before 8.0.13, same shape on PostgreSQL -- reflects exactly that way, so the batch is refused before the trigger can run, and `writeAnswers` turns that `ValueError` into one log line and drops every row. He asks for this to be documented at minimum. This is the documentation half. The three insert-lane sections (db_postgres, db_mysql, rocketride_sql; db_clickhouse has no answers lane) now name the trigger case, say that the answers lane logs and drops the batch rather than reporting it, and give the workaround: a real column default (`DEFAULT (uuid())` on MySQL 8.0.13+, `DEFAULT gen_random_uuid()` on PostgreSQL) reflects and is honoured. db_mysql's paragraph also gained the rejection sentence itself, which only the other two READMEs carried. All edits are hand-written prose; no GENERATED:PARAMS region is touched. `_generated_primary_keys`'s docstring gains the same paragraph, stated as the false positive it is: reflection describes columns, not triggers. The stronger option dylan offers -- drop the pre-flight rejection, bind nothing, let the driver's error come back through `_format_db_error` -- is not taken here. It costs the guarantee that a refused batch leaves nothing behind, which is what makes the current rejection safe, so it is put to him as a question on the thread rather than decided here. Verified: validate-node-readme.py PASS for all four DB nodes; 263 passed in packages/ai/tests/database + packages/ai/tests/ai/common/database. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * refactor(ai): make _getTableSchema the only publisher of IGlobal.schema dylan-savage (review 5215826786, nit 3), two parts. `_getTableSchema` already assigns `self.schema` and then returns the same columns as pairs; `beginGlobal` and `_insertData` rebuilt the map from those pairs and assigned it again, comment-less. The rebuild was not a no-op only in that it replaced each reflected column comment with '' -- and this map is read exclusively by `_insertData`, by key. Both rebuilds are gone; the two call sites now test the return value and say why there is nothing to assign. `beginGlobal` keeps its one real assignment, `self.schema = {}` for a table that does not exist yet, which is the case `_getTableSchema` leaves untouched. Behaviour for every reader is unchanged: same keys, same order, in the same single atomic rebind. The comment above the `dict()` snapshot claimed it guarded an in-place mutation. No publisher mutates in place any more -- both build a map and rebind it, which is what they were fixed for earlier in this PR -- so the comment now says what the snapshot actually buys: binding the map to a local is what stops a `refresh_schema` mid-batch swapping the map the per-row loop is walking, and copying it is what keeps that true if a publisher ever fills one in place again. No behaviour change. 263 passed in packages/ai/tests/database + packages/ai/tests/ai/common/database. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): cut a parenthesised ClickHouse in-scope echo as well The `in scope ` marker's lookahead required the uppercase statement keyword to follow the phrase immediately, so it fired on the analyzer's `in scope SELECT ...` echo but not on `in scope (SELECT ...)`. ClickHouse formats a scope that is itself a subquery or a CTE body through ASTSubquery, which parenthesises it, and clickhouse-driver interpolates bind values client-side -- so that form carries the caller's literal straight through the stripper. The lookahead now allows one optional `(` before the keyword. Added test_format_db_error_clickhouse_cuts_a_parenthesised_subquery_scope, which fails against the previous regex with the whole subquery (and its bound literal) still in the message. No live ClickHouse server or driver is available here, so the shape is constructed from the documented message form, as the neighbouring cases are; the test docstring says so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): name the table a refresh lost instead of echoing a bare noun `refresh_schema`'s guard formats every reflection failure through `_format_db_error`, which reads the driver's own sentence. The one case dylan-savage's item 5 names that has no such sentence is a table dropped between `get_table_names` and its `get_columns`: SQLAlchemy raises `NoSuchTableError`, built from the table name alone and carrying no `.orig`, so the generic branch returned `args[0]` and the caller saw "Schema refresh failed: ghost" -- a bare noun with no verb. A `NoSuchTableError` arm before the generic one now says what happened ("table \"ghost\" disappeared while it was being reflected") and logs the same sentence server-side. The all-or-nothing publication is unchanged: the arm raises before either cache is assigned. Added test_refresh_schema_names_a_table_dropped_mid_walk, which fails against the previous code with the bare table name. The drop is injected through the inspector rather than raced against a live server, and the docstring says so. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(ai): name beginGlobal when a global reaches the lock without one `reflect_lock` defaults to None on DatabaseGlobalBase so that a global built without `beginGlobal` cannot silently share one lock across every node in the process. The failure it produced was loud but anonymous: `with None:` raises "'NoneType' object does not support the context manager protocol", naming neither the attribute nor the call that creates it, and on the answers lane `writeAnswers` reduces any exception to a single log line -- the quiet failure this PR removes elsewhere. `_insertData` and `refresh_schema` now take the lock through `_reflectLock()`, which raises RuntimeError('reflect_lock is created in beginGlobal; this global never ran it'). No production path reaches it: no DB node overrides `beginGlobal`. Added test_a_global_that_never_ran_begin_global_names_the_missing_lock, which fails against the previous code with the bare TypeError. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(ai): correct the explicit-null sentence in _generated_primary_keys The docstring still described the pre-item-7 rule -- "anywhere else a supplied null is bound as given" -- which caf5bc6e4 made false when it widened the explicit-null branch to every server-default and identity column, and the trigger-limitation paragraph added in fd5e29eb7 had been stitched onto that sentence, leaving a 130-character line. The paragraph is now its own, and states the live rule: the generated keys this returns plus the server-default / identity columns _insertData reads off the reflected table are one set, and an explicit null on any of them means "no value". Documentation only; no behaviour change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic…
* fix(account): close DB broker HTTP errors Closes #2324 * fix(microsoft-365): close HTTP error responses --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* fix(db): avoid coroutine leak in sync DSN resolver Closes #2318 * test(db): clarify running-loop regression guard --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* test(db): repair SQL integration fixture imports Closes #2319 * test(db): isolate config utils fixture * test(db): verify config alias teardown * test(db): restore loaded module aliases * docs(test): document database fixtures * test(db): restore temporary module aliases * test(db): restore all temporary node modules --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* fix(authoritative-overlay): enforce SEC period scope Closes #2320 Closes #2323 * fix(authoritative-overlay): reject conflicting restatements * fix(authoritative-overlay): bound submission lookups * fix(authoritative-overlay): reject invalid matched values --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* fix(crustdata): expose search filters and field projections * docs(crustdata): align person operator wording --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
* fix(chunker): preserve Unicode token boundaries Closes #2321 * test(chunker): install real tokenizer in harness * test(chunker): pin overlap and tokenizer constraints --------- Co-authored-by: Krish Garg <kg@g14-node1.tailb99b1b.ts.net>
…ree) + PR-comment Action (#1607) * feat(sdk): rocketride.pipediff — semantic diff engine for .pipe files (#1606) A raw `git diff` of a `.pipe` file is dominated by canvas coordinate churn: every node carries a `ui` block whose `position.x`/`position.y` move whenever the node is nudged, plus a top-level `viewport` that shifts on pan or zoom. Those lines say nothing about behavior, yet they bury the changes that do — a swapped LLM provider, a re-tuned chunk size, a rewired retrieval step. `rocketride.pipediff` compares two parsed `.pipe` documents instead of their text. Components are matched by `id`; the result is grouped into nodes (added / removed / re-provisioned), edges (reconstructed from each component's `input[]` data lanes and `control[]` orchestration lanes), and config (deep-diffed into dotted paths such as `config.default.strlen`). Layout is collapsed to a single "changed" flag unless the caller asks for it, and JSON values are compared with JSON typing so `1` and `true` are never conflated. The package is import-only and does no I/O beyond reading the files it is given: `gitref` shells out to `git show <ref>:<file>` for the ref-vs-worktree case and maps every failure mode onto an explicit error, and `reporters` renders the same diff as human text, stable sorted JSON, or Markdown with `|` and backticks escaped so untrusted values cannot break a PR comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(cli): add `rocketride diff` subcommand (#1606) Wires `rocketride.pipediff` onto the CLI as `rocketride diff`, the review half of the "pipelines as code" loop: `validate` says a `.pipe` is well-formed, `diff` says what changed in it. The subcommand is deliberately the odd one out in `setup_parser`: it is local only — it never contacts an engine or the network — so it does not take the shared `--uri`/`--apikey`/`--token` connection arguments every other command gets from `_add_connection_args`, and its `--json` is a plain format flag rather than the shared `--json [FILE]`. It compares two files positionally, or a single working-tree file against a git ref via `--git REF`. `--markdown` renders a PR-comment-friendly report, `--include-layout` opts the canvas churn back in, and `--exit-zero` turns a gating run into an informational one. Exit codes are the CI contract: 0 for no semantic changes (or any successful run under `--exit-zero`), 1 when semantic changes are found, and 2 for a usage error, an unreadable or unparseable file, or a bad git ref. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(ci): add pipe-diff composite action — semantic `.pipe` diff as a sticky PR comment (#1606) Because `rocketride diff` is local and exits non-zero on change, it drops straight into a pull-request check. This composite action does the assembly: it installs the CLI, resolves the PR base commit (fetching it itself, so the default `fetch-depth: 1` checkout is fine), finds every changed `.pipe` file, diffs each against the base, and maintains one sticky comment so reviewers see "OpenAI → Anthropic, chunk size 512 → 1024, retrieval rewired through a reranker" instead of a wall of coordinate noise. Every run also writes the full report to the job summary, which is what keeps the action useful on a fork pull request: GitHub hands those jobs a read-only `GITHUB_TOKEN` regardless of `permissions:`, so the comment API returns 403 — the action warns instead of failing and the diff stays visible in the summary. The summary is written on the base-fetch failure path too, rather than leaving a run with no output at all. `install-from` exists because `rocketride diff` ships in a release after 1.3.0 and PyPI does not serve it yet; it points pip at a checkout or a pinned commit instead. Markdown assembled from file names and diff output is escaped, and the action distinguishes "no `.pipe` files changed" from "could not determine what changed" rather than reporting the reassuring answer blind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: document `rocketride diff` in the CLI reference and examples (#1606) Documents the new subcommand where the restructured docs tree puts CLI content — `docs/public/product/connect/cli.mdx` — as a `### diff` section alongside `### validate`, the other file-oriented, CI-friendly command. The section covers the flags, the exit-code contract, what does and does not count as a semantic change, all three output modes, and the `pipe-diff` action recipe including the fork-PR token caveat and the pre-release `install-from` workaround. Two surrounding statements needed amending rather than just appending: the Python-vs-TypeScript note now records that `diff` is the one Python-only command, and the "Connecting" preamble now excludes `diff` from the commands that take connection options, because it takes none. `examples/pipe-diff-example.md` puts a raw `git diff` next to `rocketride diff` on the same edit to `rag-pipeline.pipe`; the examples index gains a Guides entry pointing at it. Note for reviewers of the earlier revision: this content previously lived in `docs/README-python-client.md`, `packages/client-python/docs/index.md`, and `packages/docs/content-static/cli.mdx`. All three paths were removed by the documentation revamp, so the content is re-homed here instead of resurrecting them, and the example's cross-link now points at the published `/connect/cli#diff` anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(client-python): ship rocketride_common when installing from the source tree (#1607) `pip install ./packages/client-python` from a clean checkout produced a `rocketride` binary that could not start: File ".../rocketride/cli/utils/env.py", line 29, in <module> from rocketride_common.env import ( ModuleNotFoundError: No module named 'rocketride_common' `cli/__init__.py` eagerly imports `.main`, `main.py` imports `.utils.env` at module scope, and that imports `rocketride_common` — so every subcommand, `diff` included, dies at import time. `rocketride_common` is a source library shared with the TypeScript client rather than a published distribution, so it cannot be added to `dependencies`. It lives at `packages/client-common/python/src` and the builder copies it into the staging tree (`client-python:wheel-source`) before the released wheel is built — which is why the published wheel has always been fine and only the source tree is broken. The source tree is also exactly what the new pipe-diff action's `install-from` input points at, in both documented forms (`./packages/client-python` and `git+https://…#subdirectory=packages/client-python`, where pip clones the whole repository), so the documented pre-release recipe could never work. Adding the sibling source root to `packages.find.where` makes setuptools find the shared package in the source-tree layout. setuptools skips a `where` entry whose directory does not exist, so the builder's staged layout — where `rocketride_common` is already inside `src/` and there is no sibling directory — is untouched. Verified end to end: from a clean `git archive` tree `rocketride diff --help` now exits 0, and rebuilding the staged layout through `python -m build` still yields an sdist and a wheel carrying both `rocketride` and `rocketride_common`, byte-identical in top-level contents to before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ci): diff .pipe files against the merge base, not the base branch tip (#1607) The action took `github.event.pull_request.base.sha` -- the tip of the base branch at event time -- and used it directly for both `git diff "${BASE_SHA}" HEAD` and `rocketride diff --git "${BASE_SHA}"`. That tip keeps moving as other pull requests merge, so every commit merged into the base since this branch forked was reported as a change made by THIS pull request, and reported backwards: those commits sit on the left-hand side of the diff, so a node someone else added shows up as a node this PR deleted. Reproduced in a scratch repository -- fork at C0, this branch adds C1 touching `a.pipe`, the base then gains C2 adding `b.pipe`: git diff --name-status $C2 HEAD -> M a.pipe / D b.pipe (wrong) git diff --name-status $(git merge-base $C2 HEAD) HEAD -> M a.pipe (right) The action now resolves `git merge-base "${BASE_SHA}" HEAD` once and feeds that to the changed-file enumeration, the deletion enumeration and every per-file `rocketride diff --git`. This is the comparison GitHub's own "Files changed" tab shows, and the same one the action's README already recommended in its manual recipe. Fetch depth had to change with it. The base commit was fetched with `--depth=1`, which leaves it with no ancestry for `merge-base` to walk, so that flag is gone. A default `fetch-depth: 1` checkout also leaves HEAD with no ancestry, so when the first `merge-base` attempt comes back empty the action deepens (`--unshallow`, falling back to `--deepen`) and retries; a workflow that already checks out with `fetch-depth: 0` never pays for that. Verified against a real depth-1 clone: the first attempt returns nothing, the deepen recovers, and the merge base resolves to the fork point. If the merge base still cannot be computed the step now fails with a report and an annotation pointing at `fetch-depth: 0`, rather than falling back to the base tip -- silently diffing the wrong thing is the defect being fixed. The action README and the CLI reference are updated to describe the merge-base comparison and the optional `fetch-depth: 0`. Verified: action.yml parses as YAML, `bash -n` and shellcheck pass on all four `run:` blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(client-python): assert the Markdown table escaping without cmarkgfm (#1607) `test_gfm_renders_escaped_cells_back_to_the_original_value` was guarded by `@unittest.skipIf(cmarkgfm is None)` and `cmarkgfm` was in no dependency group, so the one test that decides whether `_md_cell`'s backslash handling is right never ran — not locally, not in CI. Declaring the dependency is not enough on its own: CI does not `pip install` this package's extras at all. `builder test` runs `client-python:run-pytest`, which invokes pytest through the bundled engine binary in its own isolated environment (packages/client-python/ scripts/tasks.js -> scripts/lib/pytest.js), so an entry under `[project.optional-dependencies] test` would still leave the test skipped on every CI run. So the coverage now comes from an assertion that needs no C extension. `_split_gfm_row` applies cmark-gfm's own table-row rule directly: a `|` splits the row only when no backslash precedes it, and a `|` preceded by a run of n backslashes is escaped and loses exactly one of them. The new `test_escaped_cells_survive_gfm_row_splitting` runs that over the same value corpus — which both tests now share — and asserts three columns and an exact round-trip to the original value. That rule was derived from cmark-gfm rather than from the spec prose, and then checked against it: comparing `_split_gfm_row` with `cmarkgfm` over several thousand random values built from the punctuation that shows up in pipeline config produced zero disagreements. Mutation-checked too — making `_md_cell` double the backslash instead of adding one fails six of the eight corpus values. The cmarkgfm test is kept as an oracle rather than deleted: it is the stronger check when the library is present, it now shares the corpus, and `cmarkgfm` is added to the `test` extra so `pip install .[test]` enables it instead of leaving the guard permanently dead. Its skip message says which check still covers the rules. Verified: 131 passed with cmarkgfm installed; 130 passed and 1 skipped (only the oracle) without it. Ruff check and format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: run the pipe-diff action on this repo and pin-check composite actions (#1607) Two gaps, both of which let the new action ship unexercised. Nothing referenced the action. No workflow under `.github/workflows/` mentioned pipe-diff, so the install step, the base resolution, the `rocketride diff` invocation and the comment step had never run as a GitHub Action — the first consumer to adopt it would have been the one to find out. `.github/workflows/pipe-diff.yml` runs it on `pull_request` against the repository's own 45 `.pipe` files, and the paths filter also fires on changes to the action, the diff engine and the CLI subcommand, so a change to any of them is exercised before it merges even when no pipeline changed. It is `pull_request`, not `pull_request_target`, for the reason the action's own README gives: a `pull_request_target` job checks out the base branch (nothing to diff) and holds a writable token, and this workflow installs the CLI from the checkout. On the `pull_request` event a fork's code runs with a read-only token and no secrets, and the report lands in the job summary — the documented fork behaviour. `install-from` points at the checkout because PyPI does not serve `rocketride diff` yet; the comment says to switch to `cli-version` once it does. Second, `scripts/verify-action-pins.mjs` only enumerated `.github/workflows/*.y(a)ml`, so the SHAs pinned inside composite actions were outside the pin guard entirely — both this PR's `.github/actions/pipe-diff/action.yml` and the pre-existing `.github/actions/validate-pipes/action.yml`. A composite action could carry a mismatched or unpinned `uses:` and CI would pass. The verifier now also enumerates `.github/actions/<name>/action.{yml,yaml}`, treating a missing directory as an empty list. Verified: the existing verifier unit tests still pass (7/7); the live check goes from 26 files to 29 and reports "Verified 87 action pins across 29 workflow and composite-action files" with no errors; and drifting the pipe-diff action's github-script comment from v7 to v6 is now caught with the correct file and line annotation, which it was not before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(client-python): run the cmarkgfm escaping oracle in CI `test_gfm_renders_escaped_cells_back_to_the_original_value` is the test that decides how `_md_cell` escapes a pipe, but `cmarkgfm` was only in the `test` extra and CI never installs that extra — it runs pytest on the bundled engine interpreter against the source checkout. So on every CI run the decisive test skipped, and the gate was `test_escaped_cells_survive_gfm_row_splitting`, which resolves the same corpus with our own model of cmark-gfm's cell scanner. Code and test then shared one assumption: if the model were wrong, both would be wrong together and nothing would go red. The values are untrusted `.pipe` content rendered into a PR comment, so a cell that splits its row is exactly the failure the escaping exists to prevent. Add `cmarkgfm` to `packages/client-python/tests/requirements.txt`, which `client-python:setup-test-deps` installs into the engine interpreter through `depends()` — the mechanism #1612 added for the otel-bridge tests, so the wheel resolves under the same merged constraints as everything else and both packages stay on one route. The floor stays `>=2024.1.14`: 2024.11.20 publishes no macOS wheels, and a tighter pin would make the macOS leg build cmark-gfm from source. It resolves to 2025.10.22, which has wheels for every CI leg. The `pyproject.toml` comment that explained why the extra was not enough now names the route CI actually takes. The library-free check keeps its own docstring but no longer claims CI never installs the renderer. Verified with the engine interpreter, the same way CI will: depends(tests/requirements.txt) -> cmarkgfm==2025.10.22 (wheel) before: 1 passed, 1 skipped ("cmarkgfm is not installed") after: 2 passed, 16 subtests passed The oracle agrees with the scanner model on the whole corpus, so `_md_cell`'s rule is now checked against the renderer rather than against our model of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(client-python): document rocketride diff and the pipediff API in the python docs tree The Python-SDK half of this PR's documentation lived in docs/README-python-client.md. The documentation revamp (#2139, 92a1b7e) deleted that file wholesale as part of moving the client docs into docs/public/{python,typescript}/, and this branch's merge of develop took the deletion without relocating the pipe-diff content. The result was a regression against the state this PR was reviewed in: `rocketride diff` and the importable `rocketride.pipediff` package had no page anywhere under the new docs tree, and nothing in the Python client's own docs folder mentioned them. Add docs/public/python/pipe-diff.md as a Python-only site page (sidebar_position 14, the slot after otel-bridge at 13) carrying the material that used to live in the deleted README: install/quickstart, the flag table and exit codes, what counts as a change, the human/JSON/Markdown output modes, the CI recipe that points at the bundled composite action, and a reference of every name exported from `rocketride.pipediff` (engine, data model, reporters) plus troubleshooting and limitations. The three sample outputs are the real renderers' output, reproduced byte for byte. Wire it into the docs spine the same way otel-bridge is wired: - scripts/validate-client-docs.py: declare `pipe-diff` in PY_ONLY_PAGES. This is required — an undeclared page in one client folder and not the other fails the site-page-set parity check. - docs/development/clients/readme-schema.md: the schema doc enumerates the declared single-language extras that the validator constant mirrors, so it would otherwise name otel-bridge as Python's only extra. - docs/public/product/clients/index.md: the spine paragraph that lists what the Python package additionally ships would otherwise claim the OTel bridge is the only such surface. - docs/public/python/index.mdx: a side card next to the OTel bridge card. - docs/public/product/connect/cli.mdx: a pointer from the `diff` section to the in-depth guide. The CLI reference keeps its own flag table; the guide is the one that also documents the importable API. No behaviour change. Verification: `python3 scripts/validate-client-docs.py` ok (6 README sections, 13 shared site pages, symbol parity); `node scripts/build.js docs:test docs:build docs:check` exit 0 (250 pages staged, docs exports in sync); `ruff check` / `ruff format --check` clean on the touched .py file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(client-python): say the cmark-gfm oracle runs in CI, not that it is optional asclearuc asked for the cmark-gfm oracle to actually execute in CI rather than skip, so the backslash-escaping question is settled by the renderer instead of by our model of it. That was done by adding `cmarkgfm` to packages/client-python/tests/requirements.txt, which the `client-python:setup-test-deps` step installs into the engine interpreter the suite runs on. The prose around the test was never updated and still described the renderer as an optional local nicety: - `_md_cell`'s docstring said the round-trip was "cross-checked against ``cmarkgfm`` when that optional renderer is installed"; - the `skipIf` reason pointed only at `pip install .[test]`, so a reader of a skip line had no way to tell whether CI had skipped it too; - the library-free sibling test read as the primary check rather than the fallback. Rewrite all four strings (the guarded import comment, both test docstrings, and the skip reason) to state where the renderer comes from in CI and what a skip therefore means: a local interpreter without it, never the CI leg. The library-free `_split_gfm_row` check is named as the fallback over the same corpus; the renderer test is named as the one that decides the rule. Comments only -- no behaviour change, and the skip condition itself is unchanged. Verification: `./dist/server/engine -m pytest packages/client-python/tests/test_pipediff_reporters.py -q` -> "32 passed in 0.08s" (exit 0), with test_gfm_renders_escaped_cells_back_to_the_original_value running rather than skipping; `ruff check` / `ruff format --check` clean on both files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(client-python): pin the top-level keys diff_pipes reads and ignores Author-side finding, not a reviewer ask. `diff_pipes` reads exactly three top-level keys of a `.pipe`: `components`, `version` and `viewport`. Every other top-level key the schema defines -- `project_id`, `isLocked`, and anything a later schema adds -- is ignored on purpose, because it is editor/session metadata rather than pipeline behaviour. That rule is load-bearing for the exit code (it is why opening a pipeline in the editor and saving it does not fail a CI gate) and it was implicit: nothing asserted it, and nothing stated it. A later contributor widening the read set by one key would turn every `project_id` rewrite into a gate failure, with no test to catch it. Pin it: - tests/test_pipediff_engine.py: `test_unknown_top_level_keys_are_ignored` changes `project_id`, `isLocked`, a `name`, and an unknown future key, and asserts an empty diff under both `include_layout=False` and `True`; `test_version_is_compared_but_viewport_only_under_include_layout` pins the other half -- `version` always counts, `viewport` is enumerated and counts only under the flag. - engine.py: one paragraph in the `diff_pipes` docstring stating the read set and naming the test. - docs/public/product/connect/cli.mdx and docs/public/python/pipe-diff.md: one bullet each in "What counts as a change", so the contract is stated where a user decides whether `diff` is safe to gate on. No behaviour change -- the tests characterize what the code already does, so they pass on the pre-change engine. Proof that they bite: with a temporary mutation adding `project_id` to the compared set, test_unknown_top_level_keys_are_ignored fails with "assert [NodeChange(i... d_changes=[])] == [] / Left contains one more item: NodeChange(id='__project__', kind='config', ...)" -- "1 failed, 61 deselected in 0.04s" (log impl-1607-toplevel-pin-mutation.log). The mutation was reverted before this commit. Verification: the four pipediff/CLI modules through the bundled engine -> "133 passed in 0.35s" (exit 0, was 131 before the two new tests); `ruff check` / `ruff format --check` clean on both .py files; `node scripts/build.js docs:test docs:build docs:check` exit 0; `python3 scripts/validate-client-docs.py` ok. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * docs(pipediff): pin note and two accuracy clauses on the Python guide The CI snippet on the Python guide carried no pin caveat, unlike its two siblings in cli.mdx and examples/pipe-diff-example.md, so a reader landing here was not told that `@develop` is a placeholder. Add the one sentence. Two accuracy clauses in the same page: the `--git` row now says a file that is absent in the ref also reports a version change from `null` (the CLI substitutes an empty pipeline), and the `resolve_git_ref` row names the remaining `PipeDiffError` case — contents at the ref that are not a valid pipeline. --------- Co-authored-by: nihalnihalani <24360630+nihalnihalani@users.noreply.github.com>
…ly falling back (#2292) A read-only node whose "Default Graph" doesn't exist on the server was silently resolving to whatever preconfig.default declares (agent), rather than the graph the user actually intended. FalkorDB accepts a query against a missing graph and only fails on the first result, with "ERR Invalid graph operation on empty key" -- an error that gives no hint the graph itself was the problem (#2155). - _open_driver now raises a clear error naming the missing graph and the ones that do exist, at pipeline start rather than on first query. - _probe_connection warns the same way at save time. - Both skip the check when allow_writes or allow_execute is on, since FalkorDB creates a graph lazily on its first write -- a write-capable node may legitimately target one that doesn't exist yet. - The canvas tile was empty, so the active graph -- or that it had silently fallen back to one -- was invisible without opening the inspector. It now shows "Graph: <name>". The original report described the fields as unexposed/unpersisted; they already were (confirmed by a prior investigation on the issue). The real defect was the silent, unvalidated fallback, which is what this fixes. Co-authored-by: madhumithachandrasekaran31 <madhumitha2110549@ssn.edu.in>
…quests Graph scopes (#2373) LoginWithMicrosoftButton looked up SERVICE_TIER_SCOPES[formContext.provider], but formContext.provider is the node protocol name (tool_excel, tool_word, tool_onedrive, tool_outlook_mail, tool_outlook_calendar) while the map was keyed excel/word/onedrive/outlook_mail/outlook_calendar. Every lookup missed, so no scope param was sent, the broker requested identity only, and the node rejected the token (e.g. "Missing: Files.ReadWrite"). Rekey the map by protocol name (scope values unchanged; they mirror core/microsoft_access.py) and hoist it to an exported module constant. Add a test that reads every tool_microsoft_365 services.*.json and asserts the map has non-empty scopes for each protocol and each access tier. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… changes (#2378) * fix(ci): cancel * fix(ci): labeler
…ion, history, relationships, plain EXPLAIN, honest designer (#2282) * test(sql-ui): add node:test harness and pure-logic suites SQL Explorer had no test setup. Add the explorer-ui harness (tsx + node:test, a `sql-ui:test` builder action over tests/) and six pure-logic suites covering the modules where the audit found bugs. Unlike explorer-ui, docs.ts imports Documents / NOOP_VFS from 'shell' as values, so the runner preloads a local stub-shell.cjs (apps/shared's pattern, copied rather than shared: extending apps/shared's stub would change a file its own suite depends on). Every other tested module reaches the platform through `import type` alone, which tsx erases. 15 of the 71 tests fail on purpose. They encode the fixed behaviour for two audited correctness bugs, ahead of the fix in the next commit: - sql/paging.ts inlines filter and search values, passing numeric-looking strings through unquoted, so `code >= '0100'` on a VARCHAR column becomes a number comparison (MySQL coerces the column, Postgres raises). - docs.ts leaves the table name unencoded in `table:` / `design:` URIs, so a table whose name contains a colon splits one segment too late and yields an endpoint key that does not exist. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 8f788cf75fd14235ed07d3bb5ba0acac24f8041b) * feat(sql-ui): parse plain EXPLAIN output into a plan tree Adds the pure half of the query-plan feature: buildExplain() prefixes the dialect's plain EXPLAIN form (mysql FORMAT=JSON, postgres (FORMAT JSON), clickhouse text) to the statement exactly as it would run, and returns null for dialects with no support here. parseExplain() normalises the three documented result shapes into one IPlanNode tree, flagging planner numbers so the UI can suffix them with `est.`. Plain EXPLAIN only — nothing here emits ANALYZE, so the explained statement is never executed. Postgres arrives either already parsed or as a JSON string depending on the driver; both are handled. A shape the parser does not recognise reports `ok: false` with a reason rather than throwing, so the caller keeps showing the database's raw output. Every fixture is hand-built from vendor documentation, not captured from a live server: no MySQL, PostgreSQL or ClickHouse instance was reachable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit a1c0678b2531f0dfe049c7c41b07841b4c6c30a9) * fix(sql-ui): bind filter values, remote sort, page errors, URI table encoding Five audited correctness bugs in the data browser and its URI scheme, plus the two session capabilities the rest of the upgrade needs. paging: filter, range-bound, and search values were rendered into the statement by quoteValue, which passed numeric-looking strings through unquoted — `code >= '0100'` on a VARCHAR column became a number comparison, matching the wrong rows on MySQL and raising a type error on Postgres. buildPageStatements now returns `params` alongside the two statements and binds every user value as `$1..$n`; quoteValue is gone and introspect's always-quoting quoteLiteral moves to sql/quote.ts as the app's single literal quoter. LIKE patterns are assembled as bind VALUES, because the node rewrites `$n` textually before binding. TableDataView: SELECT and COUNT now run in parallel; header sorts reach `req.sort` (remoteSort); a failed page shows an error Banner instead of failing silently behind the grid's transient toast; and a monotonic request sequence keeps a slow, superseded page from overwriting a newer one. A table with no primary key and no user sort pages without any ORDER BY, so the view says the order is not guaranteed. docs: `table:` / `design:` URIs percent-encode the table segment. The endpoint key contains colons, so these schemes split on the last one — a table named `odd:name` split one segment too late and produced an endpoint key that does not exist. connect: `execute` takes optional bind params, and `ISqlSession` gains `refreshSchema()` over the node's `refresh_schema` tool, falling back to `get_schema` with `stale: true` when the node does not have it. The engine's exact wording for an unowned tool is not readable from this repo (it comes from the compiled engine), so `isUnsupportedToolError` classifies conservatively: false only for errors that clearly came from a tool that ran, true otherwise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 98f7303c6b8cef104a80ddce0c43d327ffa200ad) * feat(sql-ui): map declared foreign keys into a relation graph and join paths The record drawer and the join-path finder need to answer three questions from a schema snapshot alone: what points at this table, how do I get from here to there, and what does that join look like as SQL. Everything here comes from keys the engine DECLARED. Nothing is inferred from a column or table name, anywhere — a name that looks like a key is a guess, and a guess presented as a relationship is worse than no answer. A schema that models its relationships only by convention (and ClickHouse, which declares none at all) therefore produces an empty graph, which is the honest result rather than a bug. findJoinPaths returns EVERY shortest path instead of picking one. Two tables are frequently joinable in several equally short ways — through parallel keys, or through different intermediate tables — and which one is meant depends on the question being asked, which this module cannot know. Self-referential keys are shown in the drawer but never walked, so a path cannot loop; composite keys carry all their column pairs into a multi-column ON clause; and keys pointing outside the snapshot are kept as edges for the quality rules but are never walkable. The generated SELECT says what it is on its first line: generated from declared foreign keys, review before running. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit e2c893e669d798fc6d60e9125283ecbfeac04189) * feat(sql-ui): record finished statements and bound them before they are kept Query history rides the app's workspace preferences file, which the shell reads and writes on every app switch. An unbounded list would make that switch slower for everyone, so the bounds are enforced in pure functions before every write rather than left as good intentions: 100 entries per connection, 8 KB per statement, 1 KB per error, 256 KB across the whole bag. Pins buy priority, not immunity. They count against the same 100 and are dropped last, because an unbounded pinned list is the same problem wearing a nicer name. When a repeat of the same statement collapses onto the entry before it, the OLDER entry survives with its id, pin and note intact — a person who annotated a statement meant that statement, and re-running it must not quietly discard what they wrote. runEvents is a one-way bus so the runner never imports the store: a run is never blocked by, or aware of, whether anything is recording it, and a listener that throws cannot fail the query that just succeeded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit e1fbc2e8a48ee9b60953dd9d074992f7e933f7f4) * fix(sql-ui): tell the truth about the schema after a designer Apply The node reflects its schema once, at task start. The designer used to re-read that same snapshot after Apply and repaint it, so a freshly added column appeared to vanish. Apply now asks for a fresh reflection and says which of the two things actually happened: - re-read succeeded: `Applied HH:MM · schema re-read from the database.` - the node has no refresh_schema tool, or the re-read failed: a warning saying the tree and diagram will not show the change until the pipeline restarts. Because the answer depends on the snapshot that comes back, the banner is composed in an effect against the post-refresh snapshot rather than inside the Apply closure, which cannot see it. A batch that stops part-way now reports what committed, which statement failed with the database's own first line, what never ran, and how many statements the plan still holds — instead of a bare `Apply failed:`. The confirmation no longer says schema changes "cannot be undone from here", which reads as if a rollback existed elsewhere. It states the statement count and the engine's actual behaviour for a half-finished plan. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 167ea69d4c081f8e85746c6f82000c95f403e3b5) * feat(sql-ui): add dialect-aware statement splitter and classifier The database node executes exactly ONE statement per `execute` call, so a buffer holding several statements has to be split in the app before anything is sent. `sql/split.ts` is a hand-written scanner rather than a regex because a `;` separates statements only in code: inside a string literal, a quoted identifier, a line or block comment, or a PostgreSQL dollar-quoted body it is ordinary text. Every dialect difference the scanner claims is asserted in the tests instead of assumed — MySQL's `#` line comments (which ClickHouse does not have), PostgreSQL's nested block comments, dollar-quoted bodies and `E'...'` backslash escapes, and ClickHouse's plain backslash escapes. MySQL's `DELIMITER` directive is NOT supported and says so in the JSDoc and in a test that pins the wrong-but-documented split, because a half-working implementation of a client-side directive is worse than an honest gap. `sql/classify.ts` decides whether the runner may pass `idempotent: true` (which permits one silent retry with a fresh token), so anything not clearly read-only is not a read: `EXPLAIN ANALYZE` is classified by the statement it runs, and a `WITH` chain containing a data-modifying keyword is a write. Its `patternCheck` is the text heuristic behind the confirmation dialog; the false positives it is allowed to have (a WHERE that exists only inside a subquery) are pinned by test so they are not "fixed" into false negatives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 1942a0b4f4487f06bc8972e4f99cb1c9e512f7d3) * feat(sql-ui): add pattern notes and raw-text rendering to the plan parser planNotes() turns a handful of documented planner markers into plain-language observations, each citing the exact field it read: MySQL access_type = ALL, filesort and temporary tables (in either the tabular Extra column or the FORMAT=JSON booleans), and PostgreSQL sequential scans. Row estimates carried into a note keep the `est.` suffix. These are text rules over planner output, not a cost model. A node no rule matches gets no note — silence is not a verdict — and the UI labels the column `detected by pattern` so nobody mistakes them for measurements. formatRawPlan() renders the result as the text a reader would have seen at a SQL prompt, which is what the panel shows first and by default. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 3de518dd4d52aec8ab06d5b5214632192f4dd2fd) * feat(sql-ui): keep query history in the workspace, never in the browser History lives in the shell's workspace preferences — server-side and per user — so it follows the person to another browser or machine, which is the whole reason it is worth keeping at all. That store has one sharp edge, and it decides the design of this file. `setPref` patches whichever app is active AT WRITE TIME, and the shell re-reads each app's preferences from disk on every app switch. A write issued while SQL Explorer sits in the background would be filed under the app in the foreground and then lost. So the store writes only while a HistoryPrefsBridge is mounted, and the shell unmounts the app's whole subtree when another app takes over, which makes "mounted" mean "SQL Explorer is on screen". On unmount the pending write is dropped rather than flushed: up to 300 ms of history can be lost if someone switches apps immediately after a run, and that is the honest cost of never writing into another app's preferences. Everything read back from the workspace file is narrowed field by field. The file outlives this version of the app and a person can edit it by hand, so it is treated as untrusted input rather than as our own types. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit f2a265442a71369598e73a94d232422de4c8fd20) * feat(sql-ui): add a plain EXPLAIN plan panel ExplainPanel is self-contained: given an endpoint, a dialect and the statement exactly as it would run, it performs the EXPLAIN itself through the session and shows what came back. Integration into the query runner is a separate change. The database's RAW OUTPUT is the default view. The interpreted tree sits behind a second toggle, and a plan shape the parser does not recognise is an info note beside the raw text — not an error, because nothing failed on the database's side. A node that refuses execute gets the allow_execute sentence instead of driver text, since the statement never reached the database. PlanTree renders the plan as an ARIA tree: arrow keys walk and expand rows, every planner number carries `est.`, and the notes column is labelled `detected by pattern` with each note citing the field it read. No cost bars and no colour as meaning — every state is text. Plain EXPLAIN only. Nothing here runs the statement being explained. The parsers are fixture-tested against documented output shapes; no live MySQL, PostgreSQL or ClickHouse server was reachable to validate them against. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 98c005937c34c7b67caa5db6a244e4813cbbad2b) * feat(sql-ui): add the query history drawer A modeless right drawer, so the editor behind it stays usable — the point of history is to work alongside the statement being written, not instead of it. Search, filter by reads/writes/errors/pinned, expand a row for the full text, then load it into the editor, rerun it, pin it, annotate it, delete it, or open it in a new query document. The privacy notice is permanent, not a first-run tip. These statements are written verbatim to the user's workspace file on the RocketRide server, literal values and all. Someone who assumed browser-local storage would be wrong in a way that matters, so the drawer says where the text goes for as long as it is open. Rerunning anything that changed data asks first, and the warning quotes what the statement did LAST time rather than pretending to know what it will do now — nothing is counted or checked against the database beforehand. The kind badge comes from a text pattern, which the wording reflects. "Load into editor" does not ask before replacing the editor's text: the drawer cannot tell an untouched editor from an edited one, so the query view owns that confirmation. "Open in new query" deliberately does not mark the document as generated — the statement is the user's own text and must not claim to be something this app wrote. announce() lands here because the shell has no live region: useAnnouncements fetches GitHub product announcements and only Banner sets aria-live, so pinning or clearing an entry would otherwise be silent to a screen reader. The visible live region that reads this channel is mounted by the app shell. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 7c88324f53c787838395a86a2829213a10dbe82f) * fix(sql-ui): retry a failed tool call only when re-running it is safe A session caches the owning task's token, and that token goes stale whenever the task restarts. The transport reports a stale token exactly like a statement the database refused, so the blanket one-retry meant a write could be sent twice: the first INSERT may well have committed before the failure surfaced. Retrying is now a per-call decision. `execute` takes `opts.idempotent` and retries only when the caller sets it; reflection and dialect reads keep the retry, since repeating them is free. TableDataView's SELECT and COUNT pass it; the query runner will pass it only for statements its classifier marks as reads. An unmarked statement is assumed to write and its first failure is the answer. refreshSchema no longer inspects error text to decide whether the node has the refresh tool. The engine offers no dependable signal for "no such tool", and matching its wording would rot the first time it changed, so ANY failure falls back to get_schema with `stale: true` — and schemaStore remembers that per endpoint, so later refreshes on a node without the tool read the snapshot directly instead of paying for a doomed call each time. isUnsupportedToolError and its marker lists are gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 9c2f7826d52c0d2ab4ce7eaca010b2de82d6647f) * feat(sql-ui): show inbound keys, preview queries and join paths in the record drawer The drawer already said what a table declares. It could not say what points AT the table, which is the half of a relationship a person usually arrives looking for, and it left the obvious next step — actually querying the thing — as an exercise. "Referenced by" lists the inbound foreign keys and opens the referencing table in a STACKED drawer, so following a relationship keeps the way back under the shell's own back arrow instead of quietly replacing what was on screen. The join-path finder walks declared keys undirectedly to four joins and lists EVERY shortest path rather than picking one, because which join is meant depends on a question the schema cannot answer. Parallel keys and composite keys each come out as their own path, with all their column pairs in the ON clause. Both sections carry "from schema snapshot HH:MM". A snapshot read at task start can be older than the database it describes, and a relationship presented without its age invites a decision based on a stale reading. Where there are no declared keys there is no guessing: the section says the schema declares none, and ClickHouse says so in its own terms. Nothing here infers a relationship from a column name. Select top 100 and Count rows open a NEW query document whose first line marks it as a generated preview. Neither runs anything. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 27c89777443c87ca3b4e217b812e27546a5f5d18) * feat(sql-ui): show inbound foreign keys before applying a designer plan The Apply confirmation now has an Impact section naming every table that declares a foreign key on a column the plan drops, renames or retypes, and stamps where those facts came from: `from schema snapshot HH:MM`. The wording says the database MAY reject or cascade the change, because the tool asks the snapshot, not the database. Renames are traced back to the name the snapshot knows. A plan that renames a column and then retypes it under its new name still finds the keys pointing at the original, which a match on the operation's own name would miss. Composite keys pair their columns positionally. Dropping a column adds a line saying its data is not recoverable by this tool. Column rows already acted as buttons on click; they now carry role, a tab stop, an accessible name and Enter/Space, so the designer is reachable without a mouse. Dropped rows and create-mode drafts stay inert, as they were. ExplainPanel announces a finished plan through the app's live region: unlike a Banner, a tree and a `pre` do not announce themselves. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit c7dc728d9fbab75f4bbe3f0dc701bed40eec2732) * feat(sql-ui): type result columns and suggest from the schema snapshot `sql/resultTypes.ts` replaces the blanket `rrType: 'string'` on query results, which disabled numeric sorting and the grid's number and date range filters. Types come from the schema snapshot only when the source table is CERTAIN — one FROM, no join, and every returned column belongs to that table — and from the returned values otherwise; which of the two was used is reported so the column tooltip can say `BIGINT (schema)` or `number (inferred from 200 rows)` rather than presenting a guess as a fact. Type-name matching is whole-token, fixing the substring test that claimed POINT as a number and missed MONEY, REAL and SERIAL. Value inference accounts for the node's sanitisation (Decimal arrives as a float, datetime as an ISO string) and keeps `0123` a string, since a leading zero is the difference between a product code and a number. `sql/completion.ts` builds the suggestion model: alias-scoped columns for an `alias.` prefix, table names after FROM/JOIN/UPDATE/INTO, columns of the tables the buffer mentions, and keyword snippets — ordered entirely through `sortText` because Monaco merges these with its own keyword list. JOIN clauses are prefilled from DECLARED foreign keys, in both directions, with composite keys producing a multi-column ON; nothing is inferred from a column's name. Identifiers that need quoting get it in the inserted text only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit c4af7d8cad2d922b71a6413be6d7ee67486f5ee4) * fix(sql-ui): mark the EXPLAIN round trip idempotent Plain EXPLAIN plans the statement without running it, so the session may retry it once with a re-resolved token after a task restart. Without the flag the EXPLAIN failed outright whenever the cached token had gone stale. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 24ad6e1e361a7a1d6e0ebc01bb4ffe5b74e557ef) * fix(sql-ui): keep history recorded before preferences were reachable Runs can finish before any prefs bridge is mounted — a query document open while its connection workbench is closed records straight into memory. The store then replaced that bag wholesale on hydration, throwing those entries away at the exact moment they first became saveable. Hydration now merges instead: the in-memory entry wins on a shared id, the two lists are combined by age and re-trimmed, and a write is scheduled for whatever was waiting. A recording switch flipped before the bridge mounted outranks the stored one for the same reason. The connection workbench also mounts a bridge, so an open workbench keeps the write channel alive for every query document on that connection rather than only while the History drawer happens to be open. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 0a70a2429089b4087e6d63b91aa65c5cb2ed0340) * feat(sql-ui): add the Insights page — schema shape and snapshot-only review A second page on the connection workbench that says how the schema is shaped and what is demonstrably wrong with it, without touching the database: hubs, leaves and isolated tables from declared keys, then three rules over the same snapshot. It works with execute disabled, because it never executes anything. Each rule stops where the evidence stops. R1, a table with no primary key, is certain. R3, a key naming a table or column the snapshot does not have, is certain within the snapshot — and a snapshot read before a migration can raise it against a perfectly consistent database, which is why the page states when it was read. R2, the two sides of a key spelling their type differently, is a TEXT comparison after normalisation rather than a compatibility check, so it is information and never a verdict. Normalisation drops parenthesised arguments (INT(11) is INT) and resolves a short, exact alias list — every entry is a spelling the engines themselves treat as identical, not a judgement that two types are close enough. There is no naming rule. A column called customer_id that happens to match another table's primary key is a guess, and a guess sitting in a list of findings reads as a fact. The limitations banner is not boilerplate. It is the scope of every claim the page makes, and it stays on screen. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit ef3359f00dd523ca249c4bfcf848e6f54d565baa) * feat(sql-ui): give the editor a run handle, schema completion and decorations `SqlEditor` grows the surface the runner needs: a ref handle for the live selection, caret offset and statement decorations, `Ctrl/Cmd+Shift+Enter` for run-all, and `Ctrl/Cmd+Shift+E` behind an OPTIONAL `onExplain` prop so the keybinding exists before the panel that answers it does. Two things are module-level on purpose. The completion provider is registered once per language id, not per editor: `registerCompletionItemProvider` is global and every query document stays mounted, so per-instance registration would stack providers and repeat every suggestion once per open tab. What varies per editor is looked up by model URI in a map each instance maintains, and the providers are deliberately never disposed — they belong to the module, and tearing them down when one tab closes would break the others. The decoration stylesheet is injected once behind an id guard, the pattern `InputField` uses for its placeholder colour, because inline styles cannot reach a Monaco decoration class. `sql/batch.ts` holds the per-statement run model and its wording, including the batch summary that has to say a failed batch left earlier statements committed, and `applyRowLimit`, which appends the header limit only to statements that return rows and leaves a user's own LIMIT alone. `sql/failure.ts` reads a failed execute against the four messages the node actually produces, matching the machine-stable fragment (`allow_execute`, `max_execute_rows`) rather than a whole sentence. It recognises the generic placeholder that node versions predating the error-text fix return, so the app can say the driver's message was not returned instead of presenting the placeholder as what the database said. `LiveRegion` renders the existing `a11y/announce` store into one polite live region, keyed by the store's revision so announcing the same sentence twice still speaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 6a429e2790cd938a54b277bb06be6e5f58a367b7) * feat(sql-ui): run the selection or statement at the cursor, honestly The query runner previously sent the WHOLE editor buffer as one `execute`, which the driver rejects for anything multi-statement, and reported failures through a bare error string. It now splits the buffer and runs one statement per call, because that is what the node supports. Everything the runner says is tied to how the node actually executes: - Each statement runs in its OWN transaction, so a batch that fails partway leaves the earlier statements committed. The status strip shows every statement's outcome and the summary line says which ones committed, which failed and which never ran. The error banner repeats it for the failure. - There is no cancel anywhere in the tool protocol. "Stop waiting" appears after a second, bumps a run sequence so the late answer is discarded, and says the database may still be running the statement. It never claims to have stopped anything. - Transaction-control statements would each land in a separate autocommit call, so they are refused before anything is sent rather than run into a false sense of safety. - `idempotent` is passed only for statements the classifier reads as READ, so the session's retry-with-a-fresh-token can never re-send a write. - The confirmation dialog for an unbounded UPDATE/DELETE, TRUNCATE, DROP or ALTER calls itself a text check and says it is not a database safeguard. It is not called a safe mode, because it prevents nothing. - When the node returns its generic placeholder instead of the driver's message, the banner says the message was not returned rather than showing the placeholder as the database's words. When the driver's text IS there, it is shown verbatim in a collapsible block. What a run would send is named in TEXT under the editor before the key is pressed, not only by a decoration, and the same applies to the result meta: the applied limit is stated, and a result that exactly fills the limit is badged as possibly incomplete rather than presented as the whole answer. Export is a text hint pointing at the grid's gear menu. `IDataGridHandle` exposes the Tabulator instance but the shell registers 13 Tabulator modules and DownloadModule is not among them, so `table.download()` does not exist; adding a second CSV writer to the app was explicitly out of scope. Result columns are typed from the schema when the source table is certain and from the values otherwise, which restores numeric sorting and the range filters, and the column tooltip says which of the two it was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 863dc06a7f6ae2a9f2b9d994e9e560db21803fed) * fix(sql-ui): read the live selection on the run path, settle a pending confirm Two narrow correctness fixes in the runner. The run path took the selected TEXT from the editor handle but its OFFSETS from the debounced cursor state, so a run fired within the debounce window of a drag attributed the run to the previous selection's lines — the statement sent was right, the lines reported for it were not. The handle now exposes `getSelection()`, returning text and offsets together from the live editor. A pattern-check confirmation open when the document unmounts left the run loop awaiting a promise nothing could settle. The unmount now answers it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit f753c373e15dbad037b4e43e6729864385214cec) * docs(apps): add the SQL Explorer guide SQL Explorer had no product page: the only mention of the app anywhere in docs/ was a builder task reference. This adds the spine page and its Apps sidebar entry, describing the app as it actually behaves today. The page leads with the constraints rather than hiding them, because they are what surprises people: every statement travels through a pipeline database node, so `allow_execute` gates execution, each statement commits on its own, transaction control is refused, nothing can be cancelled, and the schema is a snapshot taken when the task started. The verbatim UI microcopy is quoted so the docs and the app say the same words. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 1601764e6db8eabbf2de7e20b7dc77c0cd605271) * feat(sql-ui): finish the runner's drawers — focus return, explain gating, prefs bridge Completes the integration points QueryView owns, now that the history and explain panels are on the branch. Focus return. Closing either drawer puts focus back on the button that opened it. The shell's Button forwards no ref, so each opener sits in a `display: contents` wrapper the view can query — no layout change, and the alternative (leaving focus on the document body) strands keyboard users at the top of the page after every close. Explain gating. The button is disabled with a reason rather than silently doing nothing: `buildExplain` returns null for engines with no supported EXPLAIN form and for an empty statement, and the title says which it was. The panel is now mounted unconditionally and driven by `open`, as its own contract asks, and is handed the statement exactly as Run would send it — applied LIMIT included — so the plan describes the real statement. History prefs bridge. The history store only writes while a bridge is mounted. One at app level covers a query document whose connection tab has been closed; the bridge is reference-counted, so the copies in ConnectionView and in the drawer stay harmless. Announcements. `Banner` is itself a live region for every variant — assertive for error, polite otherwise — so announcing an event that is also rendered in a Banner reads it out twice. The transaction refusal, the stop-waiting notice and the failure announcement are dropped for that reason; what remains announced is only what has no live region of its own: the batch summary, the row and affected counts, the pattern-check switch, and loading a statement into the editor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 902f2cd76c9b0d0361225a091ba6e1560919726f) * fix(sql-ui): ask for a fresh read when the user presses Refresh Every user-facing Refresh called `refreshSchema` without `{ fresh: true }`, so it re-read the node's task-start snapshot and returned the same answer it already had — even on a node that has the `refresh_schema` tool and could have gone to the database. The one place that asked for a fresh read was the designer's post-Apply path. Pressing Refresh after a CREATE TABLE therefore did nothing visible, which is the worst possible result: the control looks broken when the capability was there all along. The workbench header, the Overview retry and the sidebar tree's refresh now all ask for a fresh read. Automatic first loads still do not, because a page opening does not need two round trips to say what it already serves. The header stamp stops blurring the two things a "read" can mean. A node that re-read the database says `snapshot re-read HH:MM`; a node serving the reflection taken when its task started says `task-start snapshot, read HH:MM`, however recently we asked. The old wording let the second pass for the first, which is what made the missing table look like our bug rather than a node capability that is not there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 17353eb86de6cabda7ec205f70b834c65b67afce) * fix(sql-ui): stop quoting the node's placeholder as the database's words A node without the execute error-text fix returns `SQL execution failed (check server logs for details)`. The Explain panel and the designer both printed it after `Database reported: `, presenting the node's placeholder as if the database had said it. Both now route through describeFailure and show the fallback sentence when the message is generic, with no "Database said" block to open — there is nothing in it. Where the driver's text IS real, both surfaces now match the query runner: the first line as the headline, the full text behind a collapsible block. The designer had no such block before, so multi-line driver text was lost. The Explain subtitle said "Validated by EXPLAIN". Nothing is validated — the planner returned an estimate, which the same line goes on to say. It now reads `EXPLAIN ran HH:MM`. The ClickHouse note claimed ALTER runs as an asynchronous mutation. That holds for ALTER ... UPDATE/DELETE, not for metadata operations like ADD COLUMN, so it now says some ALTER forms do. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 724d70696fcaa8dbde790ce66bd9d7f620738685) * fix(sql-ui): let the review rules know the dialect, and say what they mean R1 had no dialect input, so "table has no primary key" fired on every ClickHouse table — the engine reflects no primary-key constraint for any of them, a MergeTree ORDER BY not being one. The Insights tab then showed a number equal to the table count and the grid called each row "definitely wrong in this snapshot". Every part of that was false, and the loudest part was the verdict. R1 now takes the dialect and drops to information on ClickHouse with wording that names the engine rather than accusing the schema. Severities read `warning` and `info` throughout: a missing primary key is worth fixing on MySQL or PostgreSQL, but a log table without one is a choice, not an error, and "definitely wrong" was never ours to say. The tab badge counts warnings only. Information is something to know, not something to do, and a badge lit on every healthy schema stops being read. The Rule column shows the rule's name instead of R1/R2/R3. The ids were for cross-referencing a document the reader does not have, and there was no legend anywhere on the page. Hub chips say what their number counts — `orders · 5 referencing tables` rather than `orders 5`, which could have been rows, columns or anything else. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 8698af8c30b79c04b840d69f79447f027fb79472) * fix(sql-ui): give history rows their limit, and stop announcing a load that may not happen A history row said `1000 rows`. That number cannot tell you whether it is the whole answer or the ceiling cutting it off, and those mean opposite things — every other count in this app carries its suffix for exactly that reason. The entry gains an optional `limit`, the badge reads `12 rows (limit 1000)` when one was applied and `12 rows returned` when none was, and the runner fills the field in. "Load into editor" no longer announces. The query view may raise a "Replace editor text?" confirm that the user cancels, in which case the announcement was a lie, and on the path where the load does happen the query view already announces it — so the drawer was either wrong or repeating. The recording toggle now says `Recording: on` / `Recording: off`. It was a pressed ghost button whose text never changed, which left the state carried by colour alone for anyone reading it by eye. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 3aadbae8ebc612b1a5a6bcec68a672b9a7c8808b) * fix(sql-ui): say when the join-path list hit its cap The finder stops enumerating at 25 equally short paths, and the drawer reported that as "25 paths found — choose one", which claims there are exactly 25 and that one of them is the one you want. At the cap the list is a sample, so it now reads "25 paths shown (cap reached — more may exist)". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit dfef8bab31035f140aa045a598c56f8c1e71f89b) * refactor(sql-ui): type the query-document payload and hide module internals `IQueryDocPayload` existed but every producer built the payload as an untyped object literal, so a misspelled field would have shipped silently and shown up as a query document that opened blank. The type moves to docs.ts, beside the URI builders every producer already imports — nobody should have to import a view to describe what they are handing it — and the two call sites that build a payload now declare it. Opus B: SqlApp's local copy can become a re-export from docs.ts; the shapes are identical, so nothing breaks in the meantime. `DEFAULT_MAX_DEPTH`, `JOIN_SQL_HEADER` and the history store's preference keys, `attachPrefs` and `recordRun` were exported with no caller outside their own module. An export is a promise to keep something stable, and these were promises nobody had asked for. `MAX_JOIN_PATHS` stays exported with a note saying why: the drawer compares against it to admit the path list is a sample rather than the answer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit d6a927e390f93aa18892d5a94af2009d264c73ce) * fix(sql-ui): split a selection instead of sending it as one statement A dragged selection was sent as a single `execute`, classified by whatever keyword happened to come first. Selecting `SELECT …; DELETE FROM t` therefore read as a SELECT: the statement was marked read-only and so became eligible for the session's retry-with-a-fresh-token, which could run the DELETE twice; the pattern check only ever saw the first verb; the transaction refusal only fired when the FIRST statement was transaction control, so selecting `UPDATE …; ROLLBACK;` sent both and the ROLLBACK silently did nothing. psycopg2 executes multi-statement strings, so this was reachable, not theoretical. `splitStatementsIn` splits a slice of the buffer and rebases every offset and line number onto the whole buffer, so each statement in a selection gets its own classification, pattern check, limit, history entry and strip position while decorations still point at the real text. A selection holding part of one statement still yields that one fragment, which is what a partial selection wants. The pre-run line now says how many statements a selection holds, so a multi-statement selection cannot be mistaken for one before it runs, and a buffer holding only comments reads `no statement found` rather than claiming the editor is empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit e7c2adbd935f159e1cc207132c1c0f9fe5f115f2) * fix(sql-ui): publish the completion model once the editor model exists Schema autocomplete was silently dead on the ordinary path. The effect that publishes this editor's completion model into the shared provider's map read the model URI from a ref and returned early when it was null, keyed only on `[completion, dialect]`. @monaco-editor/react mounts asynchronously — loader promise, then editor, then onMount — so the URI is null on the first pass, and nothing re-ran the effect unless the snapshot happened to arrive afterwards. Browsing a connection and then opening a query document, which is the normal order, therefore produced a completion model that was never published: the provider found no entry for the model and returned nothing, leaving only Monaco's built-in keyword list. It appeared to work only when the schema finished loading after the editor had mounted. The URI is now state as well as a ref, so the effect runs again once it is known, and `handleMount` also publishes immediately from prop mirrors rather than waiting a render. The JSDoc records why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 09f046243fcc62ae6b84836d12fd5119d10b6c41) * fix(sql-ui): say "ran" for reads, and stop claiming no limit was applied Three wording and modelling corrections. The QueryView side of each landed in e7c2adbd; this is the model and the tests. A read that finished now says `ran`; only a write or DDL says `committed`. The strip could previously report `1–3 committed` for three SELECTs, which dilutes the one word the app uses to mean the change is durable and cannot be undone from here. Anything not clearly read-only is still called committed, which is the safe direction. `formatPriorStatements` builds the failure banner's second sentence the same way, so a batch that failed after two reads says they ran rather than implying data changed. `applyRowLimit` now reports which of three states applies. A statement carrying its own LIMIT used to render `100 rows returned (no limit applied)` — true of the app and badly misleading about the result, since the generated "Select top 100" reads exactly that way and a reader concludes the table holds 100 rows. It now reads `(limit in statement)`, and the check runs before the header selection so `All` reports it too. A statement rerun from the history drawer has no position in the editor buffer, so `startLine`/`endLine` are optional rather than faked as line 1 — the status strip omits the line reference instead of pointing at unrelated text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 91fb9fe13ed64c2aaf3f415b1fa72720af8e0153) * fix(sql-ui): stop telling users Refresh will find a missing table The suggestion hint read "Missing a table? Refresh schema on the connection page." On a node without the `refresh_schema` tool that advice does nothing at all: the node reflects once at task start and serves that reflection however often Refresh is pressed, so a table created since will not appear no matter what the user does from here. It now reads "Missing a table? The snapshot dates from HH:MM; Refresh re-reads it only on nodes with refresh_schema." — which dates the data and says what the button can and cannot do, rather than sending the user round a loop that cannot succeed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit a1c7ca002cf2a1f6c20bf55bc9762989235b3d35) * chore(sql-ui): drop the unused table-name decoder `tableFromTableDataUri` was added as the symmetric counterpart to the URI encoder and has never had a caller — the views read their table from the document's content payload. An export with no caller is a maintenance claim nobody is checking, so it and its tests go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit 497540e230285405ac222b1104419400a7e9343e) * fix(sql-ui): make Explain give the same answer however it is invoked The button's disabled state was computed from the debounced caret while the click handler read the live editor handle, so the two could briefly disagree: the button could look available and then do nothing. Worse, Ctrl/Cmd+Shift+E returned silently whenever the statement could not be explained — a keybinding that does nothing at all reads as a broken app, and unlike the button it has no tooltip to explain itself. Both paths now go through one `explainRefusal` function, so they can never offer different reasons. The live handle still decides WHAT is explained, so a caret move that has not settled cannot send the previous statement, and when the answer is that nothing can be explained the reason is announced — the same sentence the button's title carries. Also drops SqlApp's own copy of the query-document payload type in favour of the one in docs.ts, which is where the panels that build the payload already read it from. The shapes were identical; two declarations of the same contract is one too many. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit d83e48224c0ab1d840ba86411f8a6861af2f724d) * fix(sql-ui): ask for a fresh schema read from the diagram's Retry and Reverse Engineer The two user-initiated refreshes in DiagramView still served the task-start snapshot even on nodes that provide refresh_schema. Also drops the unused IQueryDocPayload re-export from SqlApp (producers import it from docs.ts). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit c9331827e435ab0f1bf3a8baf5be92b36c39d3ef) * docs(apps): sync the SQL Explorer guide with the reviewed UI wording Reads say "ran" and writes "committed" in batch outcomes; the results line has three limit states; the ClickHouse ALTER note covers only mutation forms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4fLnswQF6yKQ6x7eWzSdd (cherry picked from commit f9c7664ab4a6fbd27752db8fd59449874511e7d8) * fix(sql-ui): make the appended row limit reach the statement it bounds `applyRowLimit` ran its checks on comment-stripped text but built the statement from the raw one and joined the clause with a space, so `SELECT * FROM orders -- daily` was sent as `... -- daily LIMIT 200`: the database ran an unbounded SELECT while the meta line reported "limit 200". The clause is now joined with a newline, which terminates a trailing line comment before LIMIT. The `in-statement` check had the mirror-image problem: `/\blimit\b/i` matched anywhere in the statement, so a LIMIT inside a subquery (or inside a string literal) made the app skip its own limit and label the result "limit in statement" while the outer query streamed every row. It now uses the splitter's `hasTopLevelKeyword`, which already masks strings and comments and only accepts a match at parenthesis depth 0. The existing comment test asserted `.limit` alone and so never saw the broken SQL; the regression tests assert `.sql` and re-strip comments from the emitted statement to prove LIMIT survives as code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): enforce each dialect's delimiter preconditions in the splitter Two delimiters were recognised more eagerly than the engines recognise them, and in both cases the scanner swallowed a statement separator. MySQL only starts a line comment when whitespace (or a control character) follows `--`: it parses `SELECT 1--2; SELECT 3` as arithmetic across two statements, while the scanner hid the semicolon and returned one. `opensDashComment` applies that precondition for MySQL alone — Postgres and ClickHouse keep taking a bare `--`. PostgreSQL allows `$` inside an unquoted identifier, so `foo$tag$` is one identifier and opens no dollar-quoted body. `continuesIdentifier` refuses a dollar quote whose `$` follows an identifier character, so `SELECT foo$tag$; SELECT 2` splits again. Near-miss cases for both are in the splitter's table, including the contrasting Postgres reading of the same `--` text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): stop one bad refresh from disabling fresh reflection Two ways a refresh could quietly stop happening, both leaving the app describing the database against a snapshot it knows is out of date. A single fallback latched `refreshTool: 'unavailable'` for the rest of the app session, and nothing ever cleared it. But the session cannot tell a missing tool from a restarted task or a timed-out call — it says so itself in `connect/session.ts` — so one fallback during a `fresh` refresh made every later Refresh Schema, Reverse Engineer and post-DDL read serve the task-start snapshot. The state now carries a `suspect` step: one fallback suspects, a SECOND CONSECUTIVE one latches, and a fresh read that answers clears the suspicion. A node that really has no `refresh_schema` still stops paying two round trips after two tries. `refreshSchema` also returned immediately whenever a refresh was already loading, so Apply in TableDesignView could neither start nor await its post-DDL reflection — and the outcome effect composes its banner once, from the snapshot present when Apply resolves, so the change was reported against pre-DDL structure. Refreshes are now serialised per connection: an ordinary read joins whatever is running, while a `fresh` request queues behind it and runs its own, because a plain re-read cannot answer a caller that just ran DDL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): do not record history before the recording switch is read A persisted `sql.historyRecording[key] === false` is unreadable until a bridge mounts and `hydrate` runs. In that window `recording[key]` is merely absent, which the guard read as the default — on — so a run that finished first stored the user's SQL, and `hydrate` then merged that entry and scheduled it to the workspace preferences file. History the user had explicitly turned off was persisted, statement text and literal values included. Runs that arrive before hydration now wait in a bounded queue and are decided once the stored flags are in hand: replayed oldest-first when recording is on for that connection, dropped when it is off. The queue is capped at 100 entries for the same reason the history itself is bounded — it holds the user's SQL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): keep a history entry's row limit and its full statement `IHistoryEntry.limit` exists because a row count without its limit is ambiguous — 1,000 rows is either the whole answer or the ceiling cutting it off. Two transformations lost it anyway: `collapse` kept the OLDER run's limit beside the NEWER run's row count, and `readEntry` dropped the field entirely on hydration, so every limit vanished the first time the workspace file was re-read. The limit is now replaced with the rest of the run facts on collapse, and validated as a number on hydration. `trimHistory` also deduplicated AFTER truncating, so two different statements sharing their first 8 KiB collapsed into one entry and one of them was lost. Repeats are now collapsed on the untruncated text and the storage bound is applied to what survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): make "Run and stop asking" hold for the rest of the batch `setPatternChecks` only updated state, and the ref the run loop reads was mirrored by a passive effect. The loop resumes from its awaited confirmation in a microtask and reaches the next statement's check before React runs that effect, so iteration i + 1 still saw `true` and opened the dialog again — for every further matching statement in the same batch, however many times the user chose "Run and stop asking". The ref is now written next to the state update, which is the only write ordering the run loop can rely on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): restore the mounted flag when StrictMode replays the effect The cleanup set `mountedRef.current = false` and only `useRef(true)` ever set it back, which a re-run of the effect on the SAME instance does not re-execute. Under the shell's `React.StrictMode` root that leaves the flag false after the replayed setup, so `record` returns early and no completed statement reaches history for as long as that instance lives. The setup now asserts the flag it owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): bind the table name in both foreign-key catalog queries `fetchForeignKeyNames` interpolated the table name through `quoteLiteral`, which doubles `'` and leaves `\` alone. On a MySQL connection that honours backslash escapes — the default outside NO_BACKSLASH_ESCAPES — a name containing `\'` ends the literal early and the remainder of the name becomes part of the WHERE predicate. The name is a VALUE compared against INFORMATION_SCHEMA columns, so both dialects now take it as a `$1` bind, the way every value in `sql/paging.ts` already travels; the node rewrites `$n` per driver. That left `sql/quote.ts` with no callers. Its own header says it exists for the statements that cannot bind, and there are none left, so the module and its suite go with the last use rather than standing by as a ready-made way to reintroduce the same class of defect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): do not render an error headline with nothing after it `describeFailure('')` returned `generic: false`, and both the query view and the Explain drawer show the headline whenever `generic` is false — so an Error carrying an empty message produced the banner "Database reported:" followed by nothing. An empty (or whitespace-only) message is the same situation as the node's own placeholder: there is nothing real to quote, which is exactly what the generic wording says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): make the drawers report what actually happened Four places where the UI said nothing, or said the wrong thing. Copy in the row inspector took an optional chain through `navigator.clipboard`, so an absent Clipboard API skipped both callbacks and the button neither copied nor announced anything. A missing API now announces the same failure a rejection does. The Explain drawer showed "Explaining…" for as long as the shell connection was down, because `run` returns before sending anything when there is no client — so `running` never became true and no request was ever in flight. It now says it is not connected. The history row's statement and note were inline spans, where `overflow`, `text-overflow` and vertical margins have no effect: a long one-line statement overflowed the drawer instead of ending in an ellipsis, and the note ran on beside the statement instead of under it. Both are block boxes now. The live region replaced its child by key, which performs the removal and the insertion in ONE commit; assistive technology can coalesce that into no change and miss a repeated sentence. It now empties and restores the text in separate tasks, cancelling a pending restore when a newer announcement arrives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): keep a control's outcome to the control the user pressed The data grid's error banner survived until the NEXT request succeeded, so after one failed page a fresh search, sort or page left the previous request's failure on screen while the grid loaded different parameters — the banner described a question the user had already moved on from. The error is now cleared where the new sequence is taken. In the designer, the column row carries button semantics and contains the Drop button, so a click (or Enter/Space) on Drop bubbled into `selectColumn`: the column was dropped and its editor opened at the same time, leaving the user staging an edit for a column that is on its way out. The cell around the button stops propagation — the shell's Button passes no event to its handler, so it cannot be stopped at the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): open the table record drawer where it can be seen `requestTableRecord` is raised by the Insights panel and by the sidebar tree, but the drawer it opens is rendered by OverviewPanel — and TabPanel hides the inactive panel with `display: none`, so a table opened while Insights was in front raised a drawer inside a hidden subtree. The connection view now brings Overview forward when a request for its own connection arrives, which covers the sidebar route as well. Requests raised before the document mounted are ignored, so opening a connection never overrides the page the user left it on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(apps): state where a statement's own LIMIT has to be Two sentences in the SQL Explorer guide now match what the code does: `limit in statement` reports a LIMIT on the outermost query only, and the splitter paragraph names the two dialect preconditions a reader would otherwise have to discover — MySQL's whitespace after `--`, and a `$tag$` that continues an identifier opening no dollar quote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(sql-ui): test MySQL's comment precondition without a control regex The dash-comment check spelled its "whitespace or control character" rule as a regex class covering U+0000-U+001F, which the repo's `no-control-regex` lint rejects. A `\s` test plus a code-point comparison says the same thing and keeps `eslint apps/sql-ui` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): never collapse a history entry that is already truncated trimHistory compares statements on their text, and the previous fix moved the collapse ahead of the per-entry bound so a FRESH pair is compared on full text. That protects an entry only up to its first truncation. historyStore.hydrate reads bounded entries back out of the workspace file and passes them through trimHistory again on every reload. By then `sql` is a prefix, so two different statements that agree for their first maxSqlChars characters satisfy sameStatement and one real entry is collapsed away — silently, and once per app open. A prefix cannot prove two statements are the same, so a truncated entry is no longer merged at all. The alternative, persisting a full-text fingerprint, was rejected: it widens the persisted schema with another field read back from an untrusted, hand-editable file, and every entry already on disk lacks it, so the conservative rule would still be needed as the fallback. What it buys is collapsing consecutive re-runs of an 8 KB+ statement, which costs one extra row when it does not happen. Tests: a reload-shaped regression (trim, then trim the output again) and the fresh-repeat-of-a-truncated-entry case. Both fail without the guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-ui): write the history recording switch through, not on the batch setRecording only called schedulePersist, a 300 ms debounce, and the last bridge detach CANCELS a pending write. Switching apps within that window dropped the write: the workspace file kept saying recording was on, and the next page load resumed recording SQL on a connection the user had switched off. The queue added for pre-hydration runs narrowed the exposure but never closed it, because it only governs runs that arrive before the flag is readable, not a flag that never reached disk. The switch is now written through immediately. Runs still batch — they arrive in bursts and one lost run costs a list row — but a switch is flipped once, deliberately, and losing it costs the user a decision about their own data that they will never be told was discarded. A second defect sat behind the same line. hydrate merged the whole in-memory `recording` map over the stored flags, so any flag from an earlier attach outranked the file for the rest of the session — a stale `true` surviving a detach turned recording back on for a connection the file says is off. Only overrides that have NOT yet reached disk may outrank the file, so they are tracked separately in `unsavedRecording`, cleared on a successful write, and hydrate now schedules a write when one is still outstanding (a switch flipped while no bridge was mounted). attachPrefs is exported: it is the store's real seam onto the shell, and the rules worth testing live there rather than in the null-rendering bridge component. The module had no test coverage at all. It now has seven tests driving it the way the app does — attach, act, detach — asserting on what reached a fake workspace file, because that file is the privacy boundary. Three of them fail against the previous code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(apps): state what "no limit applied" means with All and an inner LIMIT applyRowLimit recognises only a LIMIT on the outermost query. With the header set to All, a statement whose only LIMIT sits in a subquery therefore reports state `none`: QueryView prints `no limit applied` and nothing is appended. The guide described `no limit applied` as the case for All "with no LIMIT in the text", which reads as a contradiction the first time someone sees the line on a statement that plainly contains one. The guide now names that combination and shows a join against a limited subquery, where the inner bound genuinely does not bound the result, so the line is read as a statement about the result rather than the text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(sql-ui): fail the test target when discovery finds no tests `builder test` runs sql-ui:test in CI, and the action returned cleanly — task.output set, exit 0 — both when tests/ was absent and when the *.test.ts(x) filter matched nothing. Either state reported the whole target green while not a single assertion had run, over a suite of 521 tests that is the only cover this app has. Both cases are now errors. tests/ is tracked source: a directory that has gone missing, or a…
…2376) * fix(ai): pass single-component validate payloads through unwrapped * style(ai): drop extra blank line so ruff format passes
* fix(db_hotdata): match the live Hotdata API result contract
Three defects, all traced to the node disagreeing with what the API
actually returns. Verified against api.hotdata.dev.
Result rows never got their column names. /v1/query and /v1/results both
answer with `columns` (the names) and `rows` (lists of *values*), never
with objects:
{"columns": ["city","units"], "rows": [["Reno",200],["Austin",120]]}
_run_sql passed `rows` straight through, so everything downstream saw
positional lists. _rows_to_markdown fell through its isinstance(dict)
check to str(row) and emitted Python list reprs - "['Reno', 200]" - on
the answers, table and text lanes instead of a Markdown table. The same
cause silently emptied _schema_via_sql, whose `if not isinstance(row,
dict): continue` dropped every row, so natural-language SQL was
generated against no schema at all. Names are now attached once, at the
_run_sql boundary, with repeated and empty column names disambiguated so
`SELECT city, city` cannot collapse to one value.
get_result and get_query_run never sent X-Database-Id. Both endpoints
are database-scoped and answer 400 "this endpoint is scoped to a
database" without it, so the whole deferred-query path - every truncated
result and every async run poll - failed 100% of the time.
get_data retried failures no rewrite could fix. A bare `except
Exception` fed anything back to the generator, so a bad workspace (HTTP
404 workspace_not_found) spent three LLM calls and ~100s before
reporting "could not answer after 3 attempts", with the real cause
buried in the tail. Retries are now limited to what a different
statement could plausibly fix: HTTP 400, plus a deferred run that failed
server-side (SqlStatementError, which carries no HTTP status). Auth,
workspace, database and transport failures raise as-is, keeping the
server's own wording. Same case now fails in ~8s naming the 404.
Also floors async_after_ms at 1000; the API rejects anything lower, so a
pipeline configured below it failed every query.
Five existing tests changed: their mocks returned `rows` as dicts and
raised SQL errors as bare RuntimeError, neither of which the live API
does. That unfaithful mocking is why the row-naming bug shipped green.
Mocks now fail the way the client actually fails, and 13 new tests pin
the behaviour (162 collected, up from 149).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(db_hotdata): stop a generated column key overwriting a real one
Review catch. Both cases drop a value silently, which is the exact
failure the naming helper exists to prevent.
_name_columns checked a generated name against the base it was derived
from, not against the names already taken. `SELECT city, city, city_1`
renamed the second `city` to `city_1` and collided with the real third
column, so the dict kept two values out of three.
The same hole sat in the wider-than-header tail: `column_3` is a legal
column name, so for columns ['a', 'column_3'] a three-value row parked
its third value on the key already holding the second.
Both now allocate through _free_name, which suffixes until the key is
unclaimed and reserves it. Two regression tests, which fail on the
previous code with 'city_1' twice and [1, 3] respectively.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(db_hotdata): retry on the statement-error contract, not a bare 400
Review catch. _run_sql drives four endpoints - create_database, query,
get_query_run, get_result - and `status_code == 400` treated a failure
from any of them as SQL the model could rewrite. A bad ttl, a malformed
limit or a missing scoping header would each burn every LLM attempt and
then be replaced by "could not answer after N attempts", which is the
exact masking this branch set out to remove.
The server distinguishes the two itself: it answers with a query_run_id
only once it has created a run and executed the statement. Verified
against the live API - missing table, parse error and unknown function
all return 400 with a run id; bad ttl and out-of-range limit return 400
without one.
HotdataError now carries query_run_id off the error body, and
_is_sql_fixable requires it. SqlStatementError still covers the deferred
path, where a terminal run failure is reported in the poll body with no
HTTP status to read.
Four tests: the client lifts the run id when present and leaves it empty
when absent, and get_data retries the first while raising the second
as-is without spending a second LLM call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(db_hotdata): close out the review - signature, schema, docs, rendering
Review catch, plus a full pass over the branch for the same class of
problem so they land together rather than one per round.
get_result kept `offset` and `limit` in their original positions.
Database scoping had been inserted ahead of them, so a positional
get_result(result_id, offset, limit) written against the old signature
would have sent its offset as the X-Database-Id header. `database_id` is
now keyword-only and last.
services.json advertised `minimum: 0` for async_after_ms while the node
floors it at 1000, so the form accepted a value the node then silently
overrode. Both now read 1000 - the smallest the API accepts - and a test
ties the schema to the constant so they cannot drift apart.
A cancelled run is no longer classified as a statement failure. A
rewritten query cannot un-cancel a run, so it must not earn an LLM turn.
Two rendering bugs that only became reachable once rows were objects:
header cells were not escaped, so an alias such as "a|b" split its
column in two, and SQL NULL rendered as the Python word "None". Headers
go through _cell like body cells, and NULL is an empty cell.
README: get_data retries a rejected statement, not any failure, and the
troubleshooting table says why a 401/404 surfaces on the first attempt.
Corrects a claim made earlier on this branch. The docstrings cited
`SELECT city, city` as the source of repeated column names; the planner
rejects that outright. Verified against the live API, the real source is
a join - `SELECT a.city, b.city` answers ['city', 'city'] and `SELECT *`
over a self-join answers ['city', 'units', 'city', 'units']. The naming
helper is needed for exactly that, and a test now pins the live shape.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* fix(db_hotdata): feed the real failure reason back for a deferred run
Found by driving the deferred path against the live API, which no test
had done: every earlier live query was small enough to return inline.
A run that fails after deferring reports its cause as `error_message`:
{"status": "failed",
"error_message": "query execution failed: Arrow error: Divide by zero error"}
_await_run read `error` or `message`, found neither, and fell back to the
status, so the exception read "query failed: failed". That text is what
the model is handed for its corrective attempt, so every deferred
failure was being retried blind. The jobs poller in the same file
already reads `error_message`; this was the one place with guessed keys,
covered by a mock that encoded the guess.
_run_failure_text reads `error_message` first, then `error` as text or
as the {"message": ...} object the query endpoint uses, then `message`.
Verified live with a 20M-row window query that divides by zero on its
last row: the run deferred, polled four times, failed, and the caller
now receives the Arrow error. The same session confirmed the deferred
success path (query -> 4x get_query_run -> get_result) and the truncated
path (query truncated=true -> get_result) end to end for the first time.
Also adds docstrings to the functions this branch touches; coverage on
touched functions goes from 66% to 90%.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* fix(db_hotdata): wait for the run before reading a truncated result
Addresses the review on #2372, read against the published contract at
https://www.hotdata.dev/openapi.yaml.
A result that was still being saved came back as an empty answer. A
truncated /v1/query response always carries a result_id, so the old
`result_id is None` test never waited for it - and per QueryResponse
that id "is issued while the save is still in flight, so on its own it
does not mean the result can be retrieved". Reading it early answers 202
{status, result_id} with no rows; _request only raises from 400 up, so
_run_sql returned zero rows for a result large enough to be truncated.
Reproduced live with an 8M-row query: 200 truncated, then 202
"processing", then 200 once the run had succeeded. _run_sql now waits on
the run whenever the server deferred or truncated, and get_result raises
on a pending/processing body instead of letting it read as empty.
`interrupted` is a documented terminal run status the poll loop did not
know, so it was polled until job_timeout_secs ran out and reported as a
timeout that never happened. It is now terminal and raised as a plain
RuntimeError, not SqlStatementError: a rewritten statement cannot
un-interrupt a run. `cancelled`/`canceled` were never seen live; the
comment on the set says so.
_query_run_id reads inside the `error` object as well as beside it, the
way _error_code does, since the documented error body is
{"error": {code, message}}.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* Update nodes/src/nodes/db_hotdata/IInstance.py
Co-authored-by: Alexandru Sclearuc <asclearuc@gmail.com>
* fix(db_hotdata): run an interrupted query once more, as the contract says
Addresses the three notes left with the approval on #2372.
`interrupted` was classified correctly - terminal, not the statement's
fault, never SqlStatementError - but nothing acted on the other half of
the contract, which calls it "terminal, and safe to retry". The caller
lost a whole tool call because Hotdata replaced a server, and so did
`execute` and `get_schema`, which share the path.
_run_sql now runs the same statement once more when the run comes back
interrupted. It is bounded at one re-run, so a run interrupted every
time still ends; it sends the same SQL, so it costs no LLM turn; and it
is safe to send twice because only read-only statements reach it.
_await_run signals the case with its own RunInterrupted type, a
RuntimeError, so `cancelled` and failed statements are never re-run and
everything that already treats it as "not the SQL's fault" still does.
Pins the suggestion applied in e5fa0d8: a run that reaches a
terminal-OK status without a result_id still reads the id the query
answered with, instead of falling through to an empty result. The test
fails on c628275 and passes from e5fa0d8.
Settles the result_id handed out on the inline fast path. It is issued
while the save is still in flight, and it is safe to hand out anyway:
the loads endpoint documents 409 "the result is still being computed"
rather than loading part of a result, and three loads issued right
behind a 9,500-row query each returned all 9,500 rows. The code says so
where the id is returned.
The re-run is covered by mocks only - a server replacement cannot be
provoked from outside - so it rests on the spec's wording.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* fix(db_hotdata): a run is finished when its status says so
Addresses the review on #2372 at 1c67cd9.
Blocking: _await_run returned as soon as a run named a result_id, even
while it was still `running`. The spec's `running` covers "still
executing, or still being saved", so that id names a result that is not
ready; a read against it answers 202, which raises and fails the whole
question. Truncated results have come through this path since c628275,
which is what made it reachable. It now returns on the status alone.
A run that ends succeeded with no result_id "saved nothing", so the id on
the query body names a result that was never saved. e5fa0d8 fell back
to reading that id, which answers 404 or 202. The statement did succeed,
and a truncated response carries a preview of its rows, so that preview
is returned instead, cut to the limit, and no result_id is offered. A
deferred query has nothing inline, so it raises with the run's
warning_message rather than report an empty result.
Not changed, deliberately: a `failed` run behind a truncated preview is
still handed to the model as a statement failure. The preview is
streamed before the statement finishes, so it can fail on a later row.
Checked live - an 8M-row query whose last row divides by zero answered
200 truncated with 10,000 rows in 0.3s, and its run then ended failed
with "query execution failed: Arrow error: Divide by zero error". A
rewrite fixes that. `failed` also covers a failed save, but only
error_message tells the two apart, and that is prose, not contract. The
code and a test now record this.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Alexandru Sclearuc <asclearuc@gmail.com>
… binding, drop the scaffolded launch.json (#2386) F5 app debugging had three defects: - Breakpoints never bound: the app bundles' sourcemaps carry sources relative to the build root's parent (the workspace root), so after the browser clamps the leading "../"s js-debug sees "<origin>/apps/<name>/src/*.tsx". With webRoot set to the APP folder the mapped path doubled ("…/apps/home-ui/apps/home-ui/…") and never matched a real file. webRoot is now the workspace root and the stale sourceMapPathOverrides (written for webpack:/// URLs the bundles do not emit) are gone. - The debug session was hardwired to Edge. New browser.ts resolves the developer's OS default browser onto a js-debug CDP target: Chrome/Edge map to their native types, other Chromiums (Brave, Vivaldi, Opera, Arc) launch through the 'chrome' type with the registry-resolved runtimeExecutable, and a non-Chromium default falls back to Edge with a log line explaining why. Resolved once per session. - The scaffolder wrote a static launch.json entry that could not ensure the watch session, so F5 from it opened a preview with no dev bundle served. The entry (and ensureLaunchConfig) are removed: rocketride.app.debug is the single debug entry point and builds its config in-memory. Supporting fixes: - appLoader.isDevPreviewPage() now returns false for TOP-LEVEL pages: only framed pages (App Builder panel) receive the rrdev:registerRemote postMessage injection. An F5 external-browser tab with rrdev=1 has no embedder, and its dev entry is already in the server manifest (resolved via the ?rrsession= nonce) — holding for a message that can never come left the tab blank. - DesignView: the DEV watch-target badge is now a link that launches the same debug session as the gear menu's Debug (F5) row. - AGENTS.md: correct the './builder build' description (global command, builds every module, not server-only). README touch-ups: pin tiktoken <1.0.0 in context_optimizer docs; regenerate core README wording. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…hon (#2369) * fix(engine): keep a Python thread's own name in the engine and in Python A thread Python starts, such as the asyncio workers the data path hands every pipe open, write and close to, reached the engine without a thread context. The engine made one named "External", and the first call back into Python (setupDebug) copied that over the thread's Python name, so the profiler listed every such thread as "External" and engine logs could not be matched to Python ones. syncThreadName() now settles one name per thread on both sides: a thread Python started keeps its name and the engine takes it; a thread Python knows only as a _DummyThread takes the engine's name, as before. It runs where Python enters the engine (UnlockPython, which now holds gil_scoped_release as a member initialized after the name sync, so the GIL is still held) and where the engine enters Python (setupDebug). The placeholder becomes ThreadApi::ExternalName. New engtest cases cover a Python-started thread, the same with a context made before UnlockPython, an engine thread and a foreign thread; the first two failed before this change ("External" == "rr-py-thread"). * test(engine): keep the thread-name workers from outliving their state The Python worker was not a daemon, so a hang in a GIL path would have left it alive and Py_FinalizeEx() at the end of the run waits for such a thread: the process would hang after reporting the failure. The engine and foreign workers kept their Names and Done on the Catch2 thread's stack, which a worker abandoned on timeout writes into after Catch2 has unwound the case. Both now share one State through a shared_ptr the worker holds a copy of.
* chore(test): remove legacy test * feat(server): shared engine library
* feat(nodes): add streamed media inspect node * fix(nodes): address media inspect review and portability findings * test(nodes): initialize media inspect dependencies before parallel tests * fix(media-inspect): reject non-finite settings and protect native imports * test(media-inspect): retain real loader across stock test collection
…2400) Nothing has reached npm, PyPI, Open VSX or the VS Code Marketplace since 2026-07-07. All five artefacts sit at 1.3.0 because `main` has not moved since that date, while `develop` carries 758 commits of work that no release train has ever picked up. The versions on develop could not be released as they stood: server 3.3.0 < published 3.3.1, tag server-v3.3.0 taken vscode 1.2.0 < published 1.3.0, tag vscode-v1.2.0 taken client-typescript 1.3.0 = published, tag taken client-python 1.3.0 = published, tag taken client-mcp 1.5.0 unpublished, tag free Every one of them would have been skipped by the tag and version-exists checks, so a release run would have been green and shipped nothing. This bumps the four that needed it and leaves client-mcp at 1.5.0, which is already a valid unused candidate. Verified free before choosing them: the tags server-v3.4.0, vscode-v1.4.0, client-typescript-v1.4.0, client-python-v1.4.0 and client-mcp-v1.5.0 are all absent, and npm 1.4.0, PyPI 1.4.0, PyPI mcp 1.5.0 and Open VSX 1.4.0 all return not-found. The changelog cut rides this PR as RELEASE.md requires, so the release body is a scoped section rather than an ever-growing [Unreleased] blob. Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP Co-authored-by: Dmitrii Kataraev <dmitrii.kataraev@rocketride.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four files conflicted when promoting develop to stage. All four resolve
to develop's side:
packages/ai/src/ai/modules/task/commands/cmd_misc.py
packages/ai/tests/ai/modules/task/commands/test_cmd_misc.py
stage carries 562ec48, a cherry-pick of develop's 1180074 (#2346),
byte-identical by `git patch-id --stable` (f5afd116...). develop then
superseded that approach in 9e902c1 (#2376, 21 Sep): a root-level
`component` is passed through unwrapped rather than rewrapped as a
one-item `components` list. stage simply stopped at #2346.
docs/agents/context/ROCKETRIDE_python_API.md
docs/agents/context/ROCKETRIDE_typescript_API.md
generated; develop's copies are the current output.
After resolving, the merged tree is identical to develop's: zero files
differ. That confirms stage carries nothing develop lacks, consistent with
all 19 stage-only commits matching develop by patch-id.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP
kwit75
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
September 23, 2026 14:36
Contributor
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (5)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: rocketride-org/rocketride-server/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Contributor
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
github-actions
Bot
added
docs
github-actions
Bot
added
module:ai
Rod-Christensen
approved these changes
Sep 23, 2026
dk-rocketride
added a commit
that referenced
this pull request
Sep 23, 2026
…on bump only (#2409) #2405 merged all of develop onto stage, including 34 commits that were never deployed to staging or tested. The release is meant to ship the stage that was captured and tested (d3b6eea), not develop. This reverts e9c7d8c (#2405), which restores stage's tree exactly to d3b6eea, and applies only what publishing mechanically requires. The published versions and tags already exist at the old numbers: rocketride-server 3.3.0 -> 3.4.0 vscode 1.2.0 -> 1.4.0 client-typescript 1.3.0 -> 1.4.0 client-python 1.3.0 -> 1.4.0 client-mcp 1.5.0 (unchanged, unpublished) CHANGELOG [Unreleased] cut to [3.4.0] Diff against d3b6eea: those five files and nothing else. No code. Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
13 participants
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.
Replaces #2403, which conflicted, and a conflicting PR runs no CI here.
This branch is
develop(9d07cef5, with the #2400 version bump and changelog cut) plus a merge ofstage, and the conflicts are resolved toward develop. Its tree is identical todevelop: zero files differ. So this PR moves stage onto exactly what develop holds.The four conflicts, and why develop's side is correct
cmd_misc.pyandtest_cmd_misc.py. stage carries562ec48c, a cherry-pick of develop's11800744(fix(ai): accept the single-component validation payload the node panel sends #2346). They are byte-identical bygit patch-id --stable. develop then superseded that approach three days later in9e902c1c(fix(ai): pass single-component validate payloads through unwrapped #2376): a root-levelcomponentis passed through unwrapped rather than rewrapped into a one-itemcomponentslist. stage simply stopped at fix(ai): accept the single-component validation payload the node panel sends #2346.docs/agents/context/ROCKETRIDE_{python,typescript}_API.md. These are generated, and develop's copies are current.That the resolved tree equals develop confirms what the patch-id check found earlier: all 19 stage-only commits are already in develop by content.
Versions after this lands on stage
server 3.4.0, vscode 1.4.0, client-typescript 1.4.0, client-python 1.4.0, client-mcp 1.5.0.
This publishes nothing. Publishing happens at #2404 (stage → main), which stays a draft with its approval checklist until this merges and its diff is re-read.
Watch the first CI run. CI has never run on this promotion before.
🤖 Generated with Claude Code
https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP