Skip to content

fix(example-todo): arm task_completion — it bound to nothing and gated on a key nothing reads (#6882) - #7039

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-6882-todo-flow-trigger
Aug 9, 2026
Merged

fix(example-todo): arm task_completion — it bound to nothing and gated on a key nothing reads (#6882)#7039
os-project-manager merged 1 commit into
mainfrom
claude/issue-6882-todo-flow-trigger

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #6882

Premise: confirmed on origin/main, both faults verbatim

git show origin/main:examples/app-todo/src/flows/task.flow.ts still carries, on TaskCompletionFlow's start node, objectName: 'todo_task' plus triggerCondition: 'record.status != previous.status && record.status == "completed"' — no triggerType, and the predicate on a key nothing reads. The issue's mechanism reading of resolveTriggerBinding and getTriggerBindingAudit is accurate line for line.

Proof it was dead, and is now alive

The whole defect is that nothing noticed, so a diff that merely looks right is not evidence. Both states measured through the platform's own instruments.

Before — real AutomationEngine + RecordChangeTrigger, app-todo's real allFlows:

--- getFlowRuntimeStates() ---
{"name":"task_reminder","enabled":true,"bound":false,"status":"draft","triggerType":"schedule"}
{"name":"overdue_escalation","enabled":true,"bound":false,"status":"draft","triggerType":"schedule"}
{"name":"task_completion","enabled":true,"bound":false,"status":"draft"}          <-- no triggerType at all
{"name":"quick_add_task","enabled":true,"bound":false,"status":"draft"}
--- getTriggerBindingAudit() ---
[ { "flowName": "task_reminder",      "triggerType": "schedule", ... },
  { "flowName": "overdue_escalation", "triggerType": "schedule", ... } ]          <-- task_completion ABSENT
--- registered hooks ---
[]

task_completion is missing from the audit entirely, and zero lifecycle hooks were registered even though the record-change trigger was registered. That absence is the defect: the audit opens if (!resolved) continue, so an unresolvable flow is read as "manual/screen, nothing to bind", and both consumers (the plugin's kernel:bootstrapped warn loop, the CLI startup summary's unbound list) inherit the silence.

After — real kernel (ObjectQL + sqlite + automation + record-change trigger), real object, real writes:

[record-change] bound flow 'task_completion' -> afterUpdate on 'todo_task'
=== getFlowRuntimeStates ===
{"name":"task_completion","enabled":true,"bound":true,"status":"active","triggerType":"record_change","object":"todo_task"}
=== getTriggerBindingAudit ===
[]
1. INSERT status=not_started     -> runs = 0   (an insert must not reach an after-update flow)
2. UPDATE priority (no status)   -> runs = 0   (condition not met)
3. UPDATE status -> completed    -> runs = 1   (fired)
4. re-save an already-completed  -> runs = 1   (did not re-fire)

and the run itself reached the end, rather than merely being dispatched:

[ { "status": "completed",
    "steps": [ "start:success", "get_task:success", "check_recurring:success", "create_next_task:skipped" ] } ]

The dialect was decided by measurement, not preference

The issue asks whether the record. / previous. prefix or the corpus's bare-field style is intended. Measured, it is not a correctness question at all. AutomationEngine.execute binds record, spreads every record field to top level, and always binds previous; evaluateCondition then evaluates with { extra: { ...vars, vars }, record: vars }. Both spellings resolve identically:

predicate UPDATE into completed re-save completed unrelated update INSERT completed (previous == null)
record.status != previous.status && record.status == "completed" true false false THROWS No such key: status
status == "completed" && previous.status != "completed" true false false THROWS No such key: status
status == "completed" && (previous == null || previous.status != "completed") true false false true

So the real fork is the trigger token, not the dialect. PM assumption 3 is confirmed with a sharper mechanism than expected: a null previous does not misbehave quietly, it aborts the whole predicate (ADR-0032 §1c — an unevaluable predicate is a fault, never a quiet branch decision).

Chosen: record-after-update + status == "completed" && previous.status != "completed", which is the issue's own "scope the trigger so insert cannot reach it" resolution.

  • "marked as complete" is a transition; a task created already completed is an import, not a completion.
  • showcase_task_completed uses exactly this token and this predicate shape (status == "done" && previous.status != "done") for exactly this semantic. Verified on origin/main before copying (PM assumption 2): both it and showcase_urgent_task_alert are still correctly wired.
  • Adding previous == null || under -after-update would be a lenient consumer-side guard that converts a producer fault into a silent "fire anyway" — the opposite of what §1c asks for. The previous == null leg belongs to -after-write flows, which genuinely have a create leg.

PM assumption 1 is confirmed: nothing in README.md, the app's docs, its views or its actions points at a manual invocation; the object, the action handlers and the flow all describe a completion event.

The third fault, and why it is in this PR

get_task filtered on {taskId} — an isInput variable nothing ever binds. A record-change run seeds params from the triggering record, which carries id, not taskId, and seedDeclaredVariables binds an input only from context.params[name] or a defaultValue (#4697); this had neither. Invisible while the flow was dead. The first armed run:

ERROR Trigger-fired run of flow 'task_completion' failed (trigger 'record_change')
  {"error":"Node 'get_task' failed: get_record: refusing to run — 1 filter condition(s) resolved to nothing
   and were dropped from the query: `{taskId}` (at id)."}

Leaving it would have shipped a flow that fires and then fails on every completion — the same "quieter form of the defect" the ruling forbids, one node along. It now reads {record.id} (how every other record-change flow in the corpus addresses the triggering row) and the dead declaration is deleted rather than re-plumbed. Flagging it explicitly since it is beyond the two faults the ruling enumerated.

status: 'active' is the other half of "arm it deliberately" — see the lint section below for why it is a declaration and not a silencing.

Answer to the #6637 question (asked explicitly, answered by running the real rule)

#6637 has landedflow-trigger-unroutable and its pin test are on origin/main, not on a branch. So this is measured, not reasoned. Running validateFlowTriggerReadiness over app-todo's real stack:

REPAIRED tree  -> 2 findings, both `flow-draft-status-ambiguous` (warning) on task_reminder / overdue_escalation
PRE-#6882 tree -> 2 findings, the SAME two

flow-trigger-unroutable does NOT fire on the repaired file, and it is a true negative. Its criterion is config.triggerType != null && !routesToSomeTrigger; this PR supplies record-after-update, which is record--prefixed (so isRecordTriggered, so routesToSomeTrigger) and matches VALID_RECORD_TRIGGER (so rule 1c does not fire either). Net authoring-lint delta from this PR: zero. objectstack validate passes.

One intermediate state is worth recording because it is the thesis in miniature. Arming the trigger without status: 'active' made flow-draft-status-ambiguous start reporting task_completion — a rule that had been skipping it, because while the flow routed nowhere even the draft-status rule had nothing to say about it. Declaring status: 'active' is the rule's own prescription ("Declare status: 'active' to arm it deliberately"), and it is what the showcase flows do; it changes no runtime behaviour (draft already fires — only obsolete/invalid disable). The two schedule flows keep their pre-existing draft, which is not this card's call.

On the pin test — it asserts against its own inline fixture, so it is unaffected. packages/lint/src/validate-flow-trigger-readiness.test.ts:862 builds unroutable({}) from a local candidate_hired / app_candidate literal and never reads examples/app-todo. It stays green and non-vacuous. What does go stale is its prose: the comment says the corpus measurement "found a LIVE instance of it in examples/app-todo (TaskCompletionFlow, #6882)", and after this PR there is no live instance in the tree. packages/lint/** is out of scope per the ruling, so that is reported here rather than edited — along with the card's closing question (whether the wider absent-triggerType criterion should now ship), which after this PR would fire on nothing in the tree.

The test, and its reverse verification

examples/app-todo/test/task-completion-trigger.test.ts — five cases against the app's real metadata through a real kernel (no test double anywhere in the chain; #6882 was precisely a chain that read correctly and bound to nothing).

Reverse-verified by restoring origin/main's flow file and re-running. Direction predicted: red, and every case named the actual defect:

× declares the two keys the engine actually reads
    expected undefined to be 'record-after-update'
× no start node in the app writes its predicate to an unread key
    flow 'task_completion' start node: expected { objectName: 'todo_task', ... } to not have property "triggerCondition"
× BINDS: the flow is wired to afterUpdate on todo_task at runtime
    the flow resolved a binding and the trigger accepted it: expected false to be true
× REVERSE: drop `triggerType` and the flow vanishes from every audit channel
    expected false to be true            <-- its own non-vacuity assertion
× GATES: only a transition INTO completed fires the flow
    the completion transition must launch the flow: expected +0 to be 1
 Test Files  1 failed (1)   Tests  5 failed (5)

The REVERSE case deserves a note, because its assertion shape looks wrong until the direction is stated. It rebuilds the pre-#6882 shape from the real flow, strips triggerType, and then asserts an absence: the stripped flow is bound: false with triggerType: undefined and still produces no audit entry. Removing the key does not add a diagnostic; it removes the flow from every diagnostic channel there is. That is why this needed a test rather than a lint rule reading a warning nobody emits, and the case carries its own non-vacuity check (the real flow, in the same engine, is bound).

Where it lives, and why the package changed. @objectstack/example-todo's test script was objectstack test — the Quality-Protocol runner, which needs a live server at :3000 and resolves qa/*.test.json against a directory this app does not have, so it warned and returned 0. Its existing src/translations/translation-completeness.test.ts (78 assertions) had therefore never run in CI. The script is now vitest run, matching app-crm and app-showcase; both suites run and pass (83 tests). Two workspace devDependencies were added for the test only (@objectstack/service-automation, @objectstack/trigger-record-change), the same practice app-showcase already follows with @objectstack/formula / @objectstack/objectql; the driver is @objectstack/driver-sqlite-wasm, already a dependency and already the driver this app's own test/mcp-actions.e2e.ts boots.

Two out-of-scope findings, filed not fixed

Both are in app-todo but outside the flow's trigger wiring, and both need an app-semantics decision. Searched open issues first (keyword + file path); no duplicates.

No baseline or exemption entry anywhere, per the ruling.

Verification

  • pnpm --filter @objectstack/example-todo test (vitest) — Test Files 2 passed (2) · Tests 83 passed (83)
  • pnpm --filter './examples/*' run typecheck — all four apps Done
  • pnpm --filter @objectstack/example-todo validateValidation passed, findings identical to origin/main
  • pnpm lint (ESLint) — clean
  • Every check:* step enumerated from .github/workflows/lint.yml, both jobs, run one by one — all OK. check:i18n-coverage first reported COULD NOT MEASURE on an unbuilt @objectstack/connector-mcp; green after turbo run build --filter='./packages/*' --filter='./examples/*^...', which is what the CI job does first.
  • node scripts/check-nul-bytes.mjs OK, plus a widened self-scan (grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]') over every touched file — clean.

A changeset is included (@objectstack/example-todo: patch), following the precedent of .changeset/flow-branch-gates-and-inert-condition.md, which named this same package for a change to this same file. No skip-changeset label is needed.


Generated by Claude Code

…ted on a key nothing reads (#6882)

`TaskCompletionFlow` declared `type: 'record_change'` and then declared neither
key that arms one, so it was 1 of the 34 authored flows across the three bundled
apps and the only dead one.

- no `config.triggerType`: `resolveTriggerBinding` claims a record-change flow
  only for a `record-`-prefixed token, so it returned `undefined` and
  `activateFlowTrigger` never bound. `getTriggerBindingAudit` skips exactly that
  case (`if (!resolved) continue`), which is why no channel ever reported it.
- the predicate sat on `config.triggerCondition`, a key nothing reads. Arming
  the trigger alone would have fired the flow on EVERY update.

Repaired with `triggerType: 'record-after-update'` plus
`condition: 'status == "completed" && previous.status != "completed"'` — the
shape `showcase_task_completed` uses for this semantic. `-after-update` rather
than `-after-write` keeps the insert leg out, where `previous` is `null` and
`previous.status` aborts the predicate with `No such key: status`.

A third fault surfaced the moment the flow could run: `get_task` filtered on
`{taskId}`, an `isInput` variable nothing binds (a record-change run seeds
`params` from the record, which carries `id`). It reads `{record.id}` now and
the dead declaration is gone. `status: 'active'` declares the firing.

`@objectstack/example-todo` runs `vitest run` now instead of `objectstack test`
(a Quality-Protocol runner needing a live server, matching no `qa/*.test.json`),
so its test files execute in CI for the first time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei
@vercel

vercel Bot commented Aug 9, 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)
objectstack Ignored Ignored Aug 9, 2026 10:45am

Request Review

@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file tests tooling labels Aug 9, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review August 9, 2026 11:03
@os-project-manager
os-project-manager added this pull request to the merge queue Aug 9, 2026
Merged via the queue into main with commit 6dd3c25 Aug 9, 2026
26 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-6882-todo-flow-trigger branch August 9, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants