Let a native chat pin a skill and a node, and open a play's Edit chat bound to it

[malibio]

Closes #3520. Implements ADR-090 §3–§5.

What changed

Node type change (node type sequence): a new declared relationship on the core subtype ai-chat-native (an ai-chat subtype).

• pins (chat → any node, many; reverse pinned_in, many; no edge fields). No fields added or changed, no structural or participation rule changed. Generated TypeScript is unchanged. The sheet in node-types.md already lists it.

Routing (agent)

• SkillCandidate.pinned; a pinned candidate clears the score gate without a score.
• routing::with_pinned_skills adds each pinned skill to Stage 2's candidates after the top-K bound, so it takes no retrieved skill's place. A skill retrieval also selected is marked, not repeated, and keeps its score.
• A pinned skill counts toward the offered set like any linked candidate.
• With retrieval unavailable (embeddings still loading) a pinned skill is still offered, with no Stage-1 generation.
• A session with no pinned skills routes exactly as before.

Entity list (daemon)

• chat_pins::load_chat_pins reads a chat's pins each turn and splits them into skills (routing candidates, via skill_ops::skills_by_id) and other nodes.
• Other pinned nodes are stated to the model on every turn as a system record (id, title, type), placed just before the user's message. It is rebuilt from the edges each turn, so it is outside the per-message cap on looked-up entities.

Edit chat

• New RPC LocalAgentService.CreatePlayEditChat (Tauri command local_agent_create_play_edit_chat, dev-proxy POST /api/agent/play-edit-chat). It creates the chat titled Edit <play title>, its pins to the play-authoring skill and the play, and the opening assistant message (outcome: clarified, five options, a link to the play). No model call.
• PlayNodeViewer gets an Edit button that creates the chat and opens it in the other pane, beside the play.
• The opening card renders through the existing options rendering; picking one sends it as the user's turn.

Decisions worth a look

• The opening message is not counted as a request's clarification. Stored clarified (so its options render), it otherwise tripped the one-clarification-per-request contract: after the user's first pick the agent was re-prompted "do not ask anything further". prior_turns_from_history now leaves out assistant messages that precede the first user message.
• Creation is two writes, not one transaction. The chat and its pins commit together; the opening message follows, and the chat is deleted if the message fails. Core's parent check does not see a parent created in the same transaction.
• A pinned skill has no score bar, including a destructive one if one were ever pinned. Play Authoring is not destructive.
• A pinned skill with no applies_to link carries no schema metadata: find_skills' fallback is a guess from the query, and a pin has none.

Tests

• Core: the schema declares pins.
• Agent: with_pinned_skills (added, not repeated, keeps retrieval's score, no-op without pins), gate, offered set; turn-level Stage 2 prompt and tools with and without pins, with retrieval finding the pinned skill, finding nothing, and unavailable; the seeded Play Authoring skill as a pinned candidate.
• Daemon: pin loading and split; the pinned record; Edit chat creation (title, pins, opening message fields, mentions edge, model, missing skill, non-play); first turn answers the reply to the opening message; pinned node present past the cap; no record for a chat without pins.
• Frontend: client command (Tauri and proxy), store createPlayEditChat, the Edit button, the opening card's options.

The assembled prompt is unchanged for a chat with no pins, so the prompt golden test is untouched. The routing and decisions evals were not re-run: they exercise chats without pins, whose routing path is covered as unchanged by unit tests.

Docs: two sentences added to architecture/ai-chat-node.md and architecture/prompt-skill-system.md in nodespace-docs.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3575 (issue #3520)

Review Type: Initial Review

Reviewed at f135a0eb by reading the diff and its callers (route, run_turn_unguarded, run_ai_chat_turn, stage2_permitted_names, create_node_with_relationships, append_message, maybe_summarize_history, ai_chat_title). Nothing was built or run: a test run was in progress in the worktree, so every statement about test results below is from reading the tests, not from running them.

Summary

The change is a clear net improvement and follows the design in ADR-090 §3 to §5 closely. The layering is right: one declared relationship, pins read from edges each turn into an ephemeral session (so nothing goes stale), one pure routing function (with_pinned_skills) applied after the top-K bound, and one backend operation for the Edit chat. The skill_candidate_from / skill_entry extractions remove duplication instead of adding a second copy. No Critical findings. Three Important items are worth settling before merge: two are small hardening changes, one is a check that the tests cannot make.

Requirements Check

#	Acceptance criterion	Status	Notes
1	ai-chat-native declares pins / pinned_in, target any node; node-types:check; generated types	✅	core_schemas.rs: Out, Many/Many, target_type: None, no edge fields. Name defined once (AI_CHAT_PINS in nodespace-types, re-exported by core). node-types.md line 267 lists it. No wire struct changed, so no generated TypeScript change is expected. node-types:check not run by me.
2a	A pinned skill is in every turn's Stage-2 block with instructions and schema_metadata, no duplicate	✅	with_pinned_skills marks a retrieved candidate instead of repeating it; skills_by_id supplies instructions and applies_to schemas. One exception: a Stage-1 generation failure returns no candidates, pinned included (finding 4).
2b	It counts toward the offered set	✅	offered_types filters on clears_score_gate, which a pin now satisfies. Tested alone, beside a linked candidate and beside an unlinked one.
2c	A chat without pins routes exactly as before	✅	with_pinned_skills returns selected untouched for an empty slice; clears_score_gate is unchanged for pinned: false; the daemon skips set_session_pinned_skills and appends no record.
3	A pinned non-skill node is in every turn's entity list, exempt from the cap	✅	Implemented as its own system record rebuilt from the edges each turn, placed just before the user's message, so the per-message cap cannot reach it. It also survives history summarization at turn start (the last three messages are kept verbatim).
4a	One backend operation creates the chat, pins and opening message	✅	One RPC. Two writes with a compensating delete, not one transaction; the reason given (the parent check in prepare_create_node_with_parent reads the store outside the transaction) matches the code.
4b	No model call	✅	create_play_edit_chat touches only NodeService. Asserted in an_edit_chats_first_turn_answers_the_reply_to_its_opening_message.
4c	The link records a mentions edge from the message to the play	✅	append_message with no edges goes through create_node_with_parent, which calls sync_created_mentions. Asserted in the_opening_message_mentions_the_play.
5	The opening card renders through the existing options rendering; a pick is sent as the user's turn	✅	No rendering code added; chat-message-clarify-options.test.ts covers the five options and the click.
6	PlayNodeViewer's Edit creates the chat and opens it	✅	handleEdit calls createPlayEditChat then navigateToNodeInOtherPane.
7a	Tests: routing with and without pins; pinned skill also found by retrieval	✅	Unit (routing.rs), turn-level (agent_loop.rs) and seeded-skill (pinned_skill_candidates.rs).
7b	Tests: the entity list keeps a pinned node past the cap	✅	a_pinned_node_stays_in_the_entity_list_past_the_cap.
7c	Tests: Edit chat creation (pins, opening message fields, mention edge)	✅	play_edit_chat.rs tests. The rollback path is untested (finding 5).
7d	The routing and decisions evals don't regress	❌ not demonstrated	Not re-run. The PR's argument (the evals use chats without pins, and that path is unchanged) holds on reading, but the criterion asks for the evals. Either run them or have the author accept the argument explicitly.
8	Prompt golden test updated if the assembled prompt changes	✅	Unchanged for a chat without pins; the golden fixtures only gain pinned: false.
9	quality:fix clean; test:changed passes	❓ not verified	Not run by me.

Code Review Findings

🟡 Important

1. A pin clears the destructive bar as well as the read and mutating ones
📁 packages/agent/src/local_agent/routing.rs:555-560

clears_score_gate returns true for any pinned candidate. For a destructive skill that removes the one rung ADR-038 says is gated hardest. stage2_permitted_names still admits destructive tools only from the top-scoring tool-bearing contender, but a pinned candidate is now a contender at any score, so it is the top one whenever nothing else clears a bar, or whenever its sub-bar retrieval score is the highest.

Failure scenario: a chat pins a skill that whitelists delete_node (the pins edge is an ordinary relationship: chat_pins.rs's own tests write it with create_relationship, so any client of the generic relationship write can add one, not only create_play_edit_chat). The user sends a message unrelated to deletion; retrieval returns nothing above its bar. The pinned skill is the only contender, is_top is true, and delete_node is on the surface with the skill's instructions in the prompt, on a turn that scored it at 0. The held-delete confirmation still stands between that and data loss, so this is defense in depth, not an open hole.

Recommendation: let a pin stand in for the read and mutating bars only, for example (candidate.pinned && !skill_is_destructive(candidate)) || candidate.score >= score_bar_for(candidate), with a test. It costs nothing today (Play Authoring's tools are get_play, update_play, search_nodes, route_clarify) and the PR description already names this as an accepted gap.

Principle: least privilege; keep an existing safety invariant unless the feature needs it relaxed (YAGNI applies to the relaxation).

2. An assistant-first conversation is a new shape for the chat template, and no test renders it
📁 packages/daemon/src/services/play_edit_chat.rs:131-145, packages/daemon/src/services/local_agent_service.rs:848-855

The first turn of an Edit chat sends system, assistant, system (pinned record), user. Every other native chat starts with the user's message. apply_chat_template hands the messages to the model's embedded Jinja template through llama.cpp, and some templates (and some OpenAI-compatible servers) reject or mis-render a conversation whose first non-system message is the assistant's. The daemon tests assert this exact role order, but against StubEngine, which renders no template, so they cannot show that the real engine accepts it. Mid-history system records already work in production, which is encouraging, but it does not cover this case.

I could not check this by reading: it depends on the locked model's template. If it fails, the first turn of every Edit chat fails with a template error, which is the feature's main path.

Recommendation: one live turn in an Edit chat on the locked native model before merge (the run-app flow or an existing live-test harness), and on one OpenAI-compatible config if that provider is meant to work here. If it does fail, the fix is small: drop leading assistant messages from the rendered history, or fold the opening question into the pinned record.

Principle: test at the boundary where the risk is; a stub at the seam verifies the caller's intent, not the callee's contract.

3. Nothing tests the seam between a chat's pins and routing
📁 packages/daemon/src/services/local_agent_service.rs:903-909

Each half is tested: load_chat_pins returns the skill, and a session with pinned_skills set routes correctly. The four lines that join them in run_ai_chat_turn have no test. The daemon's Edit-chat tests never seed the Play Authoring skill, so pins.skills is empty in all of them, and they would pass with the set_session_pinned_skills call deleted. That call is the whole of acceptance criterion 2 in production.

Recommendation: one daemon test that seeds the skill, creates the Edit chat, runs a turn on the recording engine and asserts the request's system prompt contains the skill's instructions (or that the offered tools include update_play).

Principle: integration tests belong on critical paths where two separately tested units meet.

🟢 Suggestions

4. A Stage-1 failure drops the pinned skill, while "routing unavailable" keeps it
📁 packages/agent/src/local_agent/agent_loop.rs:5295-5303 (compare 5185-5192)

When the Stage-1 generation errors, route returns outcome with no candidates, so the turn fails open to the general surface without the pinned skill's instructions or, possibly, its tools (fail_open_surface withholds some). The unavailable path three screens up was changed to keep the pin for exactly this reason. Applying with_pinned_skills(Vec::new(), ...) on that return too makes the two degraded paths agree. Principle: consistency between equivalent failure paths.

5. The compensating delete has no test
📁 packages/daemon/src/services/play_edit_chat.rs:140-166

remove_chat is the only thing that upholds "the caller gets the whole chat or none". It is hard to trigger through NodeService, so an injected failure may not be worth the plumbing; if not, say so in the function's comment so the next reader knows it is unexercised.

6. PlayEditChatError::Service maps every service error to Status::internal
📁 packages/daemon/src/services/local_agent_service.rs:1470-1491

A provider or model the schema rejects is the caller's mistake and reaches the client as an internal error. provider and model are also accepted as Some("") (the dev-proxy filters empty strings, the Tauri command does not). Mapping validation failures to invalid_argument and treating blank as absent would make the error the Edit button shows more useful.

7. The warning for a skipped pin assumes which one was skipped
📁 packages/daemon/src/services/play_edit_chat.rs:125-131

unpinned holds indexes. The message says the play-authoring skill is missing whichever index it holds. The play was read a moment earlier, so index 1 is a narrow race, but logging the skipped targets costs one field and cannot mislead.

8. A long ReAct loop can summarize the pinned record away mid-turn
📁 packages/daemon/src/services/local_agent_service.rs:850-855

maybe_summarize_history runs each iteration and keeps the last three messages. At turn start the record is among them. After several tool calls in one turn it is not, and if the budget trips it goes into the summary with everything else. Unlikely in practice and the next turn restores it; worth a sentence in the comment that claims the record does not age out.

Nitpicks

• Nit: 📁 packages/agent/src/local_agent/tools.rs:678-681: the new functions were inserted between node_uri and its doc comment. "Prefix a bare node ID with nodespace://..." now documents skill_candidate_from, and node_uri (line 733) has none.
• Nit: 📁 packages/agent/src/local_agent/routing.rs:2905-2907: the same thing in the tests. The comment for linked_metadata_naming_no_type_yields_no_set (line 3022) now sits on the pinned helper.
• Nit: 📁 packages/agent/src/agent_types.rs:730: #[serde(default)] on pinned, where its neighbour schemas_linked has none. If nothing deserializes a SkillCandidate written before this change, drop it so the two fields read alike.
• Nit: 📁 packages/desktop-app/src/lib/components/viewers/play-node-viewer.svelte:92-97: createError is shared with the sidebar's "New chat", so a failed Edit also shows its message there. Returning the error from createPlayEditChat would keep it in the viewer.
• Nit: 📁 packages/daemon/src/services/chat_pins.rs:33: "in the order it was pinned" depends on the order get_edge_targets_by_source returns, which no test pins down.

What is done well

• Pins are re-read per turn into an ephemeral session, so there is no cache to invalidate.
• prior_turns_from_history solves the opening-message problem with a general rule (assistant messages before the first user message answer no request) and not a special case for this chat, and the affected existing test was updated to a realistic history.
• with_pinned_skills runs after select_candidates, so a pin never takes a retrieved skill's place, and keeps the retrieval score so leadership still follows relevance.
• Standards: type checks go through type_is_a / CoreNodeType, participation through governance::participates, no literal node_type comparisons outside tests, no lifecycle_status reads, no compatibility code, no issue numbers in comments, createLogger in the store, no Pro markers. Docs for both architecture pages are updated.
• The auto-title job leaves Edit <play title> alone (is_untitled matches only the sentinel).

Recommendation

APPROVE, with findings 1 to 3 recommended before merge: 1 and 3 are a few lines each, and 2 is one live turn. Criterion 7d (the evals) needs either a run or the author's explicit acceptance of the unchanged-path argument.

[malibio]

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

[malibio]

Address review

Changes are in 2548df28. bun run test:changed passes on it (frontend, scripts, browser, types:check, node-types:check, Rust workspace).

Addressed

• 1 (🟡) A pin clears the destructive bar. A pin now stands in for the read and mutating bars only: clears_score_gate requires a pinned destructive skill to clear its own bar. Test: a_pin_does_not_clear_the_destructive_bar.
• 2 (🟡) Assistant-first conversation not rendered by any test. Added a live test, nlp-engine/tests/it/assistant_first_conversation.rs (ignored by default, like the other E4B tests). Run once on Gemma 4 E4B with system, assistant, system, user: it rendered and the model replied "Which condition on the "Close finished parents" node would you like to change?". The GGUF's template also contains no role-alternation check. Not checked on an OpenAI-compatible endpoint.
• 3 (🟡) No test joins a chat's pins to routing. Added a_chats_pinned_skill_is_put_in_front_of_the_model: seeds the skill, creates the Edit chat, runs a turn and asserts the skill's guidance is in the system prompt, and is absent for a chat without pins.
• 4 (🟢) Stage-1 failure dropped the pinned skill. That return now keeps it, as the unavailable path does.
• 5 (🟢) Compensating delete untested. Left untested; the comment on remove_chat now says so and why (no seam to fail the second write on demand).
• 6 (🟢) Blank provider/model. A blank value is treated as none. Test: a_blank_model_is_no_model.
• 7 (🟢) Skipped-pin warning. It now logs the targets actually skipped.
• 8 (🟢) Pinned record and mid-turn summarization. Noted in the comment.
• Nits: the two displaced doc comments are back on node_uri and linked_metadata_naming_no_type_yields_no_set; #[serde(default)] removed from pinned.

Found by the full test run, not the review

pins targets any node, so every node's relationship list gained an empty pinned_in group (test_get_node_relationships_inbound_multiple_sources_not_duplicated failed). get_node_relationships now lists the reverse of an any-target relationship only on a node it reaches. This also applies to a user schema's untyped relationship: its empty inbound group no longer appears on every node. Test: test_pinned_in_is_listed_only_on_a_node_a_chat_pins.

Skipped

• 6, error mapping to invalid_argument. Not done: telling a validation failure from another service error means matching NodeServiceError variants in the RPC, for a message the Edit button shows either way.
• Nit: createError shared with the sidebar's "New chat". Left: both creates already share createBusy, and the error clears on the next attempt.
• Nit: pin order untested. get_edge_targets_by_source orders by rowid; no behaviour depends on the order.
• 7d, routing and decisions evals. Not re-run. They exercise chats without pins, and that path is unchanged: with_pinned_skills returns its input for an empty pin list and clears_score_gate is unchanged for an unpinned candidate, both under test.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the fixes change the routing gate, add a second degraded-path branch, and change which relationship groups core lists for every node, which the first review did not see.

[malibio]

Pragmatic Code Review: PR #3575 (issue #3520)

Review Type: Re-Review

Covers 2548df28 only (git diff f135a0eb..HEAD, 10 files, +260 / -11). Reviewed by reading the diff, its callers and the docs; nothing was built or run, so test results below are the author's (bun run test:changed reported passing), not mine.

Previous Review Summary

	Count
Previous recommendations	13 (3 Important, 5 Suggestions, 5 Nits), plus the open acceptance criterion 7d
Addressed	10 (finding 6 in part)
Not addressed, skip accepted	3 (half of finding 6, two nits), plus 7d
New issues found	4 (0 Critical, 1 Important, 3 Suggestions)

Previous findings

#	Finding	Status	Note
1 🟡	A pin clears the destructive bar	✅ Addressed	clears_score_gate is now (pinned && !skill_is_destructive) || score >= bar (routing.rs:563-566). Every consumer goes through that one function (is_tool_bearing_contender, the candidate block, offered_types, stage2_permitted_names), so no path still treats a pinned destructive skill as eligible. a_pin_does_not_clear_the_destructive_bar checks both sides of the bar and that the tool surface is empty. prompt-skill-system.md states the exception. See new findings 2 and 3 for two side effects.
2 🟡	Assistant-first conversation not rendered by any test	✅ Addressed for the locked model	nlp-engine/tests/it/assistant_first_conversation.rs sends system, assistant, system, user through the real template; it follows toolcall_json_shape.rs (same feature gate, #[ignore] reason, config, skip when the GGUF is missing). The author reports one passing run on E4B. Not checked on an OpenAI-compatible endpoint: accepted, because mid-history system records already ship on that path and a server strict about role order would reject those too, so the added risk is small. It stays a known gap, not a verified case.
3 🟡	No test joins a chat's pins to routing	✅ Addressed	a_chats_pinned_skill_is_put_in_front_of_the_model seeds the skill, creates the Edit chat and asserts the guidance is in the system prompt. The control (a chat without pins, same message, guidance absent) makes the positive assertion mean something: the test fails if set_session_pinned_skills is removed. It runs on the routing-unavailable path, which is the path that call feeds in the daemon's test service.
4 🟢	Stage-1 failure dropped the pinned skill	✅ Addressed	agent_loop.rs:5302-5308 applies with_pinned_skills as the unavailable path does. No test covers the new branch (new finding 4).
5 🟢	Compensating delete untested	✅ Addressed as offered	Left untested; the comment on remove_chat says so and why. That was the alternative the review gave.
6 🟢	Error mapping; blank provider/model	◐ Half	Blank is now none (a_blank_model_is_no_model). Mapping validation errors to invalid_argument skipped: acceptable, the Edit button shows the message either way and no caller branches on the code.
7 🟢	Skipped-pin warning assumed which pin	✅ Addressed	Logs missing from the indexes create_node_with_relationships returned; pinned and pins share one order, so the index maps correctly.
8 🟢	Pinned record and mid-turn summarization	✅ Addressed	The comment now says the record can be summarized within a long turn and is restated on the next.
Nit	Displaced doc comment on node_uri	✅	Back on node_uri.
Nit	Displaced doc comment on linked_metadata_naming_no_type_yields_no_set	✅	Back on the test.
Nit	#[serde(default)] on pinned	✅	Removed.
Nit	createError shared with the sidebar	⏭ Skipped	Acceptable: cosmetic, clears on the next attempt.
Nit	Pin order untested	⏭ Skipped	Acceptable: nothing depends on the order.
7d	Routing and decisions evals not re-run	⏭ Skipped	Acceptable. The argument still holds after this commit: for pinned: false the new clears_score_gate reduces to score >= score_bar_for(candidate), as before, and with_pinned_skills returns its input for an empty pin list. Both are under test.

New Findings

1. 🟡 Important: the any-target rule changes every untyped relationship, and only pins is tested or documented
📁 packages/core/src/ops/rel_ops.rs:795-801 (and the function's doc comment, 632-650)

The skip is keyed on rel.target_type.is_none(), not on pins, so it applies to any untyped relationship a user schema declares. I checked the consumers (relationship-grouping.ts, the Relationships modal, the promoted form field, farEndHolders; no Rust caller other than the daemon RPC). Nothing breaks, and the rule matches what resolve_relationship_name already does for untyped reverse names (rel_ops.rs:255-264). wrote / resolved / pending_delete are unaffected: their source type is already unlisted. But two behaviours change for a user's untyped relationship:

• A bare one (no edge fields) used to be an entry in the target node's "Add relationship" chooser (partitionGroups → addable). It is gone until the first edge exists, so the first edge can now be added only from the declaring side.
• One whose reverse cardinality is one used to be a promoted form field on every node (isFormPromoted promotes "whether or not they have an edge yet"). The field is now absent until an edge is written from the other side, then appears.

Both are defensible (an Add entry and a form field on every node in the graph for every untyped relationship is the same noise pinned_in was). The problems are in what describes the rule:

• The function's doc comment still says "Declared groups are always returned on both sides — outbound and inbound — even when empty, so the viewer can add the first edge", and the comment just below the new code says "Emit the group even with no edges yet". Both are now wrong for untyped relationships, in the function a reader goes to for the rule.
• test_pinned_in_is_listed_only_on_a_node_a_chat_pins covers pins only. No test pins the rule for a user schema, so a later "fix" that special-cases pins would pass the suite.
• The docs state it only for pinned_in (ai-chat-node.md); typed-relationships.md, where untyped relationships are described, does not.

Recommendation: correct the doc comment (typed groups are listed when empty; an untyped one only on a node it reaches, with the reason), add a few lines to the new test or a sibling for a user schema's untyped relationship (empty on an unlinked node, present on a linked one), and add one sentence to typed-relationships.md.

Principle: a comment that contradicts the code is worse than none; a general rule needs a test at its general case, not only at the case that prompted it.

2. 🟢 Suggestion: a pinned destructive skill now logs "the bar is set too high" on every turn
📁 packages/agent/src/local_agent/routing.rs:1096-1102, packages/agent/src/local_agent/agent_loop.rs:3117-3131

destructive_candidates_below_bar filters on skill_is_destructive(c) && !clears_score_gate(c). A pinned destructive skill that retrieval did not return is added with score 0.0, so it now matches on every turn of its chat and is logged as name=0.000 with "If the user was in fact asking to delete something, the bar is set too high". That line exists to read the score distribution of real deletion requests off the logs, and a candidate retrieval never scored is not a data point in it. No shipped chat pins a destructive skill, so this costs nothing today. Excluding candidates present only by pin (or carrying the retrieval score as an Option) keeps the measurement clean. Principle: a diagnostic should report only the event it is defined for.

3. 🟢 Suggestion: the destructive exception is all-or-nothing for the skill, and nothing guards Play Authoring against it
📁 packages/agent/src/local_agent/routing.rs:555-566

A pinned skill whose whitelist includes one data-removing tool loses the pin's effect entirely below the destructive bar: its instructions, its schemas and its read and write tools, not only the destructive tool. That is the safe direction and the right default. The consequence is that adding delete_node to Play Authoring's whitelist would silently turn the Edit chat back into an ordinary chat on most turns ("Change a condition" will not score 0.45 against a deletion-capable skill description), with no test failing. A one-line assertion that the seeded Play Authoring skill is not destructive (beside the existing seeded-skill tests in pinned_skill_candidates.rs) would catch it. Also worth one clause in ADR-090 §5, which still says each pinned skill is added to the candidate block without the exception that prompt-skill-system.md now states. Principle: make an invariant the feature depends on fail loudly when it is broken.

4. 🟢 Suggestion: the Stage-1 failure branch has no test
📁 packages/agent/src/local_agent/agent_loop.rs:5295-5310

The unavailable path has a_pinned_skill_is_offered_when_routing_is_unavailable; the failure path added here has none. It is three lines that mirror a tested branch, so the risk is low, but the two degraded paths are now meant to agree and only one is held to it. A copy of that test with an engine whose first generation errors would do. Optional.

Checked and found sound

• Routing gate: for an unpinned candidate the new expression is the old one. The pinned destructive test constructs its scored variant with ..deletion, so it stays pinned and proves the bar, not the pin, admits it.
• play_edit_chat.rs: given filters blank and whitespace-only values; missing borrows from pinned, which outlives the call.
• Standards: no literal node_type comparison outside tests, no lifecycle_status read, no compatibility code, no lint suppression, no issue numbers in comments, no Pro markers. The new live test is a module of tests/it/main.rs as the repo requires.
• The rel_ops skip runs after the per-node narrowing, so it tests the group that would be shown, and it adds no query.

Recommendation

APPROVE

All three Important findings from the first review are resolved in code, the skips are reasonable, and the new commit introduces no correctness or security problem. New finding 1 is worth doing before merge: it is a comment correction, a short test and one docs sentence, for a behaviour change that reaches user schemas. Findings 2 to 4 are optional.

[malibio]

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

[malibio]

Address re-review

Changes are in ee0c81a8. bun run test:changed passes on it (frontend, scripts, browser, types:check, node-types:check, Rust workspace).

Addressed

• 1 (🟡) The any-target rule reached every untyped relationship. Narrowed instead of documented: the empty inbound group is now left out only when the declaring type is a chat (type_is_a(source, AiChat)), since a chat writes its own edges and nothing adds a pin from the pinned node. A user schema's untyped relationship is listed on a node with no edge, as before this PR, so the "Add relationship" entry and the promoted form field are unchanged. The function's doc comment states the exception. Tests: test_pinned_in_is_listed_only_on_a_node_a_chat_pins and the new test_a_user_schemas_untyped_relationship_is_listed_when_empty.
• 3 (🟢) Nothing guarded Play Authoring against becoming destructive. The seeded-skill test now asserts it is not. ADR-090 §5 states the destructive exception.

Skipped

• 2 (🟢) A pinned destructive skill would log the "bar is set too high" line every turn. No shipped chat pins a destructive skill; worth doing with the first one that does.
• 4 (🟢) No test for the Stage-1 failure branch. Three lines mirroring the tested unavailable branch.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the one code change narrows a rule back toward the behaviour before this PR, in the direction the re-review asked about, with a test on each side of it. The rest is a test assertion and comments.