Repository navigation
fix(check): an empty list fallback keeps the list's element type - #31
Conversation
[] typed as list<any>, and joining it with list<T> gave list<any>, so the items of a fallback such as xs || [], if(c, xs, []) or a host's coalesce(xs, []) went unchecked: a misspelled field passed Check and failed in Run. An empty list literal now has an element type that Join absorbs; alone it is still unchecked, like Any. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Approve: The empty-list join is sound on the paths I traced, and the one gap I found is narrow enough to fix after merge.
This is the join side of the empty-list fallback that the workflow fuzz test found. I read Join, the ||, ??, if, and list-literal paths, and the builtins that return an element type.
Should fix
S1. first([]) and find([]) drop the nil from the element type (types.go:89)
OrNil returns an empty-list element unchanged, because isAny() is true for it. So first([]) and find([], ...) type as a non-nil empty (check.go:735, check.go:1006). Joined with a string, if(c, first([]), "s") == 1 is rejected as "never true", but Run returns false with no error when c is true. Only a literal [] passed straight into first, last, or find hits this. Fix: in OrNil, return Any when t == empty, so the element of a literal empty list reads as unknown.
Prompt for your AI tool: fix S1
Fix this review finding in deepnoodle-ai/expr, PR #31.
1. S1, types.go, func OrNil: OrNil returns an empty-list element type
unchanged, because isAny() is true for it. So first([]), last([]) and
find([], ...) (check.go elemOrNil and the find case) type as a non-nil
empty type. Joined with a string, `if(c, first([]), "s") == 1` is
rejected as "never true", but Run returns false with no error. Wanted:
OrNil returns Any when t == empty, so the element of a literal empty
list reads as unknown. Test: add `if(c, first([]), "s") == 1` to the
accept cases in TestCheckMatchesRun, and check that it passes.
Run the existing tests after the changes.
Joining the empty-list element at every Join let it escape: first([]) typed as a value that is never nil, so if(c, first([]), "s") == 1 was rejected as never true, though Run returns false. The element now takes the other side's type only when two lists are joined; anywhere else it is unchecked, like Any. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixed in 03762e9. The empty-list element now takes the other side's type only when two lists are joined. Anywhere else it is unchecked, like 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Approve. Re-reviewed the new commit 03762e9 on top of the approved 25be60a.
S1 is resolved. The earlier finding was that first([]) and find([]) kept a non-nil empty type, so if(c, first([]), "s") == 1 was rejected even though it can be true at run time. This commit makes a bare empty join to Any (Join checks isAny() before the list case), so that expression now passes. The new TestCheckMatchesRun cases lock it in. The fix is not the OrNil change suggested in S1, but it reaches the same result. I did not change OrNil.
Checked the new join path:
Join(list<empty>, list<T>)keepsT.Join(list<empty>, list<any>)givesany, so untyped lists stay permissive.Join(list<empty>, list<empty>)staysemptyand still prints aslist<any>.- The list-literal path (
check.go:770) is unchanged.
No blocking findings. Tests, builds and linters were not run, per the review rules.
|
S1: resolved. I confirmed this at A bare No new findings and no new review. The approval at |
* feat!: engine API for hosts: identity keys, idempotent cancel, Abort - StepID.Unit, StepID.Step and StepID.Attempt. The unit is the same for every attempt, so with the run ID it is the idempotency key for side effects outside the host; the StepID stays the attempt's key. - A cancel decides the outcome once. Delivering it again no longer stops the finally steps it started; Event.Force stops or skips them. - Abort fails a run a host cannot advance, without its definition, so the postgres host no longer edits engine bookkeeping. - State's paths, visits and joins are unexported; the stored JSON is the same. The exported fields, InFlight and Unit are the read view. State.Definition is DefinitionHash. - Limits are a Load option (WithLimits) and read with Definition.Limits; a Definition is read-only once loaded. - Job, Handler, ErrDeferred and StepErrorOf are the handler contract the local and postgres hosts share. The postgres host moves to the same API: - Worker.Handlers are workflow.Handler and take a workflow.Job; workflow.ErrDeferred replaces postgres.ErrDeferred. - Result.RunID names the run, including one Start generated. - CancelTx cancels in the caller's transaction. - StepRecord.Kind is StepKind, and StepRecord.Status is a StepStatus with constants. Run.Definition is DefinitionHash. - The store loads every definition itself with its kinds and WithLimits, so there is one place limits come from. - abort uses workflow.Abort. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: Load rejects joins that can fail and edges that read as an else - A join that names a step a path can skip, so the join could fire without it, is a Load error, not a run-time join failure. - Under the default match, an edge without when after a conditional edge reads as an else but is always followed. Load asks for match: first or an explicit match: all. - Schema problems in input and outputs say where they are, and defaults in outputs.<name>.schema are checked. - An on_failure or finally step with an unknown read is reported once. - The schema true is accepted (anything); false is rejected. - A reserved step name says whether it clashes with a global or a function. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(local)!: run handles, deferred steps and the shared Handler Runner.Start and Runner.Resume return a Run with Wait, Deliver, Cancel and Done; Runner.Run is Start then Wait. Handlers are workflow.Handler, the postgres host's signature, so ErrDeferred works in local and a deferred step is completed with Run.Deliver. Run.Cancel replaces the context-cause pattern: cancelling the context stops a run for Resume, and a resumed attempt has Job.Claims 2. The CLI cancels with Run.Cancel on an interrupt and says that finally steps still run. The retry example derives the payment idempotency key from the run ID and StepID.Unit, and its slow provider shows a retry that does not charge twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * style: format the JSON definitions with Prettier Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): escalation, a deferred ack with a timeout that escalates Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): side effects survive retries, crashes and early callbacks Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): checkpoint, resume a CLI run from a JSON state file Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): conformance suite across the pure driver, local and postgres Runs fan-out/join with a retried each, a catch route, a bounded loop with a sleep, and on_failure plus finally on every host with one scripted handler, and requires the same status, outputs, error, FinallyErrors and dispatched StepIDs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): StartTx, DeliverTx and WithOnFinished commit or roll back as one Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): generated run IDs round trip; a retried cancel keeps cleanup Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): authoring, Load's problems as feedback for a model Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): rolling deploy with a worker that lacks a kind A worker whose store cannot load the definition postpones the job; a correctly configured worker that appears within MaxClaims completes the run. If none appears, the run aborts with definition_unavailable and runs neither on_failure nor finally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): replay, a recorded run replayed through Apply and reordered Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): specialised workers share timers and the outbox Either an agent-only or an action-only worker fires a sleep's timer, and a step whose kind no worker handles fails through its timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): two versions of one workflow side by side Runs on v1 and v2 advance from a store with an empty cache, each on its pinned definition; a v1 StepID delivered to the v2 run is rejected as an unknown step and changes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): a long poll loop keeps state under tight limits Fifty poll and sleep passes of 20 KiB each complete under a 64 KiB state cap; the steps and events tables keep every pass in order, and a late duplicate for a dropped visit is not applied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): supersede, cancel a PR's older runs and start the new one in one transaction Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): codefix, an agent loop bounded by max_visits Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): toolcall, a workflow as a tool and as a child run Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): canary, a health poll loop with rollback on failure Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): saga, compensation for only what happened Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): mapreduce, each with concurrency and a dead-letter list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): a timeout racing a completion on each items Delivers an item's TimerFired and StepCompleted from concurrent transactions, repeated eight times: exactly one applies, per-attempt records match the winner, no timer jobs remain and the final state is the same either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(postgres): a stale claim stays fenced out after release and reclaim A claims and stalls, B claims and releases (claims back to 1, fence stays 2), C claims (fence 3) and completes; A's heartbeats and settle are fenced out and the step completes once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: the join check counts a branch whose conditions all fail as ending A branch whose only edges are conditional ends when none holds, so a join after it can fire without the steps on that branch. Load accepted such a join and the run failed at the join. The check now treats every condition as possibly false, as the type checker does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: one host contract, a guide-ordered definitions reference - documentation/hosts.md is the host contract: the two identity keys (run ID with StepID, run ID with StepID.Unit), the Handler and Job both hosts call, ErrDeferred in local and postgres, the local run handle, idempotent cancel and Force, Abort, and deploys and versioning: kind schemas are the host's current contract, not pinned with the document. README, doc.go and CLAUDE.md summarize it and link to it. - definitions.md reads as a guide: a glossary, then steps, routing, expressions (with try, coalesce and a function reference), paths and joins, failure and finally, input and outputs, types and limits. Joins are described as synchronization plus an assertion. - The order-independence claim covers successful completions only; failures and timeouts depend on order. - The README leads with a short workflow that loads, links the full PR review, and lists what is not yet supported. - llms.txt matches the new API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(planning): mark the old engine's plans historical; refresh the summary Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): a poll loop example; prreview shows its concurrency cap Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(examples): index the pattern examples Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: explain the ~N suffix on path IDs Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: hold the type promise against random definitions Generate definitions with typed outputs, set and with: reads, when and switch routing, catch, each, loops, joins, on_failure, finally and outputs; run every one that loads in several completion orders and check no state ever holds an expression, invalid_params or invalid_output error. A skipped case pins a bug the sweep found: an empty list fallback makes list items unchecked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(local): crash and resume mid-each, mid-loop, mid-finally and after a failed save Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: point the empty-list promise gap at its expr fix Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: name the empty-list fallback gap in the promise until expr v1.3.2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: concat joins its arguments as lists Joining the lists, not their element types, lets an empty list literal take the element type of the others once expr types it that way (deepnoodle-ai/expr#31). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * build: require expr v1.3.2, so an empty list fallback keeps item types expr v1.3.2 types xs || [], if(c, xs, []) and coalesce(xs, []) with xs's element type, so Load rejects bad item reads through such a fallback. Unskip the regression test and drop the temporary gap from the promise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
What changes for the user
Checkkeeps the element type of a list when an empty list[]is the fallback.xs || [],if(c, xs, [])and a host function such ascoalesce(xs, [])now type aslist<T>, notlist<any>. So a misspelled field on an item is reported, not left forRunto fail on.See it
Why
[]is the usual fallback for a list that may be nil. The workflow library's random-definition soundness test (deepnoodle-ai/workflow,soundness_fuzz_test.go) found definitions that passedCheckand then failed inRun, all through this join.Decisions to check
first([]), it is unchecked, likeAny, and it still prints aslist<any>. Rejected: absorbing it in everyJoin. Thenfirst([])typed as a value that is never nil. Rejected: typing[]aslist<never>with a new exported kind. That adds public API for one case.[], not everylist<any>. Alist<any>from an untyped value may hold anything. Joining it tolist<T>must staylist<any>.How to verify
go test ./...TestCheckMatchesRunhas the new cases. Three rejects are checked againstRunfailing. Four accepts are checked againstRunsucceeding, includingif(c, first([]), "s") == 1from the review. The workflow library's random-definition test passes 30,000 definitions against this branch.Risk and rollout
Checkmay now be rejected, but only whereRuncould fail on a missing field.TestEmptyListFallbackKeepsItemType.🤖 Generated with Claude Code