[rendering-scripts] Fix Codex JSONL parser dropping error/warning log items in step summaries - #58195
Conversation
Codex CLI's JSONL log format emits item.completed events with item.type "error" (e.g. model-fallback warnings), but parseCodexJsonl() silently dropped any item type it didn't recognize, so these messages never reached the rendered step summary or logEntries. Found while verifying the rendering pipeline against real output from workflow run 33729471900 (Issue Monster, codex engine): the log contained a "Model metadata ... not found" error item that was completely absent from the generated markdown.
github-actions
Bot
added
automated-fix
javascript
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review for #58195; submitted overall review with no inline comments because no actionable blocking issues were found in changed lines.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. This PR (fix/codex-jsonl-error-events) only modified the production file actions/setup/js/parse_codex_log.cjs to surface error/warning events from Codex JSONL logs in step summaries. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
🟡 Changes recommended
Add the requested regression test and address the downstream formatter dropping the error message.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates Codex JSONL parsing to capture completed error items and propagate them into summaries and structured results.
Changes:
- Parses
item.type === "error"messages. - Adds error details to markdown and result metadata.
- Emits results containing errors even without usage data.
File summaries
| File | Description |
|---|---|
actions/setup/js/parse_codex_log.cjs |
Adds Codex error handling; regression coverage is needed for no-usage results and final summary rendering. |
Review details
Suppressed comments (1)
actions/setup/js/parse_codex_log.cjs:490
- This field is not rendered by the actual step-summary path.
runLogParserreplaces the parser's markdown wheneverlogEntriesexist and callsgenerateCopilotCliStyleSummary; after normalization, that formatter never readsresult.errors. As a result, anitem.completederror is present insession.result.data.errorsbut remains absent from the rendered step summary andcore.info. Extend the shared summary formatter to emit result errors instead of relying only on this field.
errors: errorMessages.length > 0 ? errorMessages : undefined,
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| case "error": { | ||
| const message = typeof item.message === "string" ? item.message : JSON.stringify(item); | ||
| if (message.trim()) { | ||
| parsedData.push({ type: "error", content: message }); |
There was a problem hiding this comment.
Added a no-usage item.completed error regression test asserting session.result.data.errors and the Copilot CLI-style rendered summary. The downstream formatter now includes result errors in Statistics.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking issue in the changed lines.
The only plausible risk here is minor and non-blocking
The new JSON.stringify(item) fallback for non-string error payloads could still misbehave on exotic circular objects, but this parser is consuming Codex JSONL where item.message is expected to be a plain string and the fallback is only a last resort. That is not strong enough to block this PR.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 28.8 AIC · ⌖ 7.32 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
The fix correctly adds handling for item.type === "error" events in parseCodexJsonl, prepends an Errors section to the markdown, and propagates error messages into the result log entry's errors array (consumed properly by generateInformationSection/convertLegacyLogEntriesToCopilotEvents in log_parser_shared.cjs). The usage || errorMessages.length > 0 guard correctly avoids emitting an empty result entry while still surfacing errors without usage data.
No blocking issues found. One non-blocking suggestion left inline: add a JSONL-format regression test for the new error-item case, since the existing JSONL test suite only covers agent_message/mcp_tool_call/command_execution/reasoning items.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 42.4 AIC · ⌖ 13.4 AIC · ⊞ 8.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — this is a well-scoped, narrow bug fix with an existing 71-test regression suite that keeps passing. Two suggestions, neither blocking.
📋 Key Themes & Highlights
Key Themes
- Missing permanent regression test: the PR body describes a manual harness run against the real production log (run 33729471900) proving the fix works, but that exact scenario (an
item.completedevent withitem.type: "error") isn't captured as a test inparse_codex_log.test.cjs. The file already has a dedicateddescribe("Codex experimental JSONL event format", ...)block that would be the natural home for it. - Minor whitespace inconsistency:
message.trim()is used only as a guard condition, but the untrimmed value is stored/rendered, so incidental leading/trailing whitespace from the JSON payload could leak into markdown and theerrorsarray.
Positive Highlights
- ✅ Correctly reuses the existing
errorsconvention already wired throughgenerateInformationSection/convertLegacyLogEntriesToCopilotEvents, keeping this consistent with other engines rather than inventing new plumbing. - ✅ Good defensive fallback (
JSON.stringify(item)) whenitem.messageisn't a string. - ✅ Correctly changes the
resultentry condition tousage || errorMessages.length > 0so errors surface even without usage data — this was the root cause of the original silent-drop bug and is properly addressed, not just patched around.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 48.1 AIC · ⌖ 14.8 AIC · ⊞ 10.3K
Comment /matt to run again
| break; | ||
| } | ||
| case "error": { | ||
| const message = typeof item.message === "string" ? item.message : JSON.stringify(item); |
There was a problem hiding this comment.
[/diagnosing-bugs] This fix for item.type === "error" events has no regression test in parse_codex_log.test.cjs — the PR description mentions verifying against a real production log with a temporary harness, but that verification wasn't captured as a permanent test.
💡 Suggested regression test
Add a case alongside the existing describe("Codex experimental JSONL event format", ...) block using the exact reported payload:
it("surfaces item.completed error events in markdown and logEntries", () => {
const logContent = [
'{"type":"item.completed","item":{"id":"item_0","type":"error","message":"Model metadata for `mai-code-1-flash-picker` not found. Defaulting to fallback metadata; this can degrade performance and cause issues."}}',
].join("\n");
const result = parseCodexLog(logContent);
expect(result.markdown).toContain("<summary>Errors</summary>");
expect(result.markdown).toContain("Model metadata for");
const resultEntry = result.logEntries.find(e => e.type === "session.result");
expect(resultEntry).toBeDefined();
expect(resultEntry.data?.errors).toContain(expect.stringContaining("Model metadata for"));
});Without this, a future refactor of parseCodexJsonl could silently reintroduce the exact bug this PR fixes.
@copilot please address this.
There was a problem hiding this comment.
Added permanent regression coverage for item.completed error events, including markdown and structured session.result.data.errors assertions.
| } | ||
| case "error": { | ||
| const message = typeof item.message === "string" ? item.message : JSON.stringify(item); | ||
| if (message.trim()) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Minor inconsistency: the guard checks message.trim() but the untrimmed message is what gets pushed into parsedData/errorMessages, so leading/trailing whitespace from the raw JSON can leak into the rendered > message blockquote and the errors array.
💡 Suggested fix
case "error": {
const rawMessage = typeof item.message === "string" ? item.message : JSON.stringify(item);
const message = rawMessage.trim();
if (message) {
parsedData.push({ type: "error", content: message });
}
break;
}@copilot please address this.
There was a problem hiding this comment.
Normalized Codex error messages with trim() before storing and rendering them, so whitespace cannot leak into the errors array or summary.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the PR-finisher pass. Added regression coverage for Codex JSONL errors without usage and fixed the downstream step-summary formatter to render them. Local formatting, lint, JavaScript, Go unit, integration, and recompilation checks pass in commit |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please take another pass on this PR. Please review the current status, refresh the branch if needed, address any outstanding review feedback or failing checks, and then use the
|
Rechecked the PR and recent workflow runs. The Codex parser fix, formatter change, regression coverage, trimming fix, and local validation are complete on |
|
🎉 This pull request is included in a new release. Release: |
Summary
The Codex CLI JSONL log parser (
parse_codex_log.cjs) silently droppeditem.completedevents whoseitem.typewas"error", so error/warning messages emitted by Codex (e.g. model-fallback warnings) never appeared anywhere in the rendered step summary or structured log entries. This means genuine Codex errors could be invisible to anyone reviewing a workflow run's output.Trigger
This was found during the daily rendering-scripts verification pass: the run's raw
agent-stdio.logwas fed through the current parser and the resulting markdown/logEntries were checked for correctness. The run's underlying agent execution itself completed successfully (0 errors/warnings reported by the workflow audit), but the log contained an unsurfaced item:This message was completely absent from the rendered step summary before this fix.
Changes
actions/setup/js/parse_codex_log.cjs:parseCodexJsonl()now handlesitem.type === "error"events, collecting their messages.<details><summary>Errors</summary>section listing any such messages, ahead of the existing Reasoning/Commands sections.resultlog entry now includes anerrorsarray (propagated through the existinggenerateInformationSection/convertLegacyLogEntriesToCopilotEventsconventions already used by other engines) and is emitted even when nousagedata is present, as long as there are errors to report.Diff detail
Test Results
Existing regression suite: parse_codex_log.test.cjs
No regressions.
Verification against real production log (run 33729471900)
Used a temporary vitest-based harness to run
parseCodexLog()directly against the realagent-stdio.logcontent from this run (not a synthetic fixture).logEntries[].data.errors(session.result.data.errors).{ "errorMessagePresentInMarkdown": true, "errorLogEntryPresent": true }render_template.cjs conditional rendering check
Also ran the existing
render_template.test.cjssuite (handlebars-style{{#if}}conditional rendering) as part of this verification pass — no issues found, no changes needed:Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
api.anthropic.comcodeload.github.comgithub.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.