Skip to content

Fix CLI version path resolution - #2454

Open
geofffranks wants to merge 1 commit into
steipete:mainfrom
geofffranks:fix/cli-version-resolution
Open

Fix CLI version path resolution#2454
geofffranks wants to merge 1 commit into
steipete:mainfrom
geofffranks:fix/cli-version-resolution

Conversation

@geofffranks

Copy link
Copy Markdown

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 25, 2026, 5:09 PM ET / 21:09 UTC.

ClawSweeper review

What this changes

This PR changes CodexBar’s CLI version lookup to derive metadata from the running executable rather than a potentially relative argv[0], with an XCTest covering app-bundle executable resolution.

Merge readiness

Blocked until real behavior proof is added - 3 items remain

Keep this PR open for normal review. The proposed executable-path lookup is a focused fix for an established CLI version-reporting bug, but the external PR does not yet include after-fix proof from a real relative-path or symlink invocation.

Priority: P2
Reviewed head: 2e273d18a8d8e687d234b8f79ef90d7dbb56bb97

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch is focused and has a relevant regression test, but required real behavior proof is absent.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body describes the intended behavior and includes an XCTest, but it contains no after-fix terminal output, recording, log, or other real invocation proof for relative-path or symlink launches. Please post redacted proof; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body describes the intended behavior and includes an XCTest, but it contains no after-fix terminal output, recording, log, or other real invocation proof for relative-path or symlink launches. Please post redacted proof; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Focused implementation change: The proposed patch makes currentVersion obtain an executable path from a new platform-aware resolver when a caller does not supply one, then retains symlink-aware adjacent-file and app-bundle metadata lookup.
Regression coverage added: The PR adds an XCTest that constructs an app bundle, supplies a bundle whose executable is adjacent to VERSION, and expects the adjacent version to be returned when no explicit executable path is supplied.
Repository policy applied: The repository AGENTS.md calls for focused CLI/parser tests where possible and requires real bundle/runtime validation only when UI behavior needs it; this is a CLI-path change, so a focused terminal demonstration is the appropriate missing merge proof.
Findings None None.
Security None None.

How this fits together

CodexBar’s CLI reports its version by looking next to its executable for a VERSION file and, when applicable, inside an app bundle for Info.plist metadata. The resolved version is then displayed to CLI users and scripts that query the installed tool.

flowchart LR
    A[CLI invocation] --> B[Resolve running executable]
    B --> C[Resolve symlinks]
    C --> D[Adjacent VERSION lookup]
    D --> E[App bundle metadata fallback]
    E --> F[Reported CLI version]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body describes the intended behavior and includes an XCTest, but it contains no after-fix terminal output, recording, log, or other real invocation proof for relative-path or symlink launches. Please post redacted proof; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P2) - The added unit test covers the app-bundle seam but does not provide observed after-fix behavior for the reported relative-path and symlink CLI invocations; a platform-specific executable-path fallback could otherwise remain unexercised in the contributor’s real setup.
  • Complete next step (P2) - No discrete code defect was established from the supplied diff; the remaining merge gate is contributor-provided real behavior proof rather than an automated repair.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 2 files affected; 60 additions, 2 deletions The change remains focused on CLI version resolution and its regression coverage.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the narrow resolver change if a redacted terminal proof shows --version returning the same expected version when launched through both a relative path and a symlink, alongside the focused regression test.
  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 narrow resolver change if a redacted terminal proof shows --version returning the same expected version when launched through both a relative path and a symlink, alongside the focused regression test.

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

No, not at high confidence in this review: the report provides a clear source-level path—launching through a relative executable path or symlink—but the environment prevented running that path against current main.

Is this the best way to solve the issue?

Unclear: resolving the actual executable path is a narrow, maintainable way to avoid argv[0] working-directory dependence, but real relative-path and symlink output should confirm that the platform fallbacks solve the reported behavior.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: An incorrect or missing CLI version affects installed-tool diagnostics and scripts, but the reported failure has a limited blast radius.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body describes the intended behavior and includes an XCTest, but it contains no after-fix terminal output, recording, log, or other real invocation proof for relative-path or symlink launches. Please post redacted proof; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: An incorrect or missing CLI version affects installed-tool diagnostics and scripts, but the reported failure has a limited blast radius.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body describes the intended behavior and includes an XCTest, but it contains no after-fix terminal output, recording, log, or other real invocation proof for relative-path or symlink launches. Please post redacted proof; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Focused implementation change: The proposed patch makes currentVersion obtain an executable path from a new platform-aware resolver when a caller does not supply one, then retains symlink-aware adjacent-file and app-bundle metadata lookup. (Sources/CodexBarCLI/CLIIO.swift:55, 2e273d18a8d8)
  • Regression coverage added: The PR adds an XCTest that constructs an app bundle, supplies a bundle whose executable is adjacent to VERSION, and expects the adjacent version to be returned when no explicit executable path is supplied. (Tests/CodexBarTests/CLIEntryTests.swift:95, 2e273d18a8d8)
  • Repository policy applied: The repository AGENTS.md calls for focused CLI/parser tests where possible and requires real bundle/runtime validation only when UI behavior needs it; this is a CLI-path change, so a focused terminal demonstration is the appropriate missing merge proof. (AGENTS.md:1, cc8da27cec92)
  • Current-main and history inspection unavailable: The read-only command sandbox failed before repository commands could start (bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted), so current-main source/history provenance could not be independently verified in this review.

Likely related people:

  • steipete: The target repository is steipete/codexbar; deeper file-history attribution could not be completed because the read-only command sandbox did not start. (role: repository owner and likely decision owner; confidence: low; files: Sources/CodexBarCLI/CLIIO.swift, Tests/CodexBarTests/CLIEntryTests.swift)

Rank-up moves

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

  • Post redacted terminal output showing the CLI’s expected version when invoked via a relative path and through a symlink.
  • Include the focused XCTest result if available, without exposing private filesystem paths or account data.

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.

@geofffranks

Copy link
Copy Markdown
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

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.

1 participant