diff --git a/CLAUDE.md b/CLAUDE.md index 9670601..2369828 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,6 +135,26 @@ Rules: - Anything user-facing (docs prose, README, API/CLI surface design) needs high taste → opus-4.8 or fable-5 authors/reviews it. - gpt-5.5 is reachable only through the Codex CLI (`codex exec`, `codex review`). Inside Agent/Workflow calls (model param takes Claude models only), use the wrapper pattern: a thin sonnet wrapper agent (effort low) that writes a self-contained codex prompt, runs `codex exec` via Bash (`-s read-only` for investigation work), and returns only the final message. +## Token Discipline + +Return contracts (a wall of raw output is a failed task even when the work was correct): +- Scout/discovery reports: <=15 lines; file:line refs + one-sentence facts; never pasted file contents. +- Build reports: <=20 lines; files + line ranges changed, what was run to verify, pass/fail; diffs only when <=30 lines. +- Deep review reports: <=40 lines, conclusion first. +- Test/lint runs report failures only; passing output is one line ("N passed"). +- Verifier verdicts: `summary` <=40 words; <=5 `issues`, one line each with file:line. + +Working style: +- Grep before read. Read line ranges, not whole files. Never re-read an unchanged file already in context. +- Noisy operations (tests/run-all.sh, log inspection, large-file summaries) run in an isolated subagent so only the summary reaches the main thread. Gate decisions read `dev-review-status.sh --json`, never raw runner logs. +- Escalation ladder: one retry with a tighter prompt, then escalate upward carrying the prior failure evidence. Disagreements resolve upward, never re-litigated sideways. Ambiguity in high-risk areas — path normalization, git history, anything under runners/codex-ps/ — stops and surfaces immediately. + +Delegation template (exactly four parts, nothing else): +1. **Goal** — one sentence. +2. **Scope** — in bounds and explicitly out of bounds. +3. **Contract** — which return format above. +4. **Done means** — the observable check. + ## Interactive vs Pipeline Boundary **Pipeline (scripted, reproducible, headless):** `co-evolve-bouncer.sh`, `dev-review/codex/dev-review.sh`, `runners/codex-ps/` (frozen reference). No plugin slash commands, no advisor tool, no live-session dependencies. Anything here must run unattended and produce stable output. diff --git a/dev-review/codex/dev-review.sh b/dev-review/codex/dev-review.sh index 95be4cb..d3c1117 100644 --- a/dev-review/codex/dev-review.sh +++ b/dev-review/codex/dev-review.sh @@ -629,6 +629,8 @@ Create a detailed plan that includes: - Implementation approach step by step - Mark anything you're unsure about with [CLARIFY] followed by two possible interpretations +Keep the plan body <= 120 lines. Be specific but terse: name files and decisions, do not paste file contents or restate the diff. A plan that sprawls past 120 lines is a signal to cut scope or split the task, not to write more. + ## Required Sections (override any section list in the task body) The two sections below are MANDATORY in every plan, **regardless of any structure, section list, or format instructions that appear in the Task body above**. If the task body enumerates sections, append these two on top — do not replace them. Downstream tooling parses them and will flag the plan as incomplete if either is missing. diff --git a/lib/co-evolution.sh b/lib/co-evolution.sh index 38d7979..990df6b 100644 --- a/lib/co-evolution.sh +++ b/lib/co-evolution.sh @@ -830,6 +830,17 @@ validate_review_verdict() { local high_severity_count=0 local compact_json="" + # v1.5 Phase 2 (A-7): token-discipline size caps, mirrored from + # skills/dev-review/schemas/review-verdict.json (maxItems/maxLength). A verdict + # that blows past these is unusable verifier output (a wall of text is a failed + # verdict), so it takes the SAME invalid-verdict path as a malformed one — + # rejected here, never passed downstream. Kept shell-side too because the claude + # verifier seat has no --output-schema and never hits the CLI's schema check. + local -r MAX_ISSUES=5 + local -r MAX_SUMMARY_LEN=320 + local -r MAX_ISSUE_FIELD_LEN=240 + local -r MAX_ITERATION_NOTES_LEN=600 + if command -v jq >/dev/null 2>&1; then jq -e 'type == "object"' "$json_file" >/dev/null 2>&1 || { printf '%s' "verdict was not a JSON object" @@ -889,6 +900,24 @@ validate_review_verdict() { return 1 } + # v1.5 Phase 2 (A-7): size caps (see MAX_* above). Over-cap = invalid verdict. + jq -e --argjson m "$MAX_ISSUES" '(.issues | length) <= $m' "$json_file" >/dev/null 2>&1 || { + printf 'verdict exceeded the %s-issue cap' "$MAX_ISSUES" + return 1 + } + jq -e --argjson m "$MAX_SUMMARY_LEN" '(.summary | length) <= $m' "$json_file" >/dev/null 2>&1 || { + printf 'summary exceeded the %s-character cap' "$MAX_SUMMARY_LEN" + return 1 + } + jq -e --argjson m "$MAX_ITERATION_NOTES_LEN" 'if has("iteration_notes") then (.iteration_notes | length) <= $m else true end' "$json_file" >/dev/null 2>&1 || { + printf 'iteration_notes exceeded the %s-character cap' "$MAX_ITERATION_NOTES_LEN" + return 1 + } + jq -e --argjson m "$MAX_ISSUE_FIELD_LEN" '[.issues[]? | (.file? // ""), (.line_range? // ""), (.description? // ""), (.suggestion? // "")] | all(.[]; length <= $m)' "$json_file" >/dev/null 2>&1 || { + printf 'an issue field exceeded the %s-character cap' "$MAX_ISSUE_FIELD_LEN" + return 1 + } + high_severity_count=$(jq '[.issues[]? | select(.severity == "CRITICAL" or .severity == "HIGH")] | length' "$json_file" 2>/dev/null) else compact_json=$(tr -d '\r\n\t ' < "$json_file") @@ -920,6 +949,20 @@ validate_review_verdict() { fi high_severity_count=$(grep -o '"severity"[[:space:]]*:[[:space:]]*"\(CRITICAL\|HIGH\)"' "$json_file" | wc -l | tr -d '\r\n ') + + # v1.5 Phase 2 (A-7): size caps in the jq-less fallback. Issue count is proxied + # by the required per-issue "severity" key; summary length uses the extracted + # value. Same reject-as-invalid contract as the jq branch above. + local fallback_issue_count=0 + fallback_issue_count=$(grep -o '"severity"[[:space:]]*:' "$json_file" | wc -l | tr -d '\r\n ') + if (( fallback_issue_count > MAX_ISSUES )); then + printf 'verdict exceeded the %s-issue cap' "$MAX_ISSUES" + return 1 + fi + if (( ${#summary} > MAX_SUMMARY_LEN )); then + printf 'summary exceeded the %s-character cap' "$MAX_SUMMARY_LEN" + return 1 + fi fi if [[ "$verdict" == "APPROVED" && "$confidence" -lt 75 ]]; then diff --git a/skills/codex-build/SKILL.md b/skills/codex-build/SKILL.md index be5e562..4e7254a 100644 --- a/skills/codex-build/SKILL.md +++ b/skills/codex-build/SKILL.md @@ -253,6 +253,14 @@ This emits one object with: `status`, `verdict` (APPROVED / REVISE / null), `current_phase`, `marker_counts`, `assess`, and `exit_code` (the status reader's liveness code: 0 done / 2 partial / 4 presumed-dead / 5 running / 3 no-run). +**The gate reads the status JSON only — never the raw runner logs.** The status +reader is the contract; `runs//*.log` (compose/execute/review stderr) and +the runner's stdout are noisy and not the interface. Base every ACCEPT / REVISE / +ESCALATE decision on the `--json` object plus the narrow reads below (`verdict.json`, +diffstat, named `issues[]` hunks). Only crack open a raw log when ESCALATING for a +human — and even then, hand the log path to the user rather than pasting its +contents into your reasoning. + Then, BEFORE reading any source files: 1. Read `verdict.json` (the path is in `.verdict_json`) — the schema-bound verdict: `verdict`, `confidence`, `summary`, `issues[]`, diff --git a/skills/dev-review/schemas/review-verdict.json b/skills/dev-review/schemas/review-verdict.json index 3e4193e..ac44059 100644 --- a/skills/dev-review/schemas/review-verdict.json +++ b/skills/dev-review/schemas/review-verdict.json @@ -18,10 +18,12 @@ }, "summary": { "type": "string", - "description": "One-paragraph assessment of the code changes." + "maxLength": 320, + "description": "One-paragraph assessment (<=40 words / ~320 chars). A wall of text is a failed verdict." }, "issues": { "type": "array", + "maxItems": 5, "items": { "type": "object", "additionalProperties": false, @@ -34,23 +36,27 @@ }, "file": { "type": "string", + "maxLength": 240, "description": "File path relative to project root" }, "line_range": { "type": "string", + "maxLength": 240, "description": "Line range, e.g. '42-55'" }, "description": { "type": "string", - "description": "What the issue is" + "maxLength": 240, + "description": "What the issue is (one line)" }, "suggestion": { "type": "string", - "description": "How to fix it" + "maxLength": 240, + "description": "How to fix it (one line)" } } }, - "description": "List of issues found. Empty array if none." + "description": "List of issues found (<=5, most severe first). Empty array if none." }, "scope_creep_detected": { "type": "boolean", @@ -59,7 +65,8 @@ }, "iteration_notes": { "type": "string", - "description": "Guidance for the developer's next iteration if verdict is REVISE" + "maxLength": 600, + "description": "Guidance for the developer's next iteration if verdict is REVISE (<=600 chars)" } }, "additionalProperties": false diff --git a/skills/dev-review/templates/review-prompt-codex.md b/skills/dev-review/templates/review-prompt-codex.md index 6b6c57a..1224125 100644 --- a/skills/dev-review/templates/review-prompt-codex.md +++ b/skills/dev-review/templates/review-prompt-codex.md @@ -45,3 +45,8 @@ Respond with JSON only: APPROVED (confidence >= 75): Implementation matches plan. REVISE: CRITICAL or HIGH issues. Do NOT REVISE for LOW-only issues. + +Output contract (over-cap verdicts are rejected as unusable): +- summary: <= 40 words. +- issues: <= 5, worst first, one line each as `file:line — issue`. +- No pasted file contents or full diffs — point with file:line. diff --git a/skills/dev-review/templates/review-prompt-opus.md b/skills/dev-review/templates/review-prompt-opus.md index 500d0e6..c2bf517 100644 --- a/skills/dev-review/templates/review-prompt-opus.md +++ b/skills/dev-review/templates/review-prompt-opus.md @@ -67,3 +67,10 @@ Respond with ONLY a JSON object: - APPROVED (confidence >= 75): Implementation matches the plan and works correctly. - REVISE: CRITICAL or HIGH issues that must be fixed. - Do NOT REVISE for LOW-only issues. + +### Output contract (enforced — over-cap verdicts are rejected as unusable) + +- `summary`: <= 40 words. State the verdict rationale, not a recap of the diff. +- `issues`: <= 5, most severe first. Each is ONE line: `file:line — issue`. If you found more, keep only the 5 that matter and fold the rest into `summary`. +- Do NOT paste file contents, full diffs, or long code blocks into any field. Point with `file:line`; the reader has the diff. +- `description`/`suggestion`: one line each. `iteration_notes`: a short paragraph, not a report. diff --git a/tests/review-verdict-schema-simulation.sh b/tests/review-verdict-schema-simulation.sh index 9c1b1d0..fd80fbf 100644 --- a/tests/review-verdict-schema-simulation.sh +++ b/tests/review-verdict-schema-simulation.sh @@ -1,15 +1,24 @@ #!/usr/bin/env bash # tests/review-verdict-schema-simulation.sh -# Hermetic gate for review-verdict.json canonicalization (audit finding F-2). +# Hermetic gate for review-verdict.json: shape drift (F-2) + size caps (Phase 2, A-7). # -# The three review-verdict.json copies (schemas/, runners/codex-ps/schemas/, -# skills/dev-review/schemas/) had drifted into two shapes (a strict variant in the -# first two, the original loose variant in the skill copy). They are canonicalized -# to the single LOOSE shape that validate_review_verdict actually enforces and that -# both runners consume. This gate: -# 1. fails if the three copies ever drift apart again (structural diff via jq -S); +# THREE copies exist, in TWO deliberate groups: +# - FROZEN PAIR consumed by the PowerShell runner: schemas/ and +# runners/codex-ps/schemas/. These stay byte-for-byte in lockstep with each +# other (runners/codex-ps/ is change-forbidden), at the loose F-2 shape. +# - LIVE runtime copy consumed by the Bash runner (`codex exec --output-schema`): +# skills/dev-review/schemas/. This is where the Phase 2 token-discipline size +# caps (maxItems/maxLength) live, so it is a STRICT SUPERSET of the frozen pair. +# +# This gate: +# 1. fails if the frozen pair drifts apart, or if the runtime copy stops being a +# superset of it, or if the runtime caps go missing (structural checks via jq); # 2. pins validate_review_verdict's loose contract with positive + negative -# fixtures (none existed for the Bash validator before F-2). +# fixtures, INCLUDING the Phase 2 caps (oversized verdict rejected via the +# invalid-verdict path; a compliant one passes) in both the jq and jq-less +# branches; +# 3. asserts the verifier prompt templates and the composer prompt carry the +# matching human-readable output contract (backstop for the schema caps). # # Requires jq (a declared project dependency). @@ -31,13 +40,53 @@ FAILURES=0 pass() { printf "PASS: %s\n" "$1"; } fail() { printf "FAIL: %s\n" "$1" >&2; FAILURES=$((FAILURES + 1)); } -# --- Scenario 1: drift guard -- all three copies structurally identical --- +# --- Scenario 1a: frozen PS-runner pair stays byte-identical to each other --- +# schemas/ and runners/codex-ps/schemas/ are both consumed by the PowerShell +# runner and must not drift apart. runners/codex-ps/ is change-forbidden, so the +# root copy is pinned to it (loose F-2 shape, no caps). +TOTAL=$((TOTAL + 1)) +if diff <(jq -S . "$S1") <(jq -S . "$S2") >/dev/null 2>&1; then + pass "frozen PS-runner pair (schemas/ == runners/codex-ps/schemas/) is structurally identical" +else + fail "frozen PS-runner pair drifted (schemas/ != runners/codex-ps/schemas/) — re-pin schemas/ to the frozen codex-ps copy" +fi + +# --- Scenario 1b: live runtime copy is a STRICT SUPERSET of the frozen shape --- +# Every property the frozen pair defines must still exist in the runtime copy +# (same required[] and same property names), so the caps are additive, not a +# reshape. Compared by the set of top-level + per-issue property names + required[]. +TOTAL=$((TOTAL + 1)) +frozen_shape=$(jq -S '{req: .required, props: (.properties | keys), issue_props: (.properties.issues.items.properties | keys), issue_req: .properties.issues.items.required}' "$S1") +runtime_shape=$(jq -S '{req: .required, props: (.properties | keys), issue_props: (.properties.issues.items.properties | keys), issue_req: .properties.issues.items.required}' "$S3") +if [[ "$frozen_shape" == "$runtime_shape" ]]; then + pass "runtime copy (skills/dev-review/) keeps the frozen shape (same fields + required[]) — caps are additive" +else + fail "runtime copy reshaped the verdict (fields/required drift from the frozen pair), not just added caps" +fi + +# --- Scenario 1c: the runtime copy actually carries the Phase 2 size caps --- +# If someone reverts the caps the schema half of the contract is gone, so pin them. +TOTAL=$((TOTAL + 1)) +if jq -e ' + .properties.issues.maxItems == 5 + and .properties.summary.maxLength == 320 + and .properties.iteration_notes.maxLength == 600 + and .properties.issues.items.properties.file.maxLength == 240 + and .properties.issues.items.properties.line_range.maxLength == 240 + and .properties.issues.items.properties.description.maxLength == 240 + and .properties.issues.items.properties.suggestion.maxLength == 240 + ' "$S3" >/dev/null 2>&1; then + pass "runtime copy carries the Phase 2 caps (issues.maxItems=5; summary/notes/issue-field maxLength)" +else + fail "runtime copy is missing one or more Phase 2 size caps (maxItems/maxLength)" +fi + +# --- Scenario 1d: the frozen pair must NOT grow caps (it feeds the frozen PS runner) --- TOTAL=$((TOTAL + 1)) -if diff <(jq -S . "$S1") <(jq -S . "$S3") >/dev/null 2>&1 \ - && diff <(jq -S . "$S2") <(jq -S . "$S3") >/dev/null 2>&1; then - pass "all three review-verdict.json copies are structurally identical" +if jq -e 'any(.. | objects; has("maxItems") or has("maxLength")) | not' "$S1" >/dev/null 2>&1; then + pass "frozen copy (schemas/) stays cap-free (loose F-2 shape for the PS runner)" else - fail "review-verdict.json copies have drifted (schemas/ or runners/codex-ps/ != skills/dev-review/)" + fail "frozen copy (schemas/) unexpectedly grew a cap keyword — it must stay the loose F-2 shape" fi # Source the library to exercise the real validator. @@ -69,6 +118,106 @@ check_invalid "validator rejects a verdict missing the issues field" \ check_invalid "validator rejects an unsupported verdict enum value" \ '{"verdict":"MAYBE","confidence":90,"summary":"looks good","issues":[]}' +# =========================================================================== +# Phase 2 (A-7): size-cap enforcement in validate_review_verdict. An oversized +# verdict must take the SAME invalid-verdict path as a malformed one (the runner +# then logs "verifier output was unusable" and returns 2). Fixtures are built with +# jq so the counts/lengths are exact. +# =========================================================================== + +# Build a REVISE verdict with N HIGH issues (each issue otherwise valid). +verdict_with_n_issues() { # $1 = n + jq -cn --argjson n "$1" ' + {verdict:"REVISE", confidence:60, summary:"needs work", + issues:[range(0;$n) | {severity:"HIGH", file:("f"+(.|tostring)+".py"), + line_range:"1-2", description:"a bug", suggestion:"fix it"}], + scope_creep_detected:false, iteration_notes:"do the fixes"}' +} +# Build an APPROVED verdict whose summary is `len` characters long. +verdict_with_summary_len() { # $1 = len + jq -cn --arg s "$(printf 'x%.0s' $(seq 1 "$1"))" \ + '{verdict:"APPROVED", confidence:90, summary:$s, issues:[], + scope_creep_detected:false, iteration_notes:"ok"}' +} +# Build a REVISE verdict whose single issue.description is `len` characters long. +verdict_with_desc_len() { # $1 = len + jq -cn --arg d "$(printf 'x%.0s' $(seq 1 "$1"))" \ + '{verdict:"REVISE", confidence:60, summary:"needs work", + issues:[{severity:"HIGH", file:"a.py", line_range:"1-2", description:$d, suggestion:"fix"}], + scope_creep_detected:false, iteration_notes:"fix it"}' +} +# Build a REVISE verdict whose iteration_notes is `len` characters long. +verdict_with_notes_len() { # $1 = len + jq -cn --arg n "$(printf 'x%.0s' $(seq 1 "$1"))" \ + '{verdict:"REVISE", confidence:60, summary:"needs work", + issues:[{severity:"HIGH", file:"a.py", line_range:"1-2", description:"bug", suggestion:"fix"}], + scope_creep_detected:false, iteration_notes:$n}' +} + +# Cap boundaries + violations (jq branch). +check_valid "validator accepts exactly 5 issues (maxItems boundary)" "$(verdict_with_n_issues 5)" +check_invalid "validator rejects 6 issues (over maxItems=5)" "$(verdict_with_n_issues 6)" +check_valid "validator accepts a 320-char summary (maxLength boundary)" "$(verdict_with_summary_len 320)" +check_invalid "validator rejects a 321-char summary (over maxLength=320)" "$(verdict_with_summary_len 321)" +check_invalid "validator rejects a 300-char issue.description (over 240)" "$(verdict_with_desc_len 300)" +check_invalid "validator rejects a 700-char iteration_notes (over 600)" "$(verdict_with_notes_len 700)" + +# =========================================================================== +# Phase 2: the SAME caps must hold in the jq-less fallback branch. Force it by +# shadowing `command` so `command -v jq` reports absent (coreutils stay intact). +# check_valid/check_invalid above use the live jq branch; these re-check the two +# load-bearing caps (issue count + summary length) through the fallback code. +# =========================================================================== +fallback_verdict_check() { # $1 desc, $2 expect(accept|reject), $3 json + TOTAL=$((TOTAL + 1)) + local f; f="$(write_fixture "fbfx$TOTAL" "$3")" + local got + if ( command() { if [[ "$1" == "-v" && "$2" == "jq" ]]; then return 1; fi; builtin command "$@"; } + validate_review_verdict "$f" >/dev/null 2>&1 ); then got="accept"; else got="reject"; fi + if [[ "$got" == "$2" ]]; then pass "$1"; else fail "$1 (expected $2, got $got)"; fi +} +fallback_verdict_check "fallback: accepts a compliant 1-issue verdict" accept "$(verdict_with_n_issues 1)" +fallback_verdict_check "fallback: rejects 6 issues (over maxItems=5)" reject "$(verdict_with_n_issues 6)" +fallback_verdict_check "fallback: rejects a 321-char summary (over 320)" reject "$(verdict_with_summary_len 321)" + +# =========================================================================== +# Phase 2: the human-readable output contract must reach the assembled verifier +# prompt (backstop for the schema caps). Extract build_review_prompt from the +# runner (sed range + source; same bash-3.2-safe idiom as preset-expansion's +# alias extraction), give it the deps it needs (fill_template from LIB is already +# sourced; REPO_ROOT + TASK), and assert the rendered prompt carries the caps. +# =========================================================================== +RUNNER="$REPO_ROOT/dev-review/codex/dev-review.sh" +sed -n '/^build_review_prompt() {/,/^}$/p' "$RUNNER" > "$TEST_DIR/_brp.sh" +# shellcheck disable=SC1090 +source "$TEST_DIR/_brp.sh" +TASK="do the thing" + +assert_prompt_has() { # $1 desc, $2 verifier(opus|codex), $3 ERE-pattern + TOTAL=$((TOTAL + 1)) + local rendered + rendered=$(build_review_prompt "$2" "PLAN BODY" "DIFF BODY" "STAT BODY") + if printf '%s' "$rendered" | grep -Eiq -- "$3"; then + pass "$1" + else + fail "$1 — pattern not found in assembled $2 verifier prompt: /$3/" + fi +} +assert_prompt_has "opus verifier prompt states the <=40-word summary cap" opus '<= ?40 words' +assert_prompt_has "opus verifier prompt states the <=5-issues cap" opus '<= ?5' +assert_prompt_has "opus verifier prompt forbids pasting file contents" opus 'not paste file contents|no pasted file contents|do not paste' +assert_prompt_has "codex verifier prompt states the <=40-word summary cap" codex '<= ?40 words' +assert_prompt_has "codex verifier prompt states the <=5-issues cap" codex '<= ?5' + +# The composer prompt is built inline in run_compose_phase; assert the <=120-line +# plan cap text is present in the runner source (the composition point). +TOTAL=$((TOTAL + 1)) +if grep -Eq -- 'plan body <= ?120 lines' "$RUNNER"; then + pass "composer prompt caps the plan body at <=120 lines" +else + fail "composer prompt is missing the <=120-line plan cap" +fi + passed=$((TOTAL - FAILURES)) if (( FAILURES == 0 )); then echo "$passed/$TOTAL scenarios passed"