diff --git a/CHANGELOG.md b/CHANGELOG.md index b6217bba..cae12a53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,13 @@ All notable changes to the claude-plugins project will be documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`. +### code v1.14.12 + +#### Fixed +- `codex-review` extracts completed JSONL as UTF-8 and atomically replaces feedback only after validation, preserving a prior result when the stream is malformed or partial. +- The review wrapper retains Codex and parser diagnostics with the log ID and does not start a fresh review after a failed resume produces a nonempty stream. +- On extraction failure, the wrapper reports a thread ID already present in a valid start event without accepting incomplete feedback. + ### code-review v3.10.1 #### Added diff --git a/plugins/code/.claude-plugin/plugin.json b/plugins/code/.claude-plugin/plugin.json index 1e47cf73..d041a7c0 100644 --- a/plugins/code/.claude-plugin/plugin.json +++ b/plugins/code/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code", "description": "Code and planning framework plugin", - "version": "1.14.11", + "version": "1.14.12", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code/README.md b/plugins/code/README.md index e4c5afad..f589a4ab 100644 --- a/plugins/code/README.md +++ b/plugins/code/README.md @@ -164,6 +164,18 @@ Checks for `.closedloop-ai/closedloop-loop.local.md`, reads the current iteratio Supports resuming mid-session via a `{stem}.state` sidecar file that tracks the current round and Codex session ID. +### `/code:design-inventory` + +**Description:** Inventory a Claude Design export for review, then create draft feature tickets from the edited review document. + +**Usage:** +``` +/code:design-inventory [--repo ] [--workdir ] +/code:design-inventory --tickets --review-doc --project [--repo ] +``` + +Activates the `code:design-inventory` skill. The first form inventories the export and publishes a Design Review document; the second derives accepted decisions from the edited document and generates draft tickets. + --- ## Agents @@ -312,7 +324,7 @@ Generates a repo-local decision-table artifact that makes control-flow and state ### `design-inventory` -Staged pipeline for inventorying a Claude Design export into reviewable findings, a human decision gate, and DRAFT ticket generation. Stage A extracts the zip, runs parallel `design-unit-analyst` agents per unit (screens, regions, standalone components) from per-unit context packs, emits schema-validated findings (each with a recommended action), and publishes a platform "Design Review" Feature document with inline images. Stage B is the human editing that document - delete a section to decline, edit a line to amend, leave to accept - with survival judged from heading-line id anchors. Stage C derives decisions from the edited document and generates DRAFT feature tickets grouped per screen (UI plus optional API, with BLOCKS edges) and workdir-only design packs, only for accepted units. Invoked via the `code:design-inventory` skill when users request a design handoff, design inventory, or ticket generation from a design review. Scripts are TypeScript under `tools/design-inventory/src/` with built `dist/` bundles committed to `skills/design-inventory/scripts/dist/`. +Staged pipeline for inventorying a Claude Design export into reviewable findings, a human decision gate, and DRAFT ticket generation. Stage A extracts the zip, runs parallel `design-unit-analyst` agents per unit (screens, regions, standalone components) from per-unit context packs, emits schema-validated findings (each with a recommended action), and publishes a platform "Design Review" Feature document with inline images. Stage B is the human editing that document - delete a section to decline, edit a line to amend, leave to accept - with survival judged from heading-line id anchors. Stage C derives decisions from the edited document and generates DRAFT feature tickets grouped per screen (UI plus optional API, with BLOCKS edges) and workdir-only design packs, only for accepted units. Invoked via `/code:design-inventory` when users request a design handoff, design inventory, or ticket generation from a design review. Scripts are TypeScript under `tools/design-inventory/src/` with built `dist/` bundles committed to `skills/design-inventory/scripts/dist/`. --- diff --git a/plugins/code/skills/codex-review/SKILL.md b/plugins/code/skills/codex-review/SKILL.md index 344e3ca5..d30038da 100644 --- a/plugins/code/skills/codex-review/SKILL.md +++ b/plugins/code/skills/codex-review/SKILL.md @@ -33,19 +33,19 @@ bash ${CLAUDE_SKILL_DIR}/scripts/run_codex_review.sh \ | Argument | Required | Default | Description | |----------|----------|---------|-------------| | `--plan-file` | Yes | -- | Absolute path to the implementation plan (plan.json or plan.md) that Codex will review. The script injects this path into the review prompt so Codex can read and analyze the plan's contents. | -| `--feedback-file` | Yes | -- | Absolute path where the script writes Codex's full feedback text (parsed from the JSON stream). The orchestrator reads this file after the script completes to get the detailed findings. This file is overwritten each round. | +| `--feedback-file` | Yes | -- | Absolute path where the script writes Codex's full feedback text (parsed from the JSON stream). The orchestrator reads this file after the script completes to get the detailed findings. A completed, valid review atomically replaces the prior feedback; failed extraction preserves it. | | `--request-file` | No | -- | Absolute path to the original user request sidecar. When present and non-empty, the script tells Codex to read it before reviewing the plan so it can judge whether the plan chose the right overall approach for the request, not just whether the plan is internally consistent. If the file begins with `[synthesized]`, Codex is told to treat it as a weak hint rather than authoritative user intent. | | `--revisions-file` | No | -- | Absolute path to Claude's revision summary from the previous round, listing which findings were accepted and which were rejected with evidence. Only meaningful when round > 1 AND the file exists with actual content (the script checks `-s` for non-empty). The script injects this path into Codex's prompt so it can read the revisions before re-reviewing, but it explicitly tells Codex to verify Claude's rebuttals against the updated plan and codebase rather than trusting the summary blindly. **Omit entirely on round 1 or when no revisions file has been written yet** -- do not pass `/dev/null` or an empty file. | | `--round` | No | 1 | The current debate round number (1-indexed). Controls the review prompt phase: round 1 runs a broad but material audit, rounds 2-4 run a delta review that first checks whether prior findings were resolved, and rounds 5+ run a blocker-only convergence review. Also gates whether the revisions file is included in the prompt. | | `--codex-model` | No | gpt-5.3-codex | The OpenAI model ID passed to `codex -m`. Controls which model performs the review. | -| `--session-id` | No | -- | Codex thread ID returned as `CODEX_SESSION` from a previous round. When provided, the script attempts `codex exec resume ` to continue the conversation with full prior context. If resume fails, it falls back to a fresh session automatically. Omit on round 1. | +| `--session-id` | No | -- | Codex thread ID returned as `CODEX_SESSION` from a previous round. When provided, the script attempts `codex exec resume ` to continue the conversation with full prior context. It starts fresh only if the failed resume emitted no JSONL; a nonempty failed stream is retained for diagnosis without another model call. Omit on round 1. | | `--log-id` | No | auto-generated UUID | Identifier for the persistent JSONL log file at `~/.closedloop-ai/plan-with-codex/.jsonl`. The raw Codex JSON stream is appended here each round. Pass the same ID across all rounds of a debate to keep the full conversation history in one file. If omitted, a new UUID is generated. | ## Interpreting Output The script prints structured tokens to stdout. Parse these to control the debate loop. -All stdout responses include three tokens: a verdict (or failure indicator), `CODEX_SESSION`, and `LOG_ID`. The raw Codex JSON stream is appended to `~/.closedloop-ai/plan-with-codex/.jsonl`. Pass the LOG_ID back via `--log-id` on subsequent rounds to keep all rounds in one log file. +All stdout responses include three tokens: a verdict (or failure indicator), `CODEX_SESSION`, and `LOG_ID`. The raw Codex JSON stream is appended to `~/.closedloop-ai/plan-with-codex/.jsonl`; Codex and parser stderr is retained in the adjacent `.stderr` file. Pass the LOG_ID back via `--log-id` on subsequent rounds to keep all rounds in one log file. ### Approval @@ -90,7 +90,7 @@ LOG_ID:550e8400-e29b-41d4-a716-446655440000 ## How It Works 1. Builds a round-aware review prompt: round 1 performs a broad but material audit of both the chosen approach and the task details; rounds 2-4 verify prior findings and look only for net-new material issues; rounds 5+ only flag blocker-level issues that would likely lead to wrong behavior -2. If `--session-id` is provided, attempts `codex exec resume` for context continuity; falls back to a fresh session if resume fails -3. Parses the Codex JSON stream for `thread.started` (thread ID) and `item.completed`/`agent_message` (feedback text) +2. If `--session-id` is provided, attempts `codex exec resume` for context continuity; starts fresh only when a failed resume emitted no JSONL +3. Parses the UTF-8 Codex JSON stream for `thread.started` (thread ID), `item.completed`/`agent_message` (feedback text), and `turn.completed`; malformed or partial streams fail with retained diagnostics 4. If session resume succeeds but no new `thread.started` event appears, the input session ID is preserved and re-emitted (prevents losing session continuity) -5. Writes full feedback text to `--feedback-file`; emits only machine-parseable tokens to stdout +5. Atomically replaces `--feedback-file` after successful extraction, preserving any prior feedback on failure; emits only machine-parseable tokens to stdout diff --git a/plugins/code/skills/codex-review/scripts/parse_codex_json.py b/plugins/code/skills/codex-review/scripts/parse_codex_json.py new file mode 100644 index 00000000..ae382df5 --- /dev/null +++ b/plugins/code/skills/codex-review/scripts/parse_codex_json.py @@ -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: + 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()) diff --git a/plugins/code/skills/codex-review/scripts/run_codex_review.sh b/plugins/code/skills/codex-review/scripts/run_codex_review.sh index 4f6f6bb1..d51554a7 100755 --- a/plugins/code/skills/codex-review/scripts/run_codex_review.sh +++ b/plugins/code/skills/codex-review/scripts/run_codex_review.sh @@ -20,6 +20,7 @@ # Diagnostics go to stderr only. set -euo pipefail +umask 077 # Single source of truth for the state directory name CLOSEDLOOP_STATE_DIR=".closedloop-ai" @@ -77,6 +78,8 @@ fi LOG_DIR="$HOME/$CLOSEDLOOP_STATE_DIR/plan-with-codex" mkdir -p "$LOG_DIR" LOG_FILE="$LOG_DIR/$LOG_ID.jsonl" +ERROR_LOG="$LOG_DIR/$LOG_ID.stderr" +SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd) # ── Temp directory with cleanup ─────────────────────────────────────────────── @@ -220,40 +223,23 @@ PROMPT_EOF # ── JSON parsing helpers ───────────────────────────────────────────────────── -# Extract thread_id from thread.started event in JSON stream. -# Prints the thread_id or empty string. -parse_thread_id() { - python3 -c " -import json, sys -for line in open(sys.argv[1]): - try: - e = json.loads(line.strip()) - if e.get('type') == 'thread.started' and e.get('thread_id'): - print(e['thread_id']); break - except Exception: - pass -" "$1" 2>/dev/null || true -} - -# Extract agent_message text from item.completed events. -# Writes concatenated text to the specified output file. +# Publish feedback only after the complete JSONL stream validates. parse_feedback_text() { local json_file="$1" - local output_file="$2" - python3 -c " -import json, sys -lines = [] -for line in open(sys.argv[1]): - try: - e = json.loads(line.strip()) - if e.get('type') == 'item.completed': - item = e.get('item', {}) - if item.get('type') == 'agent_message' and item.get('text'): - lines.append(item['text']) - except Exception: - pass -sys.stdout.write('\n'.join(lines)) -" "$json_file" > "$output_file" 2>/dev/null + local parsed_session + if ! parsed_session=$(python3 "$SCRIPT_DIR/parse_codex_json.py" "$json_file" "$FEEDBACK_FILE" 2>> "$ERROR_LOG"); then + if [[ -n "$parsed_session" ]]; then + effective_session_id="$parsed_session" + fi + echo "Feedback extraction failed; details retained in $ERROR_LOG" >&2 + echo "CODEX_FAILED:feedback extraction failed; log=$LOG_FILE; diagnostics=$ERROR_LOG" + echo "CODEX_SESSION:${effective_session_id:-none}" + echo "LOG_ID:$LOG_ID" + return 1 + fi + if [[ -n "$parsed_session" ]]; then + effective_session_id="$parsed_session" + fi } # ── Run codex ──────────────────────────────────────────────────────────────── @@ -263,7 +249,7 @@ run_codex_cmd() { # Log round header printf '\n--- Round %s | %s ---\n' "$ROUND" "$(date -u +%Y-%m-%dT%H:%M:%SZ)" >> "$LOG_FILE" # Tee raw JSON stream to both the capture file and the persistent log - codex "$@" 2>/dev/null | tee -a "$LOG_FILE" > "$json_out" + codex "$@" 2>> "$ERROR_LOG" | tee -a "$LOG_FILE" > "$json_out" } effective_session_id="$SESSION_ID" @@ -280,35 +266,17 @@ if [[ -n "$SESSION_ID" ]]; then codex_exit=$? set -e - new_session=$(parse_thread_id "$codex_json") - if [[ -n "$new_session" ]]; then - effective_session_id="$new_session" - fi - # else: preserve the input SESSION_ID - - parse_feedback_text "$codex_json" "$FEEDBACK_FILE" - - if [[ $codex_exit -eq 0 ]] || [[ -s "$FEEDBACK_FILE" ]]; then - # Resume succeeded -- skip to verdict extraction - : - else + if [[ $codex_exit -ne 0 ]] && [[ ! -s "$codex_json" ]]; then + # Only an empty failed resume can safely start a fresh review. A partial + # stream may already contain paid work and must be diagnosed from its log. echo "Codex session resume failed, starting fresh session..." >&2 effective_session_id="" - rm -f "$codex_json" - - # Fall through to fresh session below set +e run_codex_cmd "$codex_json" exec "${base_args[@]}" "$prompt_content" codex_exit=$? set -e - - new_session=$(parse_thread_id "$codex_json") - if [[ -n "$new_session" ]]; then - effective_session_id="$new_session" - fi - - parse_feedback_text "$codex_json" "$FEEDBACK_FILE" fi + if ! parse_feedback_text "$codex_json"; then exit 0; fi else # No session to resume -- fresh start set +e @@ -316,12 +284,7 @@ else codex_exit=$? set -e - new_session=$(parse_thread_id "$codex_json") - if [[ -n "$new_session" ]]; then - effective_session_id="$new_session" - fi - - parse_feedback_text "$codex_json" "$FEEDBACK_FILE" + if ! parse_feedback_text "$codex_json"; then exit 0; fi fi # ── Emit structured tokens ─────────────────────────────────────────────────── diff --git a/plugins/code/skills/codex-review/tests/test_run_codex_review.py b/plugins/code/skills/codex-review/tests/test_run_codex_review.py new file mode 100644 index 00000000..280f7ff6 --- /dev/null +++ b/plugins/code/skills/codex-review/tests/test_run_codex_review.py @@ -0,0 +1,123 @@ +"""Synthetic end-to-end checks for the plan review JSONL wrapper.""" + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "run_codex_review.sh" +BASH = ( + str(Path(shutil.which("git") or "").parents[1] / "bin" / "bash.exe") + if os.name == "nt" else "bash" +) + + +def run_synthetic( + tmp_path: Path, jsonl: str, *, prior_feedback: bytes | None = None, + session_id: str | None = None, codex_exit: int = 0, +) -> tuple[subprocess.CompletedProcess[str], Path, Path, Path]: + home = tmp_path / "home" + home.mkdir() + plan = tmp_path / "plan.md" + plan.write_text("Synthetic plan", encoding="utf-8") + feedback = tmp_path / "feedback.md" + if prior_feedback is not None: + feedback.write_bytes(prior_feedback) + fixture = tmp_path / "review.jsonl" + fixture.write_text(jsonl, encoding="utf-8") + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + fake_codex = bin_dir / "codex" + fake_codex.write_text( + '#!/usr/bin/env bash\necho call >> "$TEST_CALLS"\n' + 'cat "$TEST_JSONL"\necho "synthetic codex diagnostic" >&2\n' + 'exit "$TEST_CODEX_EXIT"\n', encoding="utf-8", + ) + fake_codex.chmod(0o755) + calls = tmp_path / "codex-calls" + env = os.environ.copy() + env.update({ + "PATH": str(bin_dir) + os.pathsep + env["PATH"], + "HOME": home.as_posix(), + "TEST_JSONL": fixture.as_posix(), + "TEST_CALLS": calls.as_posix(), + "TEST_CODEX_EXIT": str(codex_exit), + "PYTHONIOENCODING": "ascii", + "PYTHONUTF8": "0", + }) + args = [BASH, SCRIPT.as_posix(), "--plan-file", plan.as_posix(), + "--feedback-file", feedback.as_posix(), "--log-id", "synthetic-review"] + if session_id is not None: + args.extend(("--session-id", session_id)) + completed = subprocess.run( + args, + env=env, capture_output=True, text=True, check=False, + stdin=subprocess.DEVNULL, + ) + return completed, feedback, home, calls + + +def event_jsonl(*events: object) -> str: + return "\n".join(json.dumps(event, ensure_ascii=False) for event in events) + "\n" + + +def test_completed_utf8_review_survives_ascii_stdout(tmp_path: Path) -> None: + """A completed review must survive the host's narrow output encoding.""" + verdict = "Finding: curly quote \u201d\nVERDICT: NEEDS_CHANGES" + completed, feedback, home, calls = run_synthetic( + tmp_path, + event_jsonl( + {"type": "thread.started", "thread_id": "synthetic-session"}, + {"type": "item.completed", "item": {"type": "agent_message", "text": verdict}}, + {"type": "turn.completed"}, + ), + prior_feedback=b"old usable feedback", + ) + assert completed.returncode == 0, completed.stderr + assert "VERDICT:NEEDS_CHANGES" in completed.stdout + assert "CODEX_SESSION:synthetic-session" in completed.stdout + assert feedback.read_bytes() == verdict.encode("utf-8") + assert calls.read_text(encoding="utf-8").splitlines() == ["call"] + diagnostics = home / ".closedloop-ai" / "plan-with-codex" / "synthetic-review.stderr" + assert "synthetic codex diagnostic" in diagnostics.read_text(encoding="utf-8") + + +@pytest.mark.parametrize( + ("jsonl", "expected_error"), + [ + ( + event_jsonl( + {"type": "thread.started", "thread_id": "synthetic-session"}, + {"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}}, + ) + '{"type":\n' + event_jsonl({"type": "turn.completed"}), + "JSONL line 3", + ), + ( + event_jsonl( + {"type": "thread.started", "thread_id": "synthetic-session"}, + {"type": "item.completed", "item": {"type": "agent_message", "text": "VERDICT: APPROVED"}}, + ), + "without turn.completed", + ), + ], +) +def test_failed_extraction_keeps_prior_feedback_and_does_not_rerun( + tmp_path: Path, jsonl: str, expected_error: str, +) -> None: + completed, feedback, home, calls = run_synthetic( + tmp_path, jsonl, prior_feedback=b"prior usable verdict", + session_id="previous-session", codex_exit=1, + ) + assert completed.returncode == 0, completed.stderr + assert "CODEX_FAILED:feedback extraction failed" in completed.stdout + 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"] + diagnostics = home / ".closedloop-ai" / "plan-with-codex" / "synthetic-review.stderr" + detail = diagnostics.read_text(encoding="utf-8") + assert expected_error in detail + assert "synthetic codex diagnostic" in detail