[malibio]

Summary

• Seeds five planning procedures adapted from MIT-licensed community skills (mattpocock/skills, garrytan/gstack): Writing a Spec, Reviewing a Plan (replaces Writing a Plan), Splitting a Plan into Tasks (replaces Breaking a Plan into Tasks), Asking the User and Explaining. Attribution is carried in each skill's body.
• Attached per ADR-094: Writing a Spec to "Specs by status", Reviewing a Plan and Splitting a Plan into Tasks to "Plans by status"; Asking the User is named by all three and Explaining by Asking the User, so each arrives with the work.
• Each speaks NodeSpace only: a draft spec with checkbox criteria that the user approves; issues (plain tasks for non-code work) linked to spec and plan with their own checklist, a brief and blocks edges; readiness is the Ready tasks queue.
• ADR-092 §8, ADR-094 and seeded-nodes.md were updated and pushed first. The per-skill source, licence, changes and replacements are in the comment on Adapt the community spec, plan-review and ticket-splitting workflows into NodeSpace planning skills #3719. What the sources do that has no typed home is Give a plan review, a spec's sections and an issue brief a typed home, or decide they stay prose #3757.

Test plan

• bun run test:changed (rust + scripts tiers) passes
• workflow_seeds: attachment, "arrives with the work", no harness/tracker/label scan, and a plan split into linked issues with briefs and checklists
• Prompt snapshots regenerated (UPDATE_GOLDEN=1); diff is only capability names
• Live skill matrix and all 41 live retrieval guards on the locked embedding model; current.tsv/current.sha256 re-recorded
• Generative-model agent evals were not run; the only prompt-surface change is the capability list in routing

🤖 Generated with Claude Code

[malibio]

Code Review: PR 3758 (issue 3719), Initial Review

Review type: Initial review. No prior reviews or comments on the PR.

Summary

A net positive that matches the issue closely. Five procedure skills are seeded, with attribution and no harness, tracker or label references. The rule fragments exist in both forms (agent and skill-md) and use the real tool names, relationship names and directions. The three planning skills are attached per ADR-094. The old Writing a Plan and Breaking a Plan into Tasks bodies are deleted. I checked the following against the code and ADRs:

• plan has spec (One) and tasks; spec has tasks and decisions; blocks is from the blocker to the waiter.
• The ["spec","decisions"] context path is declared on the schemas.
• project has tasks.
• The CLI forms (node create --type/--parent/--content, search, node get, node context --path) exist.
• The Ready tasks filter (open, has a checkbox child, no unfinished blocker, no unapproved plan, permitted to start) matches the skill text.
• The approval wording never lets the agent approve.
• Goldens, matrix and seed-count changes are consistent.

No blockers. The delivery claim below should be settled before merge.

Requirements Check

• Met: five skills seeded, attached per ADR-094, replaced skills deleted (same ids kept for the two replacements).
• Met: no harness tool, slash command, relative link, tracker or label. Enforced by a test, but see the test note below.
• Met: spec is a draft with checkbox criteria; approval is the user. APPROVAL IS THE USER, spec-approve and approval-ask-first are intact.
• Met: tasks link plan and spec, carry their own checklist and blocks edges. Expand, migrate and contract is present, including the integration-branch fallback.
• Partly met, see Important 1: "reaches an agent with the work".
• Met: matched from plain description. Matrix cases cover "this is unclear", "I am not sure what I want" and "help me think this through" (to Asking the User), and "I do not understand" (to Explaining, with a negative guard against Asking the User).
• Met: grilling asks one question at a time with plain explanation, stakes, at most four options, a recommendation worded so "yes" accepts it, and it never acts on it.
• Met: failure-mode list with GAP marking, not-in-scope list, and each point recorded with Recording a Decision and linked to the spec.
• Met: the spec skill settles test boundaries with the user and records them before splitting.
• Met: licence comment on the issue.
• Met: tests. Seed tests, goldens and matrix updated; the live runs rest on the author statement, which I could not re-run.

Findings

Important

1. "Arrives with the work" is only a name, and the docs and test overstate it.
   packages/agent/src/skill_pipeline.rs (Asking the User, Explaining: attached_to: &[]), packages/agent/tests/it/workflow_seeds.rs (every_planning_skill_arrives_with_the_work).
   Context delivery (node_context_ops::applicable_skills) returns only skills attached to the node or its queue. Names in a body are not resolved. The new test a_plan_is_split_into_linked_issues... itself asserts a plan carries exactly ["Reviewing a Plan","Splitting a Plan into Tasks"], so Asking the User and Explaining do not arrive with a spec or plan. The agent sees the words "use the skill Asking the User" and must then find it from the skill list or search. That is the "depends on the agent searching for it" case the issue wanted to avoid, and ADR-094 and seeded-nodes.md say each "arrives with the work".
   The literal acceptance criterion ("attached, or named by a skill that is") is met, so this is not a blocker. Pick one:

   • Attach Asking the User to SPECS_BY_STATUS and PLANS_BY_STATUS (attachment is many-to-many, so it is a one-line data change). Explaining stays named by it, which is the issue design. This makes the claim true for a harness agent reading a spec or plan.
   • Or keep it as is and soften the ADR-094 line and the seeded-nodes.md sentence to say "named, and found from the skill list".
     Principle: tests and docs should assert the mechanism, not a string. body.contains("Asking the User") passes whether or not anything reaches the agent.

2. Behaviour change in Reviewing a Plan with a draft spec is unflagged.
   packages/agent/src/seeds/skills/reviewing-a-plan.md, READ THE SPEC FIRST.
   The old skill drafted a plan against a draft spec and "left the plan a draft". The new text says "write no plan yet". The rules still allow a draft plan linked to a draft spec (approval is what is gated), and the new a_plan_is_split... test creates the plan before approving the spec. Either restore the old behaviour or state the choice in the issue comment and ADR (the comment says the review sits "between an approved spec and its tasks", which supports it). Make the two paragraphs (READ THE SPEC FIRST and APPROVAL IS THE USER) consistent either way.

3. Readiness sentence is imprecise.
   packages/agent/src/seeds/skills/splitting-into-issues.md, READINESS IS NOT YOURS TO SET.
   "which needs a checklist, no unfinished blocker, and an approved spec and plan" reads as always requiring a spec and plan, which contradicts the first paragraph (a small change with neither is ready). The queue requires: open, a checklist, no unfinished blocker, and a plan, if it has one, that is approved. A linked spec must be approved to start. Suggested wording: "...and, if it has a plan or a spec, those approved."

Improvements

4. The forbidden-words test scans raw bodies, not the rendered skill.
   workflow_seeds.rs the_planning_skills_name_no_harness_tracker_or_link reads seed.body, which holds <!-- include: ... --> markers. The rule fragments are where tool and CLI text lives, and they are not scanned. Resolve includes in both forms (resolve_includes with RuleForm) and scan that. Otherwise a harness or tracker reference added to a fragment passes.

5. Test special-cases skills by title.
   the_ten_procedures_are_seeded_with_their_links exempts "Asking the User" and "Explaining" by string. Drive it from the data (for example assert applies_to empty exactly for the two ids) so adding a skill does not silently join the exemption.

6. a_plan_is_split... replays tool calls rather than testing the skill.
   It is a good tool-seam test (relationship names, directions, queue readiness), but it cannot catch drift in the skill text or fragments. Add a cheap assertion that the rendered agent-form fragments name the relationship types used here (tasks, blocks, spec).

7. Pin that every skill named in a body resolves to a seeded title.
   Stale names such as "Writing a Plan" would otherwise fail silently. A test over SKILL_SEEDS that extracts "the skill X" and checks X against the titles is a few lines.

Nits

• Nit: packages/core/src/ops/skill_ops.rs:1464 and packages/agent/src/local_agent/routing.rs:3301-3307 still use "Writing a Plan" as fixture names. Harmless, but rename to avoid grep noise.
• Nit: the issue comment says "14 tests, 2 new"; the diff adds three (every_planning_skill_arrives_with_the_work, the no-harness scan and the split test).
• Nit: nodespace-docs ADR-092 section 8 says "The four planning procedures" above a five-row table.
• Nit: ask-decision-brief sends options empty, so the whole brief is prose and a UI cannot render option buttons. That is consistent with approval-ask-first and gives "yes accepts the recommendation", but a one-line comment on why would help the next reader.
• Nit: Explaining use_for ("asks what something means") is broad. The procedure lane keeps it from leading, and the matrix guards "I do not understand", but watch it against Research and Search in the live guards.

Security, performance

No findings. The change is prompt text, seed rows and tests. No new tool surface; tool whitelists are the minimal set per skill.

Verdict

APPROVE (posted as a comment: GitHub blocks self-approval). Resolve item 1 (attach, or soften the claim) and decide item 2 before merge; the rest are optional polish.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment, GitHub blocks self-approval). No blockers. Before merge, settle Important 1 (named skills do not actually arrive with the work: attach Asking the User to the spec and plan queues, or soften the ADR-094 and seeded-nodes claim) and decide Important 2 (Reviewing a Plan now writes no plan against a draft spec; the old skill drafted one). Full findings in the review comment above.

[malibio]

Review addressed (commit 4486a0c)

Addressed

• Important 1: Asking the User is now attached to "Specs by status" and "Plans by status", so a spec or a plan carries it and Explaining arrives through it. The workflow test asserts the three skills a plan carries and the two a spec carries. ADR-092 §8, ADR-094 and seeded-nodes.md updated and pushed.
• Important 2: Reviewing a Plan drafts and reviews a plan against a draft spec again, as the replaced skill did; it just cannot be approved until the spec is.
• Important 3: the readiness sentence now reads "if it has a spec or a plan, those approved".
• Improvement 4: the no-harness/tracker/label scan reads both rendered forms (agent and external), not the raw body.
• Improvement 6: a check that the splitting skill's rendered steps name tasks, blocks and issue.
• Improvement 7: a check that no planning skill names a replaced skill's old title (a rename can no longer leave a stale name).
• Nits: fixture names in two unit tests, ADR "four" wording, and the test count: the PR adds 3 workflow tests, not 2.

Skipped

• Improvement 5 (drive the Asking/Explaining applies_to exemption from data): it is one assertion over two explicit titles. Revisit if a third skill about no type is added.

Agent tests: 1130 passed; the retrieval texts are unchanged, so the recorded live matrix and the 41 guards stand.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: one attachment row (data), two sentence rewordings, and stronger tests. Nothing new in behaviour beyond what the review asked for, and the suite covers each change.