fix(directors-notes): never render the raw structured output as the report - #1331
Conversation
…eport Plain Mode fed the raw synopsis string straight to the markdown renderer whenever there was no parsed narrative, so a run whose structured output failed to parse showed a wall of raw JSON where the report should be. Rich Mode already failed loudly for the same input; Plain Mode did not. A synopsis run costs minutes of agent time, so a rejected response should not cost the whole report either. Add a salvage path that recovers what can be read and always says what it recovered. - shared/directorNotesNarrative.ts: add recoverDirectorNotesNarrative(), called only after the strict parser fails. Repairs a response cut off mid-stream (cut back to the last complete item, close what is open) and raw control characters inside strings, then validates leniently: a bad bullet is dropped, not the document. Returns a human-readable reason. - director-notes.ts handler: fall back to the salvage and return the recovered narrative plus narrativeRecovery; only a total failure returns a bare narrativeError. - AIOverviewTab: Plain Mode renders the narrative as prose or shows the parse-failure banner. It no longer dumps the raw output. One applyNarrative() helper keeps the three result paths in sync. - NarrativeParseError: now shared by both reading modes, with a softer partial-recovery state so a salvaged report never reads as complete. - CLI markdown/text output takes the same salvage, reason noted inline. - Tests for every recovery case and both Plain Mode failure surfaces.
📝 WalkthroughWalkthroughDirector’s Notes now attempts structured-output recovery after strict parsing fails. Recovered narratives and recovery reasons flow through CLI, IPC, preload, and renderer layers, while Plain and Rich modes show recovery or failure banners instead of raw structured output. ChangesDirector’s Notes narrative recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AI
participant SynopsisHandler
participant NarrativeRecovery
participant DirectorNotesUI
AI->>SynopsisHandler: structured synopsis output
SynopsisHandler->>NarrativeRecovery: recover after strict parse failure
NarrativeRecovery-->>SynopsisHandler: recovered narrative and reason
SynopsisHandler-->>DirectorNotesUI: narrative, parse error, recovery reason
DirectorNotesUI-->>DirectorNotesUI: render prose and recovery/failure banner
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function validateNarrativeLenient( | ||
| parsed: unknown | ||
| ): { narrative: DirectorNotesNarrative; dropped: number } | null { | ||
| if (!isPlainObject(parsed) || !Array.isArray(parsed.sections)) return null; |
There was a problem hiding this comment.
When the agent returns a narrative with an unsupported or missing version, strict parsing rejects it but recovery accepts it based only on sections, causing an incompatible schema to be rendered as a recovered report.
| if (!isPlainObject(parsed) || !Array.isArray(parsed.sections)) return null; | |
| if (!isPlainObject(parsed) || parsed.version !== 1 || !Array.isArray(parsed.sections)) return null; |
Greptile SummaryThis PR prevents malformed Director's Notes output from being rendered as a report.
Confidence Score: 4/5The export fallback and recovery version validation need to be fixed before merging. Unrecoverable structured output is still exported verbatim through Copy and Save, while recovery also renders narratives whose schema version the strict parser rejects. Files Needing Attention: src/renderer/components/DirectorNotes/AIOverviewTab.tsx, src/shared/directorNotesNarrative.ts Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Raw synopsis] --> B{Strict parse}
B -->|Success| C[Structured narrative]
B -->|Failure| D{Recovery}
D -->|Recovered| E[Narrative plus recovery warning]
D -->|Unrecoverable| F[Parse-failure banner]
C --> G[Rich cards or Plain markdown]
E --> G
F --> H[Raw output disclosure]
C --> I[Copy and Save]
E --> I
F --> J[Raw synopsis currently reaches Copy and Save]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/ipc/handlers/director-notes.ts (1)
789-801: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider logging
recovered.errorwhen recovery also fails.The warn log captures
narrativeErrorandrecoveryReason(only set on success), but notrecovered.errorwhen recovery itself fails - that detail (e.g. "No recoverable narrative content found in the response.") would help diagnose why a response was unsalvageable, purely from logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/ipc/handlers/director-notes.ts` around lines 789 - 801, Update the warning log in the recoverDirectorNotesNarrative failure path to include recovered.error when recovery fails, while preserving the existing recoveryReason logging for successful recovery. Keep the narrativeFields behavior unchanged.src/shared/directorNotesNarrative.ts (1)
250-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a maintained JSON-repair library instead of hand-rolled truncation/escaping repair.
closeTruncatedJsonObjectandescapeControlCharsInStringsreimplement two well-known LLM-output repair problems (closing truncated structures, escaping stray control characters in strings). A maintained library such asjsonrepairalready solves both: "The tool attempts to fix truncated JSON by adding missing closing brackets and braces." and more broadly "The jsonrepair npm package fixes over 95% of common JSON issues automatically without any configuration." The section-drop mislabeling bug found in this review is exactly the kind of subtle defect that comes from hand-rolling this class of parser. This is a non-blocking suggestion — the current implementation's per-repairreasonattribution (truncated vs. escaped) is a genuine product requirement a generic library wouldn't give you out of the box, so any switch would need to preserve that. Worth evaluating for future robustness/maintenance, not something to block this PR on.Also applies to: 406-461
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/directorNotesNarrative.ts` around lines 250 - 341, Evaluate replacing the hand-rolled closeTruncatedJsonObject and escapeControlCharsInStrings helpers with a maintained JSON-repair library such as jsonrepair, while preserving the existing per-repair reason attribution for truncated versus escaped input. Apply the same consideration to the corresponding repair logic around the additionally referenced section, and retain current behavior if the library cannot expose the required attribution reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/components/DirectorNotes/AIOverviewTab.tsx`:
- Around line 592-611: Update the outer synopsis-rendering guard in the AI
overview component so it remains active when synopsis is empty but
narrativeError is set, allowing NarrativeParseError to render instead of a blank
result. Add a regression test covering an empty synopsis with a narrative parse
failure and verifying the error alert is displayed.
In `@src/shared/directorNotesNarrative.ts`:
- Around line 349-392: Split the recovery accounting in validateNarrativeLenient
into separate section and item drop counters, incrementing each at its
corresponding validation failures and returning both counts. Update
recoverDirectorNotesNarrative to describe dropped sections and bullets
accurately, including correct singular/plural wording, and add a regression
assertion for the reason when an invalid section is discarded.
---
Nitpick comments:
In `@src/main/ipc/handlers/director-notes.ts`:
- Around line 789-801: Update the warning log in the
recoverDirectorNotesNarrative failure path to include recovered.error when
recovery fails, while preserving the existing recoveryReason logging for
successful recovery. Keep the narrativeFields behavior unchanged.
In `@src/shared/directorNotesNarrative.ts`:
- Around line 250-341: Evaluate replacing the hand-rolled
closeTruncatedJsonObject and escapeControlCharsInStrings helpers with a
maintained JSON-repair library such as jsonrepair, while preserving the existing
per-repair reason attribution for truncated versus escaped input. Apply the same
consideration to the corresponding repair logic around the additionally
referenced section, and retain current behavior if the library cannot expose the
required attribution reliably.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab2da5d3-54db-4661-add5-91e60c0b94d2
📒 Files selected for processing (13)
docs/agent-guides/SHARED-UTILS.mddocs/director-notes.mdsrc/__tests__/renderer/components/DirectorNotes/AIOverviewTab.test.tsxsrc/__tests__/renderer/components/DirectorNotes/NarrativeParseError.test.tsxsrc/__tests__/shared/directorNotesNarrative.test.tssrc/cli/commands/director-notes-synopsis.tssrc/main/ipc/handlers/director-notes.tssrc/main/preload/directorNotes.tssrc/renderer/components/DirectorNotes/AIOverviewTab.tsxsrc/renderer/components/DirectorNotes/NarrativeParseError.tsxsrc/renderer/components/DirectorNotes/RichOverview.tsxsrc/renderer/global.d.tssrc/shared/directorNotesNarrative.ts
| {narrativeError && ( | ||
| <NarrativeParseError | ||
| theme={theme} | ||
| error={narrativeError} | ||
| rawOutput={synopsis} | ||
| recovery={narrativeRecovery} | ||
| /> | ||
| )} | ||
| {/* Prose from the narrative, or the raw string when there is no | ||
| narrative to build from and nothing failed (legacy markdown | ||
| synopses and the "no history files" message). */} | ||
| {(narrative || !narrativeError) && ( | ||
| <MarkdownRenderer | ||
| content={plainContent} | ||
| theme={theme} | ||
| onCopy={(text) => safeClipboardWrite(text)} | ||
| enableBionifyReadingMode={bionifyReadingMode} | ||
| chatMath | ||
| /> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Render empty-response parse failures.
When synopsis === '' and narrativeError is set, the outer synopsis ? guard prevents Line 592 from rendering the alert, leaving the user with a blank result. Include narrativeError in that guard and add an empty-response regression test.
Proposed fix
- {synopsis ? (
+ {(synopsis || narrativeError) ? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/components/DirectorNotes/AIOverviewTab.tsx` around lines 592 -
611, Update the outer synopsis-rendering guard in the AI overview component so
it remains active when synopsis is empty but narrativeError is set, allowing
NarrativeParseError to render instead of a blank result. Add a regression test
covering an empty synopsis with a narrative parse failure and verifying the
error alert is displayed.
| function validateNarrativeLenient( | ||
| parsed: unknown | ||
| ): { narrative: DirectorNotesNarrative; dropped: number } | null { | ||
| if (!isPlainObject(parsed) || !Array.isArray(parsed.sections)) return null; | ||
|
|
||
| let dropped = 0; | ||
| const sections: NarrativeSection[] = []; | ||
|
|
||
| for (const rawSection of parsed.sections) { | ||
| if (!isPlainObject(rawSection) || typeof rawSection.kind !== 'string') { | ||
| dropped++; | ||
| continue; | ||
| } | ||
| if (!VALID_KINDS.has(rawSection.kind)) { | ||
| dropped++; | ||
| continue; | ||
| } | ||
| const kind = rawSection.kind as NarrativeSectionKind; | ||
|
|
||
| const items: NarrativeItem[] = []; | ||
| for (const rawItem of Array.isArray(rawSection.items) ? rawSection.items : []) { | ||
| if (!isPlainObject(rawItem) || typeof rawItem.text !== 'string' || !rawItem.text.trim()) { | ||
| dropped++; | ||
| continue; | ||
| } | ||
| const item: NarrativeItem = { text: rawItem.text }; | ||
| // A bad severity/agent only costs that field, never the bullet. | ||
| if (typeof rawItem.severity === 'string' && VALID_SEVERITIES.has(rawItem.severity)) { | ||
| item.severity = rawItem.severity as NarrativeItemSeverity; | ||
| } | ||
| if (typeof rawItem.agent === 'string') item.agent = rawItem.agent; | ||
| items.push(item); | ||
| } | ||
|
|
||
| sections.push({ | ||
| kind, | ||
| title: typeof rawSection.title === 'string' ? rawSection.title : DEFAULT_SECTION_TITLES[kind], | ||
| items, | ||
| }); | ||
| } | ||
|
|
||
| if (sections.length === 0) return null; | ||
| return { narrative: { version: 1, sections }, dropped }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dropped sections are mislabeled as "bullets" in the recovery reason.
validateNarrativeLenient uses a single dropped counter for both malformed/unknown-kind sections (Lines 358-365) and malformed items (Lines 370-373). recoverDirectorNotesNarrative then always reports this combined count as bullets: `${lenient.dropped} ${...'bullet was' : 'bullets were'} malformed and dropped` (Lines 446-450). If a section is dropped (e.g. unknown kind), the user-facing reason will incorrectly say "1 bullet was malformed and dropped" instead of describing a dropped section. This contradicts the function's own contract: "recoverDirectorNotesNarrative is the salvage path... it always explains what it salvaged (never silently partial)". The existing "drops a section with an unknown kind" test (test file Lines 502-515) doesn't assert on reason, so this wasn't caught.
🐛 Proposed fix: split section/item drop counters
function validateNarrativeLenient(
parsed: unknown
-): { narrative: DirectorNotesNarrative; dropped: number } | null {
+): { narrative: DirectorNotesNarrative; droppedSections: number; droppedItems: number } | null {
if (!isPlainObject(parsed) || !Array.isArray(parsed.sections)) return null;
- let dropped = 0;
+ let droppedSections = 0;
+ let droppedItems = 0;
const sections: NarrativeSection[] = [];
for (const rawSection of parsed.sections) {
if (!isPlainObject(rawSection) || typeof rawSection.kind !== 'string') {
- dropped++;
+ droppedSections++;
continue;
}
if (!VALID_KINDS.has(rawSection.kind)) {
- dropped++;
+ droppedSections++;
continue;
}
const kind = rawSection.kind as NarrativeSectionKind;
const items: NarrativeItem[] = [];
for (const rawItem of Array.isArray(rawSection.items) ? rawSection.items : []) {
if (!isPlainObject(rawItem) || typeof rawItem.text !== 'string' || !rawItem.text.trim()) {
- dropped++;
+ droppedItems++;
continue;
}
...
}
...
}
if (sections.length === 0) return null;
- return { narrative: { version: 1, sections }, dropped };
+ return { narrative: { version: 1, sections }, droppedSections, droppedItems };
}- if (lenient.dropped > 0) {
- reasons.push(
- `${lenient.dropped} ${lenient.dropped === 1 ? 'bullet was' : 'bullets were'} malformed and dropped`
- );
- }
+ if (lenient.droppedItems > 0) {
+ reasons.push(
+ `${lenient.droppedItems} ${lenient.droppedItems === 1 ? 'bullet was' : 'bullets were'} malformed and dropped`
+ );
+ }
+ if (lenient.droppedSections > 0) {
+ reasons.push(
+ `${lenient.droppedSections} ${lenient.droppedSections === 1 ? 'section was' : 'sections were'} malformed and dropped`
+ );
+ }Consider also adding a regression test asserting result.reason when a section (not just an item) is dropped, so this doesn't regress silently.
Also applies to: 406-461
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shared/directorNotesNarrative.ts` around lines 349 - 392, Split the
recovery accounting in validateNarrativeLenient into separate section and item
drop counters, incrementing each at its corresponding validation failures and
returning both counts. Update recoverDirectorNotesNarrative to describe dropped
sections and bullets accurately, including correct singular/plural wording, and
add a regression assertion for the reason when an invalid section is discarded.
1 participant
What broke
Director's Notes Plain Mode showed a wall of raw JSON instead of a report:
The synopsis agent emits a structured JSON narrative. Plain Mode renders
narrative ? narrativeToMarkdown(narrative) : synopsis, so whenever the strict parser rejected a run there was no narrative and the raw string went straight to the markdown renderer. Rich Mode already failed loudly for that same input (NarrativeParseError); Plain Mode silently dumped the object.Reproduced against the live 0.18.5-RC app: a fresh 10-day run (19 agents, 15,003 entries, 2m52s) parsed clean, so the failure is per-run, not a broken pipeline. That is exactly why the raw fallback was invisible until it wasn't.
What this changes
Plain Mode never renders the raw structured output. It shows the narrative as prose, or the shared parse-failure banner with the raw text behind its "View raw output" disclosure.
A rejected run no longer costs the whole report.
recoverDirectorNotesNarrative()runs only after the strict parser fails and handles the failures that actually happen in the field:Recovery is never silent. It returns a reason ("the response was cut off before it finished") and both modes render
NarrativeParseErrorin a softer partial-recovery state above the salvaged narrative, so a partial report never reads as a complete one. The strict parser's contract is unchanged.The CLI's
-f markdown/-f textoutput takes the same salvage with the reason noted inline.-f jsonstill returns the raw synopsis untouched.Notes
narrativeRecoveryis threaded through the IPC result, preload type, andglobal.d.ts.applyNarrative()collapses the three places (fresh run, in-flight attach, cache restore) that had to apply the narrative fields by hand.docs/director-notes.md, plus aSHARED-UTILS.mdentry so nobody hand-rolls JSON repair at a call site.Testing
directorNotesNarrative.test.ts: 46 pass, including truncation at two cut points, raw line breaks, dropped items/sections, title fallback, refusal on non-narrative input, never-throws.npm run lintandnpm run lint:eslintclean; full suite green via the pre-push hook.Summary by CodeRabbit
New Features
Documentation