Skip to content

Fix CLI version path resolution - #2454

Merged
steipete merged 3 commits into
steipete:mainfrom
geofffranks:fix/cli-version-resolution
Jul 29, 2026
Merged

Fix CLI version path resolution#2454
steipete merged 3 commits into
steipete:mainfrom
geofffranks:fix/cli-version-resolution

Conversation

@geofffranks

Copy link
Copy Markdown
Contributor

Problem

When the CLI is invoked through a relative path or symlink, version lookup used argv[0] relative to the current working directory. This could prevent CodexBar from finding the adjacent VERSION file or app-bundle metadata and result in a missing or incorrect displayed version.

Summary

• Resolve CLI version metadata from the actual executable path instead of relative argv[0].
• Preserve symlink-aware VERSION and app-bundle Info.plist lookup.
• Add regression coverage for bundle executable resolution.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 25, 2026
@clawsweeper

clawsweeper Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 29, 2026, 12:36 PM ET / 16:36 UTC.

ClawSweeper review

What this changes

The PR resolves CLI version metadata from the actual running executable rather than a possibly relative launch argument, and adds coverage for direct, relative, and symlink invocation paths.

Merge readiness

Blocked until stronger real behavior proof is added - 3 items remain

Keep this PR open: current main still derives CLI version lookup from CommandLine.arguments.first, so the relative-launch problem remains necessary to solve. The patch is focused and regression-tested, but the posted terminal transcript predates the current head and includes failing output rather than a redacted after-fix run; current-head runtime proof is still needed before merge. Likely related people: steipete is the strongest routing candidate from the recent CLI-test follow-up work (medium confidence).

Priority: P2
Reviewed head: 92f91b21db40fab5b24c4121f87d08374047fc89

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation and focused regression coverage look solid, but merge readiness is capped by missing current-head real-behavior proof.
Proof confidence 🧂 unranked krab (1/6) Needs stronger real behavior proof before merge: A terminal transcript exists, but it predates the current head and documents the failure context rather than an after-fix result; add redacted current-head output for direct, relative, and symlink invocation, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: A terminal transcript exists, but it predates the current head and documents the failure context rather than an after-fix result; add redacted current-head output for direct, relative, and symlink invocation, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed 5 items Current-main behavior remains vulnerable: Current main declares currentVersion with executablePath: String? = CommandLine.arguments.first, then treats that value as a file URL. A relative launch argument is therefore interpreted from the process working directory rather than necessarily from the installed executable location.
PR implementation is narrowly targeted: The PR changes the default source of the executable path to runningExecutablePath, using the bundle executable URL when available and platform-native executable-path fallbacks before retaining the existing symlink-aware VERSION and app-bundle lookup.
Regression coverage exercises launch forms: The added CLI entry test copies an executable beside a VERSION file, invokes it through a relative path, and creates a symlink invocation path to verify that both return the adjacent version.
Findings None None.
Security None None.

How this fits together

CodexBar’s CLI prints its installed version by locating metadata next to its executable or inside an app bundle. The invocation path feeds metadata discovery, which produces the version text returned by codexbar -V and codexbar --version.

flowchart LR
    Launch[CLI invocation] --> Executable[Running executable path]
    Executable --> Symlink[Resolve symlinks]
    Symlink --> Source{Version metadata source}
    Source --> VersionFile[Adjacent VERSION file]
    Source --> BundleInfo[App bundle Info.plist]
    VersionFile --> Output[CLI version output]
    BundleInfo --> Output
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: A terminal transcript exists, but it predates the current head and documents the failure context rather than an after-fix result; add redacted current-head output for direct, relative, and symlink invocation, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The current head lacks convincing after-fix runtime proof for direct, relative-path, and symlink invocation from a working directory outside the installation path.
  • Complete next step (P2) - No mechanical repair is indicated; the contributor must supply current-head real behavior proof before a human can safely approve the PR.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Focused surface 2 files affected; 112 added, 2 removed The change is confined to the CLI version resolver and its regression coverage, which keeps the functional review scope bounded.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the executable-path resolution approach and merge after a redacted current-head terminal transcript shows the expected version for direct, relative, and symlink launches from a different working directory.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Retain the executable-path resolution approach and merge after a redacted current-head terminal transcript shows the expected version for direct, relative, and symlink launches from a different working directory.

Do we have a high-confidence way to reproduce the issue?

No, not through a fresh runtime run in this read-only review. The current-main source makes the mechanism source-reproducible because it uses the relative launch argument as a filesystem path for adjacent metadata lookup.

Is this the best way to solve the issue?

Yes, provisionally. Resolving the actual executable path preserves the existing VERSION and app-bundle metadata behavior while removing dependence on the caller’s working directory; the remaining need is runtime proof on the current head.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fa9343a11d02.

Labels

Label justifications:

  • P2: The PR repairs a bounded but user-visible CLI version-reporting regression without evidence of data loss, security exposure, or core-runtime unavailability.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: A terminal transcript exists, but it predates the current head and documents the failure context rather than an after-fix result; add redacted current-head output for direct, relative, and symlink invocation, then update the PR body for automatic re-review or ask a maintainer to comment @clawsweeper re-review.

Evidence

What I checked:

  • Current-main behavior remains vulnerable: Current main declares currentVersion with executablePath: String? = CommandLine.arguments.first, then treats that value as a file URL. A relative launch argument is therefore interpreted from the process working directory rather than necessarily from the installed executable location. (Sources/CodexBarCLI/CLIIO.swift:57, fa9343a11d02)
  • PR implementation is narrowly targeted: The PR changes the default source of the executable path to runningExecutablePath, using the bundle executable URL when available and platform-native executable-path fallbacks before retaining the existing symlink-aware VERSION and app-bundle lookup. (Sources/CodexBarCLI/CLIIO.swift:58, 92f91b21db40)
  • Regression coverage exercises launch forms: The added CLI entry test copies an executable beside a VERSION file, invokes it through a relative path, and creates a symlink invocation path to verify that both return the adjacent version. (Tests/CodexBarTests/CLIEntryTests.swift:95, 92f91b21db40)
  • Recent follow-up provenance: The PR history shows the initial implementation in commit 40acc318304054d2764d9d4c11b07e91240389bc, followed by test-path coverage in cf79447f6df203ed1a2b2ffc0c2be4a68109c348 and a test-helper UTF-8 correction in the current head. (Tests/CodexBarTests/CLIEntryTests.swift:95, 92f91b21db40)
  • Runtime proof is not current-head after-fix evidence: The posted terminal transcript predates the current head and ends with codexbar -V from the home directory producing only CodexBar; it establishes the failure context but does not demonstrate the repaired behavior on the current branch.

Likely related people:

  • steipete: Authored the two most recent follow-up commits that expanded CLI invocation-path coverage and corrected its test helper on this PR. (role: recent area contributor; confidence: medium; commits: cf79447f6df2, 92f91b21db40; files: Tests/CodexBarTests/CLIEntryTests.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Post a redacted terminal transcript from the current head showing the expected version for direct, relative-path, and symlink launches from outside the install directory.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-07-25T21:09:22.191Z sha 2e273d1 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-29T15:56:09.766Z sha 1d8d496 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-29T16:02:30.032Z sha cf79447 :: needs real behavior proof before merge. :: none

@geofffranks

Copy link
Copy Markdown
Contributor Author

Proof:

$ alias | grep codexbar
$ codexbar -V
CodexBar
$ codexbar --version
CodexBar
$ which codexbar
/opt/homebrew/bin/codexbar
$ /opt/homebrew/bin/codexbar -V
CodexBar 0.45.2
$ cd /opt/homebrew/bin/
$ ./codexbar -V
CodexBar 0.45.2
$ codexbar -V
CodexBar 0.45.2
$ cd ~
$ codexbar -V
CodexBar

@steipete
steipete force-pushed the fix/cli-version-resolution branch from 2e273d1 to 1d8d496 Compare July 29, 2026 15:52
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 29, 2026
@steipete
steipete force-pushed the fix/cli-version-resolution branch from 1d8d496 to cf79447 Compare July 29, 2026 15:58
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 29, 2026
@steipete
steipete merged commit 256c05e into steipete:main Jul 29, 2026
9 checks passed
@steipete

Copy link
Copy Markdown
Owner

Merged after full verification. CLI version metadata now resolves from the running executable (_NSGetExecutablePath on Darwin, /proc/self/exe on Linux) with symlink resolution and normalized bundle fallback, so codexbar --version reports correctly regardless of how it was invoked.

Added during review: a real regression exercising the reported failure — invocation via a stale relative argv[0] from a different working directory, and through a symlink — since the original test only covered Bundle.executableURL and would have passed even with the bug present. Also switched a test helper to the failable UTF-8 conversion the repo lints for. Thanks @geofffranks — nice first contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants