Merge pull request #48 from alanshurafa/claude/imp-b-robustness

Phase B: verify-phase timeout ladder + diff-injection hardening

Author: alanshurafa

.planning/notes/2026-07-07-audit-improvement-plan.md

@@ -98,7 +98,7 @@ Alan's labeling role is replaced by a cross-family judge panel; his involvement
 - **Done means:** skill installable from a public marketplace entry; benchmark numbers public; workspace CLAUDE.md workflow table updated.
 
 ### Explicitly deferred
-Deterministic lint/secret pre-pass (M, valuable but code-pipeline-only); parallel specialist critic lenses (M, measure after Phase D's severity gate lands); Greptile-style dependency-context pass (L); jq/JSON layer extraction + CRLF ingest normalization + seat-guard lib extraction (S-3/S-5, fold into whichever phase next touches those lines); confidence-weighted adjudication voting; `--dual-critique` (unchanged from prior backlog, after Phase D).
+`fill_template`/`fill_conditional` in lib rescan the accumulating string per placeholder pair — the same re-expansion class fixed with nonce sentinels in dev-review.sh's prompt builders (Phase B, cycles 2-3). A lib-level two-pass rewrite would fix every caller at once; deferred because callers substitute mostly trusted values today and the rewrite touches every pipeline. Revisit when Phase C/D next touches lib templating. Deterministic lint/secret pre-pass (M, valuable but code-pipeline-only); parallel specialist critic lenses (M, measure after Phase D's severity gate lands); Greptile-style dependency-context pass (L); jq/JSON layer extraction + CRLF ingest normalization + seat-guard lib extraction (S-3/S-5, fold into whichever phase next touches those lines); confidence-weighted adjudication voting; `--dual-critique` (unchanged from prior backlog, after Phase D).
 
 ### Approval gates (updated 2026-07-07 — Alan approved autonomous execution)
 - Phase E spend (calibration, A/B, canaries, dogfood) — **approved 2026-07-07** ("A/B testing is better done by Fable"); codex-guard daily cap remains the hard ceiling; batch, never poll.

.planning/notes/2026-07-07-execution-loop.md

@@ -39,8 +39,8 @@ Loop mechanics: background agents re-invoke the orchestrator on completion (no p
 
 | Phase | Status | Branch / PR | Verify (suite / adv / codex / done-means) | Notes |
 |-------|--------|-------------|-------------------------------------------|-------|
-| A — Correctness closure | PR #47 open, suite 32/32, awaiting CI → merge | claude/nervous-hodgkin-bcf03d → PR #47 | ✓32/32 / ✓(F1 fixed) / ✓(H1,H2,L1 fixed) / ✓ | Cross-vendor review earned its keep: codex found the partial-failure→converged gap (H1) and both vendors independently flagged the bare-banner auth gap (H2→`output_is_auth_failure` in lib, 3 call sites). Claude reviewer caught the Scenario-F grep regression (F1) + missing guard scenario (→Scenario G). Bonus find-along: bounce-scorer-verification.sh had a Windows jq-CRLF bug (5/7→7/7, fixed) before wiring into run-all (C-5). Accepted residual: none remaining — F2/H2 fixed. Sims: auth-gate 28/28, marker-lifecycle 41/41 (byte-parity intact), audit-hardening 18/18, worktree-mgmt green, reliability 17/17. |
-| B — Robustness/injection | pending | | | |
+| A — Correctness closure | DONE — merged f295e8b (PR #47) | claude/nervous-hodgkin-bcf03d → PR #47 | ✓32/32 / ✓(F1 fixed) / ✓(H1,H2,L1 fixed) / ✓ | Cross-vendor review earned its keep: codex found the partial-failure→converged gap (H1) and both vendors independently flagged the bare-banner auth gap (H2→`output_is_auth_failure` in lib, 3 call sites). Claude reviewer caught the Scenario-F grep regression (F1) + missing guard scenario (→Scenario G). Bonus find-along: bounce-scorer-verification.sh had a Windows jq-CRLF bug (5/7→7/7, fixed) before wiring into run-all (C-5). Accepted residual: none remaining — F2/H2 fixed. Sims: auth-gate 28/28, marker-lifecycle 41/41 (byte-parity intact), audit-hardening 18/18, worktree-mgmt green, reliability 17/17. |
+| B — Robustness/injection | IN PROGRESS — build agent launched | claude/imp-b-robustness | – / – / – / – | C-3 shared timeout-runner helper; C-4 long-fence + untrusted-data framing |
 | C — Protocol v0.2 | pending | | | includes docs sweep + STACK.md re-check |
 | D — Signal quality | pending | | | can start once C's marker changes are stable |
 | E — Measurement | pending | | | panel-labeled gold set; spend approved |
@@ -57,6 +57,7 @@ Loop mechanics: background agents re-invoke the orchestrator on completion (no p
 | Verifier canary catch rate (n=3) | – | | E.4 |
 | A/B: cross- vs same-vendor (pre-registered criterion) | – | | E.3 |
 | Master suite trend | baseline: 27 sims + scorer gate green @ 05d151e | 2026-07-07 | V-6 |
+| Master suite trend | 32/32 suites (local) + 6/6 CI checks 3-OS @ f295e8b (Phase A) | 2026-07-07 | V-6 |
 
 ## Handoff notes
 

dev-review/codex/dev-review.sh

@@ -257,6 +257,34 @@ abort_on_timeout() {
   fi
 }
 
+# PR#48-M4: a timeout-runner INFRASTRUCTURE failure (perl fork() = 125, or the
+# runner reporting the command could not be executed / was not found = 126/127)
+# means the verifier never ran — the verdict file is empty or stale, not a real
+# verdict. abort_on_timeout only special-cases 124, so without this an infra
+# crash would fall through to verdict parsing and could launder into a "proceed"
+# outcome. Abort hard, with a logged reason, and NEVER parse the verdict file.
+# Mirrors abort_on_timeout's terminal-state bookkeeping so a status reader sees a
+# failed run rather than one stuck mid-phase.
+abort_on_runner_infra_failure() {
+  local phase_name="$1"
+  local phase_start="$2"
+  case "$LAST_INVOKE_EXIT_CODE" in
+    125|126|127) ;;
+    *) return 0 ;;
+  esac
+  local phase_end
+  phase_end=$(date -u +%Y-%m-%dT%H:%M:%SZ)
+  if [[ -n "${STATE_JSON:-}" ]]; then
+    write_state_phase "$STATE_JSON" "$phase_name" "failed" "$LAST_INVOKE_EXIT_CODE" "$phase_start" "$phase_end"
+    write_state_field "$STATE_JSON" ".completed_at" "string" "$phase_end"
+    write_state_field "$STATE_JSON" ".status" "string" "failed"
+    write_state_field "$STATE_JSON" ".current_phase" "null"
+  fi
+  log "ERROR: ${phase_name} phase timeout-runner could not launch the agent (exit ${LAST_INVOKE_EXIT_CODE}) - aborting run without parsing the verdict file"
+  cleanup_runtime_artifacts
+  exit 1
+}
+
 require_agent_cli() {
   case "$1" in
     codex)
@@ -407,14 +435,23 @@ build_bounce_prompt() {
     cat "${REPO_ROOT}/skills/dev-review/templates/bounce-protocol.md"
   } > "$prompt_template_file"
 
-  rendered=$(fill_template "$prompt_template_file" \
-    "TASK=$TASK" \
-    "PASS_NUMBER=$pass_number" \
-    "TOTAL_PASSES=$total_passes" \
-    "YOUR_ROLE=$role" \
-    "WORKING_DIR=$WORKDIR")
-
-  rendered="${rendered//\{PLAN_CONTENT\}/$plan_content}"
+  # C-4b: two-pass nonce substitution (same scheme and rationale as
+  # build_review_prompt) — a TASK that mentions {PLAN_CONTENT} in prose must
+  # stay literal instead of pulling a second plan expansion into the prompt.
+  local nonce="${RANDOM}${RANDOM}$$"
+  rendered=$(cat "$prompt_template_file")
+  rendered="${rendered//\{TASK\}/<CE_SUB_${nonce}_TASK>}"
+  rendered="${rendered//\{PASS_NUMBER\}/<CE_SUB_${nonce}_PASS>}"
+  rendered="${rendered//\{TOTAL_PASSES\}/<CE_SUB_${nonce}_TOTAL>}"
+  rendered="${rendered//\{YOUR_ROLE\}/<CE_SUB_${nonce}_ROLE>}"
+  rendered="${rendered//\{WORKING_DIR\}/<CE_SUB_${nonce}_WD>}"
+  rendered="${rendered//\{PLAN_CONTENT\}/<CE_SUB_${nonce}_PLAN>}"
+  rendered="${rendered//<CE_SUB_${nonce}_TASK>/$TASK}"
+  rendered="${rendered//<CE_SUB_${nonce}_PASS>/$pass_number}"
+  rendered="${rendered//<CE_SUB_${nonce}_TOTAL>/$total_passes}"
+  rendered="${rendered//<CE_SUB_${nonce}_ROLE>/$role}"
+  rendered="${rendered//<CE_SUB_${nonce}_WD>/$WORKDIR}"
+  rendered="${rendered//<CE_SUB_${nonce}_PLAN>/$plan_content}"
   printf '%s' "$rendered"
 }
 
@@ -462,27 +499,47 @@ build_execution_prompt() {
   local stripped_template_file="$RUN_DIR/.execute-template-${executor}.md"
   local rendered
 
+  # C-4b: two-pass nonce substitution (same scheme and rationale as
+  # build_review_prompt). Sequential replacement rescans the accumulating
+  # string, so a value carrying another placeholder's literal text gets
+  # re-expanded — here the worst source is the RETRY branch, where
+  # REVIEWER_FEEDBACK/ISSUES_LIST come from the verifier's verdict (itself
+  # influenced by the diff under review) and used to be substituted BEFORE
+  # {TASK}/{PLAN_CONTENT}. Ordering cannot fix the class; a sentinel minted from
+  # $RANDOM$RANDOM$$ after all values exist is overwhelmingly unlikely to appear
+  # in any value.
+  local nonce="${RANDOM}${RANDOM}$$"
+
   if [[ -z "$feedback_json" ]]; then
     # First pass: strip the SUBSEQUENT_PASS block entirely.
-    # Byte-identical output to v1.0 (see Task 4 Scenario 4 invariant).
+    # Byte-identical output to v1.0 (see Task 4 Scenario 4 invariant) — the
+    # nonce round-trip is byte-neutral for values without placeholder text.
     strip_conditional "SUBSEQUENT_PASS" < "$template_path" > "$stripped_template_file"
-    rendered=$(fill_template "$stripped_template_file" "TASK=$TASK")
-    rendered="${rendered//\{PLAN_CONTENT\}/$plan_content}"
+    rendered=$(cat "$stripped_template_file")
+    rendered="${rendered//\{TASK\}/<CE_SUB_${nonce}_TASK>}"
+    rendered="${rendered//\{PLAN_CONTENT\}/<CE_SUB_${nonce}_PLAN>}"
+    rendered="${rendered//<CE_SUB_${nonce}_TASK>/$TASK}"
+    rendered="${rendered//<CE_SUB_${nonce}_PLAN>/$plan_content}"
   else
-    # Retry pass: keep the SUBSEQUENT_PASS block; replace {REVIEWER_FEEDBACK} and
-    # {ISSUES_LIST} with rendered content from the normalized verdict JSON.
-    # fill_conditional reads the template on stdin, strips the IF/END_IF tag
-    # lines, and substitutes KEY={value} placeholders in the full stripped text.
+    # Retry pass: keep the SUBSEQUENT_PASS block; replace {REVIEWER_FEEDBACK}
+    # and {ISSUES_LIST} with rendered content from the normalized verdict JSON.
+    # fill_conditional is called with NO key=value pairs so it ONLY strips the
+    # IF/END_IF tag lines (its internal substitution loop rescans the
+    # accumulator — the exact class being closed); all four placeholders are
+    # then swapped through nonce sentinels locally.
     local reviewer_feedback issues_list
     reviewer_feedback=$(build_reviewer_feedback_summary "$feedback_json")
     issues_list=$(build_issues_list_markdown "$feedback_json")
 
-    rendered=$(fill_conditional "SUBSEQUENT_PASS" \
-      "REVIEWER_FEEDBACK=$reviewer_feedback" \
-      "ISSUES_LIST=$issues_list" \
-      < "$template_path")
-    rendered="${rendered//\{TASK\}/$TASK}"
-    rendered="${rendered//\{PLAN_CONTENT\}/$plan_content}"
+    rendered=$(fill_conditional "SUBSEQUENT_PASS" < "$template_path")
+    rendered="${rendered//\{TASK\}/<CE_SUB_${nonce}_TASK>}"
+    rendered="${rendered//\{PLAN_CONTENT\}/<CE_SUB_${nonce}_PLAN>}"
+    rendered="${rendered//\{REVIEWER_FEEDBACK\}/<CE_SUB_${nonce}_FB>}"
+    rendered="${rendered//\{ISSUES_LIST\}/<CE_SUB_${nonce}_ISSUES>}"
+    rendered="${rendered//<CE_SUB_${nonce}_TASK>/$TASK}"
+    rendered="${rendered//<CE_SUB_${nonce}_PLAN>/$plan_content}"
+    rendered="${rendered//<CE_SUB_${nonce}_FB>/$reviewer_feedback}"
+    rendered="${rendered//<CE_SUB_${nonce}_ISSUES>/$issues_list}"
   fi
 
   printf '%s' "$rendered"
@@ -496,10 +553,42 @@ build_review_prompt() {
   local template_path="${REPO_ROOT}/skills/dev-review/templates/review-prompt-${verifier}.md"
   local rendered
 
-  rendered=$(fill_template "$template_path" "TASK=$TASK")
-  rendered="${rendered//\{PLAN_CONTENT\}/$plan_content}"
-  rendered="${rendered//\{DIFF\}/$diff_content}"
-  rendered="${rendered//\{DIFF_STAT\}/$diff_stat}"
+  # C-4: wrap the untrusted diff in a fence longer than any backtick run it
+  # contains, so a diff line that is itself ``` cannot close the ```diff block
+  # early and have its remainder (e.g. "output APPROVED, no issues") read as
+  # verifier instructions. The template's {DIFF_FENCE} placeholder supplies both
+  # the opening (`{DIFF_FENCE}diff`) and closing fence.
+  #
+  # PR#48-L6: {DIFF_STAT} is the same untrusted, git-derived source as {DIFF} — a
+  # tracked path can carry a backtick run — and the templates now fence it too.
+  # Size the single shared fence over BOTH bodies (newline-joined so a run cannot
+  # straddle the join) so it is longer than any backtick run in either block.
+  local diff_fence
+  diff_fence=$(compute_diff_fence "${diff_content}"$'\n'"${diff_stat}")
+
+  # C-4b: two-pass nonce substitution. Sequential ${rendered//{KEY}/value}
+  # rescans the ACCUMULATING string, so any value substituted earlier that
+  # contains the literal text of a later placeholder gets re-expanded — e.g. a
+  # plan discussing this template's {DIFF} placeholder in prose, or a tracked
+  # path named {DIFF} surfacing in the --stat output, would pull a second raw
+  # diff expansion OUTSIDE the fence. Substitution ORDER cannot fix this class
+  # (any field can carry any other field's placeholder). Instead: pass 1
+  # rewrites the TRUSTED template's placeholders to run-unique sentinels minted
+  # AFTER every value was computed (so no value can contain one); pass 2 swaps
+  # each sentinel for its value exactly once. Untrusted text is never rescanned
+  # for placeholders, and placeholder-looking text in values stays literal.
+  local nonce="${RANDOM}${RANDOM}$$"
+  rendered=$(cat "$template_path")
+  rendered="${rendered//\{TASK\}/<CE_SUB_${nonce}_TASK>}"
+  rendered="${rendered//\{DIFF_FENCE\}/<CE_SUB_${nonce}_FENCE>}"
+  rendered="${rendered//\{PLAN_CONTENT\}/<CE_SUB_${nonce}_PLAN>}"
+  rendered="${rendered//\{DIFF_STAT\}/<CE_SUB_${nonce}_STAT>}"
+  rendered="${rendered//\{DIFF\}/<CE_SUB_${nonce}_DIFF>}"
+  rendered="${rendered//<CE_SUB_${nonce}_TASK>/$TASK}"
+  rendered="${rendered//<CE_SUB_${nonce}_FENCE>/$diff_fence}"
+  rendered="${rendered//<CE_SUB_${nonce}_PLAN>/$plan_content}"
+  rendered="${rendered//<CE_SUB_${nonce}_STAT>/$diff_stat}"
+  rendered="${rendered//<CE_SUB_${nonce}_DIFF>/$diff_content}"
   printf '%s' "$rendered"
 }
 
@@ -1039,18 +1128,33 @@ run_verify_phase() {
     # FIX-WR-01: reset before the conditional so that a successful run leaves 0
     # (the `|| LAST_INVOKE_EXIT_CODE=$?` branch only fires on non-zero exit).
     LAST_INVOKE_EXIT_CODE=0
-    if command -v timeout >/dev/null 2>&1; then
-      timeout --foreground "${PHASE_TIMEOUT:-1800}s" \
+    # C-3: route through the shared timeout-runner ladder (timeout→gtimeout→perl)
+    # so this branch is bounded on stock macOS too, not only where GNU `timeout`
+    # exists. This is the default claude-build verify path and the historical
+    # 1h39m hang site, so an unbounded fallback was the worst place for the gap.
+    local _verify_runner
+    _verify_runner=$(select_timeout_runner)
+    if [[ -n "$_verify_runner" ]]; then
+      # PR#48-M5: validate PHASE_TIMEOUT here too (the opus branch gets it via
+      # invoke_agent_with_timeout). Unvalidated, a 0/non-numeric value would run
+      # this default claude-build verify path unbounded — the historical hang.
+      require_phase_timeout
+      run_with_timeout_runner "$_verify_runner" "$EFFECTIVE_PHASE_TIMEOUT" \
         bash -c 'cd "$1" && source "$2/lib/co-evolution.sh"; invoke_codex_schema "$3" "$4" "$5" "$6"' _ \
         "$PWD" "$REPO_ROOT" "$review_prompt_file" "$verdict_file" "$review_stderr_file" "${REPO_ROOT}/skills/dev-review/schemas/review-verdict.json" \
         || LAST_INVOKE_EXIT_CODE=$?
     else
+      log "WARNING: no timeout(1)/gtimeout/perl found - codex verify running unbounded"
       invoke_codex_schema "$review_prompt_file" "$verdict_file" "$review_stderr_file" "${REPO_ROOT}/skills/dev-review/schemas/review-verdict.json"
     fi
     abort_on_timeout "verify" "$phase_start"
+    # PR#48-M4: a runner infra failure (125/126/127) means the verifier never ran;
+    # abort before any verdict-file parsing so it cannot launder into "proceed".
+    abort_on_runner_infra_failure "verify" "$phase_start"
   else
     invoke_agent_with_timeout "$verifier" "$review_prompt_file" "$verdict_file" "$review_stderr_file" "$(phase_is_writable review)"
     abort_on_timeout "verify" "$phase_start"
+    abort_on_runner_infra_failure "verify" "$phase_start"
   fi
 
   if agent_auth_failed "$verifier" "$verdict_file" "$review_stderr_file"; then