Skip to content

feat(reviewing-prs): native review recovery — one codex reviewer for both species - #9

Merged
SSFSKIM merged 18 commits into
mainfrom
reviewing-review
Jul 11, 2026
Merged

feat(reviewing-prs): native review recovery — one codex reviewer for both species#9
SSFSKIM merged 18 commits into
mainfrom
reviewing-review

Conversation

@SSFSKIM

@SSFSKIM SSFSKIM commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Restores the review loop's product core: one native codex exec review --base engine call returning a compact correctness + spec-compliance verdict for BOTH review-worker species, owned by the new substrate script review-engine.sh.

  • skills/reviewing-prs/scripts/review-engine.sh: temporary out-of-tree CODEX_HOME, authentication inherited from the caller's effective Codex home, SSL_CERT_FILE, CODEX_CODE_MODE_HOST_PATH, nested-only sandbox_mode=danger-full-access behind a CODEX_SANDBOX check, fixed review policy via developer_instructions, PR/ticket criteria in an explicitly untrusted data file, and rc passthrough
  • engine block rewritten around the script; per-species fallbacks merged into one (codex-only reviewer; engine down → ticket stays in-review + ENGINE-UNAVAILABLE marker)
  • each review invocation uses unique temporary criteria/findings paths, preventing concurrent reviews of the same PR number from colliding
  • sweep retries marker-carrying finished reviewers (retire → re-dispatch); terminal Codex records are retired without signaling stale PIDs
  • _codex_launch exports CODEX_CODE_MODE_HOST_PATH
  • protocol ORIENT reads diff shape only (--stat)
  • engine, dispatch, daemon, and environment-override regression coverage added
  • spec + plan + SKILL.md updated

Spec: docs/doperpowers/specs/2026-07-12-native-review-recovery-design.md
Plan: docs/doperpowers/plans/2026-07-12-native-review-recovery.md

@SSFSKIM SSFSKIM added the confident-ready review loop: reviewed and confident, human merges label Jul 11, 2026
@SSFSKIM

SSFSKIM commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Automated review trail

Engine: native codex exec review via review-engine.sh (gpt-5.6-sol, xhigh), 3 rounds against origin/main...HEAD.

Final verdict: approve. Round 3 reported: “No actionable correctness defects were found in the full change range.”

Findings and routing

Round 1:

  • [P1] Terminal reviewer retirement could signal a reused stale PID — FIX NOW. Codex retirement now signals only active (working/blocked) records; red/green regression coverage added. Commit 6fdf073.
  • [P2] Engine state polluted the reviewed tree — FIX NOW. Temporary CODEX_HOME moved outside the worktree and cleanup is tested on success/failure. Commit 64e63b8.
  • [P2] Custom CODEX_HOME authentication was ignored — FIX NOW. The caller's effective Codex home is captured before replacement and used as the auth source. Commit e8bf287.
  • [P2] Default model/effort test inherited supported overrides — FIX NOW. The default-value case now unsets those overrides. Commit e392231.

Round 2:

  • [P1] PR-number-only temporary paths could cross-contaminate concurrent reviews — FIX NOW. Each invocation now allocates one unique temporary directory for criteria and all findings rounds. Commit 6d6b750.
  • [P1] PR/ticket text was elevated into developer instructions — FIX NOW. Developer instructions now contain fixed review policy only; PR title/body and ticket criteria remain in an explicitly untrusted data file, with adversarial regression coverage. Commit a0c8e82.
  • [P2] Daemon environment test inherited worker host-path/token state — FIX NOW. The default-export test clears the relevant variables locally and passes inside the supported Codex-worker environment. Commit f70f8a9.

Round 3: no findings.

Verification

  • tests/reviewing-prs/test-review-engine.sh — pass
  • tests/reviewing-prs/test-review-dispatch.sh — pass
  • tests/orchestrating-daemons/test-codex-scripts.sh — pass
  • scripts/lint-shell.sh on all 7 shell files changed by this PR — pass
  • scripts/lint-shell.sh --all — baseline failure only in unrelated tests/claude-code/* files (existing SC2155/SC2064/SC2320/SC2088 warnings)
  • GitHub merge state: CLEAN
  • GitHub checks: no checks reported for reviewing-review

Tier judgment

HUMAN tier; not merged.

  • Final verdict clause: met.
  • Post-fix size clause: not met — 182 additions and 111 deletions across 9 files relative to the dispatched head (limit: about 150 lines and 5 files).
  • Non-default-base clause: not met — PR targets main, the repository default branch.
  • Risk-surface clause: not met — authentication handling changed; auth/security is always-on.
  • CI clause: not met — the repository reports no checks for this branch.
  • Auto-merge is disabled, but observation-mode self-merge eligibility is not reached because the clauses above fail.

PR head reviewed and pushed: f70f8a9dd98d8ef338ceeeb85b612b2ced3ac052. Added confident-ready for human merge. No board writes were made because this PR has no linked ticket.

SSFSKIM added a commit that referenced this pull request Jul 11, 2026
…liation

Merges two concurrent changes to the review criteria: the review worker's
injection hardening (PR #9 round 2: fixed policy in developer_instructions,
ticket text in an explicitly untrusted data file — a0c8e82) and the human's
content minimization (never re-instruct what native review already does).
Result: the fixed policy carries ONLY the spec-compliance addendum framed
as decision discipline, reads the ticket text from the untrusted file, and
a ticketless PR (empty criteria file) sends no developer instructions at
all. Data file carries ticket requirements only (PR-claims section
dropped). Spec Revision Notes 2-4.
SSFSKIM added 18 commits July 12, 2026 05:20
…pecies

Two codex-workers conclusions overturned by live spikes (2026-07-12):
custom criteria ride -c developer_instructions past the --base/[PROMPT]
clap wall, and codex-in-codex review works with a 3-env recipe (inner
danger-full-access confined by the outer profile + CODEX_CODE_MODE_HOST_PATH
+ SSL_CERT_FILE). New spec restores the product core: one native
'codex exec review' engine for both worker species via a substrate
review-engine.sh; engine-down parks nothing — ticket stays in-review and
the sweep retries on an ENGINE-UNAVAILABLE marker. Revision Note 6 +
FU-7 correction cross-refs added to the prior spec/shakedown docs.
…liation

Merges two concurrent changes to the review criteria: the review worker's
injection hardening (PR #9 round 2: fixed policy in developer_instructions,
ticket text in an explicitly untrusted data file — a0c8e82) and the human's
content minimization (never re-instruct what native review already does).
Result: the fixed policy carries ONLY the spec-compliance addendum framed
as decision discipline, reads the ticket text from the untrusted file, and
a ticketless PR (empty criteria file) sends no developer instructions at
all. Data file carries ticket requirements only (PR-claims section
dropped). Spec Revision Notes 2-4.
@SSFSKIM
SSFSKIM force-pushed the reviewing-review branch from a922b46 to 5098d92 Compare July 11, 2026 20:22
@SSFSKIM
SSFSKIM merged commit 252685a into main Jul 11, 2026
SSFSKIM added a commit that referenced this pull request Jul 11, 2026
…liation

Merges two concurrent changes to the review criteria: the review worker's
injection hardening (PR #9 round 2: fixed policy in developer_instructions,
ticket text in an explicitly untrusted data file — a0c8e82) and the human's
content minimization (never re-instruct what native review already does).
Result: the fixed policy carries ONLY the spec-compliance addendum framed
as decision discipline, reads the ticket text from the untrusted file, and
a ticketless PR (empty criteria file) sends no developer instructions at
all. Data file carries ticket requirements only (PR-claims section
dropped). Spec Revision Notes 2-4.
@SSFSKIM
SSFSKIM deleted the reviewing-review branch July 11, 2026 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confident-ready review loop: reviewed and confident, human merges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant