[malibio]

Closes #2242.

Ran both winnability checks the issue specified against all three scenarios. No harness bug, no capability gap — all three were the fixture measuring itself.

The leading hypothesis is refuted

The issue's strongest candidate was that the prompt template drops role="tool" history, making 11d unwinnable by construction. That is not what happens.

Tool-role messages are dropped by node_history_from_messages (the _ => return Vec::new() arm). But the writes they carried are re-rendered as terse Fact: ... lines plus a system-role write record. Rendering the real 11a→11c history shows the turn going into 11d could see:

Fact: a text node was created with title 'the reports page uses server-side rendering' (id nodespace://dec1).
Fact: a task node was created with title 'rebuild the reports page' (id nodespace://task1).
Fact: create_relationship completed ("nodespace://task1 -[mentions]-> nodespace://dec1").

Both endpoint ids, the edge, and the decision's own text — stated twice. The failure was the opposite of a missing fact: the history handed the model the answer, and TOOL_STRATEGY_RULES' first bullet ("CONVERSATIONAL TURNS USE NO TOOLS… answer directly in text") tells it to answer such a turn in prose. Every model returning tools: [] was following its instructions.

Prose tuning provably cannot fix a missing fact — but there was no missing fact to fix.

Classification and fix, per scenario

	verdict	fix
11d	fixture defect	prompt was answerable from history; now asks for the set of inbound links, and a new 11c2 records a second edge so the answer is an aggregation history does not state
6	fixture defect	required [resolve_query, update_node] to catch an indirect reference, but history replays scenario 4's write with its property values and id inline, so "the five-day one" was already a direct string match. Now asserts the outcome
8e	fixture defect	"Run through those calls for me" pointed three turns back, past 8d's Planning Cycle. Reworded

On 8e specifically, three problems compounded: the nearest antecedent to the bare demonstrative was the wrong type (which is why one model answered the previous scenario's prompt); "calls" was a pun that never appears as the stored type's name; and "run through" reads as much like a meta question as a retrieval request.

On 6, the cost is stated in the fixture rather than hidden: indirect-reference decomposition is no longer measured there. It needs a chain whose referent is not recoverable from history, which this one cannot provide — every write it makes is replayed with its particulars.

A real bug the audit surfaced

completed_writes_from read a schema write's id from r.result["schema_id"]. Both CreateSchemaOutput and SchemaUpdateOutput carry #[serde(rename_all = "camelCase")], so the wire key is schemaId and the lookup matched nothing. Every schema write silently degraded to terse_write_fact's id-less phrasing — dropping from history the one string a later create_node must copy into node_type, leaving the model to guess the normalized spelling from the display name.

This affects every multi-turn chain that creates a type then records an instance against it, which is most of the matrix.

Evidence is pinned as tests, not left in the issue

• packages/agent/tests/matrix_scenario_winnability.rs (new) — hand-executes each scenario's ideal call against a live SQLite-backed NodeService and asserts acceptance. Covers step 2 of the issue for all three. Includes a check that one traversal enumerates both of 11d's inbound edges, so its noRetry half cannot red out a correct model.
• scenario_11d_history_already_contains_the_link_it_asks_about and scenario_6_history_resolves_its_indirect_reference_directly in local_agent_service.rs — render the real history and pin the refutation, so the next reader finds it rather than re-deriving it.
• completed_create_schema_write_captures_the_schema_id — asserts against the real serialization rather than a hand-written key, so a future serde-policy change re-breaks the test rather than silently re-breaking history.

Verification

• bun run test:all — exit 0; frontend 227 files / 4794 tests, matching baseline exactly
• bun run quality:fix — clean (0 errors, 0 warnings, clippy -D warnings clean)
• guidance_is_not_contaminated_by_eval_prompts passes with the new prompts
• Pre-push gate green, including test:e2e (25 tests)

Note on ids

11c2 is added; every pre-existing id is preserved, so baseline joins are unaffected and the runner reports the new one as NEW rather than silently dropping anything.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QaGX2LndVMehjJh4rXxQe5

[malibio]

Pragmatic Code Review — PR #2246

Review Type: Initial review (no prior reviews/comments on the PR)
Scope: commit d8c7dc94 only (b5c892b0 is an unrelated main commit that landed after branching)

---

Requirements Check (issue #2242)

Issue requirement	Status	Evidence
Step 1 — dump assembled prompt, confirm every depended-on fact is present	✅	scenario_11d_history_already_contains_the_link_it_asks_about and scenario_6_history_resolves_its_indirect_reference_directly drive real AiChatMessages through the production node_history_from_messages
Step 2 — hand-execute the ideal call against a live backend	✅	matrix_scenario_winnability.rs, 5 tests, real SqliteStore + NodeService + GraphToolExecutor
Step 3 — classify each as harness / fixture / capability	✅	All three classified FIXTURE, with the reasoning pinned in-tree rather than left in the issue
Leading hypothesis (template drops role="tool") evaluated	✅	Refuted, and the refutation is mechanically correct — see below

Verification I re-ran independently:

• cargo test -p nodespace-agent --test matrix_scenario_winnability — 5 passed
• cargo test -p nodespace-daemon --lib scenario_ — 2 passed
• cargo test -p nodespace-daemon --lib completed_create_schema — 1 passed
• cargo test -p nodespace-agent --lib guidance_is_not_contaminated_by_eval_prompts — passed
• bun run test:scripts — 129 pass, 0 fail

---

Code Review Summary

This is a genuinely good piece of investigative work, and the standard of evidence is well above what an eval-fixture change normally carries. The central discipline — pin the finding as an executable test rather than as prose in a closed issue — is exactly right, and it directly serves the codebase's recorded history of eval results that turned out to be measuring stale or self-defeating fixtures.

I set out to break the refutation and could not. Three of the four substantive claims verify end-to-end against the real source; the fourth (11d's new prompt) is honestly self-labelled as a weaker guarantee. There is one real bug fix here that is worth more than the fixture work, and it is correctly diagnosed.

Findings below are all non-blocking. My one genuine reservation is a coverage question on scenario 6, discussed at length rather than dismissed.

---

Findings

🟢 Verified: the refutation is sound

I traced the mechanism rather than trusting the narrative.

📁 packages/daemon/src/services/local_agent_service.rs:2255 — node_history_from_messages does drop tool-role messages via _ => return Vec::new(). But for an assistant turn it substitutes terse_assistant_facts(&m.completed_writes) for the turn's own prose, and then appends completed_writes_message as a Role::System record. So each write reaches the next turn through two independent channels.

📁 local_agent_service.rs:2093 — terse_write_fact's create_node arm appends (id {id}) and inlines field_values via terse_value. That is what makes scenario 6's "five-day one" a direct string match: estimated_days 5 and nodespace://fw1 are both literal text in the prompt.

📁 local_agent_service.rs:2217 — create_relationship has no arm in terse_write_fact, so it falls through to format!("Fact: {} completed (\"{s}\").", w.tool), and summary for that tool is built at local_agent_service.rs:1949 as from -[type]-> to. Both endpoint ids and the relation name survive.

The conclusion holds: 11d's old prompt was answerable from history without touching the graph, and the TOOL_STRATEGY_RULES reading follows. The !history.iter().any(|m| matches!(m.role, Role::Tool)) assertion is a nice touch — it forecloses the alternative explanation that tool history simply survived.

🟢 Verified: the schemaId bug was real, and the fix is correct

📁 packages/core/src/schema/mod.rs:304-306 — CreateSchemaOutput carries #[serde(rename_all = "camelCase")]; SchemaUpdateOutput at :562-564 likewise.
📁 packages/core/src/schema/mod.rs:~475-490 — handle_create_schema returns serde_json::to_value(&output), so camelCase is genuinely what reaches the wire.
📁 packages/agent/src/local_agent/tools.rs:2975-2978 — exec_create_schema passes that value straight into ok_result, which becomes ToolExecutionRecord.result.

So the old r.result.get("schema_id") matched nothing on every production schema write, and terse_write_fact's create_schema arm silently fell to the None => format!("a schema named '{name}'") branch — dropping the normalized id that the next turn's create_node must copy into node_type. Correctly identified as affecting most multi-turn matrix chains.

I mutation-tested the new guard: removing the .or_else(|| r.result.get("schemaId")) line makes completed_create_schema_write_captures_the_schema_id fail. It is not a vacuous test. (Working tree restored clean afterwards.)

The assert!(result.get("id").is_none(), "premise broken: ...") guard is the right instinct — it prevents the test from quietly becoming tautological if create_schema later grows a top-level id.

🟡 [Improvement] The snake_case fallback is dead code under CLAUDE.md's no-backward-compat rule

📁 packages/daemon/src/services/local_agent_service.rs:1938

.or_else(|| r.result.get("schemaId"))
.or_else(|| r.result.get("schema_id"))

The justification given in the comment is "the snake_case fallback costs nothing and keeps any hand-built or externally-sourced result working." I'd push back on both halves:

• There is no externally-sourced result. ToolExecutionRecord.result is populated exclusively from exec_create_schema/exec_update_schema, which serialize the camelCase structs. The only producer of a snake_case key would be a hand-written test fixture — and the PR's own new test deliberately avoids being one, serializing the real struct instead.
• CLAUDE.md is unusually explicit here: "NO backward compatibility code — delete old patterns immediately when replaced", and "If you find yourself writing 'for backward compatibility' ... STOP." A fallback whose stated purpose is keeping an older spelling working is that pattern.

The concrete cost isn't cycles, it's that the line encodes a false claim about the wire format and gives the next reader a reason to believe schema_id is a legitimate spelling. Recommend deleting the snake_case arm.

🟡 [Improvement] Scenario 6 vs 9: two scenarios now sharing one assertion, and 6 lost its distinguishing coverage

📁 scripts/eval/fixtures/agent-matrix.ts:531-537 (6) and :588-593 (9)

The change from toolSequence [resolve_query, update_node] to toolOnce update_node is well justified as far as it goes — the winnability test proves the direct update is accepted and reports a persisted property, and the history test proves there is no indirection left to resolve. Scoring a correct end state red for taking a shortcut the fixture itself opened up is measuring the fixture. I agree with the direction.

What I want to flag is the aggregate effect, which the PR is admirably candid about but does not resolve:

1. toolSequence is now unused by any live scenario. Grepping scripts/eval/fixtures/agent-matrix.ts shows the kind survives only in its type definition (:96), its assertExpectation case (:348), and its unit tests. No scenario in GROUPS uses it. The entire multi-step-decomposition assertion capability is now unexercised by the matrix. That is a larger hole than "scenario 6 no longer measures indirect references."
2. 6 and 9 are now near-duplicates. The fixture comment at :547-559 is honest about this — "a red here and a red on 6 now mean the same thing, and should be read as two samples of one behavior rather than two behaviors." But 10b (:720) is a third toolOnce update_node, minProperties: 1. Three scenarios, one assertion shape; the matrix's effective breadth shrank more than the diff's size suggests.

I don't think this blocks the merge — the alternative is keeping an assertion that provably reds out correct models, which is strictly worse, and the PR states the cost rather than hiding it. But "Measuring it properly needs a chain built for it" is a commitment with no home. Recommend filing a follow-up issue for an indirect-reference chain whose referent is not recoverable from history (the natural design: have the setup turn write via a path that produces no terse fact, or put enough turns between setup and reference that history compaction drops the particulars). Without that issue, the honest cost-statement in the comment becomes the permanent record of a gap nobody owns.

🟡 [Improvement] 11d's new prompt is honest about being a weaker guarantee — and I think it's acceptable, with a caveat

📁 scripts/eval/fixtures/agent-matrix.ts:948-956, 963

The reasoning is right that a set is the one thing two scattered Fact: create_relationship completed lines do not state, and that adding 11c2 is a sound mechanism — it buys a second data point on create_relationship from a different source type at no extra assertion cost, which is a genuinely economical way to spend a turn.

But the admission is load-bearing: "A model that answers from history can still get it right by counting." Two lines is a very short count. A capable model reading two mentions facts and replying "the rebuild task and the caching note" in prose scores red on minCalls: 1 while being correct. That is the same failure mode this PR exists to eliminate — it has been narrowed from "trivially answerable" to "answerable with one extra inferential step," not removed.

I'd still merge this, because it is strictly better than the prompt it replaces and the limitation is documented at the point of use rather than buried. Two things would strengthen it cheaply:

• The comment argues re-wording alone cannot fix this because "the terse fact for each created node states its title, its type AND its id." That is true of create_node, but 11d asks about edges, and create_relationship has no terse_write_fact arm. A prompt asking for something derived from edge state at read time rather than edge creation events — e.g. asking about a link that a later turn removed or re-pointed — would be genuinely unanswerable from an append-only fact log. Worth considering if this scenario reds out again.
• Consider whether minCalls: 1 should be paired with a note in the runner output, since a prose-with-correct-content failure and a prose-with-wrong-content failure will look identical in the matrix.

🟢 8e still measures what its title claims

📁 scripts/eval/fixtures/agent-matrix.ts:673

New prompt: "List every decision we've recorded about how the system is built." Checked against 8a's defining prompt (:610-611, "the calls we make on how the system is built") — the rewording deliberately reuses 8a's own defining words instead of the demonstrative, which is the right fix for a referent that pointed three turns back past 8d's Planning Cycle.

The title "Query across types" survives: two custom types exist (:599, :614), each with a seeded instance (:628, :634), and scenario_8e_ideal_cross_type_read_is_accepted_and_discriminates proves a type-filtered read returns the decision and not Harbour. The discrimination is the cross-type content; the assertion stays toolOnce. The fixture's own note (:670-672) explaining why it does not ask about both types at once (two type-filtered reads would be a legitimate shape that toolOnce would wrongly red) is a good catch and the right call.

One residual: the prompt says "decision", and 8a's type is created from a prompt saying "calls." The model has to have named the type something matching "decision" for the retrieval to land. This is likely fine — the winnability test seeds it as Architecture Decision and the model naming it from "the calls we make on how the system is built" is exactly what 8a scores — but it is a soft coupling between 8a's naming choice and 8e's phrasing that did not exist when 8e used a demonstrative. Worth knowing if 8e reds out again.

🟢 Test quality is high, with real negative controls

The winnability tests avoid the classic trap: call() asserts !result.is_error with a message that names the inference ("the scenario is unwinnable by construction rather than by model capability"), so a rejected ideal call fails loudly rather than being swallowed.

Specific things done right:

• scenario_11d_traversal_without_an_explicit_relation_uses_the_documented_default pins the mentions default. This is the highest-value test in the file — it catches a drift that would make 11d unwinnable for the most likely spelling while the explicit-argument test kept passing, i.e. a silent fixture defect reported as a model failure. Exactly the class of bug this whole PR is about.
• scenario_6_ideal_update_is_accepted_and_persists_the_state_change asserts on property_count >= 1 rather than the updated: true flag, with a comment explaining that the flag-true/count-zero shape is precisely what minProperties exists to catch. Not a vacuous assertion.
• scenario_8e_..._discriminates has a genuine negative assertion (!text.contains("Harbour")), not just a positive one.
• Seeding goes through the real tool path rather than NodeService directly, so the nodespace:// id spelling matches what a real turn would see.

The one soft spot: scenario_11d_one_traversal_enumerates_every_inbound_link asserts ids.len() >= 2. Since the seed creates exactly two inbound edges, == 2 would be the tighter assertion and would also catch a traversal that started returning spurious nodes. Minor.

🟡 [Improvement] The new test is inserted above use super::*;

📁 packages/daemon/src/services/local_agent_service.rs:2353-2417

completed_create_schema_write_captures_the_schema_id was placed at the very top of mod tests, before the use super::*; and sibling imports at :2417. It compiles (Rust doesn't care about item order), but every other test in this module sits below the import block, and a reader scanning for "where do the tests start" will land on the imports and assume nothing precedes them. Move it below the use block, next to the other completed_writes_from tests.

🟢 CLAUDE.md compliance

• No lint suppression, no #[allow(dead_code)], no #[deprecated], no any types.
• No docs/ directory or stray .md files added — correct.
• Issue references in fixture comments (#2242, #1846, core#2234): I checked git show origin/main:scripts/eval/fixtures/agent-matrix.ts — #1846 and core#2234 were already in this file before the PR. CLAUDE.md's prohibition is scoped to ../nodespace-docs/ content; this is a code comment in a repo file, and the practice is pre-existing and consistent. Not a finding.
• The only rule genuinely in tension is no-backward-compat, covered above under the snake_case fallback.

🟢 Fixture id stability

11c2 is additive; every pre-existing id is preserved, so baseline joins hold and the runner reports the new scenario as NEW. Correct call — renumbering would have silently invalidated historical comparisons, which matters a lot given this codebase's recorded history of stale eval baselines.

---

Nitpicks

• Nit: 📁 scripts/eval/fixtures/agent-matrix.ts:946 — the comment reads "and 11c records a second edge for the set to be non-trivial." It is 11c2 that records the second edge; 11c records the first. The commit message and PR body both get this right, so it's a typo in the one place a future reader is most likely to consult. Worth fixing since the entire justification for 11d's new prompt hinges on there being two distinct edge-recording turns.
• Nit: 📁 packages/agent/tests/matrix_scenario_winnability.rs:~350 — ids.len() >= 2 could be == 2 given the seed creates exactly two inbound edges.
• Nit: The comment volume in agent-matrix.ts is now very high (~120 lines of commentary for ~8 lines of scenario data on 11d alone). I'm not going to argue against it — the comments explain why, they encode findings that were expensively derived, and this codebase has been burned repeatedly by re-deriving conclusions someone already reached. But it is approaching the point where the fixture data is hard to see. If it grows further, consider whether the audit narrative belongs in a doc in ../nodespace-docs/ with the fixture carrying a pointer.
• Nit: 📁 scripts/eval/fixtures/agent-matrix.ts:96, 348 — with no live scenario using toolSequence, its assertExpectation branch is now exercised only by unit tests. Not dead code (it's a supported expectation kind), but flagging it as the mechanical symptom of the coverage gap above.

---

Recommendation

APPROVE

The investigation is rigorous, the refutation of the issue's leading hypothesis verifies against the real source rather than resting on assertion, and the schemaId fix is a genuine production bug — affecting every multi-turn chain that creates a type then records an instance against it — that would likely not have been found any other way. The evidence is pinned as executable tests that fail when the behavior regresses, which I confirmed by mutation-testing the schema-id guard.

The trade-offs are stated in the code rather than hidden, which is the standard I'd want held here. This is a clear net improvement to code health.

Nothing blocks the merge. Before or shortly after merging, I'd like to see:

1. The snake_case schema_id fallback deleted (per no-backward-compat).
2. A follow-up issue opened for the indirect-reference-decomposition coverage that scenario 6 gave up — otherwise the honest cost-statement in the fixture comment becomes an unowned gap, and toolSequence stays unexercised by any live scenario.
3. The 11c → 11c2 typo at agent-matrix.ts:946 corrected.

[malibio]

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

[malibio]

Review addressed — commit 491dc1a6

Thanks for the review, and particularly for mutation-testing the schema-id guard rather than taking the test at face value.

Addressed (5)

🟡 Deleted the snake_case fallback. You were right on both halves, and acting on it surfaced something better than the fix itself.

There was a producer of the snake_case spelling: the hand-written result fixture in packages/daemon/tests/golden_scenario6_real_pipeline.rs:120. That fixture and the old lookup agreed with each other and disagreed with production — which is why real_pipeline_renders_terse_facts_not_narrative_prose passed on main while the real pipeline silently rendered the id-less phrasing. The test was green for the wrong reason, validating the renderer against a shape production never emits.

Fixed the fixture to the real wire shape (schemaId/isCore) rather than restoring the fallback. Its assertion that the schema id must reach the terse fact was always correct; only the key was stale. This independently corroborates the bug — and it means the schemaId fix now has a second test covering it through the real pipeline, not just the unit test.

I checked the other "schema_id": occurrences in the tree: all are update_schema input params, where snake_case is correct (UpdateSchemaParams has no rename attribute). The golden fixture was the only result-shaped one.

🟡 Filed the follow-up issue — #2248, for an indirect-reference chain whose referent is not recoverable from history. It records why scenario 6's assertion had to go, that toolSequence is now unexercised by any live scenario, three candidate designs (out-of-band seeding preferred; history-compaction timing explicitly discouraged as it couples to maybe_summarize_history tuning), and a note that #2245's end-clause model should probably land first. The fixture's cost-statement now points at it, so the gap has an owner rather than being a permanent comment.

🟡 Moved the test below use super::*;, with the other completed_writes_from tests.

Nit — 11c → 11c2 at agent-matrix.ts:946. Agreed this was the one that mattered; it's the line a reader consults to check whether 11d's justification holds.

Nit — ids.len() >= 2 → assert_eq!(.., 2), so the test also catches a traversal returning spurious nodes, not just one returning too few.

Skipped (2)

11d redesigned around edge state at read time, and a runner note separating prose-correct from prose-wrong failures. Both are framed in the review as "worth considering if this scenario reds out again." It hasn't been re-measured since the reword, so acting now would be scope creep on a scenario whose current behavior is unknown. Your observation that create_relationship has no terse_write_fact arm — and that edge state is therefore genuinely unrecoverable from an append-only fact log where creation events are not — is the sharpest idea in the review, and I'd rather it inform a designed change than a speculative one. It's captured in #2248's framing.

Moving the audit narrative to ../nodespace-docs/. The review declines to argue for it, and that repo holds decisions and architecture rather than audit findings. I'd rather the reasoning stay adjacent to the data it explains. Agreed it's near the ceiling — noted for whoever adds the next scenario.

Verification

• bun run test:all — exit 0; frontend 227 files / 4794 tests, matching baseline
• bun run quality:fix — clean
• Pre-push gate green

Note that the first test:all after deleting the fallback failed, which is how the masked fixture was found. Worth recording since it's the second time in this PR that removing something inert exposed a test passing for the wrong reason.

Still open: the #2245 overlap

Flagging again as it's the one thing that should gate merge order. #2245 (issue #2243) is open against the same fixture file and fixes scenario 6 better than this PR does — it keeps expect as a diagnostic and adds a path-agnostic end clause, where I removed the trajectory signal outright.

But it gives 8e and 11d only end: { expectNoWrites: true }, which both pass trivially by calling no tools — exactly what they do today. So the two PRs are complementary: #2245 fixes the scoring, this one fixes the prompts. They will conflict textually.

Suggested order: merge #2245 first, then rebase this to keep the 11d/8e rewordings and 11c2, drop the scenario-6 assertion change in favour of #2245's end clause, and give 11d an end clause with teeth.