review/adversarial-review Step 3 system-instruction hardening — required H2 headings unreliable in Codex output

[kiki830621]

Problem

The producer skills' Step 3 system-instructions request specific H2 section headings, but Codex does not emit them reliably — it's LLM prose and sometimes omits or restructures the headings.

• plugins/codex-pro/skills/review/SKILL.md Step 3 asks Codex to begin with ## Summary then ## Findings. Observed during the v0.4.x e2e cycle: Codex sometimes omits ## Summary (or both).
• plugins/codex-pro/skills/adversarial-review/SKILL.md Step 3 asks for 4 mandatory H2 sections (## Assumptions Challenged / ## Failure Modes / ## Alternative Approaches / ## Trade-off Counterarguments). These hold up better (forceful "exactly four sections, each non-empty" wording), but the same fragility class applies.

Because of this non-determinism, tests/e2e.sh currently checks these headings with verify_substring_warn (best-effort warning, not a hard failure) instead of verify_substring. That means a real regression where Codex stops emitting the required structure would pass the Layer 3 e2e silently.

Context: surfaced during the e2e-skill-invocation-tests cycle (commit e296d4a) and re-noted in the config-profile-mechanism cycle (v0.5.0, commit 196be77). The verify_substring_warn helper was introduced specifically as a stopgap for this — see its comment in tests/e2e.sh.

Type

refactor / enhancement (prompt-engineering hardening; no behavior change to the skill's contract, only to how reliably Codex satisfies it)

Expected

Codex reliably emits the required H2 headings for both producers, so the e2e checks can be promoted from verify_substring_warn back to hard verify_substring across all skill × scenario combinations.

Likely approach (to be confirmed at diagnose):

• Strengthen review's Step 3 wording to match adversarial-review's forceful pattern ("Your output MUST begin with exactly these two headings, verbatim: ## Summary then ## Findings").
• Add an explicit format scaffold / one-shot example block in the system instructions.
• Re-run the full e2e matrix (12 combos) to confirm the headings appear reliably before promoting the assertions.

Actual

• review Step 3 uses softer "Begin with a one-paragraph Summary … Follow with a Findings list" phrasing — Codex interprets loosely.
• e2e heading checks are verify_substring_warn (non-failing) for mixed / binary / oversize / empty-repo / with-profile scenarios; only all-empty (target_invalid pre-flight, written by Claude not Codex) and frontmatter markers are hard-verified.

Impact

• Test integrity: Layer 3 e2e cannot currently catch a regression in the producers' output structure — the headings are advisory. This weakens the e2e gate that exists to catch SKILL→runtime drift.
• Scope: plugins/codex-pro/skills/review/SKILL.md (Step 3), plugins/codex-pro/skills/adversarial-review/SKILL.md (Step 3, lighter touch), tests/e2e.sh (promote verify_substring_warn → verify_substring once reliable).
• Cost: verification requires re-running the e2e matrix (real codex-call quota; ~12 combos). Bundle with the next producer change to amortize, or run standalone as a release-gate validation.
• Priority: P2 — quality/test-integrity improvement, not a user-facing bug. The skill contract is unchanged; this hardens how reliably Codex honors it.

Non-Goals

• Not changing the result-file frontmatter contract or the 4-section perspectival structure of adversarial-review.
• Not changing codex-call flags or the profile mechanism (v0.5).

---

Current Status

Phase: diagnosed
Complexity: Spectra (route: spectra-discuss)
Updated: 2026-06-08

Diagnosis posted (root cause: review Step 3 instructions request "a Summary" / "a Findings list" as prose, never the literal ## Summary / ## Findings H2 tokens the contract + e2e expect). Next: /spectra-discuss to align open questions before proposal.

[kiki830621]

Diagnosis

Type

refactor / enhancement (prompt-engineering hardening + test-assertion promotion)

Root Cause / Analysis

The root cause is more specific than "LLM variability" — it's a literal wording mismatch in review's Step 3 instructions, found by reading the actual source:

plugins/codex-pro/skills/review/SKILL.md Step 3 (the system instructions sent to codex-call --instructions) asks for the output structure as prose nouns, not literal heading tokens:

- Begin with a one-paragraph Summary of overall assessment.
- Follow with a Findings list. Each finding MUST use the heading format
  "Finding N: <severity> — <file>:<line>" ...

It says "a Summary" and "a Findings list" — it never instructs Codex to emit the literal ## Summary and ## Findings H2 markdown headings. But the result-file contract (## Result file structure, lines ~234/238) and tests/e2e.sh both expect those literal tokens. So Codex is free to emit Summary:, ### Summary, a bare paragraph, or omit the heading entirely — all of which satisfy the prose instruction but fail the literal-token check.

By contrast, adversarial-review/SKILL.md Step 3 is binding:

Produce output in exactly four H2 sections, in this order:
## Assumptions Challenged
## Failure Modes
## Alternative Approaches
## Trade-off Counterarguments
CRITICAL: Each of the four sections MUST have at least one substantive paragraph.

It names the literal ## tokens + "exactly four H2 sections, in this order" + a CRITICAL non-empty clause. This is why adversarial-review holds up better — it's a prompt-wording difference, not luck.

tests/e2e.sh (lines 193–198) currently checks both producers' headings with verify_substring_warn (best-effort warning, non-failing):

verify_substring_warn '## Summary'  ...
verify_substring_warn '## Findings' ...
for h in '## Assumptions Challenged' '## Failure Modes' '## Alternative Approaches' '## Trade-off Counterarguments'; do
  verify_substring_warn "$h" ...

Note the warn was applied uniformly as a blanket stopgap — adversarial-review's headings are warned even though its wording is already strong. So the two producers need different fixes: review needs a real wording fix; adversarial-review likely needs only the assertion promotion (its wording may already be reliable).

Impact

• Affected code:
  • plugins/codex-pro/skills/review/SKILL.md — Step 3 rewrite (name literal ## Summary / ## Findings H2 tokens + "exactly, in this order" scaffold, mirroring adversarial-review)
  • plugins/codex-pro/skills/adversarial-review/SKILL.md — likely no/minimal wording change; re-verify only
  • tests/e2e.sh — promote the heading checks verify_substring_warn → verify_substring once reliable
• Affected specs (consistency-checked together): openspec/specs/review/spec.md (Step 3 output contract), openspec/specs/adversarial-review/spec.md, openspec/specs/tests/spec.md (e2e assertion strength is documented normative behavior)
• Affected user flow: none — the skill's output contract is unchanged (## Summary + ## Findings was always the contract); this only makes Codex honor it reliably.

Strategy

• Rewrite review Step 3 to explicitly require literal ## Summary then ## Findings H2 headings, "in this order", mirroring adversarial-review's "exactly N H2 sections" pattern; optionally add a short format scaffold / one-shot skeleton.
• Re-run the Layer 3 e2e matrix (real codex-call, 12 combos) and confirm the headings appear reliably across mixed / binary / oversize / empty-repo / with-profile for review, and the 4 sections for adversarial-review.
• Only after empirical reliability is observed, promote the heading checks in tests/e2e.sh from verify_substring_warn → verify_substring.
• MANDATORY producer smoke (per smoke-before-archive discipline) before archiving, since review/SKILL.md is a producer.

Conflict Class

A_parallel_safe — independent file edits (review SKILL, adversarial-review SKILL, e2e.sh, three spec files); no shared mutable runtime resource (no DB / upload endpoint / submodule). Single-issue, so this is informational.

Complexity

Spectra

• Layer 2 (necessary): both producers are published skills with documented specs in openspec/specs/; the Step 3 output contract is normative.
• Layer 3 (confirmation, ≥1 — two hit): (a) modifies existing published spec normative behavior — the tests spec's e2e assertion strength (warn → hard) and review spec's Step 3 contract; (b) affects 2+ specs needing consistency (review + tests, likely adversarial-review = 3).
• Consistent with codex-pro's established discipline: every producer-skill behavior change v0.2→v0.5 shipped through Spectra.

Risks

• Promotion-induced flakiness: flipping warn → verify_substring trades a silent miss for a possibly flaky hard failure if Codex stays non-deterministic. The wording hardening MUST be strong enough that headings are near-deterministic before promoting — otherwise the e2e gate becomes flaky on real Codex calls. Sequencing (harden → observe reliability → only then promote) is the mitigation; do not promote and harden in the same blind step.
• e2e quota cost: validation needs real codex-call across combos (~12 calls); bundle with the smoke to amortize.
• Over-hardening could make review's prompt brittle to legitimate Codex phrasing variety; keep the scaffold about headings, not content shape.

Residue

The issue's deeper intent — guarantee Codex emits the required structure — is inherently probabilistic and cannot be fully operationalized at codex-pro's layer: Codex is an LLM, not a schema-validated endpoint. We can lower the failure rate via wording + verify empirically over a finite e2e sample, but "100% deterministic headings" is unreachable without upstream structured-output / JSON-schema mode in codex-call (a parallel-ai-agents cross-repo concern, out of scope here). The honest scope is "reduce failure rate enough that a hard e2e assertion is stable", not "eliminate the failure mode." (Related, separately tracked: e2e fixture realism — not filed here; it's an independent quality candidate.)

Vagueness Pre-check

• V1 (vague WHAT): 2 — issue names exact files (review/adversarial-review SKILL.md Step 3, tests/e2e.sh), the precise mechanism (literal-token wording), and the warn→hard promotion. WHAT is concrete.
• V4 (vague ACCEPTANCE): 2 — success is operationalized: e2e heading checks promoted from verify_substring_warn to verify_substring and passing across all 12 combos. Verifiable.
• Triggered: no — both axes ≤ 3.