[malibio]

Closes #3474

What was wrong

A field cleared through the update pipeline is stored as JSON null in its bucket. cel-interpreter 0.10's has() tests key presence, so has(node.priority) was true for a cleared priority and false for one never set. Confirmed by running the new tests against the unchanged code: all of them failed.

What changed

• node_to_cel_value_at_scope leaves null-valued properties out of the node map (packages/core/src/playbook/cel.rs). Every condition read goes through it: the trigger node, and each node reached through a dot-path (GraphResolver::node_value → scoped_node_value). The drop runs after bucket precedence, so a cleared field still occupies its name and does not let a custom: field of the same bare name, or an ancestor bucket's, show through.
• The seeded Jira rules use has() only. !has(node.start_date) || node.start_date == null becomes !has(node.start_date), and has(node.completed_date) && node.completed_date != null becomes has(node.completed_date). references/jira-playbook.md is regenerated. The spec-driven rules already used has() only.
• Stored shape is unchanged: null stays in the bucket.

Behaviour

Condition	Field has a value	Cleared or never set
has(node.x)	true	false
!has(node.x)	false	true
node.x != null	true	false (missing path)
node.x == null	false	false (missing path)

node.x == null used to be true for a cleared field. It is now false for both, so has() is the one spelling for "has no value".

One deviation from the issue's proposed solution

The issue proposed applying the same rule in graph_resolver::get_node_property. I left that reader returning null. It is not on the condition path for a field's value (conditions read through scoped_node_value, so node.story.priority already agrees), but it does feed action bindings: returning None there would turn {trigger.node.story.priority} on a cleared field from null into a missing path, which fails the action. The issue's non-goals exclude changing how null reads outside conditions. The doc comment on get_node_property_at_scope states the difference, and a test pins it.

Tests

• cel::tests::cleared_field_reads_as_absent_in_a_condition: has, !has, == null, != null for a cleared, a never-set and a set field.
• cel::tests::node_to_cel_leaves_out_null_properties_in_every_stored_shape and cleared_field_does_not_unshadow_another_field_of_the_same_name.
• graph_resolver::tests::integration::cleared_field_through_a_dot_path_reads_as_absent: the same conditions through node.story.priority against a real store, plus the action-binding read.
• methodology_jira_execution_test: starting a sprint with a cleared end_date is rejected; creating one with a null completed_date is allowed. Both fail with the fix reverted.

bun run test:changed passes (rust, skill, scripts); bun run quality:fix clean.

Docs

Companion nodespace-docs edit: a "Fields with no value" section in architecture/playbook-engine.md and one sentence in components/node-types.md §1.2.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3477

Review Type: Initial review (no prior reviews or comments on the PR). Reviewed commit 43bfa27f against origin/main, plus the uncommitted companion edit in nodespace-docs (architecture/playbook-engine.md, components/node-types.md).

Summary

The change is small and placed correctly. One retain at the end of node_to_cel_value_at_scope drops null-valued properties from the node map, so every condition read (trigger node, trigger.node alias, nodes reached through a dot-path, collection items) sees a cleared field as a missing key. Dropping after bucket precedence, not at insertion, is the right choice and is pinned by a test. No blockers. Two non-blocking gaps are worth closing: a null nested inside an object-valued field, and node.x == null becoming a condition that can never be true with nothing telling the author.

What I ran: the four new unit tests and the ten methodology_jira_execution_test tests (all pass), and cargo fmt --check on the core crate (clean). What I did not run: bun run test:changed, quality:fix, or the tests with the fix reverted (I was not permitted to edit the tree). The "fails without the fix" judgement below is from reading the code.

Requirements Check

	Acceptance criterion	Notes
✅	A test evaluates has() and !has() for a field stored as null, absent, and with a value	cel::tests::cleared_field_reads_as_absent_in_a_condition runs all three nodes through evaluate_conditions.
✅	A cleared and a never-set field give the same has() result	Same test. node_to_cel_leaves_out_null_properties_in_every_stored_shape covers the bucket, prefixed-in-bucket, flat and flat-prefixed shapes.
✅	The same holds through a relationship dot-path (node.story.priority)	graph_resolver::tests::integration::cleared_field_through_a_dot_path_reads_as_absent, against a real store, and it asserts the fixture really stored null. Holds because enrich_context reads a traversed node's field from node_value → scoped_node_value, not from the raw scalar.
✅	node.<field> == null for a cleared and a never-set field is tested and documented	Tested in both tests above (false for both). Documented in the cel.rs module doc and the playbook-engine.md table.
✅	Seeded Jira and spec-driven rules use one spelling, existing tests pass	Jira's two conditions are now has() only; no node.<field> ==/!= null remains in packages/core/src/methodology/. The remaining p.old_value == null uses are on trigger.properties, a different surface. Spec-driven already used has(). jira-playbook.md is regenerated and matches.
✅	node-types.md §1.2 and playbook-engine.md state the rule	Both edits are accurate against the code, with one scope caveat (finding 1). They are still uncommitted in nodespace-docs; commit them with the merge.
⚠️	bun run test:changed passes; quality:fix clean	Not re-run by me. Targeted tests pass and formatting is clean.

Deviation from the issue's proposed solution

The issue proposed making graph_resolver::get_node_property / get_node_property_at_scope return absent for null too. The PR leaves it returning Some(Null). I agree with the PR:

• No condition value comes from that reader. A traversed node's field is taken from the owner's scoped_node_value map (graph_resolver.rs 748-758), so the criterion is met without touching it.
• It does feed action bindings (resolve_trigger_path → resolve_path → Scalar). Returning None would turn {trigger.node.story.priority} on a cleared field into Missing, which fails the action and suspends the play. That is a behaviour change the issue lists as a non-goal.
• None would also make resolve_path and the two-segment check in enrich_context fall through to a relationship lookup for every cleared field: extra schema and edge queries for nothing.

The cost is that the two "parallel" readers now differ on purpose. Both comments say so, and the dot-path test pins the binding side (Scalar(null) for cleared, Missing for never set).

Findings

🔴 Critical

None.

🟡 Important (non-blocking)

1. A null inside an object-valued field is still a present key
📁 packages/core/src/playbook/cel.rs:531 (and json_to_cel, 171-182)

• The retain filters only the top level of the node map. An object-valued field is converted by json_to_cel, which keeps every key, so {"task": {"address": {"city": null}}} gives has(node.address.city) == true and node.address.city == null true: the opposite of the documented rule.
• This is reachable. check_field_value accepts null at every depth (check_nested_fields calls it), so a nested declared field can be written as null.
• From reading the code; I did not execute it.
• Either strip null keys recursively from object values when building the node map (object keys only, not list elements, and not trigger.property values), or say in playbook-engine.md and the module doc that the rule covers a node's own fields and not keys nested inside an object value. Add a test for whichever is chosen.
• Principle: a documented rule ("has() is the one spelling") should hold everywhere the reader can apply it, or state where it stops.

2. node.x == null is now a condition that can never be true, and nothing tells the author
📁 packages/core/src/playbook/cel.rs:27-34, packages/core/src/playbook/validation.rs

• Before this PR it was true for a cleared field. It is now false in every case, and node.x != null is just has(node.x).
• For an invariant rule this fails open: conditions: ["node.end_date == null"] with a reject action silently never rejects. The issue itself notes a user-authored rule "has no reason to know".
• Save-time validation already walks each condition's paths. Rejecting a node.<path> == null / != null comparison there, with a message naming has(), turns a silent no-op into an error at authoring time. trigger.property* values and .where(...) predicates must stay exempt, since null is the correct test there.
• The PR is a net improvement without this (the old behaviour was already inconsistent), so it does not block. Worth folding in or deciding explicitly.
• Principle: fail loudly at save time rather than fail open at run time.

🟢 Suggestions

3. Collection items are covered by the fix but not by a test
📁 packages/core/src/playbook/graph_resolver.rs (new test, ~2308)

• node.issues.exists(i, !has(i.priority)) reads items through node_values → scoped_node_value, so it is fixed too. One more expression in the dot-path test would pin it; it is the third route by which a node value reaches a condition.

4. Comment accuracy

• 📁 cel.rs:529: "Conditions only" is not quite true. scoped_node_value also builds .where(...) items for action bindings (actions.rs:610). The result there is unchanged, because ItemPredicate::matches binds a missing key to null; say that instead.
• 📁 cel.rs:399-415: node_to_cel_value and node_to_cel_value_at_scope are pub, and their doc comments still say "All flattened properties as additional keys". The null rule is only in an inline comment; one line in the doc comment puts it where a caller reads.
• Nit: 📁 graph_resolver.rs:913-914: the doc comment has a stray line break ("the action. A" / "condition never sees").

5. Docs

• The playbook-engine.md section is accurate on the points I checked against the code: the table, the dot-path guard (!has(node.story.priority) alone fails because nothing materializes node.story), trigger.property / trigger.properties always carrying old_value / new_value, .where(...) binding a valueless field to null, and bindings resolving a cleared field to null.
• The anchor used from node-types.md (#fields-with-no-value) matches the new heading.
• Apply the scope caveat from finding 1 if the nested case is documented rather than fixed.

Test quality

• By reading: with the retain removed, priority stays in the map as Null, so the has() assertions, both contains_key assertions, the dot-path assertions (the owner map would hand back Null) and both new Jira assertions fail. The tests pin the behaviour, not just the outcome.
• cleared_field_does_not_unshadow_another_field_of_the_same_name also fails for the obvious alternative fix (skipping null at insertion), so it pins the design choice.
• Tests assert on ConditionResult through the real evaluator rather than on map contents alone, and the integration test verifies its own fixture.

Other readers checked, no unintended effect

• .where(...) item predicates: a missing key was already bound to null; unchanged.
• sum(...) and action bindings: read through get_node_property / JSON navigation; unchanged.
• playbook get-workflow-state: evaluates through the same function, and classifies "unresolvable" from the schema, so a cleared field reports as not yet met, like a never-set one.
• No other seeded rule (Linear, spec-driven) compares a node field with null.
• Core keys (id, node_type, content, version, lifecycle_status) are never null, so the retain cannot drop them.
• No node-type, schema, wire-type or Pro-boundary surface is touched; stored shape is unchanged and there is no compatibility code.

Recommendation: APPROVE

The change meets every acceptance criterion I could verify, the deviation from the issue is the better design, and the tests would catch a regression. Findings 1 and 2 are worth addressing but neither blocks the merge. Commit the two nodespace-docs edits alongside it.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed

Addressed (4)

• Finding 1, null nested in an object-valued field. Fixed rather than documented as a limit. Node properties are now converted with field_to_cel, which leaves out a null-valued object key at every depth, including inside a list of objects. A null list element is a value and stays. New test cleared_nested_field_reads_as_absent_in_a_condition; it fails with the filter disabled. json_to_cel is unchanged, so trigger.property values still carry null.
• Finding 3, collection items. The dot-path test now also evaluates node.issues.exists(i, has(i.priority)) and its negation from the story's side, for a cleared, a never-set and a set field.
• Finding 4, comments. The null rule is in node_to_cel_value's doc comment; the "Conditions only" note now says what happens for .where(...) items; the stray line break in get_node_property_at_scope's doc comment is gone.
• Finding 5, docs. The nodespace-docs section now states the nested and collection-item scope.

Skipped (1)

• Finding 2, rejecting node.<path> == null at save time. This is a new validation rule that refuses user-authored plays, with exemptions for trigger.property* and .where(...). It is not a correction to this change: node.x == null was already false for a never-set field before this PR. It needs a decision of its own, so it is raised with the maintainer rather than folded in here.

Checks: bun run test:changed passes (rust, skill, scripts); bun run quality:fix clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The only logic change is the one the review prescribed for finding 1: the same "a null field is left out" rule applied to nested object keys, in the same function, about ten lines, with a test verified to fail without it. Everything else is a test addition or a comment. No Critical findings existed and no design changed.

[malibio]

Merge queue: #3477 was taken out of the queue — it conflicts with main in: packages/core/src/playbook/cel.rs. Rebase onto origin/main, push, and re-run bun run merge 3477.

[malibio]

Merge queue: #3477 was taken out of the queue — it conflicts with main or with #3472, queued ahead of it, in: packages/core/src/playbook/cel.rs. Rebase onto origin/main, push, and re-run bun run merge 3477.