GLOOK-51: stop caching a truncated projects generation as an empty success

[msogin]

Fixes GLOOK-51.

What you'd see

The Integrations team's Current Projects section was empty on every reload, while smaller teams on the same report were fine. No error state — just blank.

What actually happened

The model produced good output and we threw it away. From app.log:

[team-pulse-projects] parse error for team=Integrations; raw={
  "projects": [
    { "name": "Datadog On-Call Migration",
      "summary": "Migrate connector fleet monitors/alerts from Opsgenie/Splunk to Datadog On-Call…",
      "developers": ["vfisyuk-smartling","dverner-smartling"],
      "jira_count": 2, "estimated_commits": 55, "estimated_prs": 20,

Well-formed JSON, cut off mid-object. With response_format: json_object, the only way to get invalid JSON is truncation — a flat tokenLimit(1500) against a 14-developer team.

And then it stuck. JSON.parse threw → the catch returned [] → service.ts cached '[]'. The lazy top-up only re-runs when the column is NULL, so one truncation blanked the section permanently. The timings show it plainly:

time	request	duration
08:53:43	team=Integrations&withProjects=true	14,037 ms — real LLM call, failed
08:57:46	same	12 ms
08:59:14	same	6 ms

team=Data took 10,327 ms and succeeded, which is why only the biggest team was affected.

Three defects, each necessary

1. Flat token budget. Now scales: 2000 + 400/member, capped at 8000. projects/insights.ts runs at 12000+ for comparable clustering.
2. finish_reason never read here. Truncation was reported as a parse error, which reads like a model/prompt problem rather than a budget one — it sent me looking in the wrong place first. projects/insights.ts:260 and projects/untracked.ts:297 already do this check; this module never got it, and insights.ts even carries a comment about hitting this exact truncation at a previous limit.
3. The worst one: a failure was indistinguishable from "no projects". Both returned []. Unusable output now raises TeamProjectsUnusableError, and both call sites in service.ts leave the column NULL so the next load retries. A genuine empty result is still cached, so the page doesn't pay for the LLM on every view.

Recovering existing reports

PROMPT_VERSION → v5-budget. It's part of the cache key (report_id + team_name + prompt_version), so every already-poisoned row is invalidated on deploy. No DB surgery, and it fixes every affected team on every past report, not just Integrations.

Cost note: the bump also invalidates healthy pulse summaries, so the first view of each team after deploy regenerates. That's a one-off, and it's the price of not hand-editing rows.

Tests

127 suites / 1250 tests. The existing generator suite had no coverage of the parse-error path at all, which is how this could regress silently. New suite pins: truncation raises and says so; unparseable-but-not-truncated raises differently; the two are reported distinctly; a genuine empty stays a cacheable success; nothing-to-cluster short-circuits without an LLM call; and the scaled budget is actually what gets requested.

🤖 Generated with Claude Code

[msogin]

Code review — 3 reviewers (Senior Dev, Sr. Architect, Smartling fullstack profile)

The forward-looking half of this change is right: throwing instead of returning [], and not persisting a failure, is the correct shape and matches what insights.ts already does. The recovery half does not work, and the teams this ticket was filed for stay blank after merge.

---

🔴 Blocking — the PROMPT_VERSION bump does not invalidate the poisoned rows

All three reviewers found this independently. Every link verified against the repo:

1. prompt_version is in the read key but not the write key — the constraint is UNIQUE KEY uq_report_team_pulse (report_id, team_name) (schema.sql:222, mysql.ts:117, sqlite.ts:227, conflict map sqlite.ts:492).
2. The v5 SELECT misses the v4 row, so the code regenerates and upserts — landing on the same physical row.
3. TeamPulseCard (page.tsx:416-418) fetches with no withProjects param on mount, so withProjects=false and projectsForDb = null.
4. projects = COALESCE(VALUES(projects), projects) (service.ts:163) therefore preserves the poisoned '[]' while stamping prompt_version = 'v5-budget'.
5. The row now has a truthy projects, so the lazy top-up guard row.projects === null (service.ts:64) never fires again. Blank forever, under the new version.

The same COALESCE also defeats the projectsForDb = null fix on any pre-existing row, which is the other half of the change.

Note the parsimonious reading: the bump plus its justifying comment is currently a no-op that additionally mis-stamps rows. Removing it is as valid as repairing it — but the service.ts:9-11 claim "No DB surgery needed to recover reports" is false as written and should not ship in that form.

If you do want v5 to self-heal, the VALUES(col) → excluded.col rewrite in translateSQL means this works on both drivers:

projects = CASE WHEN VALUES(prompt_version) <> prompt_version
                THEN VALUES(projects)
                ELSE COALESCE(VALUES(projects), projects) END

Deleting the COALESCE outright is not the fix — it reintroduces the race documented at service.ts:157-159.

Two related comment inaccuracies, worth correcting because these comments are the incident record for the next on-call:

• projects.ts:101-104 — "Same check projects/insights.ts and projects/untracked.ts already do." Neither does. insights.ts:260,291 reads finish_reason only to classify a failure message after a failed parse; untracked.ts:297-301 reads it and only console.warns, then continues into the parse. Only one file is a partial precedent, and neither throws pre-parse.
• projects.ts:33-36 — "projects/insights.ts runs at 12000+ for similar clustering." insights.ts:247-249 records 12000 as the value that truncated and now runs tokenLimit(32000). The cited precedent argues the 8000 ceiling is low, not that it is adequate.

And nothing would catch this. Every new test targets projects.ts; no test covers service.ts:144-153 or :76-82. Reverting projectsForDb = null to JSON.stringify([]) leaves the suite green. Worth adding once the storage behavior above is settled — a mocked-DB test written today would assert that null is passed and pass, while the row still ends up '[]'.

---

🟡 The finish_reason === 'length' branch is dead on Bedrock

bedrock-adapter.ts:70-79 builds choices[0] from decoded.content[0].text and never copies decoded.stop_reason. finishReason is therefore always 'unknown' (projects.ts:97), the new branch never fires, and a Bedrock truncation falls through to the parse and is reported as unparseable response ... finish_reason=unknown — the exact misdiagnosis this PR says it eliminates. llm-mock.ts:87-96 has the same gap.

As written, change (2) earns nothing on that provider. Either make the adapter emit it (one line, also repairs the existing checks in insights.ts:260 and untracked.ts:297) or drop the branch:

finish_reason: decoded.stop_reason === 'max_tokens' ? 'length'
             : decoded.stop_reason === 'end_turn'   ? 'stop'
             : (decoded.stop_reason ?? 'stop'),

Flagging that bedrock-adapter.ts is outside this PR's three files — your call whether it belongs here or in a follow-up.

---

🟡 A third path still caches an unusable empty result as a success

If the model returns projects but every one fails the teamSet filter (projects.ts:132), out is [] and the caller caches it as a genuine success — same permanent blank, different cause (e.g. the model emits display names instead of GitHub logins). This is not hypothetical: projects.ts:155-164 already logs it by name via dropped_no_devs.

The counter-argument is real — if the model named people who aren't on the team, [] is a defensible answer, and throwing would convert a plausibly-correct empty into a retry loop. But insights.ts:398-412 already resolved exactly this tension, and neither option above is what it chose:

// A parseable response that still attributed nothing is not worth persisting
// either — it is indistinguishable to the reader from "this report has no
// projects", and caching it would be permanent. Serve it, don't store it, so
// the next request retries.

Serve empty, don't persist. No throw, so no retry-loop cost. Suggest matching that here: when llmProjectCount > 0 && out.length === 0, return [] but signal the caller not to cache it.

---

🔵 Discriminate the catch on the error type

Both catch sites are untyped catch (err) (service.ts:76, :144), so a TypeError in the enrichment loop or a DB outage inside extractTeamProjectsData is classified identically to a truncation: transient, leave NULL, retry. Combined with NULL-means-retry, a deterministic code bug becomes an unbounded paid retry rather than an error someone sees.

Gating on err instanceof TeamProjectsUnusableError — letting anything else propagate to the 500 that route.ts:52 already produces — bounds the retry to the failure class this PR actually introduces, and gives the newly exported error type its only consumer. Without it the export is unused public surface.

(The cost direction is worth keeping in view generally: pre-PR a permanently-failing team cost one LLM call ever; post-PR it costs one per card expand. Self-limiting because expansion is deliberate, so not blocking — but the type check is the cheap half of bounding it.)

---

🔵 Nits

• team-projects-truncation.test.ts:124 — expect(...max_tokens).toBe(projectsTokenBudget(14)) compares the implementation to itself; a regression to a flat 1500 still passes. Assert the literal 7600.
• Same file — the mock returns max_tokens, but the real tokenLimit() returns max_completion_tokens for the openai provider (llm-provider.ts:51-58). The assertion is against the mock's shape, not production's.
• projects.ts:42 — Math.max(teamSize, 0) guards an input that comes from data.team_members.length and cannot be negative; the test at :36 then defends the guard. Both are candidates for deletion.
• projects.ts:86 and :108 — projectsTokenBudget(data.team_members.length) is computed twice; hoist to a const so the logged budget is provably the requested one.

---

Follow-ups (not this PR)

• report-highlights/service.ts:144-171 is a line-for-line clone of this bug: no finish_reason check, catch { parsed = { highlights: [] } }, unconditional cache write, no TTL, recoverable only by a version bump. report/summary.ts:226-263 is a milder variant with no version key at all, so it has no escape hatch. Worth a ticket — this class isn't closed.
• projects.ts:120 embeds up to 500 chars of raw model output (commit messages, Jira summaries) in err.message. Harmless while both call sites swallow it, but route.ts:52-55 serializes err.message into a 500 body — so it reaches the client the moment any call site rethrows, which the NULL-means-retry contract invites.

---

Ready to merge? Not yet — the blocking item above means the reports GLOOK-51 was filed for stay blank after deploy.

🤖 Automated review via Claude Code (3 reviewer passes + minimalist triage gate)

[msogin]

Review addressed

The blocking finding was correct and it was the worse kind of wrong: the PR claimed to fix GLOOK-51 and would have left every affected team blank, under a version stamp asserting it was fixed. Thank you for verifying it against the schema rather than taking the comment at face value.

🔴 The PROMPT_VERSION bump — fixed

I confirmed your chain link by link: UNIQUE (report_id, team_name) (schema.sql:222, sqlite.ts:227, conflict map sqlite.ts:492), prompt_version in the read key only, TeamPulseCard mounting without withProjects, and COALESCE(VALUES(projects), projects) therefore preserving '[]' while stamping v5. "No DB surgery needed" was false as written, and the comment that said so is gone.

Took your CASE WHEN — a version change now takes the incoming value verbatim, including NULL, so the row reverts to "not yet generated" and the lazy top-up regenerates it. COALESCE is kept within a version, so the race at service.ts:157-159 is still protected. Confirmed translateSQL's VALUES(col) → excluded.col rewrite is a global regex, so it applies inside the CASE on SQLite too.

And you were right that nothing would catch it. Every test targeted projects.ts. The new team-pulse-projects-cache.test.ts drives the real SQLite driver, precisely because — as you said — a mocked DB would assert null was passed and pass while the row ended up '[]'. It includes a negative control that executes the pre-fix SQL and asserts the poison survives next to the new version stamp, so the CASE is provably load-bearing rather than decorative.

Both comment inaccuracies — corrected

You were right on both, and they mattered because those comments are the record for the next reader:

• insights.ts/untracked.ts do not do this check. insights.ts reads finish_reason only to classify a message after a parse has already failed; untracked.ts logs it and continues into the parse. Neither refuses a truncated response up front. The comment now says this is a stricter check than either precedent.
• 12000 was the value that truncated for insights.ts, which now runs 32000. My citation argued the opposite of what I claimed. The comment now frames 8000 as conservative rather than known-sufficient, and says to raise CEILING if a finish_reason=length ever appears for this prompt.

🟡 Bedrock — fixed here, not deferred

Dead branch confirmed: the adapter never copied stop_reason, so finishReason was always 'unknown' and a Bedrock truncation got reported as unparseable — the exact misdiagnosis this PR claims to remove. Mapped as you wrote it. I included it despite being outside the original three files because without it change (2) earns nothing on that provider, and the one-line fix also repairs the existing reads in insights.ts and untracked.ts. llm-mock.ts had the same gap and now emits finish_reason: 'stop'.

🟡 The third empty path — fixed, following your precedent

Matched insights.ts:398-412 exactly: serve empty, don't persist. No throw, so no retry-loop cost. generateTeamProjects now returns { projects, cacheable }, with cacheable: false when llmProjectCount > 0 && out.length === 0. A genuine empty and the nothing-to-cluster short-circuit both stay cacheable, so the page doesn't re-pay for the LLM.

🔵 Typed catch — fixed

Both sites now gate on err instanceof TeamProjectsUnusableError and let anything else propagate to the 500 route.ts:52 already produces. You were right that this is what gives the exported type its only consumer — without it I'd added unused public surface.

🔵 Nits — all four

Literal 7600 instead of comparing the implementation to itself; mock now mirrors tokenLimit()'s real provider-dependent shape (max_completion_tokens as well as max_tokens); the impossible Math.max guard and the test defending it are both deleted; budget hoisted to a const so the logged value is provably the requested one.

Follow-ups — filed

Both recorded on GLOOK-51 for their own ticket:

• report-highlights/service.ts:144-171 is a line-for-line clone of this bug — no finish_reason check, catch { parsed = { highlights: [] } }, unconditional cache write, no TTL. And report/summary.ts:226-263 is worse in one respect: no version key at all, so it has no escape hatch even in principle. This class is not closed.
• err.message carries up to 500 chars of raw model output, which route.ts:52-55 would serialize into a 500 body the moment any call site rethrows — and the typed-catch change I just made means non-TeamProjectsUnusableError errors now do propagate. Worth truncating harder or dropping the raw echo.

128 suites / 1258 tests; tsc --noEmit and npm run build both clean — and I ran tsc this time rather than substituting the build for it, which is how the TS1501 reached CI on the last push.