Repository navigation
Re-theme the agent matrix around NodeSpace's dev-workflow domain - #2232
Conversation
#1977) All 16 scenarios were generic personal CRUD — equipment checkouts, album and venue trackers. Model-lock decisions (ADR-046, ADR-056) and every model re-evaluation are scored off this matrix, so native-model selection was being decided by a model's ability to track laser cutters rather than to do the job the product claims: context infrastructure for AI-native development. Re-themed IN PLACE rather than added as a second fixture. The problem was that the bar itself was measured in the wrong domain, and a second fixture alongside leaves the generic one equally citable while doubling a manual eval that costs minutes of real inference per run. The generic set's value was its tool MECHANICS, and those live in the Expectation model (noExtraTypes, minProperties, noRetry+minCalls, toolSequence), not in the vocabulary — so re-theming keeps every one of them. Every pre-existing scenario id is preserved, so a baseline recorded before this change still joins against a run after it: a domain-driven score change reads as a change rather than as 16 removals and 16 additions. New coverage: relationship traversal (11a-11d, new ids). Every existing group acts on a node's OWN fields, so nothing measured whether the model can record a link between two nodes or follow one back — the half of the data model the product's framing rests on, and never exercised end to end despite create_relationship and get_related_nodes both being registered tools. Both are reachable whether routing matches Relationship Management or falls open, since neither requires routed guidance. Winnability was the binding constraint on wording: each chain builds its own schema in its first scenario, and every later prompt names only values that schema can hold. A prompt asking for a field the type has nowhere to put is unwinnable — the model degrades reasonably and scores red for it, and the fixture then measures itself rather than the model. Dev vocabulary RAISES contamination risk rather than lowering it (the seeded skill descriptions already name Spec, ADR and Ticket), so every prompt was checked against guidance_is_not_contaminated_by_eval_prompts rather than assumed clear. It passes, and the guard's parser sees all 21 prompt sites — it fails open on a literal shape it cannot parse, so that was verified rather than inferred from the pass. Also adds five fixture-level invariants, covering properties the eval's validity rests on that no per-turn assertion can see: id uniqueness, that the carried-over ids survived the re-theme, that no scenario targets a tool actionTools() strips (unwinnable by construction — and create_relationship / get_related_nodes are the first target tools added since ROUTING_TOOLS was written), that minProperties is never asserted on create_relationship (its payload carries no schema field values, so fieldCount does not apply), and that score() reads each scenario's own expectation. Each was mutation-tested to confirm it fails on a real break rather than passing vacuously. Baseline held: frontend 227 files / 4794 tests, scripts 115 -> 120 (the five new invariants), Rust green. Docs: ../nodespace-docs/development/agent-eval.md gains a "Scenario domain" section recording the replace-vs-add decision and the winnability constraint. Co-Authored-By: Claude <noreply@anthropic.com>
Pragmatic Code Review — PR #2232Review Type: Initial Review (no prior reviews or comments on the PR) Scope: 2 files, +297/−53. SummaryThis is a net positive and should merge. The central argument is correct and well-evidenced: the matrix is the scoring surface for model-lock decisions, so its domain is part of the measurement rather than decoration, and re-theming in place (rather than adding a second fixture) is the right call for exactly the reason given — two half-signals are worse than one when single-run matrix numbers are already not decision-grade. The quality bar in the comments is unusually high, and I verified the load-bearing claims independently rather than taking them on trust. Every one I checked held:
Findings below are refinements, not blockers. Two are worth acting on before this becomes the bar that model decisions are read off. Requirements Check (issue #1977)
All six met. Code Review Findings🟡 Important: 11c scores green on a rejected
|
malibio
left a comment
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval on your own PR). Full findings in the review comment above.
All 6 acceptance criteria from #1977 are met. Independently verified rather than taken on trust: bun run test:scripts 120/120, cargo test -p nodespace-agent guidance_is_not_contaminated passes with coverage confirmed non-vacuous (21 sites / 21 parsed, max shared run 3 — nothing at the limit of 4), all five new invariant tests fail under targeted mutation, both new relationship tools confirmed reachable via the seeded Relationship Management whitelist and the fail-open surface, and chain 3→9 winnability checked prompt-by-prompt against its implied schema.
Three 🟡 items, none blocking: 11c scores green on a rejected create_relationship (wider than the comment's stated gap — isError is available but unchecked); 11d's noRetry reds out a legitimate two-endpoint traversal; and 8c's minProperties assertion rests on a schema 8a never specifies (inherited from the old fixture, but harder to guess in the new domain). Follow-up-issue material if you prefer to land and iterate.
Addresses the two 🟡 findings and two 🟢 nits from the PR #2232 review. 🟡 A REJECTED TARGET TOOL SCORED GREEN `isError` was only ever inspected on two narrow paths — `schemaCallsAreSound` (create_schema only) and `callPersistedProperties` (only when a scenario opted into `minProperties`). Every other scenario scored a rejected call green on tool name alone: 5, 7, 8d, 10a, 11a, 11b, 11c, 11d. That is not hypothetical for the scenario it matters most to. create_relationship takes two node ids the model must recover, and two invented ids are rejected outright — so the one scenario added to measure linking would have reported success on precisely the failure it exists to catch. Adds `targetToolWasNotRejected`, applied to toolOnce, toolSequence and noRetry. `minProperties` keeps precedence where a scenario opted in, since it inspects the same flag and returns a strictly more specific diagnosis. Only the TARGET tool is judged — a model that tries a bad search, recovers, and then does the right thing has still done the right thing. The first live run of the new matrix immediately caught a real instance: scenario 7's search_nodes fired and was REJECTED, which would have scored green before this change. 🟡 8c WAS WINNABLE ONLY BY LUCK 8c asserts `minProperties: 1` on whose call it was, but 8a named no fields at all, leaving it to chance whether the created type had anywhere to put that value. The sibling chain already gets this right — scenario 3 explicitly names the fields its downstream scenarios key on. 8a now names the field 8c needs. Contamination margin re-measured rather than assumed: with MAX_SHARED_RUN temporarily lowered to 2, the new wording does not appear among the hits at all, so it sits at <=2 against a limit of 4. 🟡 11d's noRetry justification was wrong (comment only) The comment claimed "the same reason 10c uses it". False: there a second search_nodes is a genuine blind retry, whereas a second get_related_nodes could be a legitimate walk of the other endpoint of a bidirectional edge. The expectation is unchanged — this prompt asks about one endpoint, so one traversal is the expected shape — but the comment now states the real reasoning and tells a future reader to check the trace for the two-endpoint shape before recording a red here as a model failure. 🟢 create_schema was uncovered by the routing-strip invariant `noExtraTypes` names no tool in its payload, so scenario 3 contributed no targets. If create_schema ever landed in ROUTING_TOOLS, scenario 3 would become unwinnable and the test written for exactly that regression would not catch it. 🟢 score() wiring test verified 2 of 20 scenarios The reviewer confirmed a score() that ignored `scenario.expect` and read from a hardcoded id table passed all 120 tests. Now loops over every scenario, synthesizing the turn its own expectation demands and a wrong turn per kind (chosen per kind because bare noRetry legitimately passes on zero calls). The hardcoded-table mutation now fails, verified. Not addressed: the guard parsing this file's own doc comment as a phantom prompt (harmless — it parses and clears the threshold; the fix belongs in the guard's preceding-char set, not this fixture) and the stale old-domain example in seeded guidance (out of scope for this PR). test:scripts 120 -> 127, expect() calls 241 -> 279. Guard passes. Co-Authored-By: Claude <noreply@anthropic.com>
Ran the re-themed matrix end to end against E4B on a purged, isolated database.
10/19 passed. Triaging each failure against its trace found one fixture defect
that accounted for the entire relationship group's failures.
11a opened with "Note that we settled on ...", which reads as a preamble to a
statement rather than a request to record one. The model called search_semantic
looking for an existing note and replied "Found 0 nodes matching your request"
— a reasonable reading of the words it was given, scored red.
That cascaded through the rest of the group. With no node created, 11c's
create_relationship went out with `to_id: null` and was rejected:
create_relationship is_error=true
args={"from_id":"nodespace://b6f6…","relationship_type":"related_to","to_id":null}
and 11d had no edge to traverse. All three of the group's failures trace to this
one word choice — the same ambiguous-verb correction scenario 4 already needed,
missed here.
Deliberately NOT changing anything else on the strength of one nondeterministic
run:
- 6 and 9 both failed asking for an id they could not have, because scenario 4
— which creates the chain's subject — was excluded as a degenerate empty
generation. Their raw output shows create_node was correctly formed before
the generation died, carrying both particulars, so the wording is sound and
the cause was inference, not the prompt.
- 8e drew a clarifying question rather than a search. Arguably correct on a
vague referent; borderline, and worth a second run before touching.
Tuning the fixture to one noisy run is the failure mode this issue exists to
prevent.
The run also validated the rejection fix from the review. Scenario 7's
search_nodes emitted malformed JSON and the model then hallucinated a tool
response, inventing a node id and reporting it to the user as real. That scored
GREEN before `targetToolWasNotRejected` — the first live scenario able to
exhibit the blind spot did exhibit it.
The relationship group remains effectively unmeasured: every one of its results
is explained by the 11a defect, so it needs a clean re-run before anything can
be said about linking behaviour.
Co-Authored-By: Claude <noreply@anthropic.com>
The first live run of the relationship group looked like a model failure and
was not. After fixing 11a's verb and re-running, 11c still failed — but the
model had done everything right:
create_relationship is_error=true
args={"from_id":"1fb911ba-…","relationship_type":"related_to","to_id":"c26d…"}
error: Validation failed: Relationship 'related_to' not defined in schema
'task'. Built-in relationships (member_of, has_child, mentions,
has_role) are universal.
Two real ids, recovered without being handed either, correct direction. The
storage layer refuses the write because create_relationship only accepts a
relation defined on the SOURCE node's schema plus four universal built-ins.
And create_relationship's own tool description (tools.rs:1055) instructs the
model to use exactly what the validator refuses:
"Use a name defined on the relevant schema(s) … otherwise a generic label
(member_of, mentions, related_to, etc.)."
`related_to` is offered as a valid generic fallback and is not accepted. The
model was rejected for following its instructions. That made 11c unwinnable by
construction — no wording of "link these two" could succeed for an ad-hoc
relation between a task and a text node — and 11d unwinnable in turn, since
there was never an edge to traverse.
11c now asks for a link the system can actually record ("Point that rebuild
task at the decision it has to respect"), which maps onto `mentions`.
Winnability PROVEN against the real store rather than argued, on the exact two
nodes the scenario produces:
$ nodespace relationship create --from ae0c2666-… --type mentions --to c26d…
Created relationship: ae0c2666-… --[mentions]--> c26d6108-…
$ nodespace relationship get ae0c2666-… --type mentions
1 related node(s): { "content": "Reports page uses server-side rendering", … }
The edge writes and the traversal returns the decision node, so both halves of
the group are passable in fact. Re-running 11c with the new wording, the model
called update_node — writing the link into a field instead of recording an edge
— which is a legitimate red, and precisely the failure the scenario exists to
catch.
The underlying affordance gap (a documented generic label the validator
refuses) is a product bug, not an eval bug. Out of scope here; filing separately.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
First live run of the new matrix — 10/19, with a fixture-defect triageRan the re-themed matrix end to end against E4B on a purged, isolated DB ( Result: 10 passed / 9 failed / 1 excluded (empty generation). The number itself is not the point and is not decision-grade — single-run matrix numbers never are (#1862). The point was to find scenarios that red out correct behaviour before this fixture becomes the bar. Classifying each failure against its trace: 🔴 Fixture defects — my bugs, not the model's (3)11a — ambiguous verb, exactly the class I fixed in scenario 4 and missed here. 11c — cascade from 11a, not an independent failure. The daemon log is unambiguous:
11d — cascade from 11c. No edge was ever recorded, so there was nothing to traverse. The model asked a clarifying question instead. My comment already documents that 11d is deliberately independent of 11c's success, and that holds — but it is not independent of 11a having produced a node at all. Notably the two-endpoint false positive I documented as 11d's known risk did not occur — 🟡 Winnability worth a second look (2)6 and 9 — both died the same way, and it is not the referent design:
The model wanted an id it had no way to have. Scenario 4 — which should have created that node — was the excluded empty generation, so the chain's subject never existed. So 6 and 9 were unwinnable in this run for an upstream reason. Worth noting the raw output shows 4's That is a well-formed call carrying both particulars — evidence the chain's wording is sound and that this was an inference failure, not a prompt defect. 8e — "Run through those calls for me" drew a clarifying question rather than a search. Arguably correct behaviour on a genuinely vague referent; the old wording ("Run through the albums for me") named a concrete type. This one is borderline and I'd want a second run before changing it. ✅ Real model failures the fixture correctly caught (3)3 — five 8b — 7 — the most valuable catch, and it validates the review fix. The model emitted malformed JSON: then hallucinated a tool response, inventing a node id ( Before the Re-run after fixing 11a — and 11c turns out to be unwinnable by constructionFixed 11a's verb ("Log a decision: the reports page uses server-side rendering") and re-ran the group on a fresh isolated daemon. 11a and 11b now create their nodes correctly. 11c still failed — but for a completely different reason, and this one is not the model's fault: The model did everything right: two real ids, recovered without being handed either, correct direction. The storage layer rejected the write because And the tool's own description tells the model to do exactly what was rejected (
So 11c as written is unwinnable by construction — an ad-hoc link between a This is the single most valuable thing the run surfaced, and it is a product bug, not an eval bug: a documented tool affordance that cannot succeed. It is out of scope for this PR to fix, and I'll file it separately. I am not touching 6, 9 or 8e on the strength of one run. 6 and 9 have a fully-explained upstream cause (an inference failure, not a wording problem) and their raw output shows the intended shape; changing them now would be tuning the fixture to one noisy run, which is the failure mode this whole issue is about. 11c reworded, and winnability PROVEN rather than arguedReworded 11c to ask for a link the system can record: "Point that rebuild task at the decision it has to respect", which maps onto I did not want to claim winnability twice on reasoning alone, so I proved it against the real store, on the exact two nodes the scenario produces: The edge writes and the traversal returns the decision node. Both halves of the group are passable in fact. Re-running 11c with the new wording, the model called Honest limits
|
Address-review summaryAll findings triaged. 4 addressed, 2 skipped with reasons, plus 2 fixture defects the first live run exposed (reported separately above). ✅ Addressed🟡 A rejected target tool scored green — Confirmed the reviewer's reproduction before fixing, and the first live run then caught a real instance on scenario 7: the model emitted malformed JSON, the search was rejected, and it hallucinated a tool response inventing a node id ( 🟡 8c winnable only by luck — 8a now names the field 8c keys on, matching scenario 3's pattern. Contamination margin re-measured rather than assumed: with 🟡 11d's 🟢 🟢 ⏭️ Skipped🟢 The guard parses this file's own doc comment as a phantom prompt. Harmless — it parses and clears the threshold, so the guard is not fail-open. The fix belongs in the guard's preceding-character set (a backtick should disqualify a 🟢 Stale old-domain example in live seeded guidance. Out of scope: it is guidance text, not eval fixture, and editing seeded guidance in this PR would change what the eval measures while this PR is establishing the new scenario set. Better done deliberately, separately. Also fixed — found by running the eval, not by the review11a's ambiguous verb and 11c's unwinnability ( Verification
Re-Review DecisionDecision: RE-REVIEW NEEDED Rationale: The scoring logic changed — |
The rejection check added in the previous commit looped over every call to the
target tool and failed the scenario if ANY was rejected. That contradicts
`noRetry`'s own semantics, which deliberately tolerate several non-adjacent
calls: a first call rejected for a malformed argument, corrected by the model
and retried successfully, is a turn that accomplished what the prompt asked for.
Verified against the shape observed live on 11a — create_node rejected for a
missing node_type, then supplied and persisted:
noRetry, err-then-ok BEFORE: red ("fired but was REJECTED")
AFTER: green
A rejection now only sinks the turn when NOTHING to that tool ever succeeded,
which is the case the check exists for: the tool name appears in the trace and
the turn still ends with nothing written. The failure message reports how many
calls were rejected when there was more than one.
11a itself still reds, on `toolOnce`'s pre-existing exactly-once rule. That is a
separate and deliberate judgement about write tools and is not this helper's
call to make.
test:scripts 127 -> 129.
Co-Authored-By: Claude <noreply@anthropic.com>
Pragmatic Code Review — PR #2232Review Type: Re-Review (2nd pass) Previous Review Summary
Every claim in the address-review summary and the eval-run report was verified independently rather than taken on trust. All of them hold except one, which is the 🟡 below. Verified addressed:
Skipped — both reasons are sound, no disagreement:
Requirements Check (issue #1977)
Five unambiguously met; AC 6 met in letter but the doc is now behind the code. Code Review Findings🟡 Important: the rejection check reds out a correct self-correction on
|
malibio
left a comment
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic code review (RE-REVIEW): APPROVE, conditional on one commit — posted as a comment because GitHub blocks self-approval on your own PR. Full findings in the review comment above.
Reviewed 2228e5b2...61e9319a (4 commits). Of the 7 prior recommendations: 5 addressed, 2 skipped with sound reasons, 0 outstanding. Both skips are the right call. All 6 acceptance criteria still hold (AC 6 met but the doc is now slightly behind the code).
Independently verified rather than taken on trust: the targetToolWasNotRejected mutation (3 tests fail when forced vacuous), the prior review's own score() id-table mutation (now caught, where it previously passed all 120 tests), guard coverage re-derived at 21 sites / 21 parsed, contamination margins re-measured at MAX_SHARED_RUN=2 (no changed prompt appears), all 20 scenarios run through both branches of the strengthened loop, and the #2234 affordance gap read directly out of relationship.rs / tools.rs:1055.
One blocker, and it is bookkeeping rather than engineering. The new rejection check loops every call to the target tool and fails on the first isError regardless of whether a later call succeeded. On noRetry — which deliberately tolerates repeated non-adjacent calls — that reds out correct self-correction:
10c: search_nodes[ERROR] -> resolve_query -> search_nodes[ok] -> FAIL
11d: get_related_nodes[ERROR] -> get_node -> get_related_nodes[ok] -> FAIL
Both accomplished what the prompt asked for. The live 11a trace has exactly this shape.
The fix for it already exists in the worktree, uncommitted — gating on own.some((c) => !c.isError), plus tests for both halves. I verified it: the false positives flip to pass, the scenario-7 hallucinated-response catch and the 11c hallucinated-ids catch both still red, test:scripts goes 127 -> 129 / 0 fail. It is correct and strictly better than what is committed. Commit and push it and this is ready to merge.
Two 🟢 follow-ups, neither blocking: 11c's rewording dropped every lexical link-cue the Relationship Management skill retrieves on (still reachable — create_relationship is not requires_routed_guidance — and I agree the live update_node result is a legitimate red, so do not change the prompt on one run); and the docs' Winnability section doesn't yet carry the relationship-validator constraint that made 11c unwinnable.
Running the eval live before merging, and treating the fixture defects it surfaced as defects rather than model failures, is exactly right for a fixture that model-lock decisions get read off. The 11c diagnosis is the strongest part of the round.
Not merging — that is your call.
Clean shakedown run — against the exact merge candidateRan the full matrix once more on Result: 10 passed / 8 failed / 1 excluded.
The thing this run was for: no scenario is unwinnable any more11a and 11b now pass. The relationship group has produced a real measurement for the first time — previously every one of its results was an artifact of the ambiguous verb or the And 11c/11d's failures are legitimate reds, verified from the traces rather than assumed: 11c — the model called
It expressed the link as a field write rather than an edge, and claimed success. That is exactly the failure this scenario was built to catch — the same create_relationship-vs-update_node choice 11d — the model found the right node, by text search rather than traversal:
Right answer, wrong mechanism. This is precisely the "answering without reading the graph" failure the scenario targets, and it is only visible because the assertion names 8d — used Comparison to the earlier run
Same headline number, entirely different meaning. Before: three of the failures were my bugs. Now: every failure is model behaviour the fixture is designed to detect. What this run does and does not establishDoes: every scenario is winnable, and every failure is real model behaviour rather than a fixture artifact. That was the blocking question for merge, and it is answered. Does not: give a decision-grade number for E4B. One run of a nondeterministic model is not a baseline — scenario 4's empty generation alone shifts the denominator, 8d flipped between runs, and per #1862 single-run matrix numbers should not be cited for model-lock decisions. Re-running comparisons is the issue's stated non-goal and belongs after merge. Standing observations, not blockers
|
1 participant
Closes #1977.
The problem
All 16 matrix scenarios were generic personal CRUD — equipment checkouts, album and venue trackers. Model-lock decisions (ADR-046, ADR-056) and every model re-evaluation are scored off this matrix, so native-model selection was being decided by a model's ability to track laser cutters rather than to do the job the product claims.
Scope decision (AC 2)
Re-themed in place; not a second fixture. Rationale posted on the issue before implementing:
Expectationmodel, not the vocabulary. Re-theming keeps all of them.Every pre-existing scenario id is preserved, so a baseline recorded before this change still joins against a run after it — a domain-driven score change reads as a change rather than as 16 removals plus 16 additions. A unit test pins that.
What changed
task, still defined-but-unsetdue_date/status)New coverage
Every existing group acts on a node's own fields. Nothing measured whether the model can record a link between two nodes or follow one back — the half of the data model the product's framing rests on ("a decision means nothing without the work it constrains"), and never exercised end to end despite
create_relationshipandget_related_nodesboth being registered tools.Reachability was verified rather than assumed: both are whitelisted by the seeded Relationship Management skill, and neither
requires_routed_guidance, so they are offered on the fail-open surface too — winnable via either routing path.11cusestoolOnce, nottoolSequence, deliberately: the id lookup has three legitimate spellings (search_nodes,search_semantic,get_node) and pinning one would red out the other two. The known cost — it cannot tell a call with two real ids from one with two invented ids — is stated in the code rather than hidden; that needs the tool result, whichToolCallRecorddoes not carry today.Contamination (AC 3)
Dev vocabulary raises this risk rather than lowering it — the seeded skill descriptions already name Spec, ADR and Ticket. Every prompt was checked against
guidance_is_not_contaminated_by_eval_prompts, which passes.The guard fails open on a literal shape its parser cannot handle, so a pass alone is not evidence. Verified separately that it parses all 21
prompt:sites (sites == parsed), and re-verified after Prettier reformatting, since a prompt wrapped onto a second line would silently exempt itself.Scoring (AC 4)
No new scoring mechanism. Every scenario uses the existing
Expectationkinds;assertExpectationis unmodified.Tests (AC 5)
bun run test:scriptspasses: 115 → 120, the five new ones being fixture-level invariants that no per-turn assertion can see:actionTools()strips — unwinnable by construction, andcreate_relationship/get_related_nodesare the first target tools added sinceROUTING_TOOLSwas writtenminPropertiesis never asserted oncreate_relationship(its payload carries no schema field values, sofieldCountdoes not apply and it would fail every correct call)score()reads each scenario's own expectationEach was mutation-tested — I broke the invariant and confirmed the test fails — rather than trusting a green run.
Winnability
The binding constraint on wording. Each chain builds its own schema in its first scenario, and every later prompt names only values that schema can hold. A prompt asking for a field the type has nowhere to put is unwinnable: the model degrades reasonably and scores red, and the fixture then measures itself rather than the model (the trap documented on scenario 9 and in #1846).
Verification
bun run test:all— exit 0. Frontend 227 files / 4794 tests, identical to the baseline recorded on the issue. Rust green. Scripts 120/120.cargo build+ full e2e) passed.nodespace-docsgains a "Scenario domain" section (AC 6) recording the replace-vs-add decision and the winnability constraint — 7b0b920.Not done (per the issue's Non-Goals)
Re-running model comparisons against the new scenarios. The numbers on record were produced against the old domain and do not carry over — that is follow-up work now that the scenario set exists.
🤖 Generated with Claude Code