fix(macos): Obsidian CLI transport never engaged; flock(1) absent breaks all vault locking - #121
Open
willyfog766 wants to merge 4 commits into
Open
fix(macos): Obsidian CLI transport never engaged; flock(1) absent breaks all vault locking#121willyfog766 wants to merge 4 commits into
willyfog766 wants to merge 4 commits into
Conversation
… macOS Three defects sharing one root cause: the plugin was never exercised end-to-end on macOS, and CI only ever ran on ubuntu-latest. - scripts/detect-transport.sh probed only PATH, but Obsidian 1.12 ships the CLI inside the app bundle and does not symlink it out. Every stock macOS install silently fell back to the filesystem floor while reporting success. The version probe also called `--version`, which the CLI rejects (it takes a `version` subcommand). Fixed with a candidate list plus a capability probe, bounded by a hand-rolled 3s timeout (macOS has no timeout(1)) so a GUI launcher cannot wedge detection. - flock(1) is util-linux and does not exist on macOS, so address allocation and every advisory lock failed on any Mac, taking 3 of 9 `make test` targets red. New scripts/lib/portable-lock.sh takes the same flock(2) kernel lock on fd 9, via flock(1) where present and python3's fcntl.flock where not (the lock lives on the open file description, so it persists after python exits and the kernel releases it on shell exit). Two earlier attempts at this were wrong and are worth recording. A hand-rolled mkdir+reap lock had a use-after-steal bug; the fixed version still let two workers into the critical section roughly 1 run in 5. macOS ships bash 3.2, which has no BASHPID, so a subshell cannot identify itself and every PID-based reaping scheme leaks. The kernel lock deletes the class: no reaping, no PID tracking, no owner tokens, no EXIT trap. - Every recipe in skills/wiki-cli/SKILL.md was wrong and none of them ran. The CLI takes key=value, not positional args; vault= takes a name, not a path; there is no `write` command (create + overwrite); daily:today does not exist. All 18 rewritten recipes were executed against a live vault. - BSD `wc -l` pads its output with leading spaces, breaking string compares in three test files. Adds a macos-latest CI job — the regression guard for this entire class — and PORTABLE_LOCK_FORCE_BACKEND so the python3 lock path gets coverage on Linux runners, which always have flock. make test: 10/10 on both lock backends. Lock suite: 960 contended acquisitions, 0 mutual-exclusion violations. No version bump or tag: leaving the release decision to the maintainer.
Same silent-wrong-target class as the transport bug in the parent commit,
in a second location.
`vault=<name>` is resolved against Obsidian's own vault registry, never
against the working directory. So a present, working CLI binary does NOT
imply it can reach the vault you are standing in:
- Obsidian may not have this directory open as a vault at all.
- Another directory may be registered under the same name, in which case
every CLI call reads and writes THAT directory and reports success.
The second case is not hypothetical. It is how this was found: a stray copy
of the repo registered under the same name absorbed an entire session's CLI
writes while the real working tree sat untouched, and detect-transport.sh
was cheerfully reporting `preferred: cli` the whole time.
detect-transport.sh now asks the CLI which vaults it knows (`vaults verbose`),
resolves symlinks on both sides, and looks for one whose path IS this vault
root. If none matches, `cli` is not preferred and we fall back to filesystem
with an actionable warning. `available.cli.present` stays true (the binary is
real; it just cannot reach us) and a new `available.cli.vault_addressable`
carries the distinction. The verified name is published as
`available.cli.vault_name` so consumers pass a checked `vault=` instead of
guessing from the directory basename.
Also splits the timeout helper into probe_with_timeout (exit status only,
stdout discarded) and capture_with_timeout (captures stdout). Capturing
`vaults` through the former silently yielded nothing, since it sends stdout
to /dev/null. The comment now says so, loudly.
tests/test_detect_transport.sh is new and hermetic (mocks the CLI): asserts
cli IS preferred when a vault matches, is NOT preferred when the only
registered vault lives elsewhere, and that the resolved name is published.
make test is now 11 suites.
skills/wiki-cli/SKILL.md documents the footgun and tells consumers to take
binary, vault_name, and preferred from the snapshot rather than hardcoding.
Author
|
Pushed a second commit (
|
…ntax in 6 skill docs Verifier findings on the previous commit. HIGH 1 — the bounded probe killed only the leader, not its children. `kill -9 "$pid"` leaves anything the command spawned running, reparented to PID 1. The hang we guard against is an Electron launcher, which spawns helpers freely, so every timed-out probe leaked orphans — and detect-transport.sh runs from several skills, so they accumulate across a session. Commands now start in their own process group (`set -m`; macOS ships no setsid(1)) and the timeout kills the negative pid, falling back to the bare pid if the group is already gone. Note _spawn_bounded returns the pid in a global rather than on stdout. Taking it via `pid="$(_spawn_bounded ...)"` runs the function in a command-substitution subshell, so the background job becomes a child of THAT subshell; `wait` in the caller then fails and the probe looks like it errored, which silently reported the CLI as absent. Caught by the test suite. HIGH 2 — no test covered the hang path, which is exactly where the leak was. tests/test_detect_transport.sh now runs a mock CLI that hangs AND spawns a long-lived child, then asserts detection still returns (~6s, not never), falls back to filesystem, and that the child is reaped. Verified the test has teeth: reverting the group-kill fails that one case and only that one. MEDIUM 1 — the fallback warning told the user to register the vault even when Obsidian simply was not running, in which case the vault may already be registered and the real advice is "start Obsidian". Message now branches on whether the app is running. MEDIUM 2 — six skill docs still documented the pre-fix CLI syntax: a `write` command that does not exist, positional args, a stdin redirect that is not supported, and a hardcoded/basename-derived vault name. wiki-ingest, save, autoresearch, wiki-query, wiki-lint and wiki/references/mcp-setup.md now show `key=value` form and take the vault NAME from `available.cli.vault_name`. wiki-lint additionally points at the CLI's native orphans/unresolved/deadends, which it currently re-rolls by hand. make test: 11/11 on both lock backends.
Residual hole in the previous commit, found while registering a vault by hand. That commit matches the vault by PATH, which is right, but then hands consumers a NAME — and the CLI resolves `vault=<name>` back through Obsidian's registry. If two registered vaults share that name, the published name is ambiguous and can resolve to the other one, reinstating the exact silent-wrong-target bug the block exists to prevent, inside the fix for it. This is not exotic. Obsidian derives a vault's name from its folder basename, so two checkouts of the same repo in different directories collide by default — and that is precisely the situation that caused the original incident (a stray clone in ~/Downloads registered as `claude-obsidian` alongside the real one). The CLI offers no way to target a vault by path, so a collision cannot be resolved here. Refuse `cli` and fall back to filesystem rather than guess, and publish no vault_name at all so a consumer cannot trust an ambiguous one. tests/test_detect_transport.sh covers it: a registry with our vault plus a decoy sharing its name must yield preferred=filesystem, vault_addressable=false, and an empty vault_name. Verified the test has teeth by disabling the guard. make test: 11/11 on both lock backends.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three defects that share one root cause: the plugin was never exercised end-to-end on macOS, and CI only runs on
ubuntu-latest. A Linux-only matrix is structurally incapable of catching any of these. I found them by running the code on a Mac, not by reading it.No version bump and no tag — that's your call, so the CHANGELOG entry is filed under
## [Unreleased].1. The Obsidian CLI transport silently never engaged on macOS (
scripts/detect-transport.sh)Detection probed only
PATH, but Obsidian 1.12 ships the CLI inside the app bundle and doesn't symlink it out. So every stock macOS install fell through to thefilesystemfloor while reporting success — the exact silent-downgrade the transport design exists to prevent. Separately, the version probe calledobsidian-cli --version, which the CLI rejects outright (it takes aversionsubcommand), so it could never have succeeded even if it had found the binary.Fixed with a candidate list (
PATH→ app bundle → bareobsidianlast) and a capability probe bounded by a 3s timeout, hand-rolled because macOS has notimeout(1). Without the bound, an Electron launcher that treats a bareversionas a file to open would hang detection — and several skills invoke it.2.
flock(1)doesn't exist on macOS, so every advisory lock in the vault failed(
scripts/allocate-address.sh,scripts/wiki-lock.sh)flock(1)is util-linux. Both scripts called it directly, so address allocation and all locking died withflock: command not found, taking 3 of 9make testtargets red on any Mac. Python was never affected —fcntl.flockis a POSIX syscall.New
scripts/lib/portable-lock.shtakes the sameflock(2)kernel lock on fd 9 by either route:flock(1)where present (Linux behavior unchanged), andpython3'sfcntl.flockon the inherited fd where not. The lock lives on the open file description, so it persists after python exits and the kernel releases it on shell exit — the same mechanismflock(1)uses internally.Worth recording, since it may save you from repeating it: I first emulated this in pure shell with an atomic
mkdirspin plus stale-lock reaping. It had a use-after-steal bug. The fixed version still let two workers into the critical section about 1 run in 5 under contention. The reason is that macOS ships bash 3.2, which has noBASHPID— a subshell cannot identify itself, so every PID-based reaping scheme leaks. The kernel lock deletes the entire class: no reaping, no PID tracking, no owner tokens, noEXITtrap, no steal semantics.3. Every recipe in
skills/wiki-cli/SKILL.mdwas wrong; none of them ranThe CLI takes
key=valueoptions, not positional args. Beyond syntax:vault=takes a vault name, not a path; there is nowritecommand (it'screate+overwrite);daily:todaydoesn't exist (daily:read/daily:path); andcontent=is a shell argument with\nescapes, not stdin — so the filesystem Write tool is genuinely better for real pages, which the docs now say. All 18 rewritten recipes were executed against a live vault.Two traps documented that only surface by running them:
base:viewsignorespath=and reads whatever file is active in the GUI (it isn't scriptable), andbase:querywithoutview=resolves the first view rather than all. I also added recipes for the CLI's nativeorphans/unresolved/deadends, whichwiki-lintcurrently reimplements by hand.Also
BSD
wc -lpads output with leading spaces, breaking string compares in three test files.Verification
make test— 10/10 green on macOS, the first time the suite has passed on the platform. Also green on Linux via theflockbackend.tests/test_portable_lock.sh: mutual exclusion under 6×20 contended workers, run 8× (960 contended acquisitions, 0 violations), plus timeout, release-on-exit, and reclaim-after-SIGKILL.macos-latestCI job. This is the most important change here — it's the regression guard for the whole class. The bugs above are symptoms; Linux-only CI is the cause.PORTABLE_LOCK_FORCE_BACKEND+ a CI step forcing thepython3backend, because Linux runners always haveflockand the python path would otherwise have zero coverage anywhere. The suite asserts the forced backend was actually taken — an earlier version of that check silently ranflockwhile claiming to test python, which is worse than no check.