mirror of https://github.com/simstudioai/sim.git synced 2026-09-22 05:19:54 +08:00

Files

T

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

* fix(v2): close seven correctness and honesty gaps found sweeping the API

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.

* fix(v2): correct three regressions this branch introduced, and harden 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.

* fix(execution): report a workflow-group cancellation as the write it 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.

* fix(execution): have a workflow-group cancellation report the writes 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.

2026-08-14 14:14:30 -07:00