Skip to content

[bench] cli, ledger: put a timeout on every subprocess and detach its stdin - #53

Merged
dburks-svg merged 31 commits into
mainfrom
bench/subprocess-timeouts
Sep 5, 2026
Merged

[bench] cli, ledger: put a timeout on every subprocess and detach its stdin#53
dburks-svg merged 31 commits into
mainfrom
bench/subprocess-timeouts

Conversation

@dburks-svg

@dburks-svg dburks-svg commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Roadmap v2.1 item 2.4.

What changes

Three subprocess calls had no timeout: the git ls-remote and gh api probes in cli/commands.py and the git helper in ledger/migrate.py. ledger/sanitize.py, named on the roadmap card, spawns nothing.

  • Each now runs with a 60 s timeout and stdin=DEVNULL, so a credential or auth prompt fails at once instead of holding the call open until the timeout. A timeout returns the typed failure each helper already returned for other errors (None from the ref probe, 0 from the status probe which classify_removal reads as inconclusive, (1, "") from the migration helper), with a stderr line.
  • tests/test_subprocess_timeouts.py walks cli, hooks, ledger, pipeline, and utils with ast and fails on any subprocess.run, check_output, check_call, or call without a timeout keyword, and on any Popen (its timeout lives on a later communicate the scan cannot pair with it). It resolves import aliases so a call cannot dodge it by spelling.
  • README gains one sentence in the fail-closed section.

Receipts

Proof the gate can fail, both ways: an inline reconstruction of the three pre-change call sites reports three offenders, and run against origin/main's sources it reports the same three lines (cli/commands.py:365, cli/commands.py:396, ledger/migrate.py:48). On this branch it reports none.

Each timeout branch is exercised with a mocked TimeoutExpired asserting the return value and the stderr line, and each call is asserted to carry the timeout and the detached stdin. 790 tests green, ruff and strict mypy clean. Eleven governed edits, all PASS.

Self-audit before opening, per request: it added the detached stdin (a prompt would otherwise still block for the full 60 s), the behavioral tests for each timeout branch, alias resolution in the scan, and a check that the timeout log line cannot carry a secret (the probe's arguments are a public URL and a local path).

Follow-up

Codex's post-merge comment on #50's successor, PR #52, about the deep-copy handler rejecting a valid response with a deeply nested unknown field, is not in this PR. It gets its own fix-forward next.

… stdin

Roadmap v2.1 item 2.4. The provider call has had a timeout since the
claude_code path landed; the git ls-remote and gh api probes in the CLI
and the migration helper's git calls did not, so a hung remote or a
credential prompt could hang the command forever. ledger/sanitize.py,
named on the roadmap card, spawns nothing.

- cli/commands.py: _git_refs and _gh_status run with a 60 s timeout and
  stdin detached, so a prompt fails at once. A timeout is the typed
  failure each already returned for other errors (None, and 0 which
  classify_removal reads as inconclusive), with a stderr line.
- ledger/migrate.py: _run_git the same, returning (1, "").
- tests/test_subprocess_timeouts.py: an ast scan of cli, hooks, ledger,
  pipeline, and utils that fails on any subprocess.run, check_output,
  check_call, or call without a timeout keyword and on any Popen. It
  resolves import aliases (import subprocess as sp, from subprocess
  import run as go) so a call cannot dodge it by spelling. Proof it can
  fail: an inline reconstruction of the three pre-change sites yields
  three offenders, and run against origin/main's sources it reports the
  same three lines.
- tests: each timeout branch is exercised with a mocked TimeoutExpired,
  and each call is checked for the timeout and the detached stdin.
- README: one sentence in the fail-closed section.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d16bda7a41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…o_migrate (Codex P1)

_run_git reported a timeout as (1, ""), the same result the history
probes read as a negative answer, so a slow `git log` made
_last_ref_with_chain return None and migrate_ledger report
nothing_to_migrate over a chain that was in history, after which the
next governed edit would have opened a fresh genesis and forked it. A
timeout during a cat-file or show probe could likewise skip the newest
commit with the chain.

_run_git now raises GitTimeout, a typed error distinct from a non-zero
exit, and migrate_ledger catches it around both the probe and the
restore and returns status failed with failure_type GIT_TIMEOUT and a
detail that says retry, not start over. Tests cover the raise, a timeout
during the probe (failed, nothing written), and a timeout during a show
after the probe answered.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c6b7712f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py
… out (Codex P1)

A timeout on the second or later `git show` left the files already
written in place and reported zero. The next attempt then met the
already_started guard and returned already_migrated over a chain that
might be missing its tips, and every append after that would have
extended a silently truncated history.

_restore_from_git now tracks the paths this attempt created and removes
them before the GitTimeout propagates; a path that already existed is
never removed by the rollback. A test times out the second show, asserts
the target holds no chain files afterwards, and asserts the retry then
completes as a verified migration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56f8920a0b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…mpt leaves nothing in the target (Codex P1)

The rollback added for the previous finding removed this attempt's files
after a timeout, but if that removal itself failed the surviving files
still met the already_started guard on the next run. Codex's suggested
shape is the right one: fetch every blob into <target>.restoring beside
the target, and move files into place only once every fetch has
finished. Nothing reaches the target while git can still time out, so
the worst a failed cleanup can do is leave debris in staging, which the
next attempt clears before it starts (and fails if it cannot, rather
than fetching into a directory holding another attempt's files).

_restore_from_git is split into _wanted_paths, _clear_staging,
_fetch_into, and _move_into_place, each with one job, which also settles
Sonar's cognitive-complexity finding on it. Moves are renames, so a file
is either wholly in place or absent; a move that fails is logged, stops
the loop, and is reported as partial with staging left for inspection.

Tests: a timeout on the second show leaves the target empty and no
staging behind, and the retry is a verified migration; a timeout whose
cleanup fails (rmtree refused) still leaves the target empty, and the
retry clears the stale staging and completes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a1ba01e9a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
Comment thread ledger/migrate.py
Comment thread ledger/migrate.py Outdated
Comment thread ledger/migrate.py Outdated
…l back or flag a failed publish (Codex)

Four findings on the previous staging design, one change for all of
them. The staging directory now lives inside the target (the one place
the configured ledger path proves writable, and the gitignored one, so
debris can never be committed), is created by mkdtemp with an ownership
marker file, and only a directory carrying that marker is ever removed;
a same-prefix directory without it is left alone with a note.

Publishing writes an incomplete marker into the target before the first
rename and removes it after the last. If a rename fails, every file
already moved is moved back; if a move-back fails, or fewer files were
staged than the commit held, the marker stays. migrate_ledger checks the
marker before the already_started guard and returns failed with
failure_type INCOMPLETE_RESTORE and a recovery note, instead of
already_migrated over a partial chain.

Sonar S1192: the "*.json" glob is one constant.

Tests: timeout with working cleanup (target empty, no staging, retry
migrates), timeout with refused cleanup (target empty, owned staging
left inside it, retry clears it and migrates), a foreign .restoring-*
directory left untouched, a rename that fails after others succeeded
(rolled back, retry migrates), and the marker blocking already_migrated.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1f6a3ab75

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…ger path points (Codex P1)

Staging inside the target is ignored only when the target is .bench/.
BENCH_LEDGER_PATH may name a directory nothing ignores, and a restored
entry carries the full diff body of every change it recorded, so debris
from a failed attempt there could have been committed by a later
`git add -A`. _new_staging now writes a .gitignore of "*" into the
staging directory before any blob, which git honours wherever the
directory sits. A test points BENCH_LEDGER_PATH at an unignored
directory inside the repository, forces a timeout with a refused
cleanup so staging with blobs is left behind, and asserts that
`git status --porcelain --untracked-files=all` shows nothing for it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c4bdaaccf2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…n, not a success (Codex P2)

When every rename succeeded but the incomplete marker could not be
removed, the run reported migrated and the CLI exited 0, while every
later run would refuse with INCOMPLETE_RESTORE. _remove_marker now
reports whether the marker is gone, _publish raises RestoreIncomplete
when a complete publish cannot clear it or a failed publish cannot be
fully moved back, and migrate_ledger turns that into the same
INCOMPLETE_RESTORE failure result the marker check returns on a later
run (now one helper, so the two read the same). A test refuses the
marker's unlink after a full publish and asserts the failure, the
surviving marker, and the recovery note.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 86f4547efb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…osures into helpers (Sonar duplication gate)

Sonar's quality gate failed on 3.8% duplicated new lines against a 3%
limit. Three near-identical failure dicts in ledger/migrate.py become
one _failed helper that _incomplete, _timed_out, and the enumeration
failure call; the identical timeout and refuse closures repeated across
the migrate tests become module-level _hang_on_show(nth) and _refuse,
and one test that re-inlined the commit-then-untrack fixture uses the
helper. No behaviour changes; 799 tests green.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1610794eb0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…ve-create lock (Codex P2)

Two overlapping runs could each take the other's live staging directory
for a stale one and delete it. migrate_ledger now takes
<target>/.migrate.lock with an exclusive create before it looks at
anything and releases it in a finally; a second run while it is held is
refused with MIGRATION_IN_PROGRESS and a note naming the lock, and
touches nothing. Under the lock, a staging directory found in the target
can only belong to an attempt that died.

cli migrate-ledger now prints a failed result's detail line, which is
where a timeout, a held lock, or an incomplete restore says what to do.

Tests: a held lock refuses the second run and leaves the other attempt's
staging alone, then a clean run after the lock is removed migrates and
leaves no lock; the lock is released after a failed run; the CLI prints
the detail line and omits it when empty.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 857cc5c971

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
Comment thread ledger/migrate.py Outdated
…at cannot be released is a failure (Codex)

Two gaps in the lock. If os.open succeeded but writing the pid failed,
the exception escaped and the lock stayed, so every retry reported
MIGRATION_IN_PROGRESS; _acquire_lock now closes the descriptor, removes
the file, and raises LockError, which migrate_ledger reports as
LOCK_FAILED with a note. And if the lock could not be removed after a
successful run, the run still reported migrated while every later run
would refuse; _release_lock now reports whether the lock is gone, and a
stuck lock turns a migrated, already_migrated, or nothing_to_migrate
result into LOCK_NOT_RELEASED with a note saying the chain is as that
status says and the lock must be removed by hand. Tests cover a refused
pid write (lock gone, retry migrates) and a refused unlink (failure
reported, lock and chain both present).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a0e2ca5458

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 406b0423d8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
…roof

The whitespace-tolerant comparison accepted " *", which git reads as a
pattern with a leading space that ignores nothing. The runtime
directory's ignore file must now be exactly "*" with an LF, CRLF or no
line ending; anything else is refused as not Bench's own.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efd192acd6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py
_runtime_dir and the exclusive lock create shared one handler, so a
plain file or dangling link at <target>/.migrate raised FileExistsError
from mkdir and was reported as MIGRATION_IN_PROGRESS, pointing at a
lock file that did not exist, on every retry. The runtime directory is
now prepared in its own block and any failure there is LOCK_FAILED
naming the path; FileExistsError means a held lock only on the create.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 484d069290

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/commands.py
subprocess.run(timeout=...) kills only the process it started, so a git
probe hung inside ssh or a credential helper left the helper running
after the timeout, one leaked process per timed-out probe. utils/procs.py
adds run_isolated: the child starts in a process group of its own (a
session on POSIX, a new process group on Windows), stdin detached, output
captured, and on timeout the whole group is ended before TimeoutExpired
is re-raised, so callers keep their handling. The git and gh probes in
cli/commands.py and _run_git in ledger/migrate.py use it.

The timeout gate allows exactly one Popen, in utils/procs.py, and a new
test checks that module pairs it with a communicate(timeout=...).
tests/test_procs.py starts a child that spawns a grandchild and hangs,
and proves the grandchild is gone after the timeout on both platforms.

The judge subprocess in utils/api.py is unchanged in this PR: it
predates it, is on the hook path, and is a follow-up of its own.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b17e81667

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils/procs.py Outdated
Comment thread utils/procs.py Outdated
On Windows the tree kill's result was discarded, so run_isolated could
return from a timeout with a descendant still running and no note of
it. A nonzero taskkill status is now logged with its output.

Where PID 1 does not reap adopted children (some minimal containers) a
grandchild killed with its group stays listed as a zombie; the helper's
docstring says why Bench does not make itself a subreaper for that
case, and the test reads a zombie as ended, since it runs nothing.

The gh probe now refuses an endpoint that is not a plain API path
(letters, digits, dots, underscores, dashes and slashes, never starting
with a dash, which gh would read as a flag) and reports it as
inconclusive without asking. The endpoint comes from the purge manifest
on disk, which SonarCloud flagged as untrusted input reaching the
process call. "working tree" is a constant in migrate.py.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc3cd862c9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils/procs.py Outdated
…g's arguments

run_isolated ended the process group only on TimeoutExpired, so a Ctrl-C
during a probe left git and whatever it had launched running. Any
exception raised while waiting now ends the group before it propagates
unchanged. tests/test_procs.py interrupts the first wait and proves the
grandchild is gone.

verify-sanitation-binding passes --repository and --mirror to git.
An owner/name repository and an existing mirror directory are now
required before any process is started with them; SonarCloud traced the
command's arguments into the process call.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93a3ff159e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ledger/migrate.py Outdated
Comment thread tests/test_procs.py Outdated
…nterrupt test

Git does not read a symlinked .gitignore, so a .migrate/.gitignore that
links to a file saying "*" ignores nothing; only a regular file with an
effective "*" line now proves ownership. The refusal test covers a
symlink to such a file, skipping where symlinks need a privilege.

The interrupt test raised before the child had started, so on Linux the
group was ended before the grandchild's pid was written and the test
failed on the read. The simulated interrupt now waits for the pid file.

verify-sanitation-binding's argument checks move into a helper so the
command stays under Sonar's complexity ceiling; the checks are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 048cb74efc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils/procs.py Outdated
Comment thread ledger/migrate.py
…runtime directory

The child leads its own session, so a SIGTERM to Bench's process group
(a CI runner cancelling the job) never reached it, and SIGTERM ends
Python without passing through the except clause that ends the group.
While a probe runs on POSIX, a SIGTERM handler ends the group first and
then hands over to the previous handler, or exits with 143 where the
default disposition applied; the handler is restored afterwards.

A `.migrate` symbolic link would have been followed by mkdir, putting
the lock and staged entries outside the configured target and letting
the stale-staging sweep remove directories there. It is refused.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f8e2e3bfc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils/procs.py Outdated
Comment thread ledger/migrate.py Outdated
…ction runtime directory

A process that had set SIGTERM to SIG_IGN was exited with 143 by the
probe's handler, since SIG_IGN is not callable either. The group is
ended and the handler returns, leaving the disposition as it was.

Path.is_symlink does not report a Windows directory junction, which
mkdir follows just the same, so a junction at <target>/.migrate passed
the symlink guard. _is_link reads the reparse-point attribute on
Windows as well; a Windows-only test creates a junction with mklink /J.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@dburks-svg

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37f9ae11e1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread utils/procs.py Outdated
dburks-svg and others added 2 commits September 5, 2026 11:35
utils/procs.py, its tests, and the call-site migrations in cli/commands.py
and ledger/migrate.py are withdrawn to a follow-up of their own, along
with the argument validation added while chasing the SonarCloud finding
it raised. The three probes keep their timeouts and detached stdin as
they stood at 484d069; the migrate.py hardening (self-ignoring runtime
directory, ownership proof, symlink and junction refusal, lock before
any state read, named restore source) stays.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVBooRMkd73P8J3PPLt1Uu
@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2026

Copy link
Copy Markdown

@dburks-svg
dburks-svg merged commit 5bfb1f1 into main Sep 5, 2026
9 checks passed
@dburks-svg
dburks-svg deleted the bench/subprocess-timeouts branch September 5, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant