fix(code): preserve completed Codex review feedback #204
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| #!/usr/bin/env python3 | ||
| """Extract a completed Codex review from UTF-8 JSONL without losing old feedback.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import os | ||
| import sys | ||
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def extract(source: Path, feedback: Path) -> str: | ||
| messages: list[str] = [] | ||
| thread_id = "" | ||
| completed = False | ||
| line_number = 0 | ||
| with source.open("r", encoding="utf-8") as stream: | ||
| for line_number, line in enumerate(stream, 1): | ||
| try: | ||
| event = json.loads(line) | ||
| except json.JSONDecodeError as exc: | ||
|
Collaborator
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 feedback extraction failed from /tmp/blank.jsonl: JSONL line 2: Expecting valueexit=2, no feedback writtenOne 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 |
||
| raise ValueError(f"JSONL line {line_number}: {exc.msg}") from exc | ||
| if not isinstance(event, dict): | ||
| raise TypeError(f"JSONL line {line_number}: event must be an object") | ||
| kind = event.get("type") | ||
| if kind == "thread.started" and isinstance(event.get("thread_id"), str): | ||
| thread_id = event["thread_id"] | ||
| elif kind == "item.completed": | ||
| item = event.get("item") | ||
| if not isinstance(item, dict): | ||
| raise ValueError(f"JSONL line {line_number}: item must be an object") | ||
| if item.get("type") == "agent_message": | ||
| message = item.get("text") | ||
| if not isinstance(message, str): | ||
| raise ValueError(f"JSONL line {line_number}: agent text must be a string") | ||
| if message: | ||
| messages.append(message) | ||
| elif kind == "turn.completed": | ||
| completed = True | ||
|
|
||
| if not completed: | ||
| raise ValueError(f"JSONL ended after line {line_number} without turn.completed") | ||
| if not messages: | ||
| raise ValueError("completed JSONL contains no agent message") | ||
|
|
||
| payload = "\n".join(messages).encode("utf-8") | ||
| feedback.parent.mkdir(parents=True, exist_ok=True) | ||
| staged: Path | None = None | ||
| try: | ||
| with tempfile.NamedTemporaryFile( | ||
| mode="wb", prefix=".codex-feedback-", dir=feedback.parent, delete=False, | ||
| ) as stream: | ||
| staged = Path(stream.name) | ||
| stream.write(payload) | ||
| stream.flush() | ||
| os.fsync(stream.fileno()) | ||
| os.replace(staged, feedback) | ||
| staged = None | ||
| finally: | ||
| if staged is not None: | ||
| staged.unlink(missing_ok=True) | ||
| return thread_id | ||
|
|
||
|
|
||
| def session_from_partial(source: Path) -> str: | ||
| """Recover an already-started session without accepting partial feedback.""" | ||
| try: | ||
| with source.open("r", encoding="utf-8") as stream: | ||
| for line in stream: | ||
| try: | ||
| event = json.loads(line) | ||
| except json.JSONDecodeError: | ||
| return "" | ||
| if isinstance(event, dict) and event.get("type") == "thread.started": | ||
| thread_id = event.get("thread_id") | ||
| return thread_id if isinstance(thread_id, str) else "" | ||
| except (OSError, UnicodeError): | ||
| pass | ||
| return "" | ||
|
|
||
|
|
||
| def main() -> int: | ||
| if len(sys.argv) != 3: | ||
| print("usage: parse_codex_json.py JSONL FEEDBACK", file=sys.stderr) | ||
| return 2 | ||
| source, feedback = map(Path, sys.argv[1:]) | ||
| try: | ||
| thread_id = extract(source, feedback) | ||
| except (OSError, UnicodeError, TypeError, ValueError) as exc: | ||
| sys.stdout.buffer.write(session_from_partial(source).encode("utf-8")) | ||
| print(f"feedback extraction failed from {source}: {exc}", file=sys.stderr) | ||
| return 2 | ||
| # Codex session IDs are ASCII. Avoid the host's stdout encoding for the payload. | ||
| sys.stdout.buffer.write(thread_id.encode("utf-8")) | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.