Uh oh!
There was an error while loading. Please reload this page.
Correct the AGENTS.md predicate rule and add a stopgap flow-predicate guard - #38
Merged
Merged
Conversation
… guard
`pnpm validate` resolves every `record.`/`previous.` read in a flow predicate
against the bound object, but deliberately never flags a BARE identifier —
`collectBoundRecordReads` skips them because in a flattened flow scope a bare
name may be a flow variable. AGENTS.md described the wrong failure model for
that case ("evaluates to null and hides the action"), which is not what a flow
does: the engine flattens the record's fields to top-level names, so a bare
name resolves, or throws (ADR-0032 §1c) — never silently false.
- AGENTS.md rule 4: state which surfaces the gate covers, what it does not
cover, the real flow failure mode, and link objectstack-ai/objectstack#14089.
- AGENTS.md rule 9 (new): metadata first — declarative metadata over handler
code; check the platform first; file upstream rather than working around.
- test/flow-predicates.test.ts: labelled stopgap pending #14089. Walks
dulyFlows and dulyJobs via the platform's own collectFlowGraphs (so loop /
parallel / try_catch bodies are covered), flags a bare identifier that names
a field of the bound object and no declared flow variable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p`FLOW_NODE_EXPRESSION_PATHS` omits `config.condition` and `edge.condition` deliberately — its docstring says so: they are structural predicate surfaces on every node and edge rather than declared configSchema slots. The earlier wording read as an omission and could have sent someone upstream with a non-issue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
The previous rewrite claimed validate does not flag a bare identifier "on any surface". Measured false: a bare `status` in duly_task's skip_needs_reason validation rule exits 1, located and corrective, and the platform's own message names the mechanism — a record-scoped expression binds the record as the `record` namespace only, so a bare name resolves to nothing and the expression silently evaluates to null. Scope decides, not surface. Record-scoped surfaces (validation rules, field conditional rules, action visible/disabled, sharing rules, hook conditions) are enforced, and there the deleted "evaluates to null" sentence was correct — it is restored under that half. Flow node and edge conditions run in flattened scope and are the single exception; the shadowing case and the ADR-0032 §1c throw stay there, where they belong. test/flow-predicates.test.ts gains the same framing at the top: it covers the only predicate surfaces the platform leaves open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
os-warren
marked this pull request as ready for review
September 1, 2026 04:53
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes#29
Three deliverables: the
AGENTS.mdrule 4 correction, the metadata-first rule, and the stopgap walk.1.
AGENTS.mdrule 4 — corrected, split by expression scopeThe governing fact is scope, not surface name. An expression is evaluated either with the record bound as the
recordnamespace and nothing at top level (record scope), or with the record's fields additionally flattened into top-level variables (flattened scope).validatejudges a bare identifier only in the first.Record-scoped surfaces — enforced, and there the failure really is
null. Object validation rules, field conditional rules, actionvisible/disabled, sharing rules and hook conditions bind the record as a namespace only, so a bare name binds nothing, the expression evaluates to null, and the rule or action silently never fires. Measured on this branch,duly_task'sskip_needs_reasonrule mutated to a barestatus:Exit 1, located, corrective — and the platform's own message states the mechanism, which is what makes the rewrite principled rather than patched.
Flow node and edge conditions are the single exception. They run in flattened scope, where a bare name may genuinely be a flow variable, so
collectBoundRecordReadsdeliberately never judges one.status == "dispatched"in a flow start condition passesvalidatewith exit 0.And in a flow the failure is not
nulleither — the paragraph that survives from the first rewrite, now correctly scoped to the flow half. A barestatusresolves: to the flattened field, or to a same-named flow variable seeded first that shadows it (the subtler bug — the predicate reads correctly and silently means something else). When a name resolves to nothing the engine throws (ADR-0032 §1c). So on this one surface the outcomes are "silently means something else" and "loud runtime fault", never the quiet null of the record-scoped surfaces.Links #14089 and points at the stopgap as deletable when it lands.
A premise divergence, recorded and since confirmed by the PM. The card quotes a rule 4 containing "(
pnpm validatenow rejects it)". That text is not in duly'sAGENTS.md— onmainrule 4 was a one-liner. The PM has confirmed they were quoting the objectstack monorepo's file. The other half of the card was verbatim true (lines 44–46 claimed a bare reference "evaluates tonulland hides the action on every record"), and that sentence turned out to be correct for record-scoped surfaces — it is restored under that half rather than deleted.2.
AGENTS.mdrule 9 — metadata firstNew rule carrying all three required parts: declarative metadata over handler code (objects, views, flows, jobs, datasets, permission sets, actions are primary; a handler is the last resort); check the platform for a declarative way before writing a handler; and if the platform cannot express it, file an issue against
objectstack-ai/objectstackand say so on the card rather than quietly working around the gap.Appended as rule 9 rather than inserted as rule 1, deliberately. Inserting at the top renumbers rules 4–8, and rule 4 is referenced by number in this card and in the dispatch prompts for #2, #7 and #11 — all in flight. Position does not encode importance in this list anyway. Happy to absorb the renumber on request.
3.
test/flow-predicates.test.ts— the stopgapLabelled at the top as a stopgap pending #14089 and written to be deleted when that lands. Its header now opens with the scope framing: it covers the only predicate surfaces the platform leaves open, which is what makes walking just
dulyFlows/dulyJobsthe right scope rather than a narrowing.The bar — a bare identifier is a finding when it (1) names a declared field of the bound object and (2) names no variable the flow declares. Deliberately the same bar #14089 proposes upstream, so the two cannot disagree about what a defect is.
Region recursion is the platform's, not hand-rolled.
collectFlowGraphsyields the flow's own graph plus every nested region —loop.body,parallel.branches[],try_catch.try/.catch, nested — with readable scope labels.FLOW_REGION_CONFIG_KEYSgives the "container config without its regions" view so nested findings are not reported twice;FLOW_NODE_EXPRESSION_PATHSsupplies the declared predicate slots. The scoped walk this repo already had readsconfig.body.edgesdirectly, which coversloopand silently missesparallelandtry_catch.The false-positive case the exemption exists for is handled, and three structural guards keep the exemption sound rather than a hole:
flow.variables[].nameplus every binder the spec declares —iteratorVariable/indexVariable/outputVariable/idVariable/errorVariable, plus anassignmentnode's top-level config keys, which are the author's variable names. A*Variable-shaped key the walk does not know fails a dedicated test rather than silently producing a false positive.record_changeflow must bind an object this repo declares, so a flow with nothing to anchor on cannot pass by having nothing to check.Narrowings, declared rather than hidden: the anchor is
Object.keys(object.fields), so system columns (id,created_at, …) are not flagged — the spec exports no list of them and hand-copying one here would drift. A bare name that is neither a field nor a variable is never flagged.dulyJobsis a tripwire, not a live check, and says so. Measured againstJobSchemaat@objectstack/spec17.2.0, a job declaresname / label / description / schedule / handler / retryPolicy / timeout / enabledand no predicate slot — its schedule is acronenvelope, not CEL, and its logic is a named handler function. Two assertions fail loudly ifJobSchemagrows a predicate slot, or if any job ever carries a CEL expression.Proof it can fail — three ablation legs
Each on real metadata, each with the mutation confirmed on disk before measuring (injected and removed literals grepped, non-empty diffstat, abort otherwise — an anchor miss reads exactly like a passing ablation), each restored by an
EXIT/INT/TERMtrap, and the work committed first so the trap'sgit checkout --restores from a real index. All three re-run on the final headd03dcea.pnpm validaterecord.status ==→ barestatus ==✓ Validation passed (260ms)isBlank(vars.existing_task)→… && needs_collection✓ Validation passed (327ms)duly_taskvalidation rule (record)record.status ==→ barestatus ==Findings from legs 1 and 2, verbatim:
Leg 2 answers the card's region-recursion requirement on real metadata: the finding is located inside
loop 'fan_out' body. Leg 3 is the rework's own check — it proves both halves of the corrected rule 4 in one run:validatedoes enforce the record-scoped surface, and the flow guard does not overclaim it. Restoration verified after each leg by absent-marker greps andgit status --porcelainempty.Six of the file's thirteen tests are a permanent in-file self-test over synthetic fixtures — the guard firing on a bare reference nested in a loop body, on the bare-string shorthand as well as the
Penvelope, on each field of a compound predicate; and not firing on a correctly qualified read, on a bare name that is a declared loop iterator, or on CEL builtins and string literals.Gates
All four green on head
d03dcea, working tree clean against it:Exit codes captured before any pipe. No changeset (this repo has none) and no
objectstack.config.tsedit.What changed in rework (
6752ca7→d03dcea)Only
AGENTS.mdrule 4 and the test file's header comment. Rule 9 untouched; the walk itself untouched.nullfailure model restored — it was correct for record-scoped surfaces, and the platform's own diagnostic uses the same words. My previous text implied null is never the failure model, which misled in the other direction.Out of scope, filed
test/assignment-fanout.test.tspasses on a real bare reference #37 — the older scoped walk intest/assignment-fanout.test.tsasks whether the source containsrecord.<field>, so a compound predicate with one qualified and one bare clause passes it. No shipped metadata is affected and the new file covers that case. Left alone here: that file is outside this card's declared surface and belongs to another card's PR.One upstream issue considered and deliberately not filed.
FLOW_NODE_EXPRESSION_PATHSdoes not listconfig.conditionoredge.condition, which looked like an incomplete predicate-slot table. Its docstring says the omission is deliberate — they are structural surfaces on every node and edge rather than declaredconfigSchemaslots, and both validators already walk them. Commit6752ca7corrects a comment of mine that had implied the gap.Generated by Claude Code