codex-review PR gate can be satisfied by a stale, identity-less marker #37

Gated on #34. Re-check once the upstream sync merges. Verified against the #34 tree
(origin/chore/upstream-sync-2026-08-14): the hook pair below survives the sync unchanged, so
the merge does not resolve this.

Related: #35 — this belongs to the same category as that issue's "Out of scope — upstream's own
bugs, inherited verbatim" section, and the same reasoning applies to where it should be fixed.

The mechanism

Two hooks in nix/modules/home/programs/claude-code/default.nix:

PostToolUse, matcher Skill (L65–81) — when the invoked skill is codex-review, create a
marker:

if [ "$skill" = "codex-review" ]; then touch /tmp/.claude-codex-review-done; fi

PreToolUse, matcher Bash (L44–64) — when the command contains gh pr create, require the
marker, and consume it:

if [ -f /tmp/.claude-codex-review-done ]; then
  rm -f /tmp/.claude-codex-review-done; exit 0;
else
  echo 'BLOCKED: You must run the codex-review skill first before creating a PR. …' >&2;
  exit 2;
fi

What happened

Observed in asktt1770/nix-hermes-agent, session of 2026-09-08. PR #2 in that repository was
created with gh pr create. The hook did not block it. No codex-review invocation preceded it
in that session segment.

Verified after the fact:

$ ls -la /tmp/.claude-codex-review-done
ls: /tmp/.claude-codex-review-done: No such file or directory

Inference, stated as such: the marker was present before the call and was consumed by it.
Which earlier event created it could not be reconstructed — the file carries no content,
timestamp trail, or provenance. The practical outcome is that a PR was opened without a code
review while the gate reported success.

The structural property

/tmp/.claude-codex-review-done is a single global path holding no identity. It does not record
which commit or diff was reviewed, which repository, which session, or when. Consequences that
follow regardless of what happened above:

1. A review of branch A can authorise a PR for branch B.
2. A review in repository X can authorise a PR in repository Y.
3. A marker left from a previous session authorises the first PR of the next one, days later.
4. A review followed by further commits still authorises the PR for the amended branch.
5. The marker is an ordinary file at a fixed, world-writable path, so any Bash invocation can
   create it. (Property, not an observed event — no such call was made.)

A second, independent weakness: PostToolUse fires when the skill is invoked, not when the
review passes. The marker therefore attests to "codex-review started", not "the changes were
reviewed and found acceptable".

Upstream status

The hook pair is verbatim upstream — it does not appear in our diff against upstream/main,
and it is unchanged in the #34 tree. This is an upstream bug that we inherited, so it is present
in ryoppippi/dotfiles too.

That makes the fix location a real decision rather than a formality. Per the reasoning already
recorded in #35, fixing an inherited upstream bug locally widens our diff in a file upstream
keeps editing and makes every future sync heavier. Reporting it upstream keeps the diff at zero
if accepted. Holding a local fix as a fallback if it is not accepted is the other half of that
trade-off.

Sketch of a remedy, for whoever picks this up

The hook stdin JSON carries more than the current hooks read — session_id and cwd are both
available alongside tool_input. Binding the marker to an identity computed from

session_id + repository (realpath of --git-common-dir) + branch + HEAD sha

closes items 1–4 above:

Item 5 cannot be closed in principle: as long as the assistant can run Bash, any state a hook
reads is state the assistant can write. The honest framing is that this defends against
accidental staleness, which is what actually occurred, not against deliberate forgery. Moving
the path off /tmp to $XDG_CACHE_HOME is hygiene — it avoids a predictable world-writable
location — but does not change that.

Two smaller points that fall out of the same design:

• Consume-on-use becomes unnecessary. Binding to the HEAD sha makes the marker self-expiring,
  so the rm can go. That also removes today's behaviour where a gh pr create that fails for an
  unrelated reason (network, auth) burns the marker and forces a re-review.
• The gh pr create substring match has gaps. PRs opened through the create-pr skill or any
  other spelling bypass the gate entirely. Whether the intent is to gate "the gh pr create
  command" or "publishing work without review" changes what the matcher should be.

A related question is whether the marker should be written by the PostToolUse hook (reliably
detects invocation, cannot tell whether the review passed) or as the final step of the
codex-review skill itself (attests to completion, but skill bodies are model instructions and
can be skipped). These pull in opposite directions and the choice should be explicit.

Same failure family as a bug this fork already fixed

Worth recording because it makes the shape recognisable. nix-hermes-agent spent the same
session fixing a CI bug where a reusable workflow rebuilt the previous commit, found it already
cached, and reported green — a stale success standing in for a verification that never happened,
with nothing failing. The marker file has the same structure: a token that outlives the thing it
attests to.

Reproducing

# the hook pair
$ sed -n '43,82p' nix/modules/home/programs/claude-code/default.nix

# the gate's entire state
$ ls -la /tmp/.claude-codex-review-done

# confirm it is inherited, not ours
$ git diff upstream/main -- nix/modules/home/programs/claude-code/default.nix