Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions plugins/codex/scripts/codex-companion.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,8 @@ async function executeReviewRun(request) {
turnId: result.turnId,
payload,
rendered,
summary: firstMeaningfulLine(result.reviewText, `${reviewName} completed.`),
summary: shorten(firstMeaningfulLine(result.reviewText, `${reviewName} completed.`), 96),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prefer native review errors in failed-job summaries

When a native review/start turn finishes with a nonzero status and an error but produces no review text, this still records the summary as Review completed.; if the review text is a formatted error body, it can still record only {. Because /codex:status renders this indexed summary rather than errorMessage, failed native reviews continue to show a misleading or meaningless summary despite the useful error now being persisted. For failed results, derive the summary from result.error?.message or stderr before considering reviewText.

Useful? React with 👍 / 👎.

errorMessage: result.error?.message ?? result.stderr ?? null,
jobTitle: `Codex ${reviewName}`,
jobClass: "review",
targetLabel: target.label
Expand All @@ -419,6 +420,7 @@ async function executeReviewRun(request) {
status: result.status,
failureMessage: result.error?.message ?? result.stderr
});
const failureMessage = result.error?.message ?? result.stderr ?? parsed.parseError ?? "";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve parse errors when stderr is empty

When an adversarial review returns malformed structured output without an app-server error, result.stderr is normally the empty string, which is non-nullish and therefore prevents parsed.parseError from being selected here. The subsequent summary falls back to the malformed response's first line (often just {), regressing the previous behavior that surfaced the JSON parse error and recreating the unhelpful status summary this change is intended to fix. Treat an empty stderr value as absent before falling back to parsed.parseError.

Useful? React with 👍 / 👎.

const payload = {
review: reviewName,
target,
Expand Down Expand Up @@ -450,7 +452,8 @@ async function executeReviewRun(request) {
targetLabel: context.target.label,
reasoningSummary: result.reasoningSummary
}),
summary: parsed.parsed?.summary ?? parsed.parseError ?? firstMeaningfulLine(result.finalMessage, `${reviewName} finished.`),
summary: parsed.parsed?.summary ?? shorten(firstMeaningfulLine(failureMessage || result.finalMessage, `${reviewName} finished.`), 96),
errorMessage: failureMessage || null,
jobTitle: `Codex ${reviewName}`,
jobClass: "review",
targetLabel: context.target.label
Expand Down Expand Up @@ -515,14 +518,19 @@ async function executeTaskRun(request) {
touchedFiles: result.touchedFiles,
reasoningSummary: result.reasoningSummary
};
const summary = shorten(
firstMeaningfulLine(failureMessage || rawOutput, `${taskMetadata.title} finished.`),
96
);

return {
exitStatus: result.status,
threadId: result.threadId,
turnId: result.turnId,
payload,
rendered,
summary: firstMeaningfulLine(rawOutput, firstMeaningfulLine(failureMessage, `${taskMetadata.title} finished.`)),
summary,
errorMessage: failureMessage || null,
jobTitle: taskMetadata.title,
jobClass: "task",
write: Boolean(request.write)
Expand Down
2 changes: 1 addition & 1 deletion plugins/codex/scripts/lib/codex.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ function applyTurnNotification(state, message) {
break;
case "error":
state.error = message.params.error;
emitProgress(state.onProgress, `Codex error: ${message.params.error.message}`, "failed");
emitProgress(state.onProgress, `Codex error: ${shorten(message.params.error.message, 96)}`, "failed");
break;
case "turn/completed":
if ((message.params.threadId ?? null) !== state.threadId) {
Expand Down
4 changes: 4 additions & 0 deletions plugins/codex/scripts/lib/tracked-jobs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ export async function runTrackedJob(job, runner, options = {}) {
try {
const execution = await runner();
const completionStatus = execution.exitStatus === 0 ? "completed" : "failed";
const errorMessage =
completionStatus === "failed" ? (execution.errorMessage ?? null) : null;
const completedAt = nowIso();
writeJobFile(job.workspaceRoot, job.id, {
...runningRecord,
Expand All @@ -163,6 +165,7 @@ export async function runTrackedJob(job, runner, options = {}) {
pid: null,
phase: completionStatus === "completed" ? "done" : "failed",
completedAt,
errorMessage,
result: execution.payload,
rendered: execution.rendered
});
Expand All @@ -174,6 +177,7 @@ export async function runTrackedJob(job, runner, options = {}) {
summary: execution.summary,
phase: completionStatus === "completed" ? "done" : "failed",
pid: null,
errorMessage,
completedAt
});
appendLogBlock(options.logFile ?? job.logFile ?? null, "Final output", execution.rendered);
Expand Down
89 changes: 89 additions & 0 deletions tests/state.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import assert from "node:assert/strict";

import { makeTempDir } from "./helpers.mjs";
import { resolveJobFile, resolveJobLogFile, resolveStateDir, resolveStateFile, saveState } from "../plugins/codex/scripts/lib/state.mjs";
import { runTrackedJob } from "../plugins/codex/scripts/lib/tracked-jobs.mjs";

test("resolveStateDir uses a temp-backed per-workspace directory", () => {
const workspace = makeTempDir();
Expand Down Expand Up @@ -103,3 +104,91 @@ test("saveState prunes dropped job artifacts when indexed jobs exceed the cap",
.sort()
);
});

test("runTrackedJob persists errorMessage on a non-throwing failed execution", async () => {
const workspace = makeTempDir();
const job = {
id: "task-fail-nonthrowing",
kind: "task",
kindLabel: "Task",
title: "Codex Task",
workspaceRoot: workspace,
jobClass: "task",
summary: "",
write: false,
createdAt: new Date().toISOString()
};

await runTrackedJob(
job,
async () => ({
exitStatus: 400,
threadId: "thread-1",
turnId: "turn-1",
payload: { status: 400, rawOutput: "{\n \"type\": \"error\"\n}", touchedFiles: [] },
rendered: "rendered output\n",
summary: "Unsupported value: 'x' is not supported.",
errorMessage: "Unsupported value: 'x' is not supported with the 'gpt-5.6-terra' model."
}),
{}
);

const storedJob = JSON.parse(
fs.readFileSync(resolveJobFile(workspace, job.id), "utf8")
);
const state = JSON.parse(fs.readFileSync(resolveStateFile(workspace), "utf8"));
const indexed = state.jobs.find((entry) => entry.id === job.id);

assert.equal(storedJob.status, "failed");
assert.equal(storedJob.phase, "failed");
assert.equal(
storedJob.errorMessage,
"Unsupported value: 'x' is not supported with the 'gpt-5.6-terra' model."
);
assert.equal(indexed.status, "failed");
assert.equal(
indexed.errorMessage,
"Unsupported value: 'x' is not supported with the 'gpt-5.6-terra' model."
);
assert.equal(indexed.summary, "Unsupported value: 'x' is not supported.");
});

test("runTrackedJob stores no errorMessage for a completed execution", async () => {
const workspace = makeTempDir();
const job = {
id: "task-ok",
kind: "task",
kindLabel: "Task",
title: "Codex Task",
workspaceRoot: workspace,
jobClass: "task",
summary: "",
write: false,
createdAt: new Date().toISOString()
};

await runTrackedJob(
job,
async () => ({
exitStatus: 0,
threadId: "thread-2",
turnId: "turn-2",
payload: { status: 0, rawOutput: "OK", touchedFiles: [] },
rendered: "OK\n",
summary: "OK",
errorMessage: null
}),
{}
);

const storedJob = JSON.parse(
fs.readFileSync(resolveJobFile(workspace, job.id), "utf8")
);
const state = JSON.parse(fs.readFileSync(resolveStateFile(workspace), "utf8"));
const indexed = state.jobs.find((entry) => entry.id === job.id);

assert.equal(storedJob.status, "completed");
assert.equal(storedJob.errorMessage, null);
assert.equal(indexed.status, "completed");
assert.equal(indexed.errorMessage, null);
});