[malibio]

Closes #3720.

What ships

• Implementing a Task (rewritten in place): the user chooses the work; claim by version; test-first one item at a time with the failing and the passing run as evidence; full suite once with bounded fix attempts; hands over to review without waiting.
• Reviewing a Task (rewritten in place): a session that implemented the task stops; two separate passes (spec audit with done/partial/not done/changed/unverifiable, then standards); a weakened check goes to the user as restore or keep; a finding is recorded with source, confidence, decision and reason before any fix; an issue that passes is ready_to_merge, a plain task done.
• Addressing Review Findings (new, attached to "Changes requested"): the implementing session fixes findings decided fix, test-first, and returns the issue to in_review.
• Queues: "Awaiting review" now targets issue; new "Changes requested" and "Ready to merge" target issue.
• Issue findings Play (written against issue): refuses leaving review while a finding has no decision, and refuses in_review / ready_to_merge while a finding decided fix is unchecked.
• A finding is a checkbox directly under the task whose text opens Finding (; the existing criteria rule keeps an unfixed one from reaching done. Documented, with the alternatives not chosen, in ADR-092 §8 (nodespace-docs, pushed before the code).

Needs your call

• "In progress" still lists changes_requested issues. It is written against task, where that status reads as in_progress, so such an issue carries both Implementing a Task and Addressing Review Findings (Implementing hands over to Addressing). Retargeting it to issue would empty the queue for teams that do not write software. This misses the acceptance criterion "not in In progress"; a fix needs either a per-filter scope or a type-split of the queue.
• Mutation check not done. Reverting each new rule's condition to show its test fails was blocked in this session; the rule tests (an_issue_moves_through_review_by_its_findings, an_unfixed_finding_cannot_reach_done, a_plain_task_is_touched_by_none_of_the_review_rules) have not been shown to fail with the condition reverted.
• Agent evals not run on the scenarios this touches.
• Per-skill source and licence comment on the issue is not posted yet.

Verification

bun run test:changed (rust, scripts, types, node-types): passed. Confusion matrix and prompt goldens re-recorded.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3767 (issue #3720)

Review Type: Initial review (no prior reviews or comments on the PR). 32 files, +668/-88.

Summary

Sound, well-scoped change. The issue-scoped queues and the issue findings Play are written against issue as asked; no node-type literal comparisons or lifecycle_status reads were added, no backward-compat code, no issue numbers in code comments, and the docs (ADR-092 §6/§7/§8, playbook-system, seeded-nodes) were pushed before the code. The flow test (an_issue_moves_through_review_by_its_findings) exercises the Play end to end through real status writes. The blockers are process/evidence items, plus one criterion that is knowingly unmet.

Requirements Check

• ✅ Seeded implementing/reviewing skills ship attached to queues; task-implement-take is deleted with no remaining references; Addressing skill added and attached to "Changes requested".
• ✅ No adapted skill names a harness skill tool, slash command, relative link, tracker or label (the forbidden-word test now covers all three skills).
• ✅ "What to work on" lists Ready tasks (project filter documented) and stops until the user chooses (STEP 1; asserted in the delivery text test and the list step of the flow test).
• ✅ Test-first per item with failing then passing run under each tick (STEP 4/5; evidence is prose-enforced, as the issue directs).
• ✅ Different-session review, and how a reviewer tells is written down (Reviewing STEP 2).
• ❌ changes_requested appears in "Changes requested" with the address procedure, but also still in "In progress" (written against task, where the status reads as in_progress). The criterion says "not in 'In progress'". The PR documents and tests this as intended (core_queries_test, workflow_seeds, ADR-092), so it is an acknowledged deviation, not a silent one. See finding 1.
• ✅ Reviewer passes an issue to ready_to_merge and a plain task to done; "Ready to merge" queue exists.
• ✅ Queues and rules are written against issue; a plain task is touched by none (a_plain_task_is_touched_by_none_of_the_review_rules, a_plain_task_is_in_none_of_the_issue_review_queues).
• ✅ Moves refused while a fix finding is open and allowed once fixed (flow test, both directions).
• ✅ Status-behaviour table for both values written down (ADR-092 table: "reads as in_progress" / "reads as in_review").
• ✅ Pass against the spec: five marks plus unrequested behaviour, written as a section on the task.
• ✅ Weakened check goes to the user as restore or keep, answer recorded as a finding (Reviewing STEP 6; Implementing STEP 4/6 and Addressing STEP 3 stop on it).
• ✅ Every shipped skill is attached to a queue or named by one that is (Addressing named by Implementing; test asserts it).
• ✅ Two separate finding sets, keyed by source (spec/standards).
• ✅ Findings recorded in the graph with decision + reason; leaving review undecided is refused (rule 1, tested for both exits and done).
• ✅ Recording approach and the alternative not chosen are written down (ADR-092 §8, "How findings are recorded").
• ❌ Per-skill comment (source file, licence, changes, replaced skill) is not yet posted on Adapt the community implement and review workflows into NodeSpace delivery skills: test-first, review by a separate session, a decision per finding #3720. Confirmed.
• ❌ Testing: workflow seed tests and goldens are updated, but (a) "each new rule test shown to fail with the condition reverted" is not evidenced and (b) no eval run on the touched scenarios is recorded. Confirmed.

Code Review Findings

Blockers

• 🔴 Mutation evidence missing (packages/core/src/playbook/core_plays.rs:517, :522; new flow tests in packages/agent/tests/it/workflow_seeds.rs). Principle: a test never seen failing pins nothing. Revert each of HAS_UNDECIDED_FINDING, HAS_OPEN_FIX and the two node.status == ... trigger arms and confirm a named test fails each time; record it on the issue. Worth checking specifically: the done assertion is the only thing exercising the || node.status == 'done' arm of rule 1, and the c.checked == false guard in HAS_OPEN_FIX is distinguished only by the FIXED rewrite.
• 🔴 Per-skill attribution comment is an acceptance criterion and the "keep attribution" requirement; post it before merge (see the licence nit below on what the shipped footers carry).

Suggested Improvements

• 🟡 "In progress" still lists changes_requested issues (core_queries.rs:253; asserted in the flow test and in issue_and_software_project_test.rs). Beyond the unmet criterion: a session working the In progress queue gets the Implementing procedure for an issue whose review is back, and relies on Implementing STEP 9 prose to hand over to Addressing. Either amend the criterion on the issue (it is documented in ADR-092, so this is the cheap route), or exclude it at the query level if the filter language can address the stored issue status; otherwise record the limitation as its own issue, as the Technical Specifications direct for anything not expressible as data. Do not leave it only as a test assertion that contradicts the issue text.
• 🟡 Finding detection is a substring match, while docs say "opens with" (core_plays.rs:517-525, ADR-092 §8). contains('Finding (') classifies any checkbox containing that text as a finding, and !contains('Decision: ') treats a finding that merely quotes a spec/ADR line containing Decision: as decided, which defeats rule 1 silently (fails open). Anchor with startsWith('- [ ] Finding (') / startsWith('- [x] Finding (') and match the decision as a token that the reviewer-written form always produces (e.g. '. Decision: '). Principle: guards should fail closed. Add a test where the finding's "what is wrong" part quotes Decision: .
• 🟡 Self-approval path is unguarded: changes_requested -> ready_to_merge is allowed once fixes are checked (rule 2 reads only checkboxes), so the implementing session can skip re-review. The criteria require only in_review -> ready_to_merge, and the skill forbids it in prose, so acceptable for now; note it as a known gap in the ADR §6 table.
• 🟡 Review take rule cannot skip an own-implemented task (seeds/rules/agent/task-review-take.md, skill-md/task-review-take.md). limit 1 always returns the same top item; if that is one this session implemented, STEP 2 says stop and the session cannot reach the next. List without context (limit about 5) and read the chosen one with context, as task-implement-list now does.
• 🟡 No test for the project-scoped list filter (task-implement-list.md; criterion 3 says "for the project"). RunSavedQueryInput.filters supports it, but the exact JSON in the snippet (relationship, path: ["project"], node_id) is only prose. One assertion running Ready tasks with it against a project-linked and an unlinked task would pin it.
• 🟡 Eval criterion has no recorded run; say which eval ran (or why none applies) in the PR description.

Nitpicks

• Nit: workflow_seeds.rs:286 the_ten_procedures_are_seeded_with_their_links now covers more than ten; the name is stale.
• Nit: core_queries.rs module doc: the inserted sentence leaves a line well over the width; rewrap.
• Nit: ADAPTED FROM: footers (implementing-a-task.md:32, reviewing-a-task.md:25, addressing-review-findings.md:17) name a licence but not the upstream copyright holder lines for OneRedOak and gstack. MIT asks the notice to accompany substantial portions and these bodies ship in the product; make sure the per-skill comment (or a licences list) carries the exact holder lines, and that "copyright 2026 Matt Pocock" matches the upstream LICENSE.
• Nit: the delivery-text test pins long prose substrings, so any rewording fails it; keep the list to the load-bearing statements.
• Nit: Addressing STEP 2 tells the session to wait for the reviewer to decide an undecided finding, but rule 1 makes that state unreachable from changes_requested; the sentence can be dropped.

Standards check

No node_type literal comparisons, no lifecycle_status reads, no new fields or statuses, no sync-boundary wording, no migration or compat shims, no issue numbers in comments. Every new rule snippet exists in both agent/ and skill-md/ forms; the skill_rules.rs list matches the files; the seed count test is updated to 25.

Recommendation

REQUEST CHANGES. The design is right and the change is a clear net positive; the blockers are cheap: (1) show each new rule condition reverted makes a named test fail, (2) post the per-skill source/licence comment, (3) resolve the "In progress" criterion (amend the issue or fix the query), and ideally (4) anchor the finding/decision matching so rule 1 cannot fail open. The rest is optional polish.

[malibio]

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

[malibio]

Review addressed

Addressed

• Finding and decision matching anchored: a finding is a checkbox starting with its marker and Finding (; a decision is one of Decision: fix. / defer. / reject., so a finding quoting the word Decision: stays undecided (test added).
• Reviewing's take rule lists the queue and reads the chosen issue.
• Per-skill source and licence comment posted on Adapt the community implement and review workflows into NodeSpace delivery skills: test-first, review by a separate session, a decision per finding #3720.
• Stale test name; ADR-092 records the tokens and the self-review gap.

Skipped: project-filter snippet test (generic filter, covered by core tests); Addressing STEP 2 wording (reachable when the Play is switched off); module-doc width nit.

Not resolved, needs the owner

• 'In progress' still lists changes_requested issues (task scope reads it as in_progress). Retargeting to issue would empty it for non-development teams.
• Mutation check of the new rules and an agent eval run were not done.

Re-Review Decision

Decision: RE-REVIEW NEEDED — the Play's conditions changed (logic, not style), and two acceptance items remain open.

[malibio]

Open items resolved

"In progress" no longer lists a sent-back issue. A property filter takes stored: true to compare each row's stored value instead of what it reads as at the queried type. "In progress" (still written against task, so plain tasks keep their queue) excludes changes_requested with it. A changes_requested issue is in "Changes requested" alone and carries Addressing Review Findings alone. Documented in query-node.md (Stored values) and ADR-092 §8. The generated QueryFilter type gained the optional field.

Mutation check. Each condition was reverted in turn and a named test failed:

• undecided-finding condition always false: an_issue_moves_through_review_by_its_findings
• open-fix condition always false: an_issue_moves_through_review_by_its_findings
• leaving-review status arm never matches: an_issue_moves_through_review_by_its_findings
• back-to-review status arm never matches: an_issue_moves_through_review_by_its_findings
• stored comparison reads the mapped value: core_queries_test::an_issue_is_in_the_review_queue_of_the_status_it_stores
• In progress keeps the sent-back issue: the same test

Evals (gemma-4-e4b-q4km, one rep, isolated daemon):

	routing	decisions
this branch	16/19	21/24 (skill routing 7/7)
merge base	7/19	11/24

The merge-base run was degraded by many 100–180 s timeouts with no tool calls, so it is not a quality comparison. Every failure on this branch except indirect-schema-expenses (routing) also fails on the merge base; that one routed to non-delivery skills and reads as model variance. These are single reps, not pass^3.

Rebase

main moved under the branch: the query-building skill took the id I had picked for Addressing Review Findings (moved to a free id), the skill count and the recorded goldens changed, and the new seeded-query sentences needed the new queues. The branch was force-pushed once by mistake during the rebase, which closed this PR; the branch was restored and the PR reopened. test:changed is green on the pushed head.

Re-Review Decision

RE-REVIEW NEEDED: new engine surface (stored on QueryFilter) and changed rule conditions since the first review.

[malibio]

Pragmatic Code Review: PR #3767 (issue #3720), Re-Review

Review Type: Re-Review of head 6950415 (rebased; commits "Address review: anchor finding...", "Leave a sent-back issue out of In progress...", "Rebase fixes...", "Describe the issue-scoped queues...").

Previous Review Summary

First review: 2 blockers, 6 improvements, 5 nits. Now: 2 blockers addressed, 4 improvements addressed, 2 skipped fairly, 1 not fully addressed (eval evidence is thin but present), nits 3/5 addressed.

First-review item	Status
Blocker: mutation evidence	Addressed. Six reverts listed, each with a named failing test. Note that four of the six name the same flow test, so which arm each one pins is not visible from the names; acceptable, the single test drives both directions.
Blocker: per-skill attribution comment	Addressed (comment present on #3720).
"In progress" lists changes_requested	Addressed in code, see Section 2 for the new surface.
Finding match is a substring	Addressed: anchored on startsWith('- [ ] Finding (') / '- [x] Finding (', decision is one of three tokens, with a quoted-Decision: test (workflow_seeds.rs:1622).
Self-approval path	Addressed as documentation (ADR-092 records the gap). Fair.
Review take rule cannot skip	Addressed (lists then reads).
Project-scoped list filter test	Skipped, rationale fair (generic filter, covered by core tests).
Eval criterion	Addressed with one-rep gemma run; merge-base run is degraded by timeouts so it is not a comparison. Acceptable for this criterion, which asks for a run, not a threshold.
Nit: stale test name, ADR tokens	Addressed.
Nit: module-doc width, Addressing STEP 2 wording	Skipped, fine.
Nit: licence holder lines	Comment posted; not re-verified line by line.

Requirements Check (#3720)

All acceptance criteria are now met in code, tests or docs:

• Changes requested queue has Addressing attached, and the issue is no longer in "In progress" (stored comparison; asserted in core_queries_test and the flow test). The criterion text is now literally satisfied.
• Mutation-reverted rule tests: evidenced in the PR comment (a PR comment, not on the issue; fine).
• Per-skill source/licence comment: posted.
• Eval run: recorded (single rep).
• Remaining open items are process only: none block merge.

Findings

Critical / Blockers

None.

Suggested Improvements

• 🟡 stored is refused for a non-property filter on one path only. packages/core/src/ops/query_ops.rs:269 rejects it in to_query_filter (agent/CLI AgentFilterItem path), but a saved query, a play selector or a direct QueryDefinition goes through validate_filter_identifiers (packages/core/src/services/query_service/mod.rs:191-215), which refuses the sibling relative_date on a non-property filter and has no equivalent for stored. So {"type":"relationship"|"permitted"|"related", "stored": true} is silently accepted and ignored; resolve_filter only clears the mapping inside the Property branch. A silently ignored flag that changes what rows come back is the failure mode the answer to "is it applied where it should be refused" is yes. Put the check next to relative_date in the validator (one place, all paths) and drop or keep the to_query_filter one as the friendlier early error. Principle: validate at the single boundary every path crosses.
• 🟡 No engine-level test of stored. The only coverage is the seeded query (core_queries_test::an_issue_is_in_the_review_queue_of_the_status_it_stores, the flow test) and a serde round-trip in query_ops.rs:1813. Missing: (a) a QueryService/query_ops test that a task query with status = changes_requested returns the issue with stored unset and does not (equals) / does (stored) compare as the stored value, both with and without negate; (b) stored on a plain task row reads the same; (c) stored inside a related nested filter, which recurses through resolve_filter; (d) the refusal in to_query_filter. The mutation check proves the seeded queue pins the behaviour, but the flag is a general engine feature now, and the next author of a query using it has no test describing its contract (negated + NULL value kept, wildcard target, nested).
• 🟡 The flag is invisible to the surfaces that author filters. AgentFilterItem accepts stored (deny_unknown_fields) but the search_nodes / run_query item schema in packages/agent/src/local_agent/tools.rs (~2540-2600) and the CLI help in packages/cli/src/commands/query.rs:44 (which documents negate) do not mention it. Either an agent cannot discover a flag the engine supports, or, if it is deliberately internal, it should not be accepted on the agent input. Project convention for any new CLI/tool input is guidance in the same change: add one line to the tool schema, the CLI help and the query-building skill, or remove it from AgentFilterItem until a consumer needs it (YAGNI; the seeded query only needs the stored QueryFilter field).
• 🟡 Other-path audit result. Checked and fine: SQL path (stored_field falls back to the subtype bucket's raw value when values is empty, which is the stored value); in-memory matchesFilter (query-node-model.ts) already compares the raw stored value and declines rather than adds, so an ignored stored there cannot over-include; play selectors share resolve_filters; no frontend filter builder reconstructs filters (so stored is not dropped on re-save). One gap: resolve_filter clears subtypes[*].values but leaves the subtype bucket routing, correct, but the in-memory PropertyScope::read path (used by sort and by any in-process comparison) gets the same cleared scope only because the clear happens on the shared scope: nothing in-process currently evaluates filters through it, so this is unexercised. Worth one test (see previous item).
• 🟡 Rejection message token mismatch. core_plays.rs reject message tells the author to add "Decision: fix, Decision: defer or Decision: reject" but the rule requires the trailing period (Decision: fix.). Someone following the message literally and omitting the period stays refused. Say Decision: fix. in the message, or accept the token without the period.

Anchored matching (HAS_UNDECIDED_FINDING / HAS_OPEN_FIX)

Correct for the stated cases: - [ ] Finding ( / - [x] Finding ( anchored, non-finding checkboxes ignored, a finding that quotes Decision: stays undecided, open-fix requires unchecked plus the fix. token. Residual fail-open: a finding whose description quotes the exact token Decision: fix. or Decision: defer. counts as decided. Narrow and tested form is the reviewer-written trailing form; acceptable, but if you want to close it, match the token against the end of the text (endsWith-style on the reason separator) rather than contains. Uppercase - [X] is not matched as a finding (fails open); the seeded skills only write lowercase, so fine.

Nitpicks

• Nit: description.rs renders a stored filter in the same words as an unstored one ("status is not changes_requested"), so a reader of the "In progress" sentence cannot tell the comparison is on the stored value. Commit 6950415 added the queue sentences; one clause would do.
• Nit: the doc comment on AgentFilterItem.stored says "opt-out of the status mapping", while the field is generic to any extended enum; say "extended enum value mapping".

Standards Check

• Node-type sequence: no new type, field or status on a node type. stored is a wire-type field on QueryFilter in nodespace-types (single definition), query-filter.ts regenerated and committed, docs updated (query-node.md "Stored values", ADR-092 §6/§8). Good.
• No backward-compat code, no node_type literals, no lifecycle_status reads, no sync-boundary wording, no issue numbers in comments, no deprecated or dead-code attributes. The seeded query's "stored": true is inside the seeded definition, not a literal type comparison.
• bun run types:check / node-types:check expected clean: generated file differs only by the new field.

Recommendation

APPROVE. Both prior blockers are resolved, all #3720 criteria are now met, and the rules are correct. The remaining items are all about hardening the new stored flag (validate it in one place, a general engine test, tell the model about it or hide it). They are cheap and worth doing in this PR since the flag is new engine surface, but none make the change a net negative. Per project process, run /address-review next.

[malibio]

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

[malibio]

Re-review addressed

Addressed

• stored is refused on a non-property filter in the validator every query path runs (beside relative_date); the duplicate check in the input struct is removed, so one boundary covers a saved query, a play selector and agent input.
• Engine-level test (a_stored_filter_compares_the_value_a_row_stores): a task query with and without stored, negated, a plain-task row, and the refusal on a relationship filter.
• stored is documented in run_query's filter schema, the CLI help and the generated command reference (skill:check is up to date).
• The finding rule's message names Decision: fix. / defer. / reject. with the trailing period.

Skipped

• Naming the stored comparison in the query description sentence: "status is not changes_requested" is true to a reader; revisit if a stored filter ever reads differently from its plain form.
• Uppercase - [X] markers: the skills write lowercase; revisit if a UI starts writing findings.

test:changed is green on the pushed head (frontend, scripts, skill, browser, types, node-types, Rust workspace).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The re-review already approved the design and the logic. These changes move one validation to the shared validator, add a test, and document the flag; no rule condition or query behaviour changed.