Skip to content

fix(cli): support --version from the installed package manifest - #304

Merged
amitpaz1 merged 2 commits into
mainfrom
fix/cli-version
Sep 10, 2026
Merged

fix(cli): support --version from the installed package manifest#304
amitpaz1 merged 2 commits into
mainfrom
fix/cli-version

Conversation

@amitpaz1

@amitpaz1 amitpaz1 commented Sep 10, 2026

Copy link
Copy Markdown
Member

Summary

Continue the interrupted fix/cli-version task, preserving its useful tests. Commander --version / -V reads the validated installed CLI package manifest relative to its module, not cwd. It exits before action dispatch or provider/session startup. No version literal is copied into source or baked into the bundle.

Coverage includes action non-dispatch, credential-free offline execution with no session files, help compatibility, and a relocated package with an alternate prerelease version and conflicting cwd manifest. The root-option allowlist explicitly admits --version and the Windows lane includes the new tests. Yoga embedded data: fetches are allowed by the offline guard; network remains denied.

Verification

  • Original fail-first: exit 1, two failures.
  • Removed-registration mutant: exit 1, four failures.
  • Hardcoded-current-version mutant: exit 1, relocated-version assertion fails. Both mutants restored.
  • Final pnpm build, pnpm typecheck, TMPDIR=/home/amit/agentrig-test-tmp.eEvVwx pnpm test: each exit 0; 247 files, 3,791 passing tests, four existing skips.
  • Default-temp preflight was blocked by /tmp/.git. No marker removed and no check bypassed; alternate temporary ancestry passed preflight.
  • First-head Windows CI caught a relocated-fixture dependency error. Canonical Yoga directory linking fixes it without production changes or weaker assertions; all four version tests passed on final-head Windows.

Independent reviews and resolutions

Both tools reviewed in separate detached worktrees. One full pair reviewed 3574b5e; one bounded delta pair reviewed final head c465ba6. No endless broad rounds.

  • Claude Code original APPROVE; final-delta APPROVE.
  • Codex original APPROVE; final-delta APPROVE.
  • Windows stripped-environment concern: resolved by actual Windows process tests; reviewer explicitly withdrew the need for an env change. Credentials remain excluded.
  • Global version scanning: accepted standard Commander behavior, no existing subcommand flag conflicts. Root flags remain explicitly pinned.
  • Review coverage limitation: spies/network/filesystem checks do not directly detect invisible construction/config reads; reviewers inspected startup paths and found no introduced startup. No material change requested.
  • Reviewers did not independently execute the full trio or Windows; execution receipts above are builder/CI evidence, not attributed to reviewers.

Exact-head CI

At c465ba6:

Authorization and landing

User direction for this exact task: “Open one PR, get independent Claude Code and Codex reviews, address material findings without endless broad review rounds, and merge after exact-head CI passes. Verify post-merge CI. This authorizes the merge.”
This is the single user-directed fix, not a roadmap band. Squash merge after final head recheck; post-merge receipts will be posted here.

@amitpaz1

Copy link
Copy Markdown
Member Author

Verdict: APPROVE, with one Windows item to confirm on exact-head CI before merge.

What the PR does. Root command gets .version() fed from the package manifest, read relative to the module. Tests cover no action dispatch, offline built-CLI runs with no session files, help still working, and a relocated package with a different prerelease version and a conflicting cwd manifest.

Verified independently

  • Manifest resolution is correct in both layouts. Source src/program.ts and the esbuild bundle both sit one level below the manifest. The bundle script uses a flat outdir: 'dist' and esbuild leaves import.meta.url untouched. The same relative-URL pattern already exists at packages/cli/src/evaluation-fixtures.ts:23.
  • No baked-in literal. The manifest is 0.0.0. The relocated-package test asserts a different prerelease string against a wrong cwd manifest, so a hardcoded mutant cannot pass.
  • No startup before exit. I probed Commander 12.1.0 directly from the main checkout's installed copy. --version and -V throw commander.version with exit code 0 in every argv position, during option parsing and before any preAction hook or action. The in-process test asserting run/tui mocks are not called is sound.
  • Help preserved. -V, --version appears in help. No test asserts exact help text, so no regression there.
  • Allowlist and Windows list. The root-flag test now pins ["--version", "--profile"]. The Windows consumer test only checks include equals the exported list, so insertion order does not matter.

Findings

  1. Windows env risk, unverified at packages/cli/test/version.test.ts:22 and :59. The spawns use a stripped env of PATH, HOME, USERPROFILE, NO_COLOR. Every other spawn test in the repo spreads process.env. On Windows, a child Node without SYSTEMROOT or TEMP can fail at startup. This file was added to the Windows include list, so the exact-head Windows job will settle it. If red, spread process.env and override HOME/USERPROFILE.
  2. Minor, accepted cost. The root scan consumes --version anywhere in argv, including agentrig run -m --version and a subcommand's own -V if one is ever added. No subcommand defines -V or --version today. Same class as the documented --profile behavior.

Not verified. This worktree has no dependencies installed and I was constrained to read-only actions, so I did not run build, typecheck, the full suite, the fail-first run, or the mutant runs. Those remain builder claims. I did not execute anything on Windows. Repro for the Commander probe:

node --input-type=module -e 'import("/home/amit/agentrig/node_modules/.pnpm/commander@12.1.0/node_modules/commander/esm.mjs").then(async ({Command}) => { const p = new Command().name("agentrig").version("9.9.9").exitOverride(); p.command("sessions").command("ls").action(()=>console.log("action")); try { await p.parseAsync(["sessions","ls","-V"],{from:"user"}) } catch(e) { console.log(e.code, e.exitCode) } })'

Review notes are also saved at /home/amit/.claude/plans/independently-review-pr-304-cozy-crown.md.

@amitpaz1

Copy link
Copy Markdown
Member Author

Independent Codex review — APPROVE

Reviewed exact head 3574b5e6c05956ee417f7fafbe9af5be1b2b2a00 in detached worktree /home/amit/agentrig-version-review-codex, independently of the builder and Claude reviewer. No material correctness or regression findings.

Inspected complete diff, CLAUDE.md, PLAN package/CLI sections, startup callers, module-relative manifest resolution, bundle layout, action dispatch, Windows junction/path handling, and Windows include-list coverage. git diff --check passed and worktree stayed clean.

Coverage limitation: action spies, network denial and filesystem checks do not directly detect invisible provider construction/config reads; static inspection found no startup introduced. This is not a requested change.

Execution limitation: independently ran fixture preflight, blocked by /tmp/.git. Review worktree had no dependencies/dist and was read-only. Did not independently run build/typecheck/tests/mutations/Windows or verify hosted CI; local green-trio receipts are the builder run, not reviewer results. No edits, commits, pushes, merges or child agents.

@amitpaz1

Copy link
Copy Markdown
Member Author

APPROVE for exact head c465ba6, with exact-head Windows CI still the merge gate.

Scope of the delta. The diff from 3574b5e touches only two files: the relocated-package test in packages/cli/test/version.test.ts and a STATUS.md paragraph. No production source, no environment change, no other tests.

The alternate-version assertion is preserved. The test still copies dist into a relocated directory, writes a manifest with version 9.8.7-version-test.1 beside it, writes a conflicting 1.2.3 manifest in the working directory, and asserts stdout is exactly the relocated version with empty stderr and exit 0. The stripped subprocess environment is byte-identical to the approved head. Only the dependency-provisioning lines changed.

The fixture repair is sound. I verified the assumptions it rests on against the installed tree in the main checkout:

  • The CLI bundle marks only three modules external, and the built entry contains a single bare "yoga-layout" specifier. The other two externals are lazy and not on the version path, which the Linux pass with only yoga linked already demonstrates.
  • yoga-layout 3.2.1 declares no dependencies and its pnpm store directory contains only itself, so junctioning the canonical realpath gives a self-contained, resolvable package. Its exports map resolves the bare specifier from that directory without needing anything else in the fake node_modules.
  • Linking the realpath removes the nested pnpm symlink chain that the first Windows run failed on. Resolving realpath before symlink also satisfies the Windows requirement that junction targets be absolute directories. On Linux the junction type is ignored and it degrades to an ordinary symlink, which matches the local pass.
  • Teardown behavior is unchanged in kind: the previous fixture already removed a junction under the temp directory, so the recursive rm path is not a new risk.

Earlier environment concern needs no change. The first-head Windows run passed the normal offline version, -V, and help subprocess tests under the identical stripped environment, and the relocated failure was a module-resolution error rather than anything environment-shaped. That is direct evidence the missing SystemRoot and TEMP variables do not affect these code paths. Leaving the env alone keeps the credential-free guarantee intact.

Minor, non-blocking. The fix hardcodes the one current external startup dependency. If a future bundle change adds another external on the startup path, this test will fail on every platform with a clear ERR_MODULE_NOT_FOUND, which is the desired signal rather than a silent gap. The STATUS.md wording accurately describes the failure and repair.

Execution limitations. I cannot run Windows here. This worktree has no node_modules or dist, so I did not re-execute the suite. I relied on the builder's reported green run and on reading the installed yoga-layout package and built bundle in the main checkout. The only thing that can confirm the Windows repair is the exact-head Windows job, which remains the gate.

@amitpaz1

Copy link
Copy Markdown
Member Author

Independent Codex final-delta review — APPROVE

Exact head c465ba67eb553236abbefe1c64e9101db2f5a66c, delta from previously approved 3574b5e only. No material issues.

  • Alternate-version assertion remains intact: relocated CLI must print 9.8.7-version-test.1, with a conflicting cwd manifest, exit 0 and empty stderr.
  • A real fixture node_modules plus a junction directly to canonical Yoga addresses Windows dependency resolution without weakening assertions. Yoga is an explicit external dependency with no dependencies in its lockfile entry.
  • Production code and stripped subprocess environment are unchanged.

Static review only: isolated read-only checkout lacks dependencies and built artifacts. Builder green-trio and earlier Windows failure are supplied evidence, not independently reproduced. Exact-head Windows CI remains the merge gate.

@amitpaz1

Copy link
Copy Markdown
Member Author

Final-head CI diagnosis; one bounded diagnostic rerun

At c465ba67eb553236abbefe1c64e9101db2f5a66c, all four version tests pass on Windows, including relocated-package resolution. The fixture repair is confirmed. Windows job 102886183200 instead timed out in the two unchanged checkpoint-worktree overlap tests at 30 seconds (packages/core/test/checkpoint-worktrees.test.ts:94). No production or test deadline is changed.

Rerun evidence: base main 9591b1b has green Windows CI (34477325860); the failing file and core implementation are outside this diff; these tests passed the same-head Ubuntu/macOS full-suite runs before the Windows failure, plus the local full suite. A fresh exact-head focused run using the Windows include config on Linux also passed all three tests (not a claim of Windows execution). The prior-head Windows run passed the overlap tests in ~9.8 seconds each. This supports ONE diagnostic rerun of the failed Windows job, not a broad repair or a repeated blind rerun loop. A second failure will block merge.

@amitpaz1
amitpaz1 merged commit 030e35f into main Sep 10, 2026
6 of 7 checks passed
@amitpaz1

Copy link
Copy Markdown
Member Author

Landing complete — post-merge CI verified

Squash merge: 030e35fa7733a2621ea1dc47d9af9bd0c40d43e4.

Both main workflows completed successfully on that exact merge SHA, first attempt:

No linked issues awaited closure. The only other open PR is #305; it overlaps packages/cli/src/program.ts, vitest.windows.config.ts, and docs/STATUS.md, so it must incorporate the moved main base and get fresh exact-head CI before its own landing. No other PR was merged and the task branch was retained.

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