[malibio]

Summary

• Seeds a core invariant Play (TASK_SPEC_PLAY_ID): a task moving to in_progress must link an approved spec unless requires_spec is false (ADR-097 §2, §6, §8). The rejection names the Writing a Spec skill, the nodespace commands, and says not to retry.
• Adds the declared task field requires_spec (boolean, schema default true), carried through the wire type, proto, daemon, app-lib, adapter, dev-proxy and generated TypeScript.
• Seeded skills now say a small change is marked requires_spec: false.
• Docs (nodespace-docs): node-types.md task sheet, playbook-system.md Plays table, ADR-092 §6 amended.

Node type: flat core type task, category unchanged. Added field: requires_spec (boolean, default true, protection core). No relationship or structural change. New Play with fixed id, appended to CORE_PLAY_IDS.

Consequences worth knowing

• Every task without requires_spec: false and without a linked approved spec is refused on the move to in_progress, in the UI as well as for agents. A to-do list now needs the field set. This follows the issue as written (default requires a spec).
• The seeded "Ready tasks" query uses the dry run, so spec-less tasks drop out of it.
• Creating a task already in_progress is not guarded, same as the lineage rule.
• Test fixtures that start a default task are moved to the light lane; the spec rule has its own tests.

Not in this PR

The gate-side criteria depend on nodespace gate check (#3708, in flight): repo-write deny when no in-progress task, exempt paths, and the per-rule advisory setting. The Play is what the gate's dry run will read, so no extra machinery is needed here.

Test plan

• bun run test:changed (frontend, scripts, browser, types:check, node-types:check, Rust workspace 5647 passed)
• bun run quality:fix
• New tests: allow, no spec, draft spec with no plan, light lane, switched off / emptied Play, rule shape and message

🤖 Generated with Claude Code

[malibio]

Pragmatic code review: PR #3715 (issue #3710)

Review type: initial, whole-diff review (Rust core, wire types, proto, generated TS, seeds, tests, sibling docs working tree).

Requirements check

Criterion	Status
Task cannot move to in_progress without an approved linked spec unless marked light lane (graph write)	Met for the move; see Finding 1 for create-as-started bypass
Repo write denied with no in-progress task / spec not approved / deny reason names skill + do not retry	Deferred to #3708 (gate check). The Play gives the dry run what it needs: a single in_progress refusal with an actionable message
Light lane allows repo writes once claimed	Play side met (requires_spec: false); gate side deferred
Exempt paths configurable; advisory setting	Deferred to #3708 (gate data/setting, not Play logic)
Ships like the other core Plays (fixed id in CORE_PLAY_IDS, editable, resettable)	Met; id pinned in tests, seeded-with-fixed-id and switched-off/emptied tests added
Tests cover allow, deny cases, light lane, edited/disabled Play	Met for the Play (advisory deferred)
Generated types regenerated, node-types.md updated	Met by inspection (typed-core-fields, task-node, task-node-update, sheet row + ADR-092 amendment + playbook-system table). I did not run types:check / node-types:check

Node type sequence

Schema field (declared, Core protection, schema default true), core_promoted_fields, TaskNode/TaskNodeUpdate, proto field 11, daemon/app-lib/dev-proxy plumbing, adapter-core TYPED_UPDATE_FIELDS (satisfies Record<keyof ...>), generated TS and docs are all present. No compatibility code, no node_type literal comparisons, no lifecycle_status reads. Absent field reads as true in task_node_to_value, which is the schema default, not a shim.

CEL correctness

• has() guards are right: !has(node.requires_spec) || node.requires_spec == true treats missing as required (fail closed); !has(node.spec) || !node.spec.exists(s, has(s.spec_status) && s.spec_status == 'approved') treats no link and a spec without spec_status as not approved. Collection semantics match node.blocked_by.exists(b, b.status ...) in the blockers rule.
• Link direction/test: the tests link spec --tasks--> task and it starts only when approved; the draft-spec-no-plan case (the gap lineage leaves) is covered.
• Subtypes: the trigger uses the same select.target_type + property_key as the existing task Plays, so it inherits whatever subtype resolution those have. No subtype test was added; Nit-level, note rather than block.

Findings

🟡 1. Creating a task directly as in_progress bypasses the Play

📁 packages/core/src/playbook/core_plays.rs:394-423, packages/core/tests/it/core_model_plays_test.rs:1343
The trigger is property_changed. The unmodified test started_at_is_stamped_on_the_first_start does h.create("task", {"status": "in_progress"}) with no requires_spec, and it succeeds, so under the new default a spec-less task is born started. bulk_create with in_review likewise. An agent can run nodespace node create --type task --property status=in_progress and the gate's dry run (which asks the same question of the task's state) will see a started, spec-less task. This is the exact class the plan-approval rule closes with its separate reject-creating-an-approved-plan rule. Add a node_created rule (or a second rule in this Play) with the same conditions, and a test, or state in the ADR that creation is out of scope. Severity is moderate rather than blocker only because #3708 is where enforcement lands, but the issue's first criterion ("cannot move to in_progress") is cheap to make real here.

🟡 2. "Ready tasks" queue and its take rule now hand the agent a task that is refused

📁 packages/core/src/services/query_service/core_queries.rs:115-152, packages/agent/src/seeds/rules/agent/task-implement-take.md
ready_tasks_filters still says a task with a checklist and no plan is ready, and its doc comment says this "is what lets a small change with a checklist of its own skip the spec and plan". That is now false: such a task without requires_spec:false is refused on start. Because the take rule runs the query with limit 1, an agent will be handed the same refused task every time (a livelock, not just a wasted call). Either add a filter (requires_spec == false OR a related approved spec; the query language already has related/negate) or at minimum update the doc comment, and make the take rule say what to do when start is refused. The PR changes two seed skills but not the queue they feed from, and the ADR-092 §7 definition of "ready" was not amended with the table row.

🟡 3. The rejection message offers the escape hatch unconditionally

📁 packages/core/src/playbook/core_plays.rs:411-421
The message ends "If this is a chore or a small fix that needs no spec, mark the task with requires_spec=false". An agent that wants to proceed will take the cheaper path with no user judgement, which empties the gate. Nothing restricts who may write the field. Suggest: "ask the user whether this is a chore or small fix before setting requires_spec=false", mirroring how the seeded skills already say to ask before approving. Also worth saying so in the skills edited in this PR (they currently tell the agent to "set requires_spec to false" for any small change).

🟡 4. Default true changes plain to-dos, with no UI path

📁 packages/core/src/models/core_schemas.rs:365-392, packages/desktop-app/src/lib/types/task-node.ts:276
ADR-092 originally said a task is also an ordinary to-do needing no spec. With default true, any user to-do moved to in_progress is refused, and the PR adds no UI control for the field (only a wire field and ?? true). The size of the fixture churn (requires_spec:false added across six test files and the shared harness task()) is the same signal. This may be the intended product direction (ADR-097 §6) and is amendment-recorded in ADR-092, so I am not blocking, but confirm the UI's refusal path for this Play surfaces the message acceptably and file/track the toggle.

🟢 5. Guarding only in_progress is defensible but leaves open -> in_review/done

📁 packages/core/src/playbook/core_plays.rs:373-395
The rationale (claim is the gated event; a started task is not re-checked) is sound and documented. Note that open -> in_review and open -> done skip the rule, and ADR-092 §5 itself says a task in review "has been started". If the gate asks "is the session's task in_progress with approved spec" this is harmless; if anything else keys off "started", consider guarding in_review too, as lineage does.

🟢 Fixture changes

The requires_spec: false additions in parent_task_completion_play_test.rs, permitted_filter_test.rs, playbook_invariant_integration_test.rs and the harness task() are legitimate isolation of the rule under test (each comment says so) and the spec rule has its own spec_gated_task() tests. They are not hiding a defect, with one caveat: they are what makes Finding 1 invisible, since the one test that creates a task started was left unmodified and silently relies on the hole. Pin that behavior one way or the other.

🟢 Nits

• Nit: packages/core/src/playbook/core_plays.rs:68 area: the doc comment at the Play says "ADR-097 §2" for the dry run; the TASK_SPEC_PLAY_ID comment cites §2 as well while the amendment is §6. Fine, just be consistent.
• Nit: task_spec_rules() unit test asserts message substrings of the same string the integration test asserts; one is enough.
• Nit: node-types.md says the typed update "carries these eight fields"; requires_spec is not tri-state and cannot be cleared, which the sheet states. Good; no change needed.

Recommendation

APPROVE WITH CHANGES (non-blocking, but fix 1 and 2 before merge). The field, wire plumbing, seeds and the CEL logic are correct and follow the node type sequence with no compatibility code. Finding 1 (create-as-started bypass, evidenced by an existing unmodified test) and Finding 2 (Ready tasks queue now returns a task the Play refuses, at limit 1) are real gaps in the "gate means something" goal and are small to fix in this PR. Gate-side criteria are correctly deferred to #3708.

[malibio]

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

[malibio]

Review response (address-review)

Addressed

• 🟡 2 Ready tasks handed out a task the Play refuses: ready_tasks_filters now ends with the permitted (dry run) filter, so the queue follows the Plays. New test the_seeded_ready_queue_leaves_out_a_task_the_spec_play_would_refuse; workflow_seeds small-change fixtures marked requires_spec: false. Stale doc comment fixed.
• 🟡 3 Self-exemption: the rejection message and the Breaking a Plan skill now say the user confirms a change needs no spec before requires_spec is set to false.
• 🟢 ADR citation on the Play id now §2, §6.
• 🟢 Creation behaviour pinned with a comment in the existing started-task test.

Skipped

• 🟡 1 node_created rule: creation is not a move, matching the lineage and blockers rules; a creation rule would also reject importing started tasks. The gate's dry run (ADR-097 §2) still refuses a repo write for such a task. Revisit if the gate lands and a born-started bypass shows up in the shadow report.
• 🟡 4 UI toggle for requires_spec: feature scope beyond the issue (declared field only). Default true is as the issue specifies; flagged for the user.
• 🟢 5 Guarding in_review: the issue scopes the rule to in_progress.
• 🟢 Duplicate message assertions and a subtype test: the two assertions cover different layers (shape vs live engine), and the trigger uses the same target_type resolution as the other task Plays.

bun run test:changed and quality:fix pass.

Re-Review Decision

Decision: RE-REVIEW NEEDED
Rationale: the Ready tasks filter is a behaviour change added in response to feedback.

[malibio]

Pragmatic code review (re-review): PR #3715 (issue #3710)

Review type: re-review of commit fb6a000 only (git diff 7e2fee469..HEAD), against the first review and the author's response.

Previous review summary

First review: APPROVE WITH CHANGES. Findings: 1 (create-as-started bypasses the property_changed Play), 2 (Ready tasks queue hands out a task the Play refuses; livelock at limit 1), 3 (rejection message offers requires_spec=false unconditionally), 4 (default true changes plain to-dos, no UI), 5 (only in_progress guarded).

Were 2 and 3 addressed

• 2: addressed. ready_tasks_filters now ends with {type: permitted, property: status, value: in_progress}. The new test the_seeded_ready_queue_leaves_out_a_task_the_spec_play_would_refuse covers gated / light lane / approved spec, and flips the gated task to light lane. The livelock is gone: run_definition drops the limit from the statement and walks candidates in sort order until it has filled it, so a refused head task no longer starves limit 1.
• 3: addressed. The rejection message and breaking-a-plan-into-tasks.md both say the user confirms before requires_spec is set to false. See nit on implementing-a-task.md below.

Skip justifications

• 1 (no node_created rule): sound enough. Creation is not a move, the lineage and blocker rules do not guard it either, and a creation rule would reject importing started tasks. The behaviour is now pinned by a comment in the existing test. Nit: that comment says the gate's dry run "still refuses it a repository write"; the gate is Let a harness ask NodeSpace whether a repo write is allowed #3708 and does not exist yet, so the comment states a future guarantee. Say "is meant to".
• 4 (no UI toggle): sound. The default is what ADR-097 §6 specifies; the toggle is a separate feature. Make sure the user is told it is untracked.
• 5 (in_review): sound. The issue scopes the rule to in_progress.

Is the permitted filter safe on every path that runs the saved query

I traced every consumer of a saved query.

• query_ops::run_saved_query_nodes / execute_query_nodes / count_query / definition_matches: all go through run_definition or split_permitted, which strip the filter and ask the dry run per candidate. Counts load every candidate and dry-run each (documented). Daemon RPCs (ExecuteQuery, CountQuery, RunSavedQuery) and the agent run_query go through these, and the daemon already surfaces permitted_unresolved.
• Skill attachment (node_context_ops::matching -> definition_matches): safe. Consequence to know: "Implementing a Task" is attached to Ready tasks, so a gated open task no longer carries it in its context. It carries the refusal message and the Writing a Spec skill instead, which is the right behaviour.
• SQL QueryService directly: refuses permitted (mod.rs 244 / 1045), but its only callers that run a saved query are selectors::selector_query (Play selectors, which already reject permitted with a clear error) and the by-title lookup, which runs a query over type query and not a saved definition. No seeded Play selects READY_TASKS_QUERY_ID. Safe.
• Seeding and save-time validation: validate_query_paths and checked_definition accept the filter (status is declared on task, in_progress is an enum value). The seed is non-fatal on failure, but it cannot fail here.
• Frontend: matchesFilter returns false for permitted ("unverifiable"), so a live-added node is never inserted by the client; the result set only changes on reload. Acceptable and already documented there.
• Bare-service tests: core_queries_test.rs uses NodeService::new with no playbook lifecycle, so the dry run is Allowed and those tests are unaffected. They do not exercise the new filter; the new Harness test does.

No path regresses.

Findings

🟡 1. Redundant blocker / plan filters contradict the documented design, and the new comment is inaccurate

📁 packages/core/src/services/query_service/core_queries.rs:116-153
The doc comment now says "The conditions before it are the ones a Play does not state". That is false: the blocker rule and the lineage rule are Plays, and the dry run asks them. The seeded blocked_by and plan_status filters are therefore duplicates of rules the last filter already evaluates. ADR-092 §7 in the docs repo says the opposite: "The query states no blocker or plan condition of its own: it asks the rules ... a team that edits or switches off a gate Play changes what is listed without touching the query." With the SQL filters kept, switching off the blocker Play still hides blocked tasks from the queue, so the documented behaviour does not hold. Same facts are in two places (DRY).

Pick one and make the code and docs agree:

• drop the two SQL conditions (status, checkbox and permitted remain) and move the blocker / plan assertions in core_queries_test.rs to the lifecycle Harness, since the bare NodeService there has no engine and the filter would be Allowed for everything; or
• keep them deliberately as cheap SQL prefilters that cut the dry runs, and say that in the comment and in ADR-092 §7 ("the rules still decide").

Not a blocker: results are correct either way (the conjunction is coherent), it is a source-of-truth and docs-accuracy question.

🟢 2. Stale sibling text

• Nit: packages/agent/src/seeds/skills/implementing-a-task.md:3 defines a ready task as "open, has a checklist, is not blocked, and has no plan waiting on approval". It now also must link an approved spec or have requires_spec false, which the very next sentence implies but the definition omits. The agent reading it will think a checklist alone is enough.
• Nit: nodespace-docs decisions/092-...md §7 last line ("The checklist condition is what lets a small change skip the spec and plan") and components/seeded-nodes.md:189 ("Open tasks with a checklist, no unfinished blocker and no plan that is not approved") no longer describe the queue.
• Nit: dry run cost. A queue with no limit (UI list, badge counts, count_query) now does one dry run per open checklist task per refresh. Fine at this scale; the SQL prefilters are what bound it, which is an argument for the second option in finding 1.

Recommendation

APPROVE. Findings 2 and 3 are properly addressed, the skips are justified, and adding permitted to the seeded queue is safe on every path (query_ops, daemon RPCs, agent run_query, counts, skill attachment, frontend). Fix finding 1 (the inaccurate comment and the code-versus-ADR-092 disagreement) in this PR or before merge; the rest is nits. No further re-review needed.

[malibio]

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

[malibio]

Review response (re-review)

Addressed (commit 22cd3b8, comment/wording only, no logic change)

• 🟡 Redundant blocker/plan filters vs ADR-092 §7: kept deliberately as prefilters that bound the dry runs; the doc comment and ADR-092 §7 now say so (docs pushed to nodespace-docs main together with the sheet, Plays table and seeded-nodes page).
• 🟢 implementing-a-task.md ready-task definition now includes the spec/light-lane condition; stale seeded-nodes.md and ADR-092 §7 lines fixed; the test comment says the gate "is meant to" refuse the write.

quality:fix and bun run test:changed pass.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED
Rationale: the re-review approved; this commit is comments, skill wording and docs only.

[malibio]

Merge queue: #3715 was taken out of the queue — the merge gate failed on it, on top of main. Reproduce by merging or rebasing origin/main and running bun run test:changed, fix, push, and re-run bun run merge 3715.

✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed project update → read back carries typed fields 11ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed spec update → read back carries typed fields 15ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed plan update → read back carries typed fields 5ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed decision update → read back carries the typed status 5ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed query update → read back carries typed fields 9ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed play update → read back carries typed fields 10ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed collection update → read back carries the typed description 7ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → typed skill update → read back carries typed fields 7ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > typed database-settings update → read back carries the typed list 5ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > createNode honors an explicit InsertPosition the same way move/reorder do 8ms
   ✓ Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > moveNode honors an explicit InsertPosition (regression: dev-proxy previously ignored it entirely) 12ms
 ✓ src/tests/e2e/watch-stream-isolation.e2e.ts (1 test) 620ms
 ✓ src/tests/e2e/node-roundtrip.e2e.ts (11 tests) 592ms
 ✓ src/tests/e2e/schema-seeding.e2e.ts (7 tests) 555ms
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
 FAIL  src/tests/e2e/adapter-contract.e2e.ts > Adapter contract: live round-trip (HttpAdapter → dev-proxy → daemon) > create → update task fields with tri-state clear/set/no-change → read back matches
BackendError: Play rule 'reject-starting-a-task-without-an-approved-spec' (play c4a8e1b6-2f73-4d95-a0b8-7e5d3c91f2a4) rejected the write to node 9272beac-2adc-42da-9310-d2f908c08290: This task was not started: it is not linked to an approved spec, and nothing was changed, so do not retry the same change. Follow the Writing a Spec skill: write the spec as a draft and ask the user to approve it, then link it with `nodespace relationship create --from <spec-id> --type tasks --to <task-id>` and start the task again. If the user confirms this is a chore or a small fix that needs no spec, mark the task with `nodespace node update <task-id> --property requires_spec=false` instead.
 ❯ handleResponse src/lib/services/http-response.ts:70:11
     68|     }
     69| 
     70|     throw new BackendError(
       |           ^
     71|       errorData?.message || fallbackMessage,
     72|       errorData?.code,
 ❯ HttpAdapter.updateTaskNode src/lib/services/backend-adapter.ts:558:12
 ❯ src/tests/e2e/adapter-contract.e2e.ts:126:21
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
 Test Files  1 failed | 7 passed (8)
      Tests  1 failed | 55 passed (56)
   Start at  13:12:16
   Duration  9.95s (transform 152ms, setup 18ms, collect 282ms, tests 9.48s, environment 0ms, prepare 37ms)
error: script "test:e2e" exited with code 1
error: script "test:e2e" exited with code 1
[stage exited with code 1]

  full output: /var/folders/6c/mkxnmpkx5zqc9jwwy20tjkyr0000gn/T/nodespace-gate-logs/_gate-merge-2026-10-10T11-06-00-012Z/test-e2e-headless-daemon-round-trip.log

✗ test:e2e (headless daemon round-trip) failed — merge blocked.
  Fix the failure, push, and re-run: bun run merge <PR#>

[malibio]

The first merge round failed in two gate-only stages (headless e2e and Tauri-seam adapter-contract tests) because their fixtures start a default task. Both now mark the task requires_spec: false in the same write and assert the typed boolean end to end. Run locally: e2e 56 passed, seam 59 passed, clippy and skill:check clean. Test-fixture changes only; no re-review needed. Re-queuing the merge.