From 7f61bcf85e5bd8e8b8983ff1739c67e35ebc2d79 Mon Sep 17 00:00:00 2001 From: Mart-Matteus Kampus Date: Fri, 21 Aug 2026 21:22:36 +0300 Subject: [PATCH] fix: key Codex state on the target basename, not its whole path target_key folds every directory component of the resolved target into the state filename. That makes state paths grow with the depth of the checkout, and on Windows they cross the 260-char MAX_PATH limit. The failure is silent and hard to attribute. Past the limit, bash still writes the events file (MSYS handles long paths transparently), but jq is a native Windows binary with no long-path support and fails with Could not open file : No such file or directory for a file that is present on disk. start.sh redirects jq's stderr to /dev/null, so THREAD_ID comes back empty and .thread is never written. Every later resume.sh then exits with "no review session for - run start.sh first", leaving the previous .review.txt in place and the working tree unchanged, so it reads as "Codex did nothing" rather than as a path-length problem. It bites hardest when the target lives inside a git worktree, because the worktree path is encoded into the filename on top of the real directory path, roughly doubling it. Measured on a Windows checkout with a 60-char plan filename: 320 chars inside a worktree (broken), and 246 in a normal checkout - only 7 chars under the limit once the .events.ndjson.stderr sibling is counted, so a slightly longer plan name breaks the normal case too. Fix: build the key from the target's basename plus the checksum. The checksum is still computed over the full resolved path, so uniqueness is unchanged - two same-named plans in different checkouts still get different keys. Only the human-readable half shrinks. The basename is capped at 80 chars so one very long target filename cannot reintroduce the problem, and degenerate targets ("/", trailing slashes, ".") fall back instead of producing an empty key. Same measurements after the change: 211 chars inside a worktree, 176 in a normal checkout. Note for release notes: this changes every existing state key, so any in-flight Codex thread is orphaned by the upgrade. Clearing the state directories as part of the upgrade avoids leaving unreachable files behind. Verified by reproducing the failure first (jq refusing a 320-char path that existed on disk), then re-running the same probe after the change; plus uniqueness, determinism, relative-vs-absolute, non-path targets, degenerate targets, and the basename cap. Co-Authored-By: Claude Opus 5 (1M context) --- skills/codex-plan-review/scripts/_common.sh | 34 +++++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/skills/codex-plan-review/scripts/_common.sh b/skills/codex-plan-review/scripts/_common.sh index fa0ba22..434d3a3 100755 --- a/skills/codex-plan-review/scripts/_common.sh +++ b/skills/codex-plan-review/scripts/_common.sh @@ -90,12 +90,28 @@ require_tools() { # Derive a per-target key from a path-like string. For real paths we # resolve to absolute; for non-path targets (branch names, commit -# ranges) we use the string as-is. The key is a sanitized, readable -# form plus a checksum of the resolved target, so distinct targets -# that sanitize identically (e.g. "foo/bar" vs "foo__bar") never -# share state files. +# ranges) we use the string as-is. The key is the target's BASENAME +# plus a checksum of the resolved target, so distinct targets that +# share a basename (e.g. the same plan in two checkouts) never share +# state files. +# +# The basename, not the whole path: folding every directory component +# into the filename makes state paths grow with the depth of the +# checkout, and on Windows that crosses the 260-char MAX_PATH. Past +# that limit bash still writes the file (MSYS handles long paths) but +# `jq` is a native binary without long-path support and fails with +# "Could not open file ... No such file or directory" for a file that +# exists. start.sh redirects jq's stderr to /dev/null, so THREAD_ID +# comes back empty, .thread is never written, and every subsequent +# resume.sh reports "no review session ... run start.sh first" -- +# which reads as "Codex did nothing" rather than as a path-length bug. +# +# The checksum is still taken over the FULL resolved path, so +# uniqueness is unchanged; only the human-readable half is shortened. +# The basename is capped so that one very long target filename cannot +# reintroduce the problem on its own. target_key() { - local target="$1" resolved sanitized sum + local target="$1" resolved base sanitized sum if [ -e "$target" ]; then resolved="$(realpath -- "$target" 2>/dev/null || readlink -f -- "$target")" if [ -z "$resolved" ]; then @@ -105,7 +121,13 @@ target_key() { else resolved="$target" fi - sanitized="$(printf '%s' "$resolved" | sed 's|^/||; s|/|__|g; s|[^A-Za-z0-9._-]|_|g')" + # Strip trailing slashes before taking the basename, so a directory + # target does not reduce to an empty string. + base="${resolved%"${resolved##*[!/]}"}" + base="${base##*/}" + [ -n "$base" ] && [ "$base" != "." ] || base="target" + sanitized="$(printf '%s' "$base" | sed 's|[^A-Za-z0-9._-]|_|g')" + sanitized="${sanitized:0:80}" sum="$(printf '%s' "$resolved" | cksum | cut -d' ' -f1)" printf '%s.%s' "$sanitized" "$sum" }