GLOOK-54: repair the model's JSON, and bust the proxy cache when repair can't

[msogin]

Fixes GLOOK-54.

The bug was one character

The Top Projects card showed "Couldn't build the project summary right now" and reloading never helped. Recurring since 2026-08-25 on roughly one report a week — five so far — while other reports of the same size worked.

Diagnostics landed first (f2c3911), because the log recorded content_chars and never the content, which is why this went five occurrences without a diagnosis. The window it produced:

…"RPS-10766","RPS-10768"],
        },                    ← trailing comma before }
        {"name":"Video/Audio Processing", …

The model closes a jira_keys array, emits ,, then }. JSON forbids that, so JSON.parse reports "Expected double-quoted property name" at the brace — it's looking for the next key. Not truncation (finish_reason=stop, 14k of a 32k budget); the rest of the 14,268-char response is well-formed.

Why reloading never helped — measured, not assumed

The AI Proxy caches completions. Probed with a prompt asking for a random hex string, so the result can't be a latency artifact:

identical #1      2431ms  ->  a3f9c2e7b1d4
identical #2       224ms  ->  a3f9c2e7b1d4   same value, 10× faster  ⇒ CACHED
message nonce     1866ms  ->  a3f9c21b7e4d   different value          ⇒ salt works
extraBody nonce     38ms  ->  a3f9c2e7b1d4   same value, cache hit    ⇒ salt IGNORED

The salt must go in the user message. A nonce in smartling_additional_properties — the obvious place, since we already send that field — is ignored by the proxy. Guessing would have shipped a retry that silently re-served the same broken JSON and looked like it worked. There was also no retry anywhere before this: each reload was a fresh request hitting the same cached completion, which is why the failure was byte-identical for 10+ hours.

Three parts

1. src/lib/llm-json.ts — parseModelJson(). Repair only after a clean parse fails, so valid output is never touched; repaired is reported so model drift stays visible. The comma stripper scans string literals rather than pattern-matching — this payload is full of Jira summaries and commit messages that can contain ", }", and a bare replace(/,\s*([}\]])/g,'$1') corrupts them silently. Same hazard as GLOOK-41's outsideStringLiterals, but JSON escapes a quote with a backslash where SQL doubles it, so it needs its own scanner.

2. Salted retry when repair can't help — covers artifacts repair can't fix. Report c28781b4 failed with Unexpected token 'r', i.e. prose rather than a comma.

3. The retry is bounded by a 10-minute per-report marker on globalThis (matching the progress and stop-signal stores). Successes are cached so the model is normally called about once per report — but failures are deliberately not cached (incident 2026-08-03), so every page load re-enters this path. Today that's cheap because the proxy serves the cached completion in ~200 ms; a salted retry is a deliberate cache miss costing a full ~200k-char generation, and da2fa78f logged 15 failure calls in one day. Without the marker, one broken report buys a fresh generation per page load.

Safety

ModelJsonError carries position/window/head/tail for logging but keeps model output out of message, because route.ts serialises err.message into the 5xx body and this payload contains internal commit text. There's a test asserting that specifically.

The 2026-08-03 invariant is preserved and pinned by a test: a truncated completion is still never repaired and never cached. Repair is for malformed output, never for severed output.

Tests

129 suites / 1276 tests; tsc --noEmit and npm run build clean.

The new coverage uses the real captured bytes from the incident, and includes the cases a naive implementation would fail: a comma-brace sequence inside a string must survive untouched, an escaped quote before one must not confuse the scanner, repair must not spend a regeneration, the salt must land in the user message with the payload unchanged, and the cooldown must suppress the second retry.

Reviewer note

Honest scope: repair is the part that can be promised — the artifact is characterised and the bytes are in hand. Whether a fresh generation avoids the comma is unknown; it may reproduce it. The retry is a safety net, not a guarantee. If the logs later show RETRIED=salted followed by another failure, that tells us fresh generations reproduce the artifact and the prompt itself needs attention.

Not closed by this PR, and still recorded on GLOOK-51: report-highlights/service.ts:144-171 and report/summary.ts:226-263 both parse model JSON with only a fence-strip, so both are exposed to the identical artifact. They should move to parseModelJson() — I kept this diff to the module that's actually broken.

🤖 Generated with Claude Code

[msogin]

Review — GLOOK-54

Three independent reviews (Sr. Architect lens, Senior Dev lens, and the standard Smartling fullstack profile) run against 165b65a..ff56c43. Every claim below was verified by execution on this repo's Node (v25.8.2) rather than by inspection; findings that didn't survive that check were dropped, and the set was deliberately trimmed to what changes the merge decision.

One blocker, two to fix before merge.

	Finding	Where
🔴	ModelJsonError.message carries model output into the 500 body — and its guard test cannot fail	llm-json.ts:29
🟡	The salted retry fires on truncated completions, burning the cost bound it was added to enforce; the same catch mislabels transport failures	insights.ts:340
🟡	The cooldown marker is never cleared on success, so a successful-but-degraded retry turns the card into a 500 for the rest of the window	insights.ts:25

The 🔴 is the blocker because it falsifies a confidentiality property the PR states in its own description and certifies with a test — and the test passes only because its fixture happens to fail at position 35, which is the one V8 message shape that carries no snippet.

Verified correct

The repair scanner itself is sound, and that's the core of the change. Traced by hand and executed: escaped quotes, an escaped backslash immediately before a closing quote, \uXXXX, a trailing backslash at EOF, commas before ] as well as }, nested [{...,},{...,},], and comma-brace sequences inside Jira summaries all behave correctly. No input was found that turns invalid JSON into different but valid JSON — desyncing inString requires an unbalanced quote, which leaves the document unparseable anyway. Truncated bodies cannot be repaired into a cacheable success: brute-forcing all 172 prefixes of a trailing-comma document yielded 0 repairs, so the 2026-08-03 invariant does hold in behaviour. The salt assertions genuinely pin "payload unchanged, salt appended", and the check-then-set at :340-341 is race-free.

Noted, not raised inline

Adjacent trailing commas aren't repaired ({"a":1,,} -> {"a":1,}, still invalid) — single left-to-right pass, low realism for a model artifact. repaired is logged but not persisted into the no-TTL report_comparisons cache, so there's no way to find which cached reports were built from machine-edited bytes. stripJsonFences keeps its opening fence when the content has a leading newline. None of these change whether this merges.

[msogin]

message does carry model output, and the test that says otherwise cannot fail.

The PR description says this class keeps model output "out of message" and that "there's a test asserting that specifically." Both halves are false, and they fail for one reason: V8 has two SyntaxError shapes.

Expected double-quoted property name in JSON at position 35   <- position only, content-free
Unexpected token 'H', "Here is th"... is not valid JSON       <- embeds the input

detail is interpolated into super() on this line, so the second shape puts model bytes into .message. That reaches the browser: insights.ts:355/:358 embed it into ProjectInsightsGenerationError, and route.ts:9 does NextResponse.json({ error: err.message }, { status: 500 }).

Verified on Node v25.8.2:

JSON.parse('Here is the JSON for INTERNAL-COMMIT-TEXT: {"projects":[]}')
  -> Unexpected token 'H', "Here is th"... is not valid JSON

The guard at llm-json.test.ts:108 passes only by fixture luck: '{"summary":"INTERNAL-COMMIT-TEXT", oops}' fails at position 35, which yields the positional shape with no snippet. A fixture failing at position 0 makes that assertion fail today.

The same fact breaks the diagnostics on lines 32-37. /position (\d+)/ matches only the positional shape, so for Unexpected token the position is -1 and window is '' — and insights.ts:309's if (err.position >= 0) then skips the window log entirely. That is precisely the prose-preamble case (c28781b4) that f2c3911 added window logging for, so the diagnostic is absent exactly where it was meant to help.

readonly detail: string;

constructor(cause: unknown, text: string) {
  const detail = cause instanceof Error ? cause.message : String(cause);
  super('model JSON unparseable after repair');   // no interpolation
  this.detail = detail;                            // logs only, same rule as window/head/tail
  ...
  this.window = this.position >= 0
    ? text.slice(Math.max(0, this.position - W), this.position + W)
    : text.slice(0, 2 * W);                        // never silently empty
}

and re-point the guard test at a position-0 fixture so it can actually fail.

[msogin]

Fixed in 93d4d18. You're right on all three parts, and I verified each by execution rather than taking it on trust:

position 0   ->  Unexpected token 'I', "INTERNAL-COMMIT-TEXT" is not valid JSON   LEAKS
prose        ->  Unexpected token 'H', "Here is th"... is not valid JSON          leaks first chars
my fixture   ->  Expected double-quoted property name … position 35               no leak

So the PR asserted a confidentiality property, certified it with a test, and the test passed only because its fixture landed on the one shape that carries no snippet. That's worse than an untested claim — it's a claim with a green check next to it.

message is now the fixed string 'model JSON unparseable after repair', and V8's own text moves to a detail field under the same rule as window/head/tail: logs only. The guard test is parameterised over all three shapes — including both leaking ones — and asserts message equality rather than absence, so a future interpolation can't slip past it.

The diagnostic half was the more damaging finding, and I'd missed it entirely. /position (\d+)/ matches only the positional shape, so for Unexpected token the position was -1, the window '', and insights.ts guarded its log on position >= 0 — meaning the window was absent for exactly the prose-preamble case (c28781b4) that f2c3911 added window logging to diagnose. The window now falls back to text.slice(0, 2 * W), the log prints unconditionally and includes detail, and there's a test asserting the window is non-empty for the no-position shape.

[msogin]

The retry spends its one regeneration on the case it cannot help, and mislabels transport failures.

Truncation is never excluded. finishReason is read at :296, reassigned at :349, and interpolated into the log at :364 and the error at :377 — it gates nothing. A finish_reason=length body therefore reaches mayRetry and spends the report's single 10-minute regeneration re-sending the same ~200k-char prompt at the same tokenLimit(32000) — with the salt appended, so the prompt is strictly longer and re-truncates with near-certainty. A permanently oversized report then buys one doomed full generation every 10 minutes, indefinitely: the runaway RETRY_COOLDOWN_MS exists to stop, slowed 10x rather than prevented.

To be precise about what is not broken: the 2026-08-03 invariant still holds behaviourally. stripTrailingCommas only deletes characters, so it can never close a severed bracket — brute-forcing every prefix of a trailing-comma document produced 0 repairs. The invariant just isn't expressed in code, which is what makes it fragile: the natural next repair (closing unterminated strings/brackets) would silently make truncated bodies parseable, and therefore cacheable.

if (finishReason === 'length') {
  failure = 'truncated completion';    // never repair, never retry, never cache
} else if (!stripJsonFences(content)) {
  failure = 'empty completion';
} else {
  // existing parse + salted retry
}

That second branch also closes a smaller gap: :324 now tests !content.trim() on the raw content, so a fence-only completion — the artifact stripJsonFences exists for — skips empty completion, fails as a parse error, and buys a regeneration.

The catch owns too much. await generate(...) and the parse share one try, so a 429, timeout, auth expiry or upstream 5xx becomes failure = "json parse after salted retry: <provider message>". That is the wrong classification for anyone reading errors.log, a second path for provider text into the 500 body, and it consumes the cooldown for something that was never a JSON problem. Because the assignments at :348-349 never run, logParseFailure(e2, content, 'retry-window') also logs the first attempt's text, and :364/:377 report the first attempt's finish_reason and content_chars.

Splitting the await from the parse lets each failure keep its own name.

Neither truncation test asserts mockCreate call count, so nothing currently catches the extra generation — expect(mockCreate).toHaveBeenCalledTimes(1) would pin it.

[msogin]

Both fixed in 93d4d18.

Truncation now gates before repair and retry. You're right that finishReason gated nothing — and the detail that makes it worse is the one I'd overlooked: the salt is appended, so the retry prompt is strictly longer at the same tokenLimit(32000) and re-truncates with near-certainty. A permanently oversized report was buying a doomed ~200k-char generation every cooldown window, forever. The runaway wasn't prevented, just decimated.

finish_reason === 'length' → failure = 'truncated completion', no repair, no retry, no cache. And I took your framing about why to state it in code: the 2026-08-03 invariant currently holds only because stripTrailingCommas deletes characters and so can't close a severed bracket. That's a property of today's repair, not a rule. Written as a rule, the obvious next repair — closing unterminated strings — can't silently make truncated bodies parseable and therefore cacheable. The comment says exactly that.

The fence-only gap is fixed too — emptiness is now tested on stripJsonFences(content), so a ```json\n``` completion is empty completion rather than a parse error that buys a regeneration. Good catch; that's the artifact the stripper exists for, slipping past the check meant to catch it.

The await is now outside the parse try/catch. A transport failure raises salted retry request failed: <error name> — name only, no provider text, since you'd correctly flagged it as a second route into the 500 body. It also no longer consumes the cooldown for something that was never a JSON problem, and no longer logs the first attempt's bytes under a retry-window label.

And you were right that nothing caught the extra generation. expect(mockCreate).toHaveBeenCalledTimes(1) is now pinned on both pre-existing truncation tests plus a new one, so the retry-on-truncation regression can't come back silently.

[msogin]

The marker is never cleared on success, and that produces a concrete bad state.

It records when a retry started (:341) and is never cleared. Consequence:

A salted retry succeeds, but the fresh generation attributes 0 projects, so degraded at :488 serves it uncached. The next page load re-enters, the unsalted generate() gets the proxy's cached broken completion, mayRetry is now false, and the card goes from "empty but rendering" to a 500 for the rest of the 10 minutes — with a good generation already discarded. retryStore.delete(report.id) on the success path closes that.

Two smaller things in the same store. Entries are only ever added, so the map grows one entry per failing report id per process lifetime with no eviction path. And because it lives on globalThis it is per Node process: this repo ships a Dockerfile, a docker-compose.yml and GHCR publishing, so more than one replica is the expected deployment, which makes the real bound N regenerations per 10 minutes, reset by every deploy. The doc comment at :9-23 states the bound unqualified — worth either saying "per process" or persisting the marker alongside report_comparisons so it is a real one.

(The globalThis placement itself matches progress-store.ts and report-runner.ts, and the check-then-set at :340-341 is correctly race-free — there is no await between them, so two concurrent loads cannot both spend a retry.)

[msogin]

Fixed in 93d4d18, and thank you for spelling out the sequence — "a good generation already discarded" is what made it obvious this wasn't theoretical.

retryStore.delete(report.id) now runs on any usable generation, before the return, so it clears on the degraded-and-uncached path too — which is precisely the one that produced your bad state. There's a test that reproduces it: retry succeeds, then the same report fails again immediately and must still be able to retry rather than being suppressed by a stale marker.

Unbounded growth — added pruneRetryStore(), called from mayRetry, which drops entries older than the cooldown. They can't suppress anything by then, so the map can't grow one entry per failing report id for the process lifetime.

Per-process bound — you're right and I've said so in the code rather than quietly leaving the claim unqualified. The comment now states the bound is per process, notes that a container image with GHCR publishing and a compose file means multiple replicas are the expected deployment, so the real ceiling is N regenerations per window across N replicas and it resets on deploy. Making it a true global bound means persisting the marker alongside report_comparisons; that's recorded on GLOOK-54 rather than smuggled in here, since it turns an in-memory guard into schema.

Thanks also for confirming the check-then-set is race-free — I'd reasoned it was (no await between) but hadn't verified it, and it's the kind of thing I'd rather have checked than assumed.

[msogin]

Review addressed — 93d4d18

finding	outcome
🔴 message carries model output; guard test cannot fail	Fixed — message is fixed text, detail holds V8's; guard parameterised over all three shapes
🟡 Retry fires on truncation; catch mislabels transport failures	Fixed — truncation gates first; await split from parse
🟡 Cooldown never cleared on success	Fixed — cleared on any usable generation, store pruned, bound documented as per-process

The blocker was worse than an untested claim

It was a claim with a green check beside it. The PR asserted model output stays out of message, pointed at a test, and the test passed only because its fixture landed on position 35 — the one V8 shape that carries no snippet. Verified by execution:

position 0   ->  Unexpected token 'I', "INTERNAL-COMMIT-TEXT" is not valid JSON   LEAKS
prose        ->  Unexpected token 'H', "Here is th"... is not valid JSON          leaks
my fixture   ->  Expected double-quoted property name … position 35               no leak

The guard now asserts message equality, over all three shapes, so an interpolation can't slip back in.

The diagnostic half of that finding was the more damaging one and I'd missed it entirely. Because /position (\d+)/ only matches the positional shape, the window was empty and the log suppressed for the Unexpected token case — which is exactly the prose-preamble report (c28781b4) that f2c3911 added window logging for. The instrumentation was absent precisely where it was built to help.

On the retry findings

The detail I'd overlooked: the salt is appended, so a retry after truncation re-sends a longer prompt at the same token limit and re-truncates near-certainly. The cooldown wasn't preventing that runaway, just decimating it.

I also took the framing about why to state the invariant in code. It currently holds only because stripTrailingCommas deletes characters and so can't close a severed bracket — a property of today's repair, not a rule. Written as a rule, the obvious next repair (closing unterminated strings) can't silently make truncated bodies cacheable.

Deliberately not done

Persisting the retry marker alongside report_comparisons to make the bound truly global — recorded on GLOOK-54, since it turns an in-memory guard into schema.

The three "noted, not raised" items are also left: adjacent trailing commas ({"a":1,,}), repaired not persisted into the cache so machine-edited reports can't be identified later, and stripJsonFences keeping an opening fence when the content leads with a newline. Happy to take any of them if you'd rather they didn't linger — the second is the one I'd argue for, since a no-TTL cache with no record of which entries were repaired is a thing we'd want to know later.

129 suites / 1284 tests; tsc --noEmit and npm run build clean.

Thanks for the verified-by-execution approach — it caught a confidentiality bug, a dead diagnostic, and a cost runaway that my own tests were structured to miss.