Skip to content

fix(freebsd): treat ${LOCALBASE}/bin as pkg-managed; resolve self via sysctl - #1777

Merged
DeusData merged 2 commits into
DeusData:mainfrom
ocochard:feature/freebsd-pkg-managed-detection
Sep 2, 2026
Merged

fix(freebsd): treat ${LOCALBASE}/bin as pkg-managed; resolve self via sysctl#1777
DeusData merged 2 commits into
DeusData:mainfrom
ocochard:feature/freebsd-pkg-managed-detection

Conversation

@ocochard

Copy link
Copy Markdown
Contributor

Follow-up to the closure of #1331. That PR was superseded by the runtime rework of install/update (#1566) and the KERN_PROC_PATHNAME self-path fix that landed on main. Two FreeBSD gaps remained, which @DeusData identified in the closing comment:

1. cli_external_manager_name() never recognised FreeBSD's package prefix

It matches only positive evidence (mise, Homebrew, nix, asdf, cargo). ${LOCALBASE}/bin (default /usr/local/bin) was not among them, so a ports/pkg install of cbm was still copied into ~/.local/bin and a shell rc was edited — exactly what externally-managed detection is meant to prevent.

Now ${LOCALBASE}/bin and /sbin are recognised on FreeBSD (#ifdef __FreeBSD__), so install skips the copy and update refuses and points at pkg upgrade codebase-memory-mcp.

LOCALBASE is configurable, so the FreeBSD port passes its real PREFIX via -DCBM_PKG_PREFIX and we honour it, falling back to the documented /usr/local default for a plain upstream build. --force-binary remains the escape hatch for anyone self-managing that prefix.

2. resolve_self_executable() in the HTTP server still read /proc/self/exe

FreeBSD does not mount /proc by default. Added a KERN_PROC_PATHNAME sysctl branch, mirroring cbm_detect_self_path() in cli.c.

Testing

cli_external_manager_detection_needs_positive_evidence_issue1566 updated: on FreeBSD ${LOCALBASE}/bin/cbm is now externally managed; a non-LOCALBASE prefix (/opt/local) is not.

Built and validated through the FreeBSD port (devel/codebase-memory-mcp) in a clean poudriere jail.

… sysctl

Two FreeBSD gaps remain after the runtime rework of install/update
(DeusData#1566) and the KERN_PROC_PATHNAME self-path fix landed on main.

1. cli_external_manager_name() matches only positive evidence (mise,
   Homebrew, nix, asdf, cargo). ${LOCALBASE}/bin was not among them, so a
   ports/pkg install of cbm was still copied into ~/.local/bin and a
   shell rc was edited -- exactly what externally-managed detection is
   meant to prevent. Recognise ${LOCALBASE}/bin and /sbin on FreeBSD so
   install skips the copy and update defers to `pkg upgrade`.

   LOCALBASE is configurable, so the FreeBSD port passes its real PREFIX
   via -DCBM_PKG_PREFIX and we honour it, falling back to the documented
   /usr/local default for a plain upstream build. --force-binary remains
   the escape hatch for anyone self-managing that prefix.

2. resolve_self_executable() in the HTTP server still read
   /proc/self/exe, which FreeBSD does not mount by default. Add a
   KERN_PROC_PATHNAME sysctl branch, mirroring cbm_detect_self_path().

Test updated: on FreeBSD ${LOCALBASE}/bin/cbm is now externally managed;
a non-LOCALBASE prefix (/opt/local) is not.

Signed-off-by: Olivier Cochard-Labbé <olivier@cochard.me>
@ocochard
ocochard requested a review from DeusData as a code owner August 21, 2026 06:17
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@ocochard

Copy link
Copy Markdown
Contributor Author

One scoping note. This PR implements exactly the two items from the #1331 closing comment (prefix detection + the sysctl resolve_self_executable). While validating it through the FreeBSD port I hit a related gap that I deliberately left out of scope here:

After detection correctly skips the binary, agent configs still resolve through cbm_agent_installed_binary_path() / the install bin_target, both of which default to ~/.local/bin/codebase-memory-mcp — a path with no file on a pkg install. So install writes an mcp.json/hooks entry pointing at a nonexistent binary.

The FreeBSD port currently fixes this locally with two extra hunks: retarget cbm_agent_installed_binary_path() to ${PREFIX}/bin, and retarget bin_target to the OS-reported self_path when we skip. The clean upstream version would use the already-resolved runtime self_path rather than a compile-time prefix, but it needs a small signature change to cbm_agent_installed_binary_path().

Want me to fold that into this PR, or keep it as a separate follow-up? Happy to do either.

@ocochard

Copy link
Copy Markdown
Contributor Author

Answering the two questions you raised in #1331 directly, since you asked me to own them rather than guess:

"Does a platform-conditional /usr/local/bin match match how ports actually behave?"
Yes. On FreeBSD, ${LOCALBASE} (default /usr/local) is where all third-party software lives — ports and pkg both install there, and pkg(8) owns every file under it. There is no separate "user prefix" convention the way ~/.local/bin is on Linux. So a binary running from ${LOCALBASE}/bin is, by construction, package-managed. Gating on #ifdef __FreeBSD__ is what keeps this from misfiring as you noted (/usr/local/bin is also Intel-Homebrew's prefix on macOS, and a legitimate --dir target elsewhere) — on those platforms the branch doesn't compile in.

"Is /usr/local/bin the only prefix worth matching, or should the check be broader?"
Two refinements over a bare /usr/local/bin:

  1. ${LOCALBASE} is configurable. A user can set LOCALBASE=/opt/local (or a repo can be built with a non-default PREFIX). Hardcoding /usr/local in upstream would miss those. So the FreeBSD port passes its real PREFIX via -DCBM_PKG_PREFIX and this PR honors it, falling back to the documented /usr/local default for a plain upstream build. That keeps upstream correct for the 99% on default localbase while staying exact for the port.
  2. /sbin as well as /bin. ${LOCALBASE}/sbin is a valid install target for daemons; matched both for completeness (harmless for this CLI, correct in general).

--force-binary remains the escape hatch for anyone who deliberately self-manages that prefix.


CI note: the one red check (test-unix (macos-15-intel)) is lock_registry_absolute_deadline_survives_repeated_wakes failing on ASSERT(tail_queued) — a thread-timing test unrelated to this diff (I touch no lock-registry code), which passed on macos-14, all Linux/Windows shards, tsan and msan. Looks like a runner-load flake; a re-run should clear it (I don't have rerun rights on the repo).

@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Approved on merit. I have rebased it — the branch was 226 commits behind, so the red you are looking at is from 21 August and means nothing now.

This is the follow-up we asked for when closing #1331, and it answers both gaps precisely.

The externally-managed detection was the more important of the two. cli_external_manager_name() matches only positive evidence, which is the right design — but a positive-evidence list is exactly the kind that silently omits a platform. A ports/pkg install landing a second copy in ~/.local/bin and editing a shell rc is the specific harm that function exists to prevent, so a missing entry there is a real bug rather than a cosmetic one.

Three things I want to name because they are the difference between a patch and a correct patch:

Honouring a configurable LOCALBASE rather than hardcoding /usr/local. LOCALBASE is a port-level knob, and a fix that assumed the default would have been wrong on exactly the systems most likely to be affected. Taking -DCBM_PKG_PREFIX from the port with the documented default as fallback is the right shape.

Anchoring with strncmp at position 0 rather than strstr. The neighbouring matchers use strstr because /.cargo/bin/ is a path fragment; a prefix must not be, or install --dir=/opt/x/usr/local/bin would be misread as pkg-managed. You changed idiom where the semantics changed, which is the harder call to make.

Leaving --force-binary as the escape hatch, and saying so in the comment, so someone genuinely self-managing that prefix is not stuck.

The KERN_PROC_PATHNAME branch mirrors cbm_detect_self_path() in cli.c, so the two self-resolution paths now agree on FreeBSD instead of one silently failing on a system with no /proc mounted.

One footgun worth documenting

strncmp(self_path, CBM_PKG_PREFIX "/bin/", sizeof(CBM_PKG_PREFIX "/bin/") - 1)

relies on CBM_PKG_PREFIX being a quoted string literal. A port passing -DCBM_PKG_PREFIX=/usr/local unquoted will not compile, and the error will point at string concatenation rather than at the flag. That fails loudly rather than silently, so it is not a blocker — but please add a line to the comment saying the define must be -DCBM_PKG_PREFIX='"/usr/local"', so the next packager reads it before hitting it.

Minor: the description says "${LOCALBASE}/bin and /sbin", but the code checks ${LOCALBASE}/sbin, not an absolute /sbin. The code is right; the sentence is what I would fix.

The honest limit

We have no FreeBSD CI leg, so nothing in our matrix exercises either branch. Your poudriere jail is the only verification this change will ever get from us, and I am taking it on that basis. What makes that acceptable rather than reckless is that every line is behind #ifdef __FreeBSD__ — the blast radius on the platforms we do test is provably zero, and the rebased run will confirm it.

Add the CBM_PKG_PREFIX quoting note and I will merge on green. Thank you for coming back to finish this after #1331 was closed — and for validating it through the actual port rather than a local build.

@DeusData
DeusData merged commit c38aa35 into DeusData:main Sep 2, 2026
34 checks passed
@DeusData

DeusData commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Merged as c38aa35c. Thank you for coming back to finish this after #1331 was closed.

I want to correct something I said. I described the CBM_PKG_PREFIX quoting comment as "not a blocker" and then wrote "add the note and I will merge on green" — which made it one. That was inconsistent of me, and holding a green, #ifdef __FreeBSD__-gated fix on a documentation line would have been disproportionate, especially for a platform we have no CI leg for at all. I have merged it and will add that note myself.

For the record, so the next packager finds it: sizeof(CBM_PKG_PREFIX "/bin/") relies on string-literal concatenation, so the define must be passed quoted — -DCBM_PKG_PREFIX='"/usr/local"'. An unquoted value fails to compile with an error pointing at concatenation rather than at the flag.

Two things about the change I want on the record because they are the reason it was easy to take:

Honouring a configurable LOCALBASE rather than hardcoding /usr/local. A fix that assumed the default would have been wrong on exactly the systems most likely to need it.

Anchoring with strncmp at position 0 rather than copying the neighbouring strstr idiom. /.cargo/bin/ is a path fragment and a prefix is not, so install --dir=/opt/x/usr/local/bin must not be misread as pkg-managed. Changing idiom where the semantics change is the harder call, and you made it correctly.

Verification

Your green was 34/34 but 31 commits behind, and #1802 had since touched src/cli/cli.c, which this also edits. Rather than assume the functions were disjoint I rebuilt the actual merge locally against current main and ran cli, agent_clients and httpd: clean build, 392 passed, 1 skipped, 0 failed.

The honest limit stands and is worth repeating on the record: we have no FreeBSD CI, so your poudriere-jail validation is the only platform evidence this will ever get from us. What makes that acceptable rather than reckless is that every line is behind #ifdef __FreeBSD__ — the blast radius on the platforms we do test is provably zero, and the local run confirms it.

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