Repository navigation
fix(analyst): report tool failures as failures, accept valid read-only syntax, retrieve long text in full - #1448
Open
anandgupta42 wants to merge 10 commits into
Open
anandgupta42 wants to merge 10 commits into
anandgupta42 wants to merge 10 commits into
Conversation
…y syntax, retrieve long text in full
Reliability fixes for the read-only analyst, each with a red-first regression
test built from real tool payloads:
- Tool results that carry a backend error (`metadata.error`, `success:false`,
an explicit error envelope) are now returned with `status: "error"` and
rejected at both SDK resolver boundaries, so the model, lint and telemetry
see the failure instead of `ok`. `bash` marks non-zero or signalled exits
and trailing Python tracebacks as failures while keeping the output; the
truncation hint no longer claims success on a failed call.
- The read-only classifier accepts DuckDB integer division `//`, negated
`GLOB` on SQLite/DuckDB, DuckDB qualified `prior.*` aliases and standalone
or CTE `PIVOT`/`UNPIVOT` bodies by adapting only the parser input; executed
SQL is unchanged and parser/lexical disagreement still fails closed.
- Mongo commands written as `{collection, find|aggregate|count|distinct}`
are normalised in the driver; the materialize refusal lists accepted shapes.
- JSONL spill rows pad absent values with null so every row has one value
per column.
- `read` gains `char_offset`/`max_chars` so a clipped long line can be read
in windows; the clip banner gives the next window. `sql_execute` gains
`full_text_columns` (limit <= 20) to return complete text cells; the
truncation banner points to it.
- Analyst prompt: one sentence asking that a rule or policy which decides the
answer be read in full and each condition evaluated against the row's own
fields and dates before choosing.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rs from the read prompt Review corrections to the tool-failure reporting change: - `bash` completes on process exit again, draining buffered output for a short bounded grace instead of waiting for stream close, so a command that leaves a background child running returns promptly and is not reported as a failure. - A non-zero `bash` exit (grep no-match, diff, test) stays an ordinary tool result the model reads from the exit code; `metadata.exit_code` and `metadata.success` record it for telemetry only. SDK error rejection is reserved for explicit error envelopes and backend `metadata.error` (`success:false` alone no longer qualifies). A Python traceback counts only when it is the final stderr block with a non-zero exit, or with empty stdout on a zero exit; tracebacks printed as data do not. - `read`'s tool description no longer contains source-marker comment lines. - Aborted tools call `completeToolCall` before the SDK-boundary check; `warehouse_test` declares its STORE LOCKED / INFRASTRUCTURE FAILURE guidance as a successful diagnostic; `batch` no longer counts an inner error envelope as a success. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
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 |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0379b21ca7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 29 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…refuse, bash failure signals narrowed Independent-review follow-ups: - The Mongo `count` → `countDocuments` alias maps `query` to `filter`, so a filtered count no longer counts every document. - Collection shorthand with a conflicting explicit argument (`find` + `filter`, `count` + `query`, `distinct` + `field`) is refused as ambiguous instead of silently dropping one side. - The production tool boundary (`SessionPrompt.resolveTools`) is covered by a test that fails if the SDK-boundary check is removed. - `bash` sets `metadata.failure_kind` only for timeout, abort, signal or a qualifying stderr traceback; ordinary non-zero exits keep `exit_code` and `success:false` for telemetry but no longer produce the failed truncation hint, an error trace status or a `core_failure` event. - `batch` appends file lessons before the SDK-boundary check, matching the direct-call path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…' into feat/analyst-v10-reliability # Conflicts: # packages/drivers/src/mongodb.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
The incremental Files Reviewed (7 files)
Previous Review Summaries (4 snapshots, latest commit 1453824)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1453824)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Static review only; no tests or code were run. The previous routing-notice and per-token spill-memory warnings are resolved at current HEAD. Previously commented issues were re-verified and not reposted. Files Reviewed (7 files)
Previous review (commit 965f97a)Status: 10 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Static review only; no code or tests were run. The seven prior formatter findings were rechecked and are resolved in normal bounded-preview and spill paths; unchanged-file findings were targetedly reverified. Previously commented defects were not reposted. Files Reviewed (7 files)
Previous review (commit 425e6e2)Status: 13 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Static review only. No tests or code were run. Previous formatter findings for missing/empty columns, projection mismatch, and full-text spill failure no longer reproduce on the current implementation. Files Reviewed (8 files)
Previous review (commit 9115eab)Status: 11 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Previously commented defects were not reposted. Static review only; the remote head moved to Files Reviewed (29 files)
Reviewed by gpt-6-sol · Input: 20 · Output: 7.7K · Cached: 786.3K Review guidance: REVIEW.md from base branch |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9115eab. Configure here.
…-schema clients and keep the projection Live sessions on a strict-schema model sent `full_text_columns` on every call (the model fills every property). Under the old contract that cost hundreds of rejected calls for an empty list, more for `limit > 20` while the mode was nominally active, and the successful calls returned only the named columns, so the model re-issued the query for the identity and metric columns it had selected. - `[]`, `null` or an omitted field turn the mode off; the call runs as a plain query. - With a non-empty list, an omitted `limit` means 20 and a larger `limit` is clamped to 20 with a note in the result instead of a validation error. Ordinary-mode limits are validated as before. - All selected columns are returned; the named columns are returned complete, without preview or spill cuts, while the other cells keep the usual clipping. A named column that is not in the result adds a one-line note listing the available columns and the rows are still returned. - Descriptions say "omit to use the default" instead of stating a literal default the model would copy. Tests cover `[]`/`null`/absent as plain queries, the 20-row default, clamping of 50 and 10001 with the note, `id, title, body` returned with `body` untruncated, the missing-column note, and the formatter's handling of oversized named cells, competing clipped cells, arrays and binary values. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bfd6bb26f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… of querying the wrong collection
A collection shorthand that also carried the native `key` field, such as
`{ collection: "items", distinct: "category", key: "status" }`, left the
shorthand argument undefined and fell through to the native-command parser,
which read `"category"` as the collection name. Shorthand `distinct` is now
validated against both `key` and `field`: agreeing values are accepted,
conflicting values are refused with an error naming both, and any document
carrying `collection` never reaches the native parser.
Tests: conflicting `key`/`field` refused naming `category` and `status`;
agreeing aliases accepted; distinct-as-field keeps the collection; unsupported,
empty and null `collection` documents do not fall through.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…verable Letting named cells bypass the formatter cap did not deliver complete text: the session applies a per-result token cap before persisting any tool output, so an oversized cell was middle-truncated anyway, and because the clean path wrote no spill and reported `truncated: false`, the missing text could not be recovered. The mode is now "priority within the same bounded output, always recoverable": - Full-text mode always writes one complete lossless JSONL spill of the returned rows (including empty results and unknown names) and sets `result_path`; the banner says `read` with `char_offset`/`max_chars` recovers clipped text. - Named cells take the shared byte/line budget first, in row order; a cell that does not fit is clipped with a continued-in-spill marker and the metadata reports the clip. Once the budget is exhausted, remaining cells are omitted behind one note instead of a marker per cell. - Structured and binary cells are serialised with the same tagged serialiser as the spill, inline and on disk, instead of `String(value)`. - The schema line keeps its ordinary alias and column caps, with named columns eligible beyond the ordinary prefix; the missing-column, limit, lint and suggestion notes are counted inside the output budget. - The formatter enforces the effective row limit (20 in full-text mode), notes when a connector returned more, and spills all returned rows. Tests: 50 KiB named cell clipped within 16 KiB with its complete spill; twenty 1 MiB cells bounded with one spill; structured and binary cells lossless inline and in the spill; empty result with a missing name held to a 64-byte budget with the clip reported; 300 long aliases bounded beside one named column; 40 connector rows limited to 20 with all rows saved; notes counted in the budget. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…under tight output limits - The full-text spill serialiser emitted one string fragment per JSON token and held them all until the join, so a permitted wide numeric result retained millions of fragments. It now emits one bounded string per row; the tagged serialisation and the 32 MiB preflight are unchanged. - Under a tiny `tool_output.max_lines` the fail-open routing notice was clipped before its text and then overwritten. The notice is reserved first and the result and remaining notes are fitted into what is left. Tests: 2,000 × 150 numeric rows spill with identical bytes and one retained chunk per row; a full-text query under a ten-line limit keeps the routing notice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1453824b2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A large `max_chars` on a very long single-line file sliced the whole requested window before the 50 KB byte limit applied, so a minified or generated file could allocate hundreds of megabytes although only about 50 KB can be returned. The slice end is now clamped to the byte budget (one character is at least one byte) before slicing; the UTF-8 refinement, surrogate-pair handling, clipping and continuation output are unchanged. Test: a 20 MB single-line file read with `max_chars: 10_000_000` returns the same clipped output and the slice requested from the string is bounded by the budget. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the reliability branch Resolves the import conflict in `packages/drivers/test/mongodb-unit.test.ts` by keeping both the `normalizeMqlCommand` import and the direct BSON `Binary` import.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 11533576-91a2-47aa-a817-559641155745) |
full receipts (29 sessions)
codex ·
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #1447
Type of change
What does this PR do?
Why. Running the read-only analyst against real warehouses showed five classes of harness friction that cost the model turns or hid failures from it: backend errors reported as success, valid read-only SQL refused as writes, Mongo command shapes rejected, ragged spill rows, and long text that could not be read in full. Issue #1447 lists the evidence.
Root causes and what changed.
Tool.define's wrapper only checkedmetadata.success === false, and both SDK resolver boundaries returned every resolved envelope as a successful result. Now a result with an explicit error envelope or backendmetadata.error(when the tool has not declaredsuccess:true) is returned withstatus:"error"and rejected at the SDK boundary viaTool.assertSuccess, so the model, lint and telemetry see the failure; the tool's text output is kept. Per review:bashresults stay ordinary SDK results (grep no-match and friends are not errors);bashrecordsmetadata.exit_code/metadata.successfor telemetry only, and a Python traceback counts only as the final stderr block with a non-zero exit (or empty stdout on exit 0).bashcompletes on process exit again with a short drain grace, so backgrounded children do not hang the tool. Aborted tools complete before the boundary check;warehouse_testdeclares its guidance a successful diagnostic;batchno longer counts inner error envelopes as success; the truncation hint no longer claims success on a failed call.//, negatedGLOB, DuckDBprior.*aliases and standalone/CTEPIVOT/UNPIVOT. The classifier now adapts only the parser input per dialect; executed SQL is unchanged, and parser/lexical statement-count disagreement still fails closed. SQLite//stays refused (the engine does not support it).{collection, find|aggregate|count|distinct}documents are normalised in the driver; the materialize refusal lists accepted shapes; embedded$out/$mergeare still refused.readgainschar_offset/max_chars(clip banner gives the next window);sql_executegainsfull_text_columns: all selected columns are still returned, the named columns take the output budget first (in row order) and a cell that does not fit is clipped with a continued-in-spill marker; full-text mode always writes one complete lossless JSONL spill and setsresult_path, so clipped text is recoverable withread; structured and binary cells are serialised losslessly; the schema line keeps its ordinary caps; notes are counted inside the budget; the formatter enforces the 20-row bound even when a connector returns more; an empty or null list (what strict-schema clients send for every unused property) or an omitted field turns the mode off; with a non-empty list an omittedlimitmeans 20 and a larger one is clamped to 20 with a note in the result; a named column missing from the result adds a one-line note listing the available columns and the rows are still returned. Byte limits and permissions are intact; the truncation banner points to the mode. The analyst prompt gains one sentence: read a deciding rule in full and evaluate each condition against the row's own fields and dates before choosing.Base. Stacked on
feat/analyst-harness-dab-2026-10(#1441); the diff shown here is only this change.Behaviour deltas (was → now).
metadata.errorwithoutsuccess:true:status: ok→status: error+ SDK tool-error event (all agents).bash: completion waited on stream close → on process exit with bounded drain; non-zero exit: unchanged for the SDK, now recorded inmetadata.exit_code/metadata.success.read/sql_execute: two new optional parameters; defaults unchanged.How did you verify your code works?
Unit/integration, run outside any sandbox after an independent review (approve-with-changes; the three P1s it raised are fixed in the second commit):
test/tool/bash.test.ts,batch,tool-define,truncation,read,test/altimate/tool-result-errors,test/session/tools-result-errors,tools-permissions,test/altimate/tools/*,test/altimate/sql-read-syntax.test.ts): 711 pass / 0 fail.test/altimate/fixtures/read-syntax-errors.json, the long-line SQL fixture forread).packages/driversMongo unit suite: 27 pass.bun run typecheckclean; every edit in upstream-shared files (src/tool/*,src/session/*) is insidealtimate_changemarkers (audited per file); thereadtool description carries no marker text.Not run: live warehouse E2E for the Mongo shapes beyond the driver unit suite; HTTP/browser journeys do not apply.
Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Changes the tool/SDK error boundary and SQL read/write classifier (security-sensitive), plus bash completion timing; behavior is heavily tested but affects all agents using these tools.
Overview
Fixes analyst harness friction by making tool failures visible to the SDK, read-only SQL/Mongo accepted when valid, and long text retrievable without changing default query behavior.
Tool outcomes. Backend failures (
metadata.errorwithoutsuccess: true, orstatus: "error") now surface as SDK tool errors viaTool.assertSuccessat session/batch boundaries while keeping the tool’s text output.bashstill returns ordinary results for grep-style non-zero exits but recordsexit_code/failure_kindfor telemetry; stderr-only Python tracebacks and timeouts are classified separately.warehouse_testinfrastructure guidance is markedsuccess: trueso diagnostics are not treated as crashes.Read-only classification. DuckDB/SQLite parser input is adapted (e.g.
//,NOT GLOB,prioraliases, standalone/CTEPIVOT/UNPIVOTmasking) so valid reads are not blocked; executed SQL is unchanged and write detection stays conservative.Mongo.
normalizeMqlCommandaccepts{collection, find|aggregate|count|distinct}shorthand with conflict checks; materialize errors mention accepted shapes.Long text.
sql_executeadds optionalfull_text_columns(caps preview at 20 rows, always spills full JSONL toresult_path).readaddschar_offset/max_charsfor line windows. Spill serialization pads missing column values withnull.Prompt. Analyst instructions add one sentence on reading full policy text and evaluating each condition against row data.
Reviewed by Cursor Bugbot for commit 6b9533b. Bugbot is set up for automated code reviews on this repo. Configure here.