Conversation
- Decode JSONL as UTF-8 and publish validated feedback atomically. - Retain parser and Codex diagnostics with the log identity. - Avoid retrying a failed resume that emitted a partial stream. - Cover completed, malformed, and partial synthetic JSONL. Testing: Focused pytest, Ruff, Pyright, bash -n, git diff --check. Risks: Incomplete JSONL now fails closed instead of replacing feedback.
|
Read-only exact-head review of bbde4b4 against upstream main: no P0-P2 behavioral findings. Two P3 findings were identified: codex-review/SKILL.md still described the prior unconditional resume fallback, and a malformed stream after thread.started could report the previous session ID in CODEX_FAILED. Both are corrected in the next local commit; the synthetic failure fixture now verifies the new session identity while preserving prior feedback. This is an author-posted review receipt, not a maintainer approval. Upstream Actions currently reports action_required for this fork PR, so the maintainer-controlled CI gate is pending. |
|
Exact-head independent read-only review receipt for
This is a contributor-posted independent analysis receipt, not a maintainer approval. Upstream Actions run for this head is |
|
This contribution is ready for maintainer review. GitHub Actions run 36267459568 for the exact head is marked action_required because the workflow requires maintainer approval for a forked PR. Could a maintainer approve that run and review the patch when convenient? The synthetic regression, focused tests, and local verification are recorded above; I have not rerun any paid Codex review. |
mikeangstadt
left a comment
mikeangstadt
left a comment
There was a problem hiding this comment.
Right fix for the right bug: gating publication on turn.completed and atomically replacing the file is what this needed. Ran it locally on Linux and your unverified CI is fine (ruff clean, pyright 0 errors, 3 passed), so that risk note can come down. Two things inline about the new strictness, neither blocking.
| for line_number, line in enumerate(stream, 1): | ||
| try: | ||
| event = json.loads(line) | ||
| except json.JSONDecodeError as exc: |
There was a problem hiding this comment.
This makes any single unparseable line fatal for the whole stream, which reintroduces the bug you're fixing with the sign flipped. Repro against your parser:
printf '{"type":"thread.started","thread_id":"t1"}\n\n{"type":"item.completed","item":{"type":"agent_message","text":"VERDICT: APPROVED"}}\n{"type":"turn.completed"}\n' > /tmp/blank.jsonl
python3 parse_codex_json.py /tmp/blank.jsonl /tmp/fb.out
feedback extraction failed from /tmp/blank.jsonl: JSONL line 2: Expecting value
exit=2, no feedback written
One blank line and a completed, approved review with the verdict sitting right there in the log gets thrown away and reported as CODEX_FAILED. The old inline parser swallowed per-line errors, so this is a behavior regression on a stdout format we don't own; any stray newline, deprecation notice, or progress line the codex CLI ever prints turns a paid review into a failure.
The guarantee you actually want is "don't publish unless turn.completed and an agent message were seen" and that doesn't require per-line strictness. Skip lines where not line.strip(), and treat other non-JSON lines as ignorable rather than fatal. The completeness gate below still does the real work.
| # Resume succeeded -- skip to verdict extraction | ||
| : | ||
| else | ||
| if [[ $codex_exit -ne 0 ]] && [[ ! -s "$codex_json" ]]; then |
There was a problem hiding this comment.
Narrowing the fresh-session fallback to a totally empty stream is right in spirit, but -s "$codex_json" is the wrong test for "work already happened." If codex emits a single JSON error event on stdout for a dead or expired thread ID and exits nonzero, this file is nonempty, the fallback never fires, and the debate is permanently wedged: plan-with-codex.md tells the orchestrator to ask retry-or-abort, retry re-runs with the same dead --session-id, and you get the identical CODEX_FAILED forever with no path back to a fresh session.
You already have the right signal in session_from_partial: no thread.started in the stream means nothing resumed and nothing was paid for, so going fresh is safe. Gate on that instead of on file size and the "partial stream may contain paid work" case stays protected.
| assert "CODEX_SESSION:synthetic-session" in completed.stdout | ||
| assert "LOG_ID:synthetic-review" in completed.stdout | ||
| assert feedback.read_bytes() == b"prior usable verdict" | ||
| assert calls.read_text(encoding="utf-8").splitlines() == ["call"] |
There was a problem hiding this comment.
This locks the no-rerun side of the new condition, but the branch that actually changed shape (empty stream + nonzero exit -> second codex call) has no test. A third case with jsonl="", codex_exit=1, session_id="previous-session" asserting two calls would pin the fallback you kept.
| @@ -0,0 +1,100 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Heads up that CLAUDE.md says new tool scripts are TypeScript, no new Python. I don't think it bites here, since run_codex_review.sh already hard-requires python3 at line 63 and this is inline heredoc Python moved into a file rather than a new tool, but worth a maintainer ruling before it sets precedent for the next skill.
2 participants
Purpose
Fix completed Codex plan reviews whose UTF-8 JSONL contains characters outside the Windows default code page. The existing wrapper can lose the final verdict, truncate feedback, and hide the parser error after the model has already completed.
Tracks pnwgmr-llc/ai-agent-skills#359: https://github.com/pnwgmr-llc/ai-agent-skills/issues/359
Changes
Verification
pytest plugins/code/skills/codex-review/tests -q— 3 passed (Python 3.13).ruff checkon new Python files — passed.pyright --pythonpath <Python 3.13 venv>on new Python files — 0 errors.bash -n plugins/code/skills/codex-review/scripts/run_codex_review.sh— passed.git diff --check— passed.Risk: Medium. The parser now refuses incomplete JSONL and retains prior feedback; synthetic tests cover that failure path. The upstream Actions run requires maintainer approval for fork code, and maintainer review is pending. A local Windows repository-wide run had 1,798 passed, 380 failed, and 3 skipped; many unrelated shell tests resolved
bashto WSL without/bin/bash, and Unix-permission assumptions also failed. The changed codex-review tests all passed. Full upstream Linux CI remains unverified.