Repository navigation
Agent guidance contains verbatim aichat-matrix prompts — the eval is measuring recall of planted examples, not generalization - #1763
Conversation
…LEVANT ENTITY TYPES (closes #1760) The agent's compiled guidance contained verbatim prompt strings from scripts/aichat-matrix.ts alongside the exact tool call each should produce, so the eval could not distinguish generalization from recall of a planted example. Guidance (packages/agent/src/agent_guidance.rs): - Replaced the three memorized examples (scenarios 5, 6, 8d) and the "contacts database" echo of 8b with rules that teach the shape. - Removed the hardcoded node_type="invoice" literal. It was also simply wrong: schemas named from "an invoice tracking database" normalize to invoice_tracker, so the model copied the example and the lookup missed. Had the literal been correct, scenario 6 would have gone green while proving nothing. - Added NODE_TYPE COMES FROM RELEVANT ENTITY TYPES: node_type arguments must be copied exactly from the injected entity-types block. No rule previously stated where those values come from, which is the actual root cause behind scenario 6 rather than the bad literal alone. Eval (scripts/aichat-matrix.ts): - Reworded every scenario off the invoice/book/contact domains to re-establish an independent signal. Scenario IDs and expectations are unchanged so baseline diffing still works. Mechanical check: - guidance_is_not_contaminated_by_eval_prompts parses prompt literals out of both eval scripts at test time and fails if any shares more than four consecutive words with a guidance constant, catching near-verbatim reuse rather than only exact substrings. It reads the real files rather than a copy so it cannot silently drift, and asserts it parsed a non-empty prompt set so it cannot pass vacuously. Verified to fail on a re-planted example. Runs under rust:test, which the pre-push gate enforces. scripts/results-e4b.json still records 11/12 from a contaminated run and is left untouched; it needs re-recording against a running daemon before it is comparable, and ADR-056's scenario 5 claim revisited with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Pragmatic Code Review — PR #1763Review Type: Initial Review SummaryThis is a net-positive change that correctly diagnoses a subtle and genuinely damaging problem. Contaminated evals are worse than no evals — they produce confident numbers that measure the wrong thing — and the analysis in both the issue and the commit message is unusually sharp. The observation that scenario 6 failed because the planted answer was wrong, and that fixing the literal to The guard's design shows real care: reading the eval files at test time rather than keeping a copy, matching on shared word-runs rather than exact substrings, the vacuity assertion, and a pinned regression test against the original contamination. That is a higher standard than most anti-regression tests are held to. My concerns are about completeness of the guard's coverage, not its correctness. The central finding: the guard omits Requirements Check
Code Review Findings🔴 Critical1. The guard misses
|
| Input shape | Parsed | Consequence |
|---|---|---|
prompt: \Mark the $500 invoice as paid`` (template literal) |
(nothing) | 🔴 silently skipped — contamination invisible |
prompt: "She said \"hi\" now" (escaped quote) |
She said \ |
🟡 truncated → weakened detection |
prompt: "Mark the " + "$500 invoice as paid" (concat) |
Mark the |
🟡 truncated → weakened detection |
// prompt: "Mark the $500 invoice as paid" (commented) |
full string | 🟢 harmless false-positive direction |
prompt : "..." (space before colon) |
(nothing) | 🟡 skipped |
Every miss fails open (guard stays green while contamination exists), which is the wrong direction for a safety check. Template literals are the realistic one — a future author parameterizing a prompt (prompt: \Add ${n} invoices``) silently removes that scenario from the guard, and nothing signals it. The vacuity assertion does not help: it only requires the set to be non-empty, so losing 11 of 12 prompts still passes.
Recommendation: minimally, accept backticks in the quote match (one added char in the match) and count parsed prompts against the number of prompt: occurrences that look like literals, failing if they diverge. Stronger and cheaper long-term: have aichat-matrix.ts export its GROUPS and emit the prompt list as JSON via a tiny bun invocation or a checked-in generated fixture, so the Rust side consumes structured data instead of re-implementing a TS lexer. Hand-rolled parsers of another language's syntax drift by construction.
Principle: Safety checks must fail closed. When a parser cannot represent the full input grammar, assert on coverage (did I parse everything I should have?) rather than only on non-emptiness.
4. Scenario 6's rewritten prompt may no longer exercise resolve_query's reason for existing
📁 scripts/aichat-matrix.ts:247-251
Old: "Mark the $500 invoice as paid" → new: "The 2400 one came back — set it to returned", paired with instance-creation prompt "Log a laser cutter checked out on the 12th, replacement cost 2400".
This is a legitimately good rewrite for the indirect reference property the resolve_query rule describes — "a bare value such as an amount or code, without naming which field it belongs to" fits 2400 precisely, and it's arguably a harder, more honest test than the original.
Two risks worth weighing before treating the new score as the baseline:
- "set it to returned" presumes the schema the model invented in scenario 3 has a status-like field with a
returnedvalue. Nothing constrains that — scenario 3's prompt is"I want to keep a record of the equipment my team checks out", which does not imply a returned state. If the model's schema has no such field,update_nodemay fail for a reason unrelated to the search-then-write behavior scenario 6 asserts. The old prompt had the same coupling (paidon an invoice schema) but "paid" is far more strongly implied by "invoice tracking" than "returned" is by "equipment checkout". Worth checking on the first honest run whether failures here are behavioral or schema-shape artifacts. - "The 2400 one" is a bare number with no unit. For a small model, disambiguating it from "the 12th" in the prior turn is a genuine additional load. That may be fine — but note the eval is now measuring something slightly harder than before, so a score drop won't be cleanly attributable to decontamination alone.
Recommendation: no change required, but on the honest re-run (AC-5), record why each scenario failed, not just pass/fail — otherwise the new baseline can't distinguish "decontamination revealed real weakness" from "the reworded prompt is harder." That distinction is the entire point of the re-run.
Principle: When you change both the measurement and the thing measured in one step, the delta is uninterpretable. Since AC-5 is deferred anyway, the mitigation is cheap: capture failure reasons on the first honest run.
Scenario 7 ("Do we have anything worth 90000 sitting out?") I checked and it looks sound — noRetry only requires search_nodes not to repeat, and an amount with no match reliably produces the empty result the scenario wants.
🟢 Suggestions
5. Nit: scenario 8's labels now actively contradict their prompts
📁 scripts/aichat-matrix.ts:259-283
"8c. Instance: book" has prompt "Put down Kind of Blue, it's by Miles Davis"; "8d. Instance: contact" creates a venue. The added header comment (lines 49-52) explaining that sub-labels name the role not the entity is a reasonable mitigation, and I agree with keeping the IDs stable for baseline diffing.
Still, a label that says "book" next to a prompt about an album is a small, permanent tax on every future reader, and the explanation lives ~200 lines away from the contradiction. Consider renaming to role-descriptive IDs (8c. Instance: first type, 8d. Instance: second type) in the same commit that re-records the baseline for AC-5 — at that point the baseline is being rewritten anyway, so ID stability costs nothing. Not worth churning the baseline for on its own.
Principle: A comment that explains why a name is wrong is weaker than a name that is right. Acceptable when renaming has a real cost; here that cost disappears at AC-5.
6. Nit: longest_shared_run is O(n·m) over full guidance text
📁 packages/agent/src/agent_guidance.rs:253-272
Nested loops with an inner walk across ~30 prompts × ~15 corpus entries × a few thousand words each. Fine in practice — it's a unit test and the inputs are small — but if the corpus grows (which Finding 1 will do), a suffix-automaton or even a simple 5-gram HashSet intersection would be both faster and simpler to read: build the set of 5-grams from guidance once, then check whether any prompt 5-gram hits it. That formulation also makes the threshold self-documenting (MAX_SHARED_RUN + 1-gram = violation). Optional.
7. Positive note: the guidance rewrite reads as behaviorally sound
📁 packages/agent/src/agent_guidance.rs:20-24, 50-54
I compared old vs new line by line for semantic drift, since removing concrete examples is the central risk of this change for a small model like Gemma E4B. My read is that the rewrites hold up, and in two places improve:
"DATABASE" = SCHEMA: loses the"a contacts database" → name="Contact"worked example but gains an explicit derivation rule ("singular form, stripped of the tracking wording itself") that the example only implied. That rule is what actually would have prevented theinvoice_trackermismatch, so this is a genuine upgrade, not just a redaction.INSTANCE vs TYPE: replacing three example phrasings with the abstract cue ("supplies the particulars of ONE record — a name, an amount, a title, a date") is the riskiest single edit here, since pattern-matching on example phrasings is exactly what small models do well. It's partly compensated by the added closing clause ("Only a request to start tracking a KIND of thing calls for create_schema"), which sharpens the contrast the rule turns on. Watch scenarios 4/8c/8d on the honest re-run — if any regress, this line is the first suspect, and the fix is a fresh example on a domain absent from both eval files, not restoring the old one.- LIST EVERY NODE OF A TYPE: the added "the empty query is what makes it return the whole type, so do NOT invent a keyword to put there" is strictly better than the old
(e.g. "list all my invoices", "show all tasks"). It states the mechanism instead of relying on the model to induce it from two samples. resolve_query: the abstract characterization of indirect reference is faithful to the original, and the replacement ofUse update_task_status ONLY for built-in task nodes — NOT for custom types like invoice, contact, bookwithfor every custom schema type, use update_nodeis a strict improvement — the old form enumerated three types and left the general rule to inference.
No drift found that I'd ask you to revert.
8. Positive note: the supporting tests are well chosen
📁 packages/agent/src/agent_guidance.rs:133-158, 320-351
contamination_guard_detects_a_planted_example is the one that matters — testing that a detector fires is the assertion most guards omit, and pinning it against the specific historical contamination gives it durable meaning. eval_prompt_parser_extracts_literals correctly covers the two shapes the parser is meant to handle plus the fixture.prompt skip. tool_strategy_rules_bind_node_type_to_relevant_entity_types sensibly asserts both presence (the rule, all four tools) and absence (the literal).
One gap consistent with Finding 3: neither parser test covers a shape the parser fails on, so the failure modes are unpinned. Adding a #[test] documenting current template-literal behavior would at least make the limitation visible to the next reader.
Recommendation
REQUEST CHANGES — for Finding 1 only.
To be clear about the weighting: the guidance rewrite (AC-1/2/3) is good work and I'd merge it as-is. The reason I'm not approving is narrow and concrete — the guard is the durable deliverable here, it is the thing that makes AC-1 stay true after everyone forgets this PR existed, and it currently has a hole with live contamination already sitting in it (skill_pipeline.rs:305, a verbatim routing-eval.ts prompt beside its expected tool call and property values). Landing a guard that is green while the defect it targets exists elsewhere in the same crate sets exactly the wrong precedent for the next person tuning prompts.
Blocking:
- Finding 1 — add
skill_pipeline.rsguidance toguidance_corpus()and fix the contamination it surfaces.
Strongly recommended in the same PR (cheap, and each is a hole in the same mechanism):
- Finding 2 — make the scenario-2 threshold tie an explicit decision, not an off-by-one.
- Finding 3 — accept backticks and assert parse coverage so the guard fails closed.
Non-blocking: Findings 4-8. Finding 4's recommendation (capture failure reasons on the honest re-run) is worth carrying into the AC-5 follow-up — without it the new baseline won't be interpretable, which would undercut the point of this whole change.
AC-5's deferral is understood and reasonable; please open a tracking issue rather than relying on the commit message, since it also gates revisiting ADR-056's scenario 5 claim.
malibio
left a comment
There was a problem hiding this comment.
Recommendation: REQUEST CHANGES (submitted as a comment — GitHub does not permit requesting changes on your own PR). Full review posted above.
The guidance rewrite and the eval rewording are good work and I'd merge them as-is. The blocker is narrow: the guard's coverage. guidance_corpus() omits skill_pipeline.rs, and there is live contamination sitting there right now — skill_pipeline.rs:305 embeds routing-eval.ts:187's prompt verbatim ("an invoice for $500 due next Friday", a 7-word shared run) alongside the exact create_node call and property values it should produce, including the same hardcoded node_type="invoice" literal this PR correctly removed from TOOL_STRATEGY_RULES.
Since the guard is the durable deliverable — the thing that keeps AC-1 true after this PR is forgotten — landing it green while the defect it targets exists elsewhere in the same crate is the part worth fixing before merge.
Also recommended in the same pass (both holes in the same mechanism, both cheap): the scenario-2 threshold tie at exactly MAX_SHARED_RUN, and making eval_prompts() fail closed on template literals.
The guard shipped in the previous commit scanned only agent_guidance.rs and skill_rules.rs. Review of PR #1763 found live contamination it did not cover, in the same crate. Coverage gap (the substantive finding): - skill_pipeline.rs:307 carried "create an invoice for $500 due next Friday" — verbatim scripts/routing-eval.ts:187 — paired with the exact create_node call and the same hardcoded node_type="invoice" this PR removed elsewhere. Seeded skill markdown_content is injected as skill instructions, so it is model-facing guidance and equally contaminable. - Rewrote that example onto placeholder values that cannot be copied, and replaced the TYPE MAPPING literals with the copy-the-type_id rule. - guidance_corpus() now pulls every seeded skill's markdown_content from seed_skill_nodes() rather than naming files. A hand-maintained list is precisely how this was missed; enumerating from the seeder covers skills added later automatically. Verified by restoring the original example: the widened guard flags it (7-word run, correctly attributed). Guard holes in the same mechanism: - eval_prompts() failed OPEN — an unparsed literal was silently exempt. It now returns a site count and the caller asserts every counted site parsed, so a shape the parser does not understand fails loudly. Added backtick/template-literal support and escape-aware quote scanning. - A prompt at or below MAX_SHARED_RUN could be quoted in full and never trip a fixed run length. Now also flags a whole-prompt match. This caught "What can you do?" in TOOL_STRATEGY_RULES, since fixed. Eval: - Scenario 3 now mentions returned status, so scenario 6 tests resolve_query routing on an indirect reference rather than whether the model guessed a status value the setup never implied. - Renamed scenario 8 labels to name each step's role, not an entity the prompts no longer use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Pragmatic Code Review — PR #1763 (Re-Review)Review Type: Re-Review Previous Review Summary
The blocking finding is resolved. Every fix was independently verified rather than taken on trust — details below. Verification of Prior Findings✅ Finding 1 (🔴 Critical) — guard missed
|
| Prompt | words | run before | run after |
|---|---|---|---|
"What can you do?" |
4 | 4 (100% quoted) | 2 |
"Hi there" |
2 | 2 | 1 |
| max across all 27 prompts | — | 4 | 3 |
The threshold is no longer load-bearing at a tie: nothing sits within one word of MAX_SHARED_RUN, and no prompt is fully quoted. The > vs >= operator is now genuinely slack rather than the only thing holding the assertion up.
Nice touch: the violation message branches on which rule fired ("the ENTIRE prompt appears" vs "shares a long phrase"), so a future failure tells you which invariant you broke.
✅ Finding 3 (🟡) — eval_prompts() fails open — RESOLVED
📁 packages/agent/src/agent_guidance.rs:209-250, 334-343
All three sub-parts landed: backtick support, escape-aware closing-quote scanning, and the prompts.len() == sites coverage assertion. I emulated the new parser against both real eval files:
scripts/aichat-matrix.ts parsed 12 sites 12
scripts/routing-eval.ts parsed 15 sites 15
Exact coverage, so the assertion is not vacuously satisfied.
On the soundness of sites (you asked me to scrutinize this specifically). The counter is incremented after the quote-character check, so shapes with no literal at all — prompt: SCENARIO_6, prompt: makePrompt("…"), prompt: fixture.prompt, shorthand { prompt } — count 0 sites and parse 0 prompts. That balances, and it is the right call for genuine references. It does mean a deliberately indirected prompt escapes the guard silently, but that is unavoidable without executing the TS, and it is a much narrower hole than "any shape the lexer didn't anticipate."
I probed the shapes that could re-open a fail-open gap:
| Shape | sites | parsed | Verdict |
|---|---|---|---|
prompt: `…${amt}…` (interp) |
1 | 1 | ✅ balanced; interpolation token normalizes away harmlessly |
prompt: "a" + b (concat) |
1 | 1 | ✅ balanced; first fragment still checked |
prompt : "…" (space before colon) |
0 | 0 | |
// prompt: "…" (doc comment) |
1 | 1 | ✅ fails closed (over-counts, harmless direction) |
* The prompt: keep them phrased… (prose) |
0 | 0 | ✅ correctly skipped |
{ turns: [{ prompt: "ok" }] } (nested) |
1 | 1 | ✅ |
The new preceded-by-identifier skip (is_alphanumeric() \|\| '_' \|\| '.') does not undercount in practice: I checked every non-key prompt occurrence in aichat-matrix.ts (lines 40-51 prose, 230 comment, 321-322 step.prompt) and none is a literal site. The prose case at line 44 is instructive — guidance_is_not_contaminated_by_eval_prompts in a comment is correctly ignored because prompt: there isn't preceded by an identifier and isn't followed by a quote.
prompt : "…" (space before colon) is the one shape that skips silently and balances, so the coverage assertion won't catch it. It is not worth chasing — Prettier normalizes it away on every commit in this repo — but it is the honest limit of the mechanism. Not blocking.
The escape handling is correct. an \"escaped\" quote inside parses in full rather than truncating at She said \. The parser retains the literal backslash in its output (the Rust test asserts "an \\\"escaped\\\" quote inside", which looks alarming but is right), and since normalize() keeps only [a-z0-9]+ runs, the backslash is stripped before comparison — tokenization is ["an","escaped","quote","inside"] either way. Trailing \\ and \t cases also behave. No correctness issue.
✅ Finding 4 (🟡) — scenario 6 may not exercise resolve_query's purpose — RESOLVED
📁 scripts/aichat-matrix.ts:234-238
Adding "and whether it's been returned" to scenario 3's prompt is the minimal, targeted fix: it makes a status field a natural consequence of the stated requirement, so scenario 6's "set it to returned" tests routing rather than whether the model happened to invent a returned enum value. The inline comment explains the coupling at the site where it matters, which is where a future editor rewording scenario 3 will actually see it.
This prompt is now 19 words with a max shared run of 3 — the largest run in the file, but still comfortably inside tolerance and against generic connective words, not distinctive phrasing.
✅ Finding 5 (🟢 Nit) — scenario 8 labels contradict prompts — RESOLVED
📁 scripts/aichat-matrix.ts:265-281
Renamed to 8a. Create type: first / 8b. Create type: second / 8c. Instance: first type / 8d. Instance: second type. The header comment was correspondingly trimmed of the now-unnecessary ("book", "contact") parenthetical. Since AC-5's honest re-run rewrites the baseline anyway, ID churn costs nothing here — this was the right moment to take it.
⏸ Finding 6 (🟢 Nit) — longest_shared_run O(n·m) — NOT ADDRESSED, AND I AGREE
I flagged it as optional and it should stay unaddressed. It is a test-only path; measured inputs are ~27 prompts × ~15 corpus entries of a few thousand words, and the suite runs in 0.03s. Replacing a straightforwardly readable nested loop with a suffix automaton to save unmeasurable time would be a net loss. YAGNI — optimize when there is a measurement, not a Big-O intuition. No disagreement.
Requirements Check
| # | Acceptance Criterion | Status | Notes |
|---|---|---|---|
| 1 | No eval scenario prompt appears verbatim/near-verbatim in guidance | ✅ Met | Previously "What can you do?" tie is gone (4/4 → 2/4 words) and skill_pipeline.rs's verbatim routing-eval.ts prompt is removed. Measured max shared run across all 27 prompts × all guidance: 3 words, against generic connectives. |
| 2 | TOOL_STRATEGY_RULES teaches node_type from RELEVANT ENTITY TYPES, no hardcoded literal |
✅ Met | Unchanged this commit, and now reinforced rather than contradicted by skill_pipeline.rs's TYPE MAPPING line. |
| 3 | Eval prompts reworded so they don't share distinctive phrasing with guidance | ✅ Met | Scenario 3 extended, scenario 8 labels fixed. routing-eval.ts remains unreworded but is now clean because the guidance moved, which is the correct direction — guidance should not quote evals, rather than evals contorting around guidance. |
| 4 | Automated check fails the build if an eval prompt reappears in guidance | ✅ Met | Previously seed_skill_nodes(); parser fails closed via prompts.len() == sites; the quoted_whole rule closes the short-prompt blind spot. Verified to fire on the historical contamination (run=7) and to be green at HEAD for a non-vacuous reason. |
| 5 | Matrix re-run, new score recorded as honest baseline | ⏸ Deferred | Acknowledged, requires live daemon + model. scripts/results-e4b.json still holds the contaminated 11/12. Please carry Finding 4's recommendation from my prior review into that work — record why each scenario failed, not just pass/fail — otherwise the new baseline cannot separate "decontamination revealed real weakness" from "the reworded prompt is harder," which is the entire point of re-running. Still worth a tracking issue rather than trusting the commit message; it also gates revisiting ADR-056's scenario 5 claim. |
Code Review Findings (new this pass)
🟢 Suggestions
1. quoted_whole will false-positive on any single-word prompt
📁 packages/agent/src/agent_guidance.rs:353-357
quoted_whole = run == prompt_words.len() fires whenever the entire prompt is matched — which for a one-word prompt means "guidance contains this one word anywhere." I probed the current corpus:
| Hypothetical prompt | words | run | quoted_whole |
|---|---|---|---|
"Thanks" |
1 | 1 | 🔴 true |
"search" |
1 | 1 | 🔴 true |
"Delete it" |
2 | 1 | false |
"hello" |
1 | 0 | false |
So a future abstention scenario with the prompt "Thanks" — an entirely reasonable thing to add to a matrix that already tests greetings and small talk — would hard-fail the build with the ENTIRE prompt appears (1 consecutive words). The message reads as a contamination claim when the real cause is that the prompt is one common word. Two-word prompts are already safe ("Delete it" above), so the exposure is narrow.
My previous review actually suggested the guard: run == prompt_words.len() && run >= 3. The && run >= 3 half was dropped. Adding a floor makes the rule say what it means — a substantial prompt reproduced in full — rather than any prompt short enough to be trivially matched:
// A one- or two-word prompt is matched by coincidence, not contamination.
let quoted_whole = run == prompt_words.len() && run >= 3;This costs nothing today (nothing is affected either way) and removes a tripwire whose failure message would actively mislead whoever hits it. Non-blocking — it is a latent papercut, not a defect in shipped behavior.
Principle: A check should fail for the reason its message states. A rule that conflates "fully quoted" with "short enough to match by chance" produces false alarms that train people to weaken the guard — the failure mode this whole PR exists to prevent.
Answers to the specific questions raised
Does guidance_corpus() now genuinely cover all model-facing guidance? Substantially yes, with one real-but-empty gap and one out-of-scope category. I scanned every candidate:
- ✅
agent_guidance.rsconstants,skill_rules.rsimperatives, all 8 seeded skills'markdown_content— covered. ⚠️ prompt_assembler.rs:269-320seed_prompt_nodes()— genuinely uncovered, and it genuinely reaches the model. These five seeds are the base system prompt (the doc comment says so: "All prompt content lives in these graph nodes — there is no hardcoded base prompt"). Three are already transitively covered because they interpolateSCHEMA_CREATION_RULES/TOOL_STRATEGY_RULES/NODE_REFERENCE_FORMAT. Two are not:"Response Formatting Rules"(~10 lines of prose) and"Core Identity"/"Tool Call Formatting". I ran the eval prompts against all of them: zero violations today. So this is a latent gap, not live contamination — but it is the same shape of gap as the one this commit just fixed, and it closes with three lines mirroring theseed_skill_nodes()loop:Worth doing, but I am not blocking on it: it is empty today, response-formatting prose is a poor host for a planted CRUD example, and the derive-from-seeds pattern is now established so a follow-up is mechanical. Your call whether to fold it in here or next time you touch this file.for t in crate::prompt_assembler::PromptAssembler::seed_prompt_nodes() { if !t.markdown_content.is_empty() { corpus.push(("prompt_assembler::seed_prompt_nodes", t.markdown_content)); } }
- ⚪
local_agent/tools.rstooldescriptionfields andparameter_schema— these do reach the model, and I scanned them (zero hits). I would argue they are correctly out of scope: a tool description is a contract about what the tool does, and it must name concepts like "invoice" or "task" to be comprehensible. Sweeping them into the contamination corpus would generate false positives and pressure authors toward vaguer tool descriptions — a real cost for a speculative benefit. Same reasoning for the skilldescriptionstrings inroot_properties(routing text, deliberately keyword-rich). - ⚪
acp/context_assembly.rs— scanned, zero hits; it assembles user graph content, not authored guidance. Out of scope.
Is the prompts.len() == sites assertion sound? Yes, with the one narrow caveat above (prompt : with a space, normalized away by Prettier in practice). The counter is placed after the quote check, so references balance at 0/0 rather than inflating sites into a permanent failure — that placement is the detail that makes the assertion usable rather than a nuisance. Doc comments over-count, which fails in the safe direction.
Is the new escape-aware parser correct? Yes — verified against escaped quotes, escaped backslashes, and escaped control chars. The retained backslash is invisible to normalize(). The quoted_whole logic has the narrow false-positive class in Suggestion 1; no false negatives found.
Did the skill_pipeline.rs rewrite preserve behavioral soundness for Gemma E4B? This was my biggest worry going in, and I think the answer is yes — the rewrite is more instructive than what it replaced, for a specific reason. The old example was a trap for a small model precisely because it was concrete: "node_type": "invoice" next to "amount": 500 invites copying, which is the invoice_tracker mismatch that started this issue. The new block front-loads the frame ("the shape of the call, NOT the values"), then closes with an explicit prohibition naming the exact tokens at risk of being copied ("Never reuse widget or these field names — they are placeholders").
Two things make this work better than abstraction usually does on a small model:
- It is still fully concrete.
widget/Shipment 24/label,quantity,received_on,conditionare real strings in a real JSON body — the model still sees a complete worked example with the same structural cues (nesting, types: string/number/date/enum-ish). Nothing was abstracted into placeholder syntax like<TYPE_ID>, which would have been a comprehension risk. - The domain is deliberately inert. "widget"/"shipment" appears in no eval and maps to no plausible user request, so if the model does copy it, the failure is loud and obvious rather than a silent near-miss like
invoice→invoice_tracker.
The residual risk is that a small model copies widget anyway on an ambiguous turn. The mitigation is adequate — a negative instruction placed immediately after the thing it negates, naming the literal token, is about as strong as prompt-level mitigation gets. Watch scenarios 4 / 8c / 8d on the AC-5 re-run; if create_node starts emitting node_type="widget", this block is the first suspect and the fix is a different inert domain, not restoring the invoice example.
Worth noting for completeness: schema_creation_guidance() (skill_pipeline.rs:60-100) still carries fully concrete Invoice and Project schema examples, and graph_editing_guidance() (:128-129) still uses invoice phrasing in its ambiguity examples. Those are fine and should stay — they are covered by the guard now, they contain no eval prompt, and they illustrate schema shape, which is exactly where a concrete example earns its cost. The distinction the PR draws is the right one: concrete examples are good; concrete examples that happen to be the eval's answer key are not.
New issues introduced by this commit? Only Suggestion 1. The parser rewrite, the corpus widening, and the prompt edits all check out; cargo test -p nodespace-agent agent_guidance is green (11 passed) and the guard is green for a verified non-vacuous reason.
Recommendation
APPROVE
The blocking finding is fully resolved, and resolved better than I asked — deriving the corpus from seed_skill_nodes() rather than adding a file to a list turns a hand-maintained invariant into a structural one, which was the actual principle behind Finding 1 rather than the specific remedy I proposed. Findings 2-5 are all genuinely addressed, not papered over: I verified the headroom numerically (max shared run 4 → 3, no prompt fully quoted), verified parser coverage is exact on both eval files (12/12 and 15/15), and verified the widened guard reproduces the historical run=7 catch.
The two carry-forwards are both non-blocking and both belong to AC-5's follow-up rather than this PR:
- Add
prompt_assembler::seed_prompt_nodes()to the corpus (three lines, empty gap today, same shape as the one just closed). - Add the
&& run >= 3floor toquoted_whole(Suggestion 1). - On the honest re-run: record failure reasons, not just pass/fail — without that the new baseline is uninterpretable, and interpretability is the whole reason for decontaminating.
This is a good example of a review response that fixed the underlying class of problem rather than the reported instance. Ship it.
malibio
left a comment
There was a problem hiding this comment.
Re-review: APPROVE (posting as comment — GitHub blocks self-approval). Blocking Finding 1 resolved; Findings 2-5 verified addressed. Full review in the PR comment.
malibio
left a comment
There was a problem hiding this comment.
Re-review: APPROVE (posting as a comment — GitHub blocks self-approval).
Blocking Finding 1 resolved, and resolved better than requested: guidance_corpus() now derives from seed_skill_nodes() rather than naming files, turning a hand-maintained invariant into a structural one. Findings 2-5 all verified addressed by independent re-implementation of the guard (max shared run 4 → 3, no prompt fully quoted; parser coverage exact at 12/12 and 15/15; historical run=7 contamination still caught). Finding 6 correctly left alone.
Two non-blocking carry-forwards for the AC-5 follow-up: add prompt_assembler::seed_prompt_nodes() to the corpus (empty gap today, same shape as the one just closed), and add the && run >= 3 floor to quoted_whole. Full review in the PR comment.
Both carry-forwards from the PR #1763 re-review, neither blocking: - guidance_corpus() now also enumerates seed_prompt_nodes(). Those templates ARE the base system prompt; three of five interpolate the constants already in the corpus, but "Core Identity" and "Response Formatting Rules" carry literal text nothing else covered. Clean today, so this closes a latent gap rather than a live one. - quoted_whole now requires >= 3 words. Below that it degenerates into "guidance mentions this word anywhere", which would flag ordinary vocabulary ("Thanks", "search") and report it as contamination — a tripwire whose message would mislead whoever hit it. The third carry-forward — recording per-scenario failure reasons on the honest re-run — belongs to the deferred AC-5 measurement, not here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
|
Re-review carry-forwards addressed in da12629:
The third carry-forward — recording per-scenario failure reasons on the honest re-run — belongs to the deferred AC-5 measurement and is tracked in #1764, along with revisiting ADR-056's Scenario 5 claim and the specific things to watch (scenarios 4/8c/8d for the Full gate green on each push. |
Completes acceptance criterion 5 of #1760, which the earlier commits deferred: the matrix had to be re-run against a live daemon and model before its score meant anything. Result: 9/12 on gemma-4-e4b-q4km, versus 11/12 recorded under contamination. The drop is the planted answers coming off. Two corrections to the issue's premises, found by running it: - scripts/results-e4b.json did not exist in the repo. The contaminated 11/12 lived only in a scratch file, so there was nothing to overwrite. It is now committed with provenance (model, date, guidance commit, host memory), which #1759 wants as eval:agent's default baseline — that issue should note the number is 9/12, not the 11/12 it assumes. - A DB purge is mandatory and was not written down anywhere. Seeding skips nodes that already exist, so a reused database keeps the OLD prompt/skill nodes and the run silently measures guidance you already changed. The daemon binary must be rebuilt for the same reason: guidance is compiled-in string constants. My first two runs were invalid this way — a stale binary reseeded the pre-fix skill_pipeline example, which I caught only by querying the seeded DB for the old text. Recorded in the results note. Scenario 8a/8b reworded again: "Set up somewhere to note down albums" dropped the tracking vocabulary the DATABASE=SCHEMA rule keys on, and the model asked for confirmation instead of calling create_schema. A control run confirmed the ORIGINAL wording still fires create_schema on decontaminated guidance, so that was a rewording artifact, not a decontamination finding. Now "Start tracking albums..." — eval- independent but still carrying the intent. The three remaining failures are genuine agent defects, not artifacts: scenario 4 calls create_schema for an instance request and errors, which starves scenario 6 of a node to update; 8c retries create_node after an error. These are real behaviors to fix, now visible because nothing is planted in the prompt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Pragmatic Code Review — PR #1763 (Third Pass)Review Type: Re-Review (third pass) Previous Review Summary
Carry-forward verification✅ CF-1 —
The comment ("Some interpolate the constants above, but others carry their own literal text that nothing else covers") accurately describes the actual situation. Good. ✅ CF-2 —
Requirements Check (issue #1760)
On AC-5 more broadly: I want to be explicit that this commit does the hard, unglamorous thing correctly. The author ran the matrix, got a worse number, and committed the worse number. They then caught two invalid runs by querying the seeded DB for pre-fix text — a failure mode that produces a plausible-looking result and that nothing in the harness would have flagged. That is exactly the discipline this PR exists to install, and it is worth more than the score itself. My findings below are about interpretation of the number, not about the integrity of the effort. Code Review Findings🟡 ImportantFinding 1 — Scenarios 8a/8b were reworded toward the guidance's trigger vocabulary; the control run does not establish what it is cited for📁 This is the question the review was asked to weigh most carefully, so I want to show the measurement rather than assert a judgment. The
Content-word overlap between each candidate wording and that rule's trigger clause:
The pattern is unambiguous: the scenario passes exactly when the prompt contains a word from the rule's enumerated trigger list, and fails when it does not. Rewording #1 was the only variant that removed all trigger vocabulary, and it was the only one that failed. Rewording #2 restored trigger words ( Why the control run does not resolve this. The control established: original wording still fires
Both predict the control's outcome identically, so the control has zero discriminating power between them. Under H2, rewording #1 was not a bad prompt — it was the single most informative datapoint in the run, the one place the eval measured generalization rather than keyword lookup, and it returned a genuine negative. Rewording #2 then converted that finding into a pass by supplying the magic word. To be clear about severity and intent: I do not read this as bad faith. The commit message is transparent about the sequence, ran a control rather than just re-rolling, and the contamination guard passes — the shared run is 2 words, well inside tolerance. This is not verbatim contamination. But #1760's stated purpose is that the eval "cannot distinguish 'the agent generalized' from 'the answer was in the system prompt.'" A prompt tuned until it contains a word the guidance enumerates as a trigger reintroduces that exact ambiguity at the vocabulary level, one layer below where the mechanical guard looks. The guard measures phrase overlap; the coupling here is lexical. The methodological hazard is the more important half: rewording a prompt because it failed, then reporting the result as a baseline, is the move that launders a weakness into a score. It is defensible only when the artifact hypothesis is independently confirmed, and here it is not. Recommendation — pick one, all cheap, none blocking on their own:
Principle: an eval's prompts must be sampled independently of the system under test. Tuning a prompt in response to a failure couples them, and the coupling is invisible to a guard that only measures phrase overlap. Related: a control must be capable of distinguishing the hypotheses it is cited against — otherwise it provides confidence without evidence. 🟢 SuggestionsFinding 2 —
|
malibio
left a comment
There was a problem hiding this comment.
Pragmatic Code Review — PR #1763 (Third Pass)
Review Type: Re-Review (third pass)
Scope of this pass: c4e4e856..HEAD — commits da126293 (guard gaps) and 805a60e4 (honest baseline). +182/-3 across packages/agent/src/agent_guidance.rs, scripts/aichat-matrix.ts, scripts/results-e4b.json.
Previous Review Summary
| Count | |
|---|---|
| Round 1 findings | 6 (1 Critical, 3 Important, 2 Nit) — resolved in round 2 |
| Round 2 carry-forwards | 3 (all non-blocking) |
| Addressed in this pass | 2 (seed_prompt_nodes() corpus gap, quoted_whole floor) |
| Partially addressed | 1 (per-scenario failure reasons — captured mechanically, not diagnostically) |
| New issues this pass | 3 (1 Important, 2 Suggestions) |
Carry-forward verification
✅ CF-1 — guidance_corpus() omits seed_prompt_nodes() — RESOLVED, and correctly.
agent_guidance.rs:199-204 now enumerates it. I verified this is the right call on all four axes round 2 asked about:
- Not test-only / not private-by-intent:
seed_prompt_nodes()ispub fnatprompt_assembler.rs:260with a production caller atdaemon/src/services/assembly.rs:229. It is genuinely the first-run seeding path for the base system prompt, not a test fixture. Including it is consistent with the existingseed_skill_nodes()precedent. - Not vacuous: all 5 templates (
Core Identity,Workspace Context Template,Tool Strategy Guide,Response Formatting Rules,Tool Call Formatting) carry non-emptymarkdown_content, so the!is_empty()filter is defensive rather than silently skipping the whole set. The three that interpolateSCHEMA_CREATION_RULES/TOOL_STRATEGY_RULES/NODE_REFERENCE_FORMATdo add duplicate coverage, but duplicates only cost runtime, they cannot mask a violation. - Not over-broad: the guard is green at HEAD and I confirmed it is green for a non-vacuous reason —
cargo test -p nodespace-agent --lib guidancepasses 11/11 includingcontamination_guard_detects_a_planted_example, which still fires atrun=7on the historical contamination. Adding this corpus did not require loosening any threshold.
The comment ("Some interpolate the constants above, but others carry their own literal text that nothing else covers") accurately describes the actual situation. Good.
✅ CF-2 — quoted_whole degenerates for 1-2 word prompts — RESOLVED.
agent_guidance.rs:364 now reads run == prompt_words.len() && run >= 3, exactly the floor round 2 proposed. The comment explains why the floor exists rather than restating the condition. This is a tripwire-quality fix: the failure it prevents is a misleading error message, and the comment is what stops a future reader from "simplifying" the >= 3 away.
results-e4b.json records a failure string per failing scenario, but it is the assertion's mechanical output ("Expected 'create_node' exactly once, got 0 (tools: create_schema)") — it says what the assertion saw, not why the model did it. Round 2's ask was specifically for the distinction between "decontamination revealed real weakness" and "the reworded prompt is harder." That analysis exists in the commit message for 8a/8b, and nowhere for scenario 4. See Finding 1 — this carry-forward turns out to be the load-bearing one.
Requirements Check (issue #1760)
| # | Acceptance Criterion | Status | Notes |
|---|---|---|---|
| 1 | No eval prompt verbatim/near-verbatim in agent_guidance.rs / skill_rules.rs |
✅ Met | Mechanically: max shared run across all prompts × the now-wider corpus is 3 words. Unchanged this pass. |
| 2 | TOOL_STRATEGY_RULES teaches node_type from RELEVANT ENTITY TYPES, no literal |
✅ Met | Unchanged this pass. |
| 3 | Eval prompts reworded, no distinctive shared phrasing | Passes the guard, but 8a/8b were reworded toward the guidance's own trigger vocabulary. See Finding 1. | |
| 4 | Automated check fails the build if a prompt reappears in guidance | ✅ Met | Now strictly stronger: corpus covers the base system prompt too, and the short-prompt blind spot is closed. |
| 5 | Matrix re-run, new score recorded as honest baseline | 9/12 recorded with provenance, on a purged DB and rebuilt binary. Genuinely honest work — the invalid-run catch is the strongest evidence of that. Two gaps: n_ctx missing (Finding 2), and 8a/8b's rewording makes two of the nine passes weaker evidence than the number implies (Finding 1). |
On AC-5 more broadly: I want to be explicit that this commit does the hard, unglamorous thing correctly. The author ran the matrix, got a worse number, and committed the worse number. They then caught two invalid runs by querying the seeded DB for pre-fix text — a failure mode that produces a plausible-looking result and that nothing in the harness would have flagged. That is exactly the discipline this PR exists to install, and it is worth more than the score itself. My findings below are about interpretation of the number, not about the integrity of the effort.
Code Review Findings
🟡 Important
Finding 1 — Scenarios 8a/8b were reworded toward the guidance's trigger vocabulary; the control run does not establish what it is cited for
📁 scripts/aichat-matrix.ts:265-272 (prompts) · packages/agent/src/agent_guidance.rs:21 (the rule they now key on)
This is the question the review was asked to weigh most carefully, so I want to show the measurement rather than assert a judgment.
The DATABASE = SCHEMA rule triggers on an enumerated surface-vocabulary list:
"DATABASE" = SCHEMA: A request to start tracking a kind of thing — worded as a database, a tracker, a list, or "track X" — means call create_schema IMMEDIATELY
Content-word overlap between each candidate wording and that rule's trigger clause:
| Wording | Content words | Overlapping the trigger list | Fired create_schema? |
|---|---|---|---|
"Create a database to track books…" (original, contaminated era) |
create, database, track, books, read | database, track |
✅ (control) |
"Set up somewhere to note down albums…" (rewording #1) |
set, up, somewhere, note, down, albums, mean, listen | none | ❌ asked for confirmation |
"Start tracking albums I mean to listen to" (rewording #2, HEAD) |
start, tracking, albums, mean, listen | start, tracking |
✅ |
"I also need a tracker for the venues I book" (rewording #2, HEAD) |
also, need, tracker, venues, book | tracker |
✅ |
The pattern is unambiguous: the scenario passes exactly when the prompt contains a word from the rule's enumerated trigger list, and fails when it does not. Rewording #1 was the only variant that removed all trigger vocabulary, and it was the only one that failed. Rewording #2 restored trigger words (tracking, tracker) drawn from the same enumeration as the original's (database, track) — different lexemes, same list, same stem.
Why the control run does not resolve this. The control established: original wording still fires create_schema post-decontamination. That is a real and useful result — it rules out "decontamination broke schema creation outright." But it does not distinguish the two live hypotheses, because the original wording is also trigger-vocabulary-bearing:
- H1 (the commit's claim): rewording #1 was unnaturally phrased; the model's failure was an artifact of awkward English.
- H2 (not excluded): the model does not generalize the DATABASE=SCHEMA intent at all — it pattern-matches the enumerated trigger words, and any phrasing lacking them fails.
Both predict the control's outcome identically, so the control has zero discriminating power between them. Under H2, rewording #1 was not a bad prompt — it was the single most informative datapoint in the run, the one place the eval measured generalization rather than keyword lookup, and it returned a genuine negative. Rewording #2 then converted that finding into a pass by supplying the magic word.
To be clear about severity and intent: I do not read this as bad faith. The commit message is transparent about the sequence, ran a control rather than just re-rolling, and the contamination guard passes — the shared run is 2 words, well inside tolerance. This is not verbatim contamination. But #1760's stated purpose is that the eval "cannot distinguish 'the agent generalized' from 'the answer was in the system prompt.'" A prompt tuned until it contains a word the guidance enumerates as a trigger reintroduces that exact ambiguity at the vocabulary level, one layer below where the mechanical guard looks. The guard measures phrase overlap; the coupling here is lexical.
The methodological hazard is the more important half: rewording a prompt because it failed, then reporting the result as a baseline, is the move that launders a weakness into a score. It is defensible only when the artifact hypothesis is independently confirmed, and here it is not.
Recommendation — pick one, all cheap, none blocking on their own:
- Preferred, and it costs one run: re-test rewording #1 (
"Set up somewhere to note down albums…") as an explicit generalization probe, kept alongside the current 8a rather than replacing it. If it still fails, that is a genuine, recordable finding — "the model keys on trigger vocabulary and does not generalize the intent" — which is a far more valuable eval output than a 10th pass. If it now passes, H1 is confirmed and the artifact conclusion is earned. - Record the control run's inputs and outcome in
results-e4b.json, not only in the commit message. A future reader comparing against this baseline sees the file; git archaeology on why 8a is worded as it is will not happen. - Soften the commit-message claim from "was a rewording artifact" to what the evidence supports: "the original wording still fires; the trigger-vocabulary-free wording does not, and which of those is the artifact is untested."
Principle: an eval's prompts must be sampled independently of the system under test. Tuning a prompt in response to a failure couples them, and the coupling is invisible to a guard that only measures phrase overlap. Related: a control must be capable of distinguishing the hypotheses it is cited against — otherwise it provides confidence without evidence.
🟢 Suggestions
Finding 2 — results-e4b.json omits n_ctx, which #1759 calls for and which a known live bug makes load-bearing
📁 scripts/results-e4b.json:1-8
Provenance recorded: label, model, recorded_at, guidance_commit, host_memory_gb, note. #1759's AC asks for "model, date, host memory, and granted n_ctx" — n_ctx is the one missing field.
This matters more than a checklist gap, because #1759 documents an active compute_effective_n_ctx bug flooring n_ctx to 4096 against a ~6,600-token agent system prompt, and documents a prior run where scenarios 1 and 2 reported as passing while every turn was dying on Context window exceeded — both assert "no tools called," and a timed-out turn calls no tools. Scenarios 1 and 2 are green in this very file.
I do not think that happened here, and I checked rather than assuming: latencies are 2.1s–28.5s with a 145s total and a coherent spread (tool-calling scenarios 8c/8d at 26–28s, trivial ones at 2–4s), and eight scenarios show real tool calls. That is a live-inference profile, not a uniform-timeout profile. So this is a provenance gap, not a correctness doubt — but recording n_ctx is what lets the next reader reach that conclusion in one glance instead of reconstructing it from latency spreads.
Recommendation: add n_ctx (and ideally the daemon commit alongside guidance_commit, since guidance is compiled in). If the value isn't recoverable now, "n_ctx": null with a note beats silent omission — it tells the reader the field was considered. #1759 owns the shared runner that should populate this automatically; this is about the one committed artifact that will be used as the default baseline before that lands.
Principle: a cited measurement must carry the conditions that produced it, especially the conditions under which it is known to fail silently.
Finding 3 — Scenario 4's failure is characterized as a genuine defect, but it carries the same rewording-artifact risk as 8a/8b — untested in the opposite direction
📁 scripts/aichat-matrix.ts:269-271 · scripts/results-e4b.json scenario 4
The commit asserts the three failures "are genuine agent defects, not artifacts." I agree for 6 and 8c, and I think 4 is unproven:
- Scenario 6 (
"The 2400 one came back — set it to returned", called no tools): correctly identified as a cascade — scenario 4 never created the node, so there is nothing to find. Sound; not independently informative until 4 is fixed. - Scenario 8c (
"Put down Kind of Blue, it's by Miles Davis"→create_nodetwice): unambiguous defect. It called the right tool and retried after an error, which is precisely theSKILL COMPLETION/IDENTICAL TOOL CALLSrules failing. Real. - Scenario 4 (
"Log a laser cutter checked out on the 12th, replacement cost 2400"→search_skills,create_schema): the model chose schema over instance. This is the exactINSTANCE vs TYPEdiscrimination — and it is the mirror image of the 8a/8b situation.
Note the asymmetry in how the two rules are written:
DATABASE = SCHEMAkeys on surface vocabulary — an enumerated word list (a database, a tracker, a list, "track X").INSTANCE vs TYPEkeys on semantic structure — "a request that supplies the particulars of ONE record — a name, an amount, a title, a date."
Scenario 4's prompt satisfies the semantic condition richly (an item, a date, an amount). "Log a…" is fair, natural instance-creation English — I do not think it is a bad prompt. But it was reworded once in 370782ef (from "Add an invoice for $500 due next Friday") and, unlike 8a/8b, never re-tested after it failed. So the run applied rewording-then-retry to the scenarios that failed and were easy to fix, and not to the one that failed and was attributed to the model. That asymmetric treatment is what makes the "genuine defect" label unproven rather than wrong — a control here (does "Add an invoice for $500 due next Friday" fire create_node on decontaminated guidance?) is the same one-run experiment that was already run for 8a/8b, and would settle it.
Given a plausible reading where the model keys on surface vocabulary generally (Finding 1's H2), a competing explanation for scenario 4 is available: "Log a laser cutter checked out…" contains no INSTANCE trigger vocabulary either, and the model fell back to create_schema. Whether that is "a genuine defect" or "the same vocabulary-keying limitation seen in 8a" changes what should be fixed — the model's generalization, or the guidance rule's structure.
Recommendation: run the same control for scenario 4 that was run for 8a/8b, and record the outcome in the results note. If the original wording also fails, "genuine defect" is confirmed and strengthened. If it passes, scenario 4 belongs in the same category as 8a/8b, and the honest baseline is arguably 9/12 with two vocabulary-sensitivity findings, which is a more useful result than 9/12 with three defect labels. Either way this is follow-up work for #1764, not a blocker here.
Principle: apply the same evidentiary standard to failures you attribute to the system as to failures you attribute to the harness. Asymmetric scrutiny biases the conclusion toward whichever explanation was convenient.
Finding 4 — results-e4b.json is committed but referenced by nothing
📁 scripts/results-e4b.json
Nothing in scripts/ or packages/ reads this file — I grepped. It is intended as eval:agent's default baseline, but the wiring is #1759's. Fine as-is; flagging only so it does not sit as an orphan artifact that silently goes stale while being cited as authoritative. #1759 should be updated to say the number is 9/12, not the 11/12 it currently assumes — the commit message notes this, but the issue itself is what the next agent will read.
Nit: guidance_commit: "da126293" is recorded, but the run measured 805a60e4's prompts (8a/8b were reworded in that same commit). The guidance constants are indeed unchanged between the two, so the field is technically accurate — but a reader reconstructing the run needs the eval commit too, and those diverge here. Consider eval_commit alongside guidance_commit.
Recommendation
COMMENT — approve-with-reservations in substance; nothing here should block the merge.
The two unreviewed commits are a clear net improvement. da126293 closes both round-2 carry-forwards exactly as proposed, and I verified the seed_prompt_nodes() addition is production guidance, non-vacuous, and did not require weakening any threshold — the guard is now strictly stronger than at round 2. 805a60e4 does the genuinely hard thing: it ran the matrix, got a worse number than the contaminated one, committed the worse number, and caught two invalid runs through a failure mode (stale binary reseeding pre-fix guidance) that produces plausible-looking results and that nothing in the harness would have flagged. Discovering and writing down the mandatory-DB-purge precondition is a durable contribution beyond this PR.
My reservation is confined to interpretation, not integrity: the 9/12 is honest about what it measured, but two of the nine passes are weaker evidence than the number implies. Scenarios 8a/8b were reworded until they contained a word the DATABASE = SCHEMA rule enumerates as a trigger, and the control offered in defense cannot discriminate between "rewording artifact" and "the model keys on trigger vocabulary and does not generalize" — both hypotheses predict the control's outcome identically. Symmetrically, scenario 4's "genuine defect" label rests on the one failure that was not given the rewording-and-retry treatment the others got.
None of this justifies holding the PR. The contamination guard — this PR's actual deliverable — is sound, wider than it was, and mechanically verified. The baseline is directionally right and vastly better than the contaminated 11/12 it replaces. The right home for Findings 1 and 3 is #1764, as two one-run control experiments:
- Re-test
"Set up somewhere to note down albums…"as an explicit generalization probe kept alongside 8a. - Re-test
"Add an invoice for $500 due next Friday"against scenario 4's expectation.
If both fail, the finding is not "9/12 with three defects" but something considerably more actionable: the guidance rules that key on enumerated surface vocabulary do not generalize, and the eval is sensitive to whether prompts happen to contain the magic words. That would be the most valuable thing this decontamination effort could surface, and it is two runs away.
Finding 2 (n_ctx) should be picked up wherever #1759's provenance work lands.
…e baseline
Third-pass review argued my "rewording artifact" conclusion for scenarios
8a/8b rested on a control that could not discriminate the two competing
hypotheses, since the original wording is also trigger-vocabulary-bearing.
That was correct. Ran the two controls it asked for; both are recorded in
the results file rather than only in a commit message.
(a) The DATABASE=SCHEMA rule DOES generalize. Two trigger-vocabulary-free
phrasings ("Set up somewhere to note down albums I mean to listen to" --
the exact wording that failed in the matrix -- and "I want a place to jot
down albums worth hearing") both fire create_schema correctly. The model
is not merely keying on database/track/tracker/list, so the alternative
hypothesis is falsified.
(b) Scenario 4's wording is not the cause, but not for the reason I claimed.
Both it and an alternative instance phrasing behave identically, because
both are downstream of a schema that never existed.
That led to the real finding: scenario 3 is a FALSE PASS. Its noExtraTypes
assertion only checks create_schema was called once, never that the schema
persisted. In the baseline run create_schema returned is_error=true --
title_template placeholders with no fields array -- so no equipment type was
registered, and scenario 4 correctly reported the type missing and was scored
a failure for it. Scenarios 4 and 6 are consequences of scenario 3, not
independent defects; 8c remains independent and real.
Confirmed pre-existing: this PR never touched the TITLE TEMPLATE or FIELD
RULES guidance. Root cause and revised scope moved to #1775.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Pragmatic Code Review — PR #1763 (Fourth Pass)Review Type: Re-Review (fourth pass) The diff is JSON, but it asserts conclusions that #1775 and #1759's baseline work will consume. I reviewed the reasoning, not the syntax, and independently verified the claims I could check read-only against Previous Review Summary
New issues this pass: 3 (1 Important, 2 Suggestions). No blockers. What I verified independentlyRound 3's most useful lesson was that a claim offered in defense of a result deserves the same scrutiny as the result. So I checked Claim B rather than accepting it, and it holds — completely. ✅ Claim B (scenario 3 is a false pass) — CONFIRMED, on four independent lines of evidence.
✅ Pre-existing, not introduced by this PR — CONFIRMED. ✅ Scenario 4 as downstream — CONFIRMED, and stronger than claimed. Scenario 4's reply is " Requirements Check (issue #1760)
All five ACs met. AC-5's residual concern is presentation (Finding 3), not substance. Code Review Findings🟡 ImportantFinding 1 —
|
malibio
left a comment
There was a problem hiding this comment.
Re-review (fourth pass): COMMENT — merge-ready as it stands (posting as a comment; GitHub blocks self-approval). Full review in the PR comment above.
Round 3 closed asking for two control experiments. 46bfed40 ran both, and one of them overturned a result in the author's own favor — scenario 3 was recorded as passing, was investigated anyway, and turned out to be a FALSE PASS that was then written down against the headline 9/12. Round 3's Finding 3 ("is scenario 4's 'genuine defect' label proven?") is answered: no, and the reason is one scenario upstream. That is the right outcome, reached by better means than the control I proposed.
I verified Claim B independently rather than on report — all four lines agree: the honest DB has album_to_listen and venue_booking but no equipment schema; daemon-e4b-decontaminated.log:1605 shows create_schema is_error=true on a title_template validation error at duration_ms=0; aichat-matrix.ts:207-216 counts tool names only; and scenario 3's own recorded reply is the model telling the user it failed and asking a clarifying question. git diff 370782ef~1 HEAD -- agent_guidance.rs confirms the defect is pre-existing and correctly scoped to #1775. Worth passing to #1775: the harness limitation is broader than the caveat states — is_error never reaches TurnResult, so no assertion in this file can detect a failed tool call.
Three non-blocking findings, all single-string edits to the JSON:
- Important —
caveats[2](a)claims the DATABASE=SCHEMA rule "generalizes" on n=2 single-turn probes. The probes support the narrower "not a literal four-word lookup." More importantly, the first probe string is the exact wording that FAILED in the matrix (as turn 1 of a multi-turn group), and the caveat records only the success — which hides a non-determinism finding rather than establishing generalization. - Suggestion — 8c is mischaracterized. The log shows one malformed
create_node(empty args, missingnode_type) then one correct call, one node created. That is successful recovery, not a retry-loop; the real defect is malformed tool-argument generation, same class as scenario 3. - Suggestion — put the false-pass asterisk in
note.passed: 9/12is reachable without ever readingcaveats[0], and this file exists to be compared headline-to-headline.
None of these justifies holding the merge. n_ctx remains absent and remains #1759's — noted for continuity only.
Sweeping the open non-blocking items from all three review rounds.
Round 3 Finding 2 - n_ctx omitted. Recovered from the run's daemon log
("Creating chat LlamaContext (n_ctx=32768...)") and recorded, with a note
on why it matters: 32768 is far above the ~6600-token agent system prompt,
so this baseline was NOT context-starved. That rules out the failure mode
#1759 documents, where every turn dies on "Context window exceeded" and the
two no-tool assertions still score as passes. Previously a reader had to
infer this from the latency spread.
Round 3 Finding 4 nit - guidance_commit alone is ambiguous. The run measured
805a60e's prompts (8a/8b were reworded there) against da12629's guidance
constants, so eval_commit is now recorded alongside guidance_commit.
Round 3 Finding 4 - a reader skimming "passed: 9/12" may never reach
caveats[0] and learn that one of the nine is a confirmed false pass. Added a
top-level `score` field carrying the asterisk inline.
Round 1 Finding 6 - longest_shared_run O(n.m): re-checked rather than
inherited. The corpus is much larger now (seeded skill nodes and the base
system prompt joined it since that call), so the YAGNI verdict was worth
retesting. Guard still runs in 0.03s. Leaving the readable algorithm.
Rounds 1 and 2 have no remaining open items.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Fourth-pass review findings, all in the baseline artifact.
8c was mischaracterized. I wrote it up from the tool-NAME sequence
(create_node, create_node) as a blind retry violating IDENTICAL TOOL CALLS.
The log shows otherwise: the first call had EMPTY arguments and failed
("missing field node_type"), the second supplied correct arguments and
succeeded. The calls were not identical and the recovery was reasonable.
The real defect is emitting an argument-less tool call - the same
argument-construction failure class as scenario 3's title_template without
a fields array, and a third instance (create_schema with args_preview={}).
Left as-is this would have sent whoever picks up #1775 hunting a retry loop
that does not exist; that issue is corrected too.
caveats[2] overstated the control probes. "The rule generalizes" is broader
than n=2 supports: both probes are still canonical record-keeping English,
neither reaches oblique phrasing, and the sampler is stochastic. More
importantly the first probe string is the exact wording that FAILED as
scenario 8a in this same run - same prompt, both outcomes - so the honest
finding is that 8a is non-deterministic, and reporting only the passing
instance was selective even though unintended. Also noted the probes ran
single-turn while 8a runs first-in-group, so they do not reproduce the
failing condition.
Headline asterisk: effective_passed=8 added, and the note now says the raw
9/12 may not move when #1775 fixes scenario 3 for real, so comparisons
should use effective_passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DkqjrfV1G6kugsEjvyU9wU
Closes #1760