Skip to content

claude-current and claude-restart-check: every launch starts the newest published Claude Code by absolute path, and a running session is told when its binary moved (opensoft/workBenches#119) - #134

Draft
brettheap wants to merge 12 commits into
mainfrom
feat/claude-current
Draft

brettheap wants to merge 12 commits into
mainfrom
feat/claude-current

Conversation

@brettheap

@brettheap brettheap commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openxfactory-5 (openXfactory-5)

Refs opensoft/workBenches#119

What this is

The openRepoTools half of opensoft/workBenches#119, on Brett Heap's rulings of 2026-09-29:

The workBenches side is governed by the OpenSpec change opensoft/workBenches#120 (launch-current-claude, ratified and merged at c2403eb9). Its code, Speckit feature 016-launch-current-claude, is a separate workBenches DRAFT that lands after this one.

Landing word: Brett Heap, "land the openRepoTools PR when green". The coordinator lands it. This DRAFT is not ready yet: the Status list below says what is still to come.

claude-current

This command picks the Claude Code a launch should start: the version npm publishes, verified with --version, and named by an absolute path. claude is never resolved through PATH.

  • Candidates, in order: the newest native version under ~/.local/share/claude/versions, the user npm prefix's bin/claude, ~/.local/bin/claude, then the image's /usr/local/bin/claude and /usr/bin/claude. Each is read once per physical file.
  • The published version: npm view @anthropic-ai/claude-code version --prefer-online, bounded at 10 s by default.
  • Update when behind, under a lock: claude update for a native install. Otherwise, or when that stops short, npm install -g --prefix <user prefix> …@<published> followed by the package's own install.cjs, which npm 12 skips. A second launch waits on the lock and reads the candidates again before it installs anything itself.
  • Choice: an equal candidate is verified. A newer one launches as ahead. Offline, the newest installed launches unverified. Still behind after the update, the launch is refused with exit 2, printing both versions and the fix, unless CLAUDE_ALLOW_STALE=1.
  • Interface: stdout carries the path. --porcelain gives path=, version=, published= and status=. --offline reads no npm and installs nothing. Exit codes: 0 resolved, 1 nothing runnable, 2 stale, 64 usage.

claude-restart-check

This command prints one green RESTART NEEDED: running <old>, installed <new>; /ctx at your next breakpoint line when the claude process above it runs a binary since replaced on disk (its /proc/<pid>/exe ends in (deleted)), or one older than the installed version. The installed version is read from the native versions directory or the npm package's package.json.

  • The walk goes at most three processes up. The status line runs under /bin/sh -c, and claude is that shell's parent (measured on py-bench).
  • It reads /proc and two small files, runs no binary, and reaches no network.
  • It always exits 0, and 64 only for an unknown argument. It never acts on the session.

Status

🤖 Generated with Claude Code

Summary by Sourcery

Ensure every Claude Code launch uses the newest published installation and notify existing sessions when their running binary has been replaced or superseded.

New Features:

  • Add claude-current to select and update the newest published Claude Code installation for launches.
  • Add claude-restart-check to notify running sessions when a newer or replaced Claude Code binary is available.
  • Integrate Claude resolution into lane-start, including version handoff and launch logging.

Bug Fixes:

  • Prevent launches from selecting an unintended Claude Code installation through PATH or continuing silently with a stale binary.

Enhancements:

  • Extend openRepoTools --install to install and manage the two Claude commands, increasing the installed artifact set from thirteen to fifteen.
  • Document Claude Code resolution, updates, restart notifications, launcher behavior, and portability.

CI:

  • Validate both new Bash commands in the macOS syntax-check workflow.

Documentation:

  • Add a dedicated Claude Code command and integration guide and link it from the main README.

Tests:

  • Add hermetic coverage for Claude version resolution, update locking, offline and stale behavior, restart detection, and lane-start integration.
  • Extend installer tests to cover the expanded command set and artifact counts.

…shed Claude Code, and tell a running session when its binary moved

For opensoft/workBenches#119, on Brett Heap's rulings of 2026-09-29:
the home decision ("this work is really for openRepoTools repo") puts
the resolver here, installed by `openRepoTools --install`, and point 2
("for 2, we can run update on every start, this ensures we have the
latest models") makes every launch run the check.

claude-current resolves the executable by ABSOLUTE PATH, never through
PATH: the newest native version, the user npm prefix, ~/.local/bin/claude,
then the image's copies, each read with --version and read once per
physical file. It asks npm for the published version with a bound, updates
the user-writable install under a lock when every candidate is behind
(claude update for a native install, else npm install -g --prefix <user>
plus the package's install.cjs that npm 12 skips), and hands back the path
whose version equals npm. An equal candidate beats a newer one, and a newer
one launches as ahead. Offline, it launches unverified. Still behind after
the update, it refuses (exit 2) unless CLAUDE_ALLOW_STALE=1.
--porcelain gives path/version/published/status, and --offline reads no npm.

claude-restart-check prints one green RESTART NEEDED line when the claude
process above it (at most three processes up) runs a binary since
replaced on disk (exe ends " (deleted)") or older than the installed one,
read from the native versions directory or the package's package.json. It
reads /proc and two small files, runs no binary, reaches no network, and
always exits 0.

Tests: tests/test_claude_current.py and tests/test_claude_restart_check.py,
both hermetic, and both commands held to test_repo_hygiene.py's bash rules.
The macOS job parses both with /bin/bash -n.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 16:40
@sourcery-ai

sourcery-ai Bot commented Sep 29, 2026

Copy link
Copy Markdown

Reviewer's Guide

This PR adds two hermetic Bash tools: claude-current resolves an absolute path to the newest published Claude Code and safely updates or refuses stale installs, while claude-restart-check detects replaced or outdated binaries in running sessions and emits a restart prompt; comprehensive fake-environment tests and macOS Bash parsing checks cover their behavior.

File-Level Changes

Change Details Files
Add a hermetic resolver that selects, verifies, updates, and reports the newest published Claude Code executable.
  • Search native, user-prefix, local, and system candidates in deterministic priority order using absolute paths.
  • Query npm with a timeout; support offline/unverified, ahead, stale refusal, and explicit stale override outcomes.
  • Update native installs or fall back to a user-prefix npm installation, including the package install hook, while serializing launches with a lock.
  • Expose plain-path and porcelain output with documented exit-code behavior.
claude-current
tests/test_claude_current.py
Add a non-invasive restart detector for sessions running a replaced or older Claude Code binary.
  • Walk up to three parent processes to locate Claude above the status-line shell.
  • Detect deleted executables and compare running and installed versions from native or npm metadata.
  • Emit an optional green restart notice while remaining silent for current sessions and always exiting successfully for runtime conditions.
claude-restart-check
tests/test_claude_restart_check.py
Integrate the new shell commands into platform syntax validation and enforce shared repository shell conventions.
  • Run bash syntax checks for both commands in the macOS workflow.
  • Test executable/failure-discipline, portability hygiene, and identical version-comparison logic across the commands.
.github/workflows/tests.yml
tests/test_claude_current.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@brettheap

Copy link
Copy Markdown
Contributor Author

Lane: openxfactory-5 (openXfactory-5)

@codex review

🤖 Generated with Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The macOS timeout fallback can exceed its bound, and stale-lock takeover can permit concurrent updates.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Adds Claude Code version resolution and restart detection utilities.

Changes:

  • Adds bounded version resolution, updating, locking, and offline behavior.
  • Adds stale-session restart notifications.
  • Adds extensive tests and macOS Bash parsing.
File Description
claude-current Resolves and updates Claude Code.
claude-restart-check Detects stale running sessions.
tests/​test_claude_current.py Tests resolver behavior.
tests/​test_claude_restart_check.py Tests restart detection.
.github/​workflows/​tests.yml Parses new scripts on macOS.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread claude-current Outdated
Comment thread claude-current Outdated
brettheap and others added 3 commits September 29, 2026 17:43
…s a dead owner's lock

Two Copilot findings on #134, both in the fallbacks a
stock macOS takes, and a third wording fault found beside them:

- With no `timeout` and no `gtimeout`, the watchdog killed only the command.
  A child the command had started (npm's lifecycle processes, a script's
  `sleep`) kept the caller's command substitution open, so a 1 s bound took
  as long as the child did. `kill_tree` now stops, walks (`pgrep -P`),
  signals and continues the whole tree, as GNU `timeout` ends its process
  group, and the watchdog's own `sleep` is ended the same way.
- With no `flock`, two launches that both read one dead owner each removed
  "the stale lock", and the second removal could take the lock the first had
  just made, so both updated at once. Only the launch holding the reaper
  directory (`<lock>.d.reap`) removes it now, and only while the lock still
  names the pid it saw dead. A reaper left by a launch killed in those lines
  blocks the takeover, and the refusal names it.
- A lock directory that could not be created was reported as a lock that
  "was not free within 330s". It is named as what it is.

tests/test_claude_current.py runs the two fallbacks on every host by hiding
`timeout`, `gtimeout` or `flock` from PATH. Against the previous head the
watchdog case takes the fake npm's full 30 s and the reaper case removes the
lock and updates; both pass here. 52 cases.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd a stale one is refused before any write

opensoft/workBenches#119, Brett Heap 2026-09-29, verbatim "for 2, we can run
update on every start, this ensures we have the latest models". The hook is
minimal and touches three places only:

- Beside the CLAUDE_BIN default: CLAUDE_VERIFIED_VERSION is read once and
  removed, so no session inherits it. CLAUDE_BIN unset, or equal to
  CLAUDE_RESOLVED_BIN with no CLAUDE_VERIFIED_VERSION (an earlier launch's
  answer inherited through a session's environment), is resolved. Any other
  CLAUDE_BIN is a pin, launched as it is.
- At the foot of 5a, once the agent and the launch are known and before the
  row, the object log and the handoff stamp are written: `claude-current
  --porcelain`, found beside this command or in $OPENREPOTOOLS_BIN_DIR and
  never on PATH. Exit 0 replaces the command's first word with the absolute
  path and exports CLAUDE_BIN and CLAUDE_RESOLVED_BIN; exit 2 ends the run
  with 2 and anything else with 1, nothing written. `--dry-run` plans the
  call and `--no-launch` makes none. Only the `claude` agent asks. With no
  claude-current installed, one note, and CLAUDE_BIN as before.
- The lane's STARTED/RESUMED payload gains `; claude <version>` when a
  version is in hand, and only when it is x.y.z.

Step 4 is untouched. The bare path a `--confirm` answered No takes still
launches CLAUDE_BIN unresolved.

tests/test_lane_start_claude_current.py: 17 cases in a cut-down copy of the
lane suite's sandbox, with a stub resolver and a decoy one on PATH.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…anual

`INSTALLABLES` gains the two commands at its END, so every index the suite
takes still names the file it did: FIFTEEN files and TWENTY-NINE artifacts,
with the usage text, the header and every in-body count moved with them
(`THIRTEEN SINCE AMENDMENT 16` and "the thirteen placements" stay: one is
history and the other a different count). The home ruling of 2026-09-29 is
why they are here: Brett Heap, verbatim "this work is really for
openRepoTools repo".

docs/README-claude-current.md is the manual: the candidates and their order,
the update and its lock, output, exit codes and variables, the hand-off
between launchers (CLAUDE_BIN, CLAUDE_RESOLVED_BIN, CLAUDE_VERIFIED_VERSION),
what `lane-start` does with it, and the restart check the status line calls.

README.md stays at 472 lines, the cap tests/test_repo_hygiene.py holds it
to: the counts are rewritten in place, and "THIRTEEN files" and
`13 of 13 placed`, which that test also asserts, survive as the history
sentence that says when they stopped being true. The doc is linked from the
existing line that names docs/README-lanes.md.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 17:43
The workBenches side (opensoft/workBenches#121, 016-launch-current-claude)
reports claude-current's exit 1 as "Claude CLI not found.", which is what 1
means for every other cause. A CLAUDE_CURRENT_* bound that is not a whole
number of seconds is a typo in a setting, not a missing Claude Code, so it
exits 64 now, as an argument it does not know already did. The launcher there
already refuses on 64 and names the status.

`lane-start` ends with 1 for any failure but 2, as before, and its message
says "named no Claude Code to launch" so it reads true for 64 as well as 1.
The header, the usage text, the manual and both suites say the same.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Some launch paths still bypass resolution, and version comparison and restart detection have correctness gaps.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Resolved since last review (2)

Comment thread claude-current Outdated
Comment thread claude-restart-check
Comment thread lane-start Outdated
Copilot AI balanced review requested due to automatic review settings September 29, 2026 17:48

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Some launch paths bypass resolution, restart detection misses cross-installation upgrades, and directory paths can pass executable validation.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Resolve bare claude launches on the --confirm No path

docs/​README-claude-current.md:185

This documented exception contradicts the PR's “every launch” contract and the ratified new-session scenario. The --confirm No branch exits at lane-start:1982, before the resolver block, so a direct lane start with unset CLAUDE_BIN still executes bare claude from PATH even when claude-current is installed. Resolve that bare launch too (without performing lane writes), rather than landing a known uncovered launch path.

Medium severity Require resolver path to be an executable regular file

lane-start:2693

-x also succeeds for searchable directories. If a malformed resolver returns path=/some/directory, this check accepts it, the lane records are written, and the final exec fails with status 126. Require both -f and -x so only an executable file can pass this pre-write gate.

Low severity Update contributor guide with the fifteen-file installer inventory

openRepoTools:28

The contributor guide still states at AGENTS.md:6 that openRepoTools places thirteen files. Since this change makes the installer contract fifteen files, update that authoritative repository guide in the same PR so contributors are not given a conflicting inventory.

…ed version across both families, and the bare --confirm launch resolves

Three findings, all real:

- Pre-release tags were compared as one string, so `2.1.284-beta.9` sorted
  after `2.1.284-beta.10` and a launch could take an older pre-release as
  ahead of npm. `precmp` compares the dot-separated identifiers by SemVer
  2.0.0 rule 11: numbers as numbers and before words, words in byte order
  (LC_ALL=C), fewer identifiers lower. It is in both commands byte for byte,
  like `numcmp` and `vercmp`; the suite checks the identity and runs rule
  11's own example chain.
- claude-restart-check read the installed version only from the running
  binary's family, so an npm 2.1.283 session beside a native 2.1.284 said
  nothing, though the next launch starts the native one. It now takes the
  highest of the running install's own, the native versions directory
  ($CLAUDE_CURRENT_NATIVE_DIR) and the user npm prefix's package, which it
  finds with claude-current's `npm_user_prefix`, byte for byte. No `$HOME`
  reads only the running install. Its suite now runs with `$HOME` and the
  npm prefix inside the fixture.
- lane-start's bare launch, the one a `--confirm` answered No takes, came
  before the resolution and started `claude` from PATH. The resolution is
  one function now, `lane_claude_resolve_launch`, called from that launch
  and from the foot of 5a, and it refuses a stale copy in both. The bare
  path gains one line; step 4 is untouched.

Also: the lane-start suite asks for the `claude <version>` sub-field by name
rather than at the end of the payload, because Amendment 18 (#83) appends
`host`, `os` and `container` after it (measured on a test-merge of #83).

Counts: tests/test_claude_current.py 70, tests/test_claude_restart_check.py
19, tests/test_lane_start_claude_current.py 20.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 17:59

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The fallback lock can become permanently orphaned, directory paths can pass executable validation, and the central Bash inventory omits the new commands.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Require a regular executable file, not just a searchable directory

lane-start:669

-x is also true for a searchable directory. If malformed porcelain names such a directory, resolution succeeds, the lane records are written, and the final exec then fails instead of preserving the “Nothing was written” guarantee. Require a regular executable file here.

Comment thread claude-current Outdated
Comment thread tests/test_claude_current.py Outdated
… as it is taken, and the two commands borrow every per-file hygiene rule

- Without `flock`, the lock was a directory made first and given its
  owner's pid a line later. A launch killed between the two left a lock with
  no owner, which the takeover never removes (it reaps only a named dead
  owner), so every later launch waited it out and refused. The lock is now a
  symlink whose text is `pid=<pid>`: one `ln -s` makes it and names the
  owner, so no ownerless lock can exist. The reaper is unchanged. A
  directory standing at the lock path (where `ln` would put the link inside
  it and succeed) is refused by name. The release removes the link only
  while it still names this launch.
- The two commands are still not in test_repo_hygiene.py's `ALL_BASH`,
  because that file is being changed by open PR #93 and this PR touches none
  of #93's files. tests/test_claude_current.py now also borrows the
  flag-spelling rule and runs the LF-index check for both. The follow-up
  after #93 lands is to put `claude-current` in `SHIPPED_BASH` and
  `claude-restart-check` in `HELPER_BASH`, then drop the borrowed list.

tests/test_claude_current.py, 72 cases:
- `test_the_link_lock_names_its_owner_while_it_is_held` reads the lock from
  inside the update. It fails on the directory lock.
- `test_a_directory_at_the_lock_path_is_refused_not_taken` is new.
- The takeover, reaper and live-lock cases now work on the link.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 18:09

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The post-lock recheck can incorrectly refuse a newly appeared ahead version as stale.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Accept newer candidates after lock wait instead of reporting stale

claude-current:657

The post-lock recheck only accepts an exact version match. If a newer-than-published candidate appears while this process is waiting, a lock timeout reaches the stale refusal even though the same candidate would have been accepted as ahead before waiting (for example, published 2.1.284 and installed 2.1.285). Accept ahead here as well before attempting another update or reporting stale.

… is ahead, not stale

Copilot's overview at 9ba858d, a finding in code unchanged since its
earlier rounds: after the wait for another launch's update, only a copy
EQUAL to npm was accepted. A copy newer than the published version that the
other launch installed meanwhile (npm moved on while this launch waited)
therefore went to another update, or, when the wait ran out, to the stale
refusal, although the same copy is `ahead` before any wait. The re-read
after the lock now gives the same two answers as the first read, in the same
order.

test_a_copy_ahead_of_npm_that_appears_during_the_wait_is_ahead: the other
launch installs 2.1.285 against a published 2.1.284 while it holds the lock
past this launch's wait. At 9ba858d this is exit 2; here it is `ahead`, and
this launch installs nothing. 73 cases.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 18:17

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Relative npm prefixes install successfully but are then rejected as non-absolute candidates.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Normalize relative npm prefixes before installation and validation

claude-current:348

Normalize the configured npm prefix to an absolute path before using it. npm accepts a relative --prefix and installs there, but add_candidate rejects non-absolute paths, so a run with NPM_CONFIG_PREFIX=relative-prefix installs the published version successfully and then exits 1 claiming nothing runnable was produced. claude-restart-check also needs the same normalization so it inspects that installation consistently. Please cover relative environment and .npmrc prefix values.

…en under $HOME

Copilot's overview at c7e3e3c, a finding in code unchanged since earlier
rounds: npm takes a relative `--prefix` and installs there, but the
candidate read back from it is not an absolute path and is refused, so an
update that worked ended in "no runnable Claude Code" (exit 1). A relative
prefix from any source ($CLAUDE_CURRENT_NPM_PREFIX, $NPM_CONFIG_PREFIX,
$npm_config_prefix, the `prefix=` line of ~/.npmrc) is now anchored at
$HOME, in `npm_user_prefix`, which both commands carry byte for byte. npm
installs into that path, the resolver reads it back, and the restart check
reads it from whatever directory the status line runs in.

Both suites now run their command from inside the sandbox rather than the
checkout, so a relative path can never land in the repository. On the
previous head the three new cases fail: the resolver exits 1 for a relative
prefix from the environment and from ~/.npmrc, and the restart check never
reads the copy. They pass here.

Suites: tests/test_claude_current.py 78, tests/test_claude_restart_check.py 20.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 18:24

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Timeout enforcement and package-version parsing have unresolved functional failures.

Review effort: Balanced
Findings: None

Previously missed (3)

In code that hasn't changed since last review

Medium severity Bound process termination after SIGTERM

claude-current:197

run_bounded can still wait forever when the command or an npm lifecycle child ignores SIGTERM: kill_tree sends only TERM, then wait "$pid" has no upper bound. The timeout/gtimeout branches have the same issue without a kill-after interval. Add a finite grace period followed by KILL for the whole process tree so the documented launch timeout is a real bound.

Medium severity Parse minified package.json versions correctly

claude-restart-check:229

package_version only recognizes a version key on its own line. A valid minified package.json such as {"name":"@anthropic-ai/claude-code","version":"2.1.284"} returns no version, so a 2.1.283 session gets no restart notice even though a newer npm package is installed. Parse the JSON independently of its line formatting while preserving the command's silent-failure behavior.

Low severity Correct artifact count from thirteen to fourteen

openRepoTools:1210

This function places fourteen non-bin artifacts (six skill files, six command files, and two hook entries), not thirteen, as the section header above already states.

…inified package.json, and one count

Three findings the overview lists as missed in unchanged code, all real:

- A command that ignored TERM, with its children, held a launch past every
  bound, because TERM was the only signal. Every branch now sends KILL
  KILL_GRACE (5) seconds after TERM: `timeout -k` and `gtimeout -k`, and in
  the watchdog, the whole tree it signalled. The watchdog and run_bounded
  settle who acts with one `mkdir` of a flag, whoever makes it first, so a
  command that ends early is never signalled and a watchdog is never stopped
  half way. A zombie counts as ended, because in a container whose first
  process reaps nothing (py-bench's pid 1 is `sleep`) it is never reaped. A
  137 from the KILL reads as the timeout's 124.
- claude-restart-check read `"version"` only from a line of its own, so a
  minified package.json gave no installed version and no notice. The file is
  cut at each brace and comma before the first "version" is taken, and a
  missing or unreadable file is still silent.
- The comment above `place_skill_and_hook` said "thirteen placements" for
  fourteen artifacts. It now says fourteen artifacts in thirteen writes (the
  two hook entries go into settings.json in one write).

On da83af3 the three new cases fail (the TERM-ignoring npm on both
branches, and the minified package); they pass here. Suites:
tests/test_claude_current.py 80, tests/test_claude_restart_check.py 21.

Refs opensoft/workBenches#119

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 18:36

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Lock coordination and custom native-directory detection have correctness gaps, and one concurrency test is timing-dependent.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Overridden native directory is not recognized as a Claude session

claude-restart-check:203

CLAUDE_CURRENT_NATIVE_DIR is used to find installed versions, but process recognition is hard-coded to paths containing /claude/versions/. A session running from an overridden native directory is therefore not recognized as Claude and exits without warning, even when a newer sibling exists. Recognize the configured native directory too, resolving symlinks consistently.

Medium severity Replace timing-dependent sleeps with an explicit synchronization barrier

tests/​test_claude_current.py:348

This helper coordinates with fixed sleeps after the holder acquires the lock. If CI does not schedule the tested resolver before the 1.5/2-second delay, the install happens before its initial scan and the updated by another launch assertions fail despite correct behavior. Use an explicit barrier that the resolver signals when it reaches lock acquisition instead of wall-clock ordering.

Low severity Empty NO_COLOR value incorrectly enables ANSI output

claude-restart-check:331

NO_COLOR is documented as presence-based, but -n treats an exported empty value as unset and emits ANSI escapes. Check variable presence so NO_COLOR= also produces plain output.

Comment thread claude-current Outdated
Comment thread openRepoTools
brettheap and others added 2 commits September 30, 2026 15:47
…holds

Copilot's round three on #134 (claude-current:516): the lock was flock
where PATH had one and the symlink where it did not, so two launches
with different PATHs held two independent locks and both ran the same
npm update at once. Every launch now takes the one symlink lock at
claude-current.lock.l, and flock is never consulted.

The symlink names its owner as pid=<pid>@<place>, where <place> is the
host's name and, where /proc shows one, the pid namespace's inode.
Containers that share a home share this lock, and a pid read in another
namespace says nothing about its owner, which flock never needed to
know: a lock made at another place is waited on and never taken over,
and the refusal names its owner and place. A lock made here whose owner
has gone (a zombie included) is taken over under the reaper as before.
A cache directory the launch cannot write in, and a file that is not
this command's lock at the lock path, are named rather than waited out
in silence.

Tests: two real launches, one with a flock on PATH (a logging shim, on
every host) and one without, exclude each other in both orders, one
install, and flock is never called; red on b90f4dc in both orders.

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot's round three on #134 (openRepoTools:286): INSTALLABLES now
holds fifteen names with claude-current and claude-restart-check, and
AGENTS.md still said thirteen. One word, line-neutral, so the file stays
at the 265-line cap; #93's AGENTS.md hunk is at line 254 and does not
touch this paragraph.

Lane: openxfactory-5 (openXfactory-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 17:19
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

The implementation, integration, documentation, portability checks, and extensive hermetic coverage are internally consistent, with prior concurrency and correctness findings addressed.

Review effort: Balanced
Findings: None

Resolved since last review (2)

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.

2 participants