Skip to content

fix(v2): close seven correctness and honesty gaps found sweeping the API - #6702

Merged
waleedlatif1 merged 4 commits into
stagingfrom
fix/v2-surface-hardening
Aug 14, 2026
Merged

fix(v2): close seven correctness and honesty gaps found sweeping the API#6702
waleedlatif1 merged 4 commits into
stagingfrom
fix/v2-surface-hardening

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

A ten-slice sweep of the live v2 surface found no regression from the recent cancellation work, but did surface a set of pre-existing defects where an endpoint lost data, hid a failure, or reported something untrue. Each is fixed at the layer that owns the behavior.

What's fixed

Force-failed runs vanished from duration queries. The two force-fail boundaries wrote status: 'failed' with no ended_at and no total_duration_ms — the same defect class already closed for cancellation, still open on its sibling. A null duration reads as "no duration recorded" and drops the run out of every minDurationMs/maxDurationMs query. The payload factory is generalized to take the status; the five cancellation call sites are untouched and still emit a byte-identical row (asserted through PgDialect.sqlToQuery).

One bad row made a whole page unreachable. GET /api/v2/custom-tools failed the entire page on a single malformed row, and because the list is keyset-paginated, that row made every page containing it permanently unreachable — two workspaces have had no access for months. The projection now validates against the same contract schema the route builder applies. Repairs are hypotheses that must still pass validation, so a wrong guess can only downgrade a row to skipped, never emit invalid data. Both rows observed in production are recovered rather than discarded; the unrepairable case (function.parameters.type, an open string where a substitution would fabricate JSON-Schema semantics) is skipped with a warning. Single-resource reads still fail loudly — there is no other row to serve.

Table filters silently matched nothing. eq/ne/in/nin compiled a wrongly-typed operand into a containment test that matched nothing, so eq: "8" returned an empty page where eq: 8 returned 1172 rows — indistinguishable from genuinely empty. The write path already coerces through the column-type registry, so this was the read and write paths disagreeing about the same column. The operand is now read through that same registry and rejected only where it refuses. null (a legitimate JSON-null query) and '' (the cleared-cell sentinel) pass through untouched; select is excluded because its operands are already resolved upstream. Error messages now name the caller's column rather than the internal col_… id.

Messages described the wrong problem. A custom error on a string schema also replaced the wrong-type wording, so {"name": 123} reported that the name was missing. Messages now distinguish an omitted field from a mistyped one, topK names its own bounds, the knowledge-search refine reports against a field instead of the whole body, and a workspace id is bounded before it reaches a lookup.

Archived files were listed but unreadable. A soft-deleted file appeared under ?scope=archived yet 404'd on metadata, leaving no way to check share state before restoring. The read takes the same scope selector the list already exposes. The default is unchanged, and the parameter relaxes only the deleted_at predicate — the workspace join, asserted-scope check, per-principal authorization, and concealment policy are all untouched.

Cancellation reported a write that never happened. Cancelling an already-terminal run returned durablyRecorded: true, reason: "recorded". The service now distinguishes the no-op and names the state it observed (already_cancelled / already_completed / already_failed), and both surfaces share one vocabulary instead of the internal route deriving its own. The status read is observational, gates no effect, and falls back to the old report on failure. No claim predicate, WHERE clause, or write changed.

Protocol. A 401 now carries a challenge naming the header the API actually reads (Bearer would be false advertising — v2 doesn't read Authorization; Basic makes browsers prompt for credentials that can't be an API key). A body that failed to parse is reported as 415 only when the caller positively declared a non-JSON type, and only after the read already failed — so curl -d '{...}' without -H, which succeeds today, still succeeds.

Verification

  • 25,116 tests pass, type-check clean, 26/26 audits including check:api-validation:strict and check:openapi.
  • Every fix has tests proven able to fail — reverted, watched red, restored.
  • The 415 guards encode "must not break" and pass either way by construction, so they were mutation-tested against a naive pre-parse implementation instead: all five went red, plus five HEAD tests (a HEAD carries no content-type), independently confirming a pre-parse gate is the wrong shape.

Behavior changes worth review attention

  • Table containment filters. A filter returning an empty page today will now either return the rows it always meant, or 400 for an uncoercible operand. A saved view holding a stale predicate (e.g. eq: "abc" after a string→number retype) will now 400 rather than render empty. The range path already carries this exact exposure.
  • executionDeadlineAt is now cleared on force-failed rows. This matches the documented column contract; the only DB reader is the stale-execution sweeper, gated on status='running'.
  • Cancel reason enum widened. Client-safe: the TS SDK exposes no cancel endpoint, and all three durablyRecorded consumers go through the internal contract, which was already a superset. None branch on a specific reason.

Deliberately not fixed

Reranking returning unavailable (configuration, not code — and rerankerStatus is already reporting the fallback correctly); cancel latency reaching 29s (a real perf issue, but a refactor of ~15 sequential awaits on the executor's terminal path that deserves its own change with measurement); error.details being an object on 403 and an array on 400 (a breaking wire change needing a product decision); validation running before the workspace-key policy check (reordering authz against parsing to fix an information-shape nit is a bad trade).


Second commit: three regressions this branch introduced

Five adversarial reviews of the first commit found that three of its "behavior-preserving" claims were wrong. All are corrected in eb50a58.

Table filters were unsafe in two ways. date is the one column type whose registry coerce is not idempotent — it drops sub-second precision — and the leaf that compiles a filter also builds the unique-constraint and upsert-conflict probes. Re-coercing an already-coerced operand could stop it matching the row it was written from, admitting a duplicate inside the write transaction with no error. Separately, throwing moved failure from submission into the delete/update/dispatch/cancel runners, because the v2 predicate grammar type-checks structure but not operand values — worst case, a filter that cannot compile means the cells it started can no longer be cancelled. Coercion is now total and non-throwing, and date is excluded alongside select. eq: "8" on a number column still resolves to 8; everything else passes through exactly as before.

Reviving a force-failed run inherited its terminal duration. Writing ended_at/total_duration_ms on the force-fail boundary was right in isolation, but a partial resume flips that row back to pending and those columns survived. The preserved value is meant to be the pause checkpoint — the run's active time — and had become wall clock measured at the failed resume, which the checkpoint rule then carried into the next terminal write. The revival clears them only for a row that was terminal, so an ordinary paused row keeps the checkpoint it should.

Cancellation reclassification had two holes, both raised in review and answered in-thread: the guard discarded genuine paused-reconciliation failures for completed/failed priors, and an entry snapshot could not see a claim that lost a race. Reclassification now requires that nothing else went wrong, and the claim's own row count decides whether this cancel terminalized the run. The snapshot also stopped costing an extra query — status rides along on the ownership read, whose own TSDoc warned against the second SELECT we had added.

Also: an unprojectable custom tool now answers consistently (list omits it, GET/PATCH by id report it absent rather than as a server fault), and the workflow_execution_cancelled analytics event no longer fires for a cancel that cancelled nothing.

Correction to the first commit's description

read-workspace-file-metadata.ts's diff is comment-onlyincludeDeleted was already plumbed through the use case. The three tests there are characterization tests for pre-existing behavior, not evidence that this change authorizes the archived path. The real change for that fix is the contract + route mapping; the authorization argument is unchanged from origin/staging and was verified independently rather than introduced here.

Test hardening

Every fix was mutation-tested — the fix reverted, the suite watched for red. Where the suite stayed green, the assertion was strengthened:

  • The absent Content-Type branch had zero coverage: the helper looked like it omitted the header, but undici derives text/plain from a string body. It now sends an encoded body and asserts the header is genuinely absent — this is the branch the source itself calls the likeliest way to turn a working client into a 415.
  • The duration encoder was pinned with expect.anything(), which would have accepted endedAt in place of startedAt — silently computing a zero interval. Now pinned to the exact column.
  • Execution ownership is pinned to both ids it must match; the mock previously discarded its where argument entirely, and the paused-only branch had no coverage at all.
  • The archived-file concealment test passed with the fix reverted; it now proves it conceals the archived read specifically.
  • Two tests asserting a paused branch they could not observe were deleted — the rendered-SQL test that can decide it already covers them.

Closed coverage gaps: multipart still succeeds on the body-lifecycle route (the 415 safety argument was prose-only), and a status-read failure logs and continues rather than blocking the cancel.

25,187 tests pass; 26/26 audits; type-check clean.

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 14, 2026 8:48pm

Request Review

@cursor

cursor Bot commented Aug 14, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches cancellation semantics, table/filter coercion (mentioned in PR but not in this diff snippet), and widespread contract validation—behavior changes for edge-case API clients, though scoped to incorrect or ambiguous requests.

Overview
This PR tightens v2 API honesty and recoverability across several surfaces, plus OpenAPI/docs alignment.

Custom tools: List projection now validates/repairs stored schema rows instead of 500ing the whole keyset page; unrepairable rows are skipped, single-resource routes return 404 (aligned with the list). Workflow cancel: Already-terminal runs report durablyRecorded: false with already_* reasons; analytics only fires when a cancel was actually recorded. Files: GET …/metadata adds optional scope=archived (same lifecycle idea as list) so soft-deleted files can be read before restore.

Request validation: workspaceId (and related ids) get maxLength: 128 in contracts/OpenAPI; Zod messages separate missing vs wrong-type fields; knowledge search topK and query/tagFilters rules report on specific fields.

Protocol: v2 401 responses include WWW-Authenticate naming x-api-key; unreadable bodies that declared a non-JSON Content-Type return 415 after a failed JSON parse (parseable JSON under wrong types still works).

Docs: OpenAPI JSON files updated for the above; cancel outcome descriptions expanded for terminal no-ops.

Reviewed by Cursor Bugbot for commit f86d733. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR corrects several v2 API behaviors involving run duration, custom-tool projection, table-filter coercion, validation messages, archived-file metadata, cancellation reporting, and request protocol handling.

  • Records complete terminal metadata for force-failed runs and clears it when reviving them.
  • Repairs or omits malformed custom-tool rows without allowing one row to break an entire page.
  • Aligns filter operand coercion with table column types while preserving write-path safety.
  • Distinguishes cancellation no-ops from durable terminal writes, including lost-claim and workflow-group sidecar races.
  • Updates API contracts, generated OpenAPI documents, and focused regression tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/lib/execution/cancel-workflow-execution.ts Attributes cancellation outcomes using terminal-write results and a targeted status re-read; both previously reported race and sidecar-reconciliation issues are addressed.
apps/sim/lib/table/workflow-group-cancellation.ts Reports workflow-log and sidecar writes explicitly so cancellation responses distinguish genuine reconciliation from no-ops.
apps/sim/lib/table/sql.ts Adds total, non-throwing operand coercion while excluding non-idempotent date and pre-resolved select values.
apps/sim/app/api/v2/custom-tools/utils.ts Validates projected custom tools and applies bounded repairs before omitting irreparable rows.
apps/sim/lib/logs/execution/cancellation.ts Produces complete terminal metadata for force-failed execution logs.
apps/sim/lib/api/server/routes/v2-json-route.ts Distinguishes unsupported declared media types only after JSON parsing fails.
apps/sim/app/api/v2/files/[fileId]/metadata/route.ts Maps the metadata scope contract to the existing archived-file read capability.

Reviews (4): Last reviewed commit: "fix(execution): have a workflow-group ca..." | Re-trigger Greptile

Comment thread apps/sim/lib/execution/cancel-workflow-execution.ts Outdated
Comment thread apps/sim/lib/execution/cancel-workflow-execution.ts
A ten-slice sweep of the live v2 surface turned up no regression from the
recent cancellation work, but did surface a set of pre-existing defects where
an endpoint either lost data, hid a failure, or reported something that was not
true. Each is fixed at the layer that owns the behavior.

Terminal execution logs. The two force-fail boundaries wrote `status: 'failed'`
without `ended_at` or `total_duration_ms`, so a force-failed run dropped out of
every duration-filtered log query — the same defect class already closed for
cancellation, still open on its sibling. The cancellation payload factory is
generalized to take the status; the cancellation call sites are untouched and
still emit a byte-identical row.

Custom tools. One malformed row failed the whole page, and because the list is
keyset-paginated that row made every page containing it permanently
unreachable. The projection now validates against the same contract schema the
route builder applies, repairing only what can be repaired without inventing
information — a stringified schema, and a missing `type` whose contract admits
exactly one value — and omitting with a warning what cannot. Both rows observed
in production are recovered rather than discarded.

Table filters. `eq`/`ne`/`in`/`nin` compiled a wrongly-typed operand into a
containment test that silently matched nothing, so a filter written against the
value the write path had stored returned an empty page instead of its rows. The
operand is now read through the same column-type registry the write used, and
rejected only where that registry refuses it. Range operators already behaved
this way; `null` and the cleared-cell sentinel still pass through untouched.

Error messages. A custom `error` on a string schema also replaced the wrong-type
wording, so supplying a number for a name reported that the name was missing.
Messages now distinguish an omitted field from a mistyped one, `topK` names its
own bounds, the knowledge search refine reports against a field rather than the
whole body, and a workspace id is bounded before it reaches a lookup.

Archived file metadata. A soft-deleted file was listed but unreadable, leaving
no way to check share state before restoring it. The read takes the same `scope`
selector the list already exposes; the default is unchanged, and the parameter
relaxes only the `deleted_at` predicate, never the authorization.

Cancellation reporting. Cancelling an already-terminal run reported a durable
write that never happened. The service now distinguishes the no-op and names the
state it observed, and both surfaces present one vocabulary instead of the
internal route deriving its own. No claim predicate or write changed.

Protocol. A 401 carries a challenge naming the header the API actually reads,
and a body that failed to parse is reported as an unsupported media type only
when the caller positively declared a non-JSON one — after the read has already
failed, so nothing that succeeds today can begin to fail.
… its tests

Adversarial review of the previous commit found that three of its "behavior
preserving" claims were wrong. Each is corrected here at the layer that owns it.

Table filters no longer coerce a `date` operand, and no longer throw. `date` is
the one column type whose registry `coerce` is not idempotent — it drops
sub-second precision — and the leaf that compiles a filter also builds the
unique-constraint and upsert-conflict probes, so re-reading an already-coerced
operand could stop it matching the row it was written from and admit a duplicate
inside the write transaction with no error. Throwing was the second mistake: the
v2 predicate grammar type-checks structure but not operand values, so a rejected
operand no longer failed at submission but inside the delete, update, dispatch
and cancel runners, where a filter that cannot compile means the cells it started
can no longer be cancelled. Coercion is now total — it rewrites what the registry
accepts and passes everything else through unchanged, exactly as before.

Reviving a force-failed run no longer inherits its terminal duration. Writing
`ended_at` and `total_duration_ms` on the force-fail boundary was correct in
isolation, but a partial resume flips that row back to `pending` and those
columns survived. The preserved value is meant to be the pause checkpoint — the
run's active time — and it had become wall clock measured at the failed resume,
which the checkpoint rule then faithfully carried into the next terminal write.
The revival clears them only for a row that was terminal, so an ordinary paused
row keeps the checkpoint it is supposed to keep.

Cancelling reports the terminal state it actually observed. Reclassification now
requires that nothing else went wrong, so a genuine paused-reconciliation failure
survives instead of being rewritten as an already-terminal no-op, and the claim's
own row count — not a snapshot read before it — decides whether this cancel
terminalized the run or lost a race to something else. The status the snapshot
needed rides along on the ownership query that already reads the row, rather than
the second read that query's own contract warns against.

A custom tool that cannot be projected now answers the same way everywhere: the
list omits it, and reading or patching it by id reports it as absent rather than
as a server fault. Analytics stops reporting a cancellation for a request that
cancelled nothing.

The tests around all of this were audited by mutating each fix and checking the
suite noticed. Where it did not, the assertion is stronger now: the absent
content-type branch is genuinely exercised rather than relying on a header the
client library supplies, the duration encoder is pinned to the column it must
measure from, execution ownership is pinned to both ids it must match, and the
archived-file concealment test proves it conceals the archived read specifically.
Two tests that asserted a paused branch they could not observe are gone; the
rendered-SQL test that can decide it already covers them.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/execution/cancel-workflow-execution.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit eb50a58. Configure here.

…performed

Cancelling a workflow-group run whose log had already been cancelled, but whose
cell sidecar still needed reconciliation, durably cancelled that sidecar and
then reported `already_cancelled` with `durablyRecorded: false` — because the
terminal-status shortcut answered from the entry snapshot alone and never asked
what this request had written. The analytics event, which now gates on that
field, stopped firing for a cancellation that really happened.

The outcome a cancel reports is the same question whichever path answers it, so
there is now one vocabulary for it rather than one the direct claim tracked and
one the group transition did not. Every group result maps to that outcome
through a total map, so a new group result cannot compile without deciding what
it wrote, and the reclassification leads with whether this request wrote at all.

A group transition that reports itself already cancelled is deliberately mapped
as unknown rather than as a no-op: it leaves the sidecar alone but still
terminalizes a log that was active, and the result does not say which happened.
That costs nothing today, because the only snapshot that would reclassify proves
the log was already terminal.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/execution/cancel-workflow-execution.ts Outdated
…it made

Three review findings landed on the same reporting logic, each a different face
of one cause: the caller could not see what the group transaction had written, so
it inferred. It inferred from an entry snapshot, then from the returned kind, and
the remaining blind spot was the kind that covers two different transactions —
a repair that terminalizes an active log, and a genuine no-op — which left a
cancel that wrote nothing still claiming a durable write when it lost a race.

The transaction now reports both writes it can make, each read from that
statement's own returning row and recorded immediately before the throw that
already depended on it, so the report cannot drift from the write. The caller
derives its outcome from those rather than from the kind, and the kind is back to
naming the situation instead of standing in for the work.

The group path can now always answer whether it wrote. The only remaining
unknown is the direct claim when its update throws or is never attempted, which
genuinely has no row count to report.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit f86d733. Configure here.

@waleedlatif1
waleedlatif1 merged commit cf78946 into staging Aug 14, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/v2-surface-hardening branch August 14, 2026 21:14
TheodoreSpeaks added a commit that referenced this pull request Aug 14, 2026
Additive only, both picked up automatically by the derived command surface:
  - cancelWorkflowRun `reason` gains already_cancelled / already_completed /
    already_failed (#6702)
  - getFile gains `scope` (active | archived), so `sim files describe` grows a
    --scope flag defaulting to active

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JNFjchn6dcM7xevh34PKHE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant