Repair bare unquoted dates in resolve_query's decomposition output

[malibio]

Closes #2185.

The issue's first acceptance criterion was to decide with evidence whether the resolve_query decomposition sub-call actually emits malformed filter shapes, and to record a negative result rather than add a repair for a shape nothing emits. Measured it first. The measurement changed the answer.

What was measured

packages/agent/tests/live_resolve_query_decomposition_shapes.rs — #[ignore]d, loads the GGUF. Drives the real exec_resolve_query over a corpus of 11 phrasings × 3 reps (33 decompositions, 21 emitted filters) against the locked E4B model, capturing each decomposition's raw text through the existing NODESPACE_PROMPT_DUMP hook.

Two deliberate choices:

• No new production instrumentation. nlp-engine's chat/prompt_dump.rs already names resolve_query as a covered caller and already dumps both prompt and raw response.
• Measured through the real tool, not a rebuilt prompt. A hand-rebuilt decomposition prompt is a different arm from the one production runs, and an arm that doesn't reach its own question looks like a clean pass.

The corpus over-samples the shapes the repairs exist for — multi-value enum phrasings, identifier-vs-update-target sentences, a comma-bearing stored value — rather than sampling typical traffic, so a null result means something.

well_formed:            21
dropped_unknown_field:   0
over_quoted_key:         0
leaked_token_key:        0
scalar_in_value:         0     <- the shape #2182/#2183 repaired
used_in_operator:        6     <- but `in` IS emitted
unknown_property:        0
coerced_scalar_type:     0
unparseable_output:      3     <- see below
total_decompositions:   33

Finding 1 — the predicted malformations are absent; the issue's stated reason is wrong

All four repairs measured inert on this path. The issue's proposed fix (normalize the scalar-in case at coerce_filter_value_to_field_type) is therefore declined — that is AC3's negative result, and adding the repair anyway would be exactly the mistake AC3 exists to prevent.

But the issue's reason — "the decomposition prompt never instructs in, so the shape is not reachable" — does not hold. The prompt doesn't instruct it, and the model emits in anyway, 6 of 21 filters:

{"property": "status", "operator": "in", "value": ["checked_out", "in_repair"]}

Always a proper JSON array, never a comma-joined scalar. So the conclusion survives and the reason doesn't — worth correcting in place, because that reason is what a future reader would rely on when widening the prompt's operator vocabulary, and it would not protect them.

Finding 2 — a real, deterministic defect the issue did not anticipate

unparseable_output: 3 is one request, 3/3 reps, byte-identical — "Show me the overdue ones":

{"query": "", "filters": [{"type":"property","operator":"lt","property":"due_date","value":2026-08-19}]}

A bare unquoted date. serde_json reads 2026 as a number, then fails on -08-19, rejecting the whole object — not just that filter. exec_resolve_query falls back to {"query":"","filters":[]} and the search degrades to a bare type listing.

Observed end to end: the request returned multiple_matches over both invoices, including the one that is not overdue, instead of resolving the single overdue one. Well-formed output, and wrong — the silent-narrowing hazard the issue flags, arriving through a wider door than the one it was watching.

The prompt provokes it directly: it instructs resolving a relative date to "a concrete YYYY-MM-DD value", and unquoted YYYY-MM-DD is what came back.

This is upstream of every repair. All four, and coerce_filter_value_to_field_type, operate on an already-parsed Value; parsing is what failed. The issue's "more uniform" alternative — routing this output through repair_parsed_tool_arguments — would not have caught it either, for the same reason. And the proposed seam can't reach it: coerce_filter_value_to_field_type runs per-filter-item, and there are no filter items.

The fix

quote_bare_date_literals repairs the raw text before from_str, the only place this is reachable. Safe because the shape isn't expressible as valid JSON under any reading — a bare 2026-08-19 is never a legal JSON value, so nothing legitimate is rewritten:

• Skips string literals, so an already-quoted date (the common case) and a date inside a value ("query": "due 2026-08-19") are untouched
• Declines longer digit runs (2026-08-19-01) rather than slicing ten characters out and inventing a value
• Leaves plain numbers alone, so number-field filters keep working
• Allocation-free (Cow::Borrowed) for already-valid output

Deliberately not generalized to other unquoted scalars — a bare word could be a typo'd true, an enum member, or a malformed number, and choosing between those is interpretation, not repair.

Verification against the real model

Same malformed emission from the model, different outcome:

	Before	After
"Show me the overdue ones"	multiple_matches over both invoices	resolved: true → the overdue one, 3/3 reps
Corpus resolution	12/33	15/33

The +3 is exactly the three reps of that request; nothing else moved.

Also in this PR

• The whole-resolution fallback now logs. It was silent, which is how this went unnoticed until measured — it discards the entire resolution, not one filter.
• AC4, the coverage boundary, recorded at the filter-parsing site — with the measured reason, not just "repairs don't run here" but "repairs couldn't help here, because this path's real failure is a parse failure", plus a pointer to the harness to re-measure before assuming the shape mix has changed.

Testing

• 6 new unit tests for the repair, incl. the verbatim measured malformation and the decline cases
• 1 new integration test through the real exec_resolve_query, asserting on resolved identity — the pre-fix behavior returned a well-formed multiple_matches, so only an identity assertion catches the regression; asserting the helper alone would pass with the call site unwired
• bun run test:all clean: frontend 227 files / 4794 tests (matches baseline), browser 2/24, all Rust suites pass
• bun run quality:fix clean, clippy -D warnings passes

🤖 Generated with Claude Code

https://claude.ai/code/session_01TDtMcV2chVWTDbMFerp5Vs

[malibio]

Pragmatic Code Review — PR #2186

Review Type: Initial Review
Scope: git diff origin/main...HEAD — 2 commits, 838 insertions across 2 files
Verdict: APPROVE (with two non-blocking follow-ups)

---

Review Summary

This is a strong change, and the strength is in the part that is easiest to skip: it answered the issue's first acceptance criterion empirically before writing a line of repair, then declined the fix the issue proposed because the measurement did not support it. That is the exact discipline #2182 established, applied to its own successor.

It also does the harder thing — it reports that the issue's stated reason for the bound was wrong (the model emits in 6/21 times despite the prompt never instructing it) while the conclusion survives. Publishing a correction that weakens your own justification, rather than quietly keeping the convenient half, is what makes the negative result trustworthy.

I independently re-derived the central risk rather than taking the doc comment's word for it. quote_bare_date_literals is a hand-rolled byte scanner over untrusted model output, and the failure mode that would matter is not "fails to repair" — it is "produces wrong-but-parseable JSON", which would convert a loud parse failure into a silent wrong answer. I extracted the function verbatim into a scratch crate and ran 25 adversarial inputs through it (negative-lead, float-prefix, exponent forms, bare timestamps, unterminated strings, escaped quotes/backslashes, multibyte UTF-8 adjacency, unquoted date keys, digit-dash ranges, out-of-range dates). No input turned valid JSON into different valid JSON, and every input that became parseable produced the semantically intended date string. The prev_ok/next_ok guards do real work: 1.2026-08-19, 2026-08-19.5, 2026-08-19e5, 2026-08-19T10:00:00Z, v2026-08-19 and 2026-08-19-01 are all correctly declined and left for the fallback rather than sliced into a plausible-looking invention. The UTF-8 concern is structurally unreachable — every byte of a multibyte sequence is >= 0x80, so it never satisfies is_ascii_digit() and never becomes a slice index — and the working tree already has a test asserting that rather than arguing it.

I also verified the substrate the harness rests on: NODESPACE_PROMPT_DUMP is real, prompt_dump.rs names resolve_query as a covered caller in its own module doc, and it emits exactly the seq / raw_response fields the harness correlates on. The claim that no new production instrumentation was needed checks out.

cargo clippy -p nodespace-agent --all-targets is clean; all 28 resolve_query tests and all 9 quote_bare_date_literals tests pass.

---

Requirements Check — Issue #2185 Acceptance Criteria

#	Criterion	Status
1	Decide with evidence whether the decomposition sub-call produces malformed filter shapes — dump raw output before writing a fix	✅ Met. 33 decompositions / 21 filters over a corpus that deliberately over-samples the target shapes, captured through the real exec_resolve_query.
2	If it does: normalize at coerce_filter_value_to_field_type, covering at minimum scalar-in	✅ Correctly declined. Measured 0/21 scalar-in. See analysis below.
3	If it does not: record the negative result rather than adding a repair for a shape nothing emits	✅ Met, at tools.rs:2231-2252, with counts and the re-measurement instruction.
4	Note the coverage boundary at the resolve_query filter-parsing site	✅ Met — explicit COVERAGE BOUNDARY: block naming all four repairs.

Is declining AC2 correct? Yes, and it is the right call for the right reason. The issue itself offered the negative-result exit, so the risk is that declining was a convenience. It was not:

• The corpus is adversarial toward the declined shape, not neutral. Five-member enums and phrasings like "checked out, in repair, or retired" are the maximal pull toward in. The model took that pull — 6/21 filters — and still emitted a proper array every time. A negative result under deliberate pressure is much stronger than one on ordinary traffic.
• The decline is load-bearing on evidence, not on the prompt-shape argument the issue offered. The PR explicitly demolishes that argument. Had it kept the issue's reasoning, a future prompt edit widening the operator vocabulary would silently invalidate the conclusion with nothing failing.
• Adding repair_scalar_in_operator_values to this seam would be a repair for a shape with zero observations — dead code by the project's own standard, and the specific liability search_nodes 'in' operator gets a comma-joined string, not an array: filter silently matches nothing #2182 was about.

On the defect actually found: the bare-YYYY-MM-DD failure is genuinely upstream of every existing repair. All four, and coerce_filter_value_to_field_type, operate on an already-parsed Value — and parsing is what failed. The issue's suggested "more uniform" alternative (routing through repair_parsed_tool_arguments) would not have caught it either. That claim is correct as stated.

---

Findings

🟡 Important

📁 packages/agent/tests/live_resolve_query_decomposition_shapes.rs:349-352 — the duplicated extract_json_object is not identical to production, and its doc comment asserts that it is.

The comment reads: "Reproduced rather than shared because the production helper is private; keeping it identical is what makes the classification describe production's behavior and not this test's." It has already drifted:

	Production (tools.rs:199)	Harness copy (line 353)
depth type	i32	usize
iteration	bytes.iter().enumerate().skip(start)	text[start..].char_indices()
slice	&text[start..=i]	&text[start..start + i + c.len_utf8()]

I verified the two are behaviorally equivalent on the cases that matter (prose wrapping, markdown fences, nested objects, } inside strings, escaped backslashes, multibyte leading text, unterminated input) — all eight probes agreed. So this is not a correctness bug today, and it is not a blocker.

But the comment is now load-bearing documentation that is false, which is worse than no comment: the next reader has an explicit assurance of identity that a diff refutes in three places. This is the drift the comment itself predicted, arriving before the PR merged. Two clean options:

1. Preferred: make it pub(crate) in production and import it, or move it to a shared test-support module. The stated obstacle ("the helper is private") is a one-word fix, and DRY here protects a semantic guarantee, not just a few lines.
2. Failing that, weaken the comment to what is true — "behaviorally equivalent, verified against the cases in tools.rs::tests; re-check on change" — and note the deliberate usize/i32 difference (safe only because the scan starts at the first {, so depth is >= 1 before any } can decrement it; a usize underflow would panic in debug otherwise). That reasoning is currently unwritten and non-obvious.

Principle: DRY, applied to invariants. A comment claiming an invariant that the code does not hold is a liability — it stops the next reader from checking.

---

🟢 Suggestions

📁 tools.rs:2231-2252 — the negative result's denominator has an unstated blind spot.

The coverage-boundary comment states the four repairs came back inert across "33 decompositions." But classify_filter only ever runs on decompositions that parsed — an unparseable one increments unparseable_output and its filters are never classified (harness lines 474-484). With 33 decompositions yielding 21 filters, some decompositions contributed no classified filters at all, and the bare-date case (3/3 reps on one request) is by construction among them.

The Tally.unparseable_output doc comment handles this honestly and the pre-repair classification choice is right. The gap is only in the production comment, which cites 33 as though every decomposition was inspected for the four shapes. Suggest making the denominator explicit — "zero across the 21 filters classified; the decompositions that failed to parse were not inspected for these shapes" — so a future reader does not over-read the negative result's coverage.

Principle: state a measurement's scope alongside its result; an unqualified denominator invites over-reading.

📁 tools.rs:2192-2205 — consider the prompt-side half of the defense-in-depth pattern this codebase already uses.

The repair is correctly placed — pre-parse is the only place this shape is reachable, and the doc comment argues that well. My question is whether it should be the only move. render_resolve_query_field_line (tools.rs:349) documents the established pattern for the closely analogous number/boolean case: the "cheaper to try first" prompt hint ("JSON number, not string") plus coerce_filter_value_to_field_type as the backstop.

The date case currently has only the backstop. And the prompt is the proximate cause — your own comment says so: it instructs the model to resolve to "a concrete YYYY-MM-DD value", and YYYY-MM-DD written bare is exactly what came back. A one-clause change (a concrete YYYY-MM-DD value, as a JSON string) targets the cause and costs nothing. Two caveats keep this a suggestion rather than a request:

• Prompt edits on this stack need measurement, not assumption — you now have the harness to do it, but it is a live E4B run, not free.
• Keep the repair either way. A prompt hint is a probabilistic mitigation; the repair is the deterministic guarantee. Removing it in favor of a prompt tweak would be the wrong trade.

Reasonable to defer. Worth a line in the comment noting the prompt-side option was considered and why it was not taken, so the next reader does not re-litigate it.

Principle: fix causes where you can and symptoms where you must — the codebase already documents this exact pairing for the adjacent case.

📁 harness:404, 507 — std::env::set_var / remove_var thread-safety.

This is the only set_var in the agent test suite, so it is worth being explicit. The crate is edition 2021 (workspace Cargo.toml:17), so these compile without unsafe — the Rust 2024 unsafe_env rules do not apply and there is no build-time gate. The mutation is process-global and this is a multi_thread tokio test: setting it concurrently with another test reading the environment is UB on glibc/musl, not merely racy.

--test-threads=1 is documented in the module header (line 32) but nothing enforces it, and it does not protect against other integration-test binaries, which cargo runs in separate processes — so that part is actually fine. The residual risk is small (#[ignore]d, run explicitly, sole test in its binary). Two cheap hardenings if you want to close it: assert the binary contains one test, or note in the comment at line 403 why the ordering is safe here (single test, own process) rather than only what the flag does.

remove_var at line 507 is also skipped on panic — harmless for an #[ignore]d harness in its own process, but worth knowing it is not a guaranteed cleanup.

📁 harness:459, 469 — the dump file is fully re-read and re-parsed twice per rep.

decomposition_responses reads and JSON-parses the entire growing dump on each call, and it is called twice for each of the 33 reps — O(n²) in dump size, with decomp_seqs.contains(seq) adding a linear scan per response. Against a ~5GB model load and 33 live generations this is thoroughly in the noise, and the simplicity is worth more than the cycles here. Noting it only so it is a known property if the corpus grows by an order of magnitude.

---

Nitpicks

• Nit — tools.rs:2216: truncate(&json_slice, 512) truncates the repaired slice, not the raw model text. When the parse fails despite repair, the raw emission is the more diagnostic artifact. Minor, since the two differ only when a repair fired and the result still did not parse — but that is precisely the case someone would be debugging from this log line.
• Nit — harness:472: after.iter().skip(before) assumes the dump only ever appends and that no other engine call interleaves a resolve_query-marked prompt. True today (append-only writer, single-threaded test), and the seq-correlation makes it robust — just an implicit assumption worth a half-line.
• Nit — working tree: three good tests (..._is_utf8_safe_around_multibyte_text, ..._handles_date_at_end_of_input, ..._respects_escaped_quotes_in_strings) exist locally but are not committed — git status shows M packages/agent/src/local_agent/tools.rs and the remote branch matches HEAD without them. They cover exactly the boundary conditions this review would otherwise have asked for. Please commit them before merge.

---

What This Gets Right

Worth naming explicitly, because these are the parts most often skipped:

• The integration test asserts identity, not parseability. resolve_query_recovers_from_bare_unquoted_date_in_decomposition asserts resolved == true and title == "Overdue invoice". Its doc comment explains precisely why that distinction matters here: pre-fix, this case did not fail loudly — it returned a well-formed multiple_matches. A parseability-only assertion would pass with the call site unwired. This is the difference between a test that guards the bug and one that guards the helper.
• The decline cases are real tests, not decoration. ..._declines_longer_digit_runs asserts the output is still unparseable — i.e. it verifies the function correctly refuses to guess, rather than merely verifying it did not crash. Asserting Cow::Borrowed for the well-formed path pins the zero-allocation claim to something enforceable instead of a comment.
• The harness guard actually catches its stated failure mode. I traced this specifically, since a stale prompt marker producing a silent all-zero tally is the worst outcome for a measurement harness. seen_responses is assigned from decomposition_responses().len(), which is already filtered by the marker — so a stale marker yields zero, and the assertion fires with a message naming the marker as the thing to check. The guard is on the right variable.
• The previously-silent whole-resolution fallback now warns. Small change, directly addresses "that is how the bare-date malformation went unnoticed until it was measured."
• Commit messages carry the evidence. Counts, reps, the corpus delta (12/33 → 15/33, attributed entirely to the one request), and the correction to the issue's reasoning. Reconstructable in six months.

---

Recommendation

APPROVE. The measurement is sound, the decline is evidence-backed rather than convenient, the repair is correctly scoped and placed at the only layer where it is reachable, and I could not construct an input that makes it produce wrong-but-parseable output. Net-positive by a wide margin.

Before merge: commit the three uncommitted tests (nit 3). The extract_json_object doc-comment inaccuracy (🟡) should be resolved but need not block — either share the helper or correct the comment. The prompt-side hint and the denominator wording are genuinely optional.

[malibio]

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

[malibio]

Review addressed — b67e6fd1

Thanks for the independent re-derivation on quote_bare_date_literals — extracting it into a scratch crate and running 25 adversarial inputs against the wrong-but-parseable failure mode is exactly the check that mattered, and it's more convincing than the doc comment arguing the same thing.

✅ Addressed (3)

🟡 extract_json_object duplication + false doc comment. You were right that the comment was load-bearing and wrong — the copy was only behaviorally equivalent (i32 vs usize depth, byte iteration vs char_indices), and the comment claimed identity. Rather than weaken the comment, I took the option you flagged as a one-word fix: made the production helper pub and deleted the copy. That removes the drift risk instead of documenting it.

Verified the swap changed nothing: re-ran the live corpus against the model after the change — identical tally, resolved: 15/33 unchanged. The equivalence now holds by construction rather than by coincidence.

🟢 Negative-result denominator. Corrected. The comment now states the honest denominator — 21 classifiable filters — and says explicitly why it's narrower than 33 (a decomposition that doesn't parse yields no filters to inspect, so the bare-date cases contributed none). Good catch; the original phrasing overclaimed the inspection surface.

🟢 Fallback logs the repaired slice. Switched to logging raw text, with the reasoning inline: when that warn fires the repairs didn't rescue the output, so the raw emission is the half that names the shape a future repair would need to cover.

Nit — uncommitted tests. Committed. UTF-8 safety around multibyte payloads, date at end-of-input with no trailing byte, and escaped quotes inside strings. Agent lib tests 536 → 539.

⏭️ Skipped (3), with reasons

🟢 Prompt-side hint demanding quoted dates. Skipping as scope creep. It's additive rather than a fix to this defect, and as you noted the repair is the guarantee while a prompt hint is probabilistic. Worth a follow-up issue if the shape recurs on other date fields — but adding it here would mean shipping an unmeasured prompt change alongside a measured repair, and this PR's whole argument is that those are different things.

🟢 O(n²) dump re-read. Skipping — your own assessment, which I agree with: noise against a 5GB model load. Revisit if the corpus grows an order of magnitude.

🟢 set_var thread-safety. Skipping. Crate is edition 2021 so the Rust 2024 unsafe-env rules don't apply, and the residual risk is bounded as you described — sole #[ignore]d test in its own binary.

Verification

• bun run quality:fix clean; clippy -D warnings passes
• Agent lib: 539 passed, 0 failed
• Push gate green (full test:all + cargo build --bin nodespaced + e2e)
• Live corpus re-run post-refactor: tally and resolution rate unchanged

Re-review

NO RE-REVIEW NEEDED. All three addressed items are documentation-accuracy and diagnostic-plumbing changes plus a de-duplication that provably preserves behavior — no logic in the repair or the decision path was touched. The one change with any behavioral surface (sharing extract_json_object) was verified against the live model with an identical result.