fix(ce-codex-reviewer): NDJSON output and schema-valid line numbers

Addresses both Codex findings on a8a5ed2 (April 2026):

P1 — Emit schema-valid line numbers for file-level findings.
The findings schema requires `line >= 1` (verified in
references/findings-schema.json). The previous prompt told codex to
emit `LINE=0` for file-level issues, which would have been dropped
by the merge validator as malformed before synthesis — silently
losing every file-level Codex finding.

Fix: the codex prompt now requires `line` to be a positive integer
and adds a `file_level` boolean. When `file_level: true`, codex sets
`line: 1` and the agent prepends a "file-level finding (no specific
line applies)" string to the evidence array so synthesis and
downstream surfaces can still distinguish "line 1 was the issue"
from "this is a whole-file concern." The `file_level` signal is
preserved in evidence rather than as a separate field because the
findings schema doesn't expose a top-level file_level flag, and
inventing one would fail the strict-validator path.

P2 — Use structured output so evidence cannot break parsing.
The previous pipe-delimited contract dropped any row that wasn't
exactly five `|`-separated fields. EVIDENCE is a raw code snippet
that can legitimately contain `|` (bitwise OR / union types, shell
pipes, markdown tables, regex alternation). When that happened,
valid findings disappeared silently.

Fix: switch the codex prompt to NDJSON — one JSON object per line.
The agent JSON-parses each line independently; embedded pipes in
evidence are no longer a parsing hazard because JSON quoting handles
them. Lines that fail to parse are skipped (no retry, no inference).

Test coverage:

- New contract test "uses NDJSON output contract so evidence can
  carry pipes safely" guards the prompt format and asserts the
  pipe-delimited shape stays gone.
- New contract test "emits schema-valid line numbers for file-level
  findings" guards the line=1 + file_level=true convention, asserts
  the old "0 means file-level" wording is gone, and reads the
  findings schema to verify minimum=1 still holds (so a future
  schema relaxation triggers a deliberate test update rather than a
  silent drift).

Both tests cite their originating Codex finding inline so future
regressions get a clear pointer to PR #356.

bun test tests/review-skill-contract.test.ts: 30 pass (was 28)
bun run release:validate: clean

Author: mvanhorn

plugins/compound-engineering/agents/ce-codex-reviewer.agent.md

@@ -45,13 +45,19 @@ If the diff is empty (no changes to review), return the empty findings JSON with
 Invoke codex in non-interactive mode. Prefer the project's configured default model — do not pin `-m` or `-c reasoning` flags here so the user's `~/.codex/config.toml` settings apply.
 
 ```bash
-codex exec --sandbox read-only --skip-git-repo-check "Review the unified diff in $DIFF_FILE for correctness, security, reliability, and contract issues. Output one finding per issue in this exact format on separate lines:
+codex exec --sandbox read-only --skip-git-repo-check "Review the unified diff in $DIFF_FILE for correctness, security, reliability, and contract issues. Output one JSON object per line (NDJSON), with these fields per object:
 
-SEVERITY|FILE|LINE|TITLE|EVIDENCE
+  {\"severity\": \"P0|P1|P2|P3\", \"file\": \"path/from/diff\", \"line\": <positive integer>, \"file_level\": <true|false>, \"title\": \"short imperative sentence\", \"evidence\": \"specific code snippet that supports the finding\"}
 
-Where SEVERITY is P0/P1/P2/P3, FILE is the path from the diff, LINE is the line number (or 0 if file-level), TITLE is a short imperative sentence, EVIDENCE is the specific code snippet that supports the finding. If you find no issues, output exactly: NO_FINDINGS.
+Rules for each field:
+- severity: exactly one of \"P0\", \"P1\", \"P2\", \"P3\".
+- file: a path from the diff's Changed files list. No quoting, no expansion.
+- line: a positive integer (1 or higher). For file-level findings (no specific line), use 1 and set file_level to true.
+- file_level: true when the issue is whole-file and no specific line applies; false when the line number is precise.
+- title: a short imperative sentence describing the issue.
+- evidence: a string with the exact code snippet, quote, or line content that supports the finding. May contain any characters including pipes, JSON-escaped per JSON rules.
 
-Do not output prose. Do not summarize. Do not output anything other than the pipe-delimited lines or NO_FINDINGS."
+Output one JSON object per line. Do not wrap them in an array. Do not output prose, headers, or commentary. If you find no issues, output exactly the literal token: NO_FINDINGS"
 ```
 
 Capture stdout. Clean up the tempfile: `rm -f "$DIFF_FILE"`.
@@ -60,15 +66,15 @@ If codex exits non-zero, return the empty findings JSON with `residual_risks: ["
 
 ## Step 4: Translate codex output into findings
 
-Parse the pipe-delimited lines from codex's stdout. Skip any line that does not have exactly five `|`-separated fields. Skip the literal `NO_FINDINGS` token.
+Parse codex's stdout line-by-line as NDJSON. Skip blank lines. Skip the literal `NO_FINDINGS` token. For each remaining line, attempt to JSON-parse it; if parsing fails, skip the line (do not retry, do not infer).
 
-For each parsed line, build a finding object that conforms to the findings schema:
+For each successfully-parsed object, build a finding object that conforms to the findings schema:
 
-- **`title`**: the TITLE field from codex.
-- **`severity`**: the SEVERITY field, one of `"P0"`, `"P1"`, `"P2"`, `"P3"`. Reject anything else; if codex emitted a different vocabulary, drop the line silently.
-- **`file`**: the FILE field. Verify the path appears in the diff's `Changed files` list from `<review-context>`; drop the finding if not.
-- **`line`**: the LINE field as an integer (0 means file-level).
-- **`evidence`**: an array containing the EVIDENCE field as one element. Always wrap in an array — a bare string is a schema violation.
+- **`title`**: the `title` field from the codex output.
+- **`severity`**: the `severity` field, one of `"P0"`, `"P1"`, `"P2"`, `"P3"`. Reject anything else; if codex emitted a different vocabulary, drop the object silently.
+- **`file`**: the `file` field. Verify the path appears in the diff's `Changed files` list from `<review-context>`; drop the finding if not.
+- **`line`**: the `line` field as an integer. The findings schema requires `line >= 1`, so reject any object with `line < 1` outright. When codex set `file_level: true`, line will already be `1` per the contract above; the agent does not re-default `0` to `1` because the contract refuses `0`. The `file_level` signal is preserved in `evidence` (see below) so synthesis and downstream surfaces can distinguish "line 1 was the issue" from "this is a file-level concern."
+- **`evidence`**: an array. Always include the `evidence` string from codex as the first element. When `file_level: true`, prepend an additional element: `"file-level finding (no specific line applies)"`. The schema requires evidence to be a non-empty array of strings; never emit a bare string.
 - **`why_it_matters`**: write 2-4 sentences explaining the observable consequence of the issue, grounded in the EVIDENCE codex provided. Lead with what a user, caller, or operator experiences. If you cannot articulate a concrete consequence from the codex output, drop the finding — the schema's why_it_matters bar is non-negotiable.
 - **`autofix_class`**: default `"manual"`. Cross-model findings carry interpretive uncertainty; the orchestrator's synthesis re-classifies during the merge step.
 - **`owner`**: default `"downstream-resolver"`.

tests/review-skill-contract.test.ts

@@ -760,6 +760,46 @@ describe("codex-reviewer contract", () => {
     expect(content).toContain("codex-reviewer skipped: already running inside Codex sandbox")
     expect(content).toContain("codex-reviewer skipped: codex CLI not installed")
   })
+
+  test("uses NDJSON output contract so evidence can carry pipes safely", async () => {
+    const content = await readRepoFile(
+      "plugins/compound-engineering/agents/ce-codex-reviewer.agent.md",
+    )
+    // Addresses Codex P2 finding on commit a8a5ed2 (April 2026): pipe-delimited
+    // output dropped any row that wasn't exactly five `|`-separated fields,
+    // so EVIDENCE containing a literal `|` (bitwise OR, shell pipes, markdown
+    // tables) silently lost real findings. NDJSON is robust to embedded pipes.
+    expect(content).toContain("NDJSON")
+    expect(content).toMatch(/JSON-?parse/i)
+    // Pipe-delimited contract must be gone from the agent's prose.
+    expect(content).not.toMatch(/SEVERITY\|FILE\|LINE\|TITLE\|EVIDENCE/)
+    expect(content).not.toMatch(/exactly five `\|`-separated fields/)
+  })
+
+  test("emits schema-valid line numbers for file-level findings", async () => {
+    const content = await readRepoFile(
+      "plugins/compound-engineering/agents/ce-codex-reviewer.agent.md",
+    )
+    // Addresses Codex P1 finding on commit a8a5ed2 (April 2026): the findings
+    // schema requires line >= 1, so emitting line=0 for file-level findings
+    // would silently drop them at the merge validator. Contract says: line=1
+    // with file_level=true, evidence array prepends the file-level annotation.
+    expect(content).toContain("file_level")
+    expect(content).toMatch(/positive integer/i)
+    expect(content).toMatch(/file-level finding \(no specific line applies\)/)
+    // Negative check: the old "0 means file-level" wording must be gone.
+    expect(content).not.toMatch(/0 means file-level/)
+    expect(content).not.toMatch(/or 0 if file-level/)
+
+    // Also assert the schema constraint is what we think it is — if upstream
+    // ever loosens line to allow 0, this test should be updated to match.
+    const schemaJson = await readRepoFile(
+      "plugins/compound-engineering/skills/ce-code-review/references/findings-schema.json",
+    )
+    const schema = JSON.parse(schemaJson)
+    const lineMin = schema.properties?.findings?.items?.properties?.line?.minimum
+    expect(lineMin).toBe(1)
+  })
 })
 
 describe("testing-reviewer contract", () => {