fix(tools/git): detect-platform died silently outside a repo, taking every wrapper with it

Conversation 4 Commits 3 Files Changed 3

+67 -3

3 Commits

Author	SHA1	Message	Date
Mos	8e677d1f21	fix: stop re-escaping non-ASCII in package.json when enumerating a test ci/woodpecker/pr/ci Pipeline was successful Details CI step `format` failed (pipeline 2255, exit 1) -- not on the test, on formatting. I enumerated the new test by loading packages/mosaic/package.json with python's json module and writing it back. json.dumps defaults to ensure_ascii=True, so the em-dash in "description": "Mosaic agent framework — installation wizard and meta package" was rewritten as —. Valid JSON, identical when parsed, and rejected by `pnpm format:check`. Enumeration is a TEXT edit now: read the file, splice the test into the test:framework-shell string, write it back. A JSON round-trip rewrites the whole document to serialiser defaults; the only safe edit to a formatted file is one that touches the bytes it means to touch. diff vs main is now exactly one line in each branch.	2026-08-06 19:11:44 -05:00
Mos	92a63e7027	test(tools/git): a skip that exits 0 is a pass -- refuse instead ci/woodpecker/pr/ci Pipeline was canceled Details be-coder-07 (#1089 review 131) showed the precondition guard exited 0 when the scratch dir turned out to be inside a git worktree: ( cd "$TMP" && git rev-parse --git-dir ) && { echo "SKIP"; exit 0; } so pointing TMPDIR beneath a git worktree made this test PASS against unchanged main. A skip that exits 0 is indistinguishable from a pass -- the same defect this suite exists to catch, in the suite itself. Now: set GIT_CEILING_DIRECTORIES to the PHYSICAL path (it is matched physically, and /tmp is commonly a symlink, so a logical path silently disables the ceiling), and if the outside-a-repo precondition still cannot be established, FAIL with an explicit message rather than skipping. Replaying be-coder-07's attack (TMPDIR beneath a worktree): before: fix rc=0, main rc=0 <- both "pass", the vulnerability after: fix rc=1, main rc=1 <- both refuse; no false pass either way Normal conditions are unchanged and still discriminate: fix rc=0, main rc=1. The test refuses to report a result it cannot establish. That is weaker than running under a hostile TMPDIR and strictly better than lying about it. Reported-by: be-coder-07	2026-08-06 19:02:29 -05:00
Mos	b4b70675b6	fix(tools/git): detect-platform died silently outside a repo, taking every wrapper with it ci/woodpecker/pr/ci Pipeline was canceled Details detect_platform() and get_repo_info() both already contained the correct error path: remote_url=$(git remote get-url origin 2>/dev/null) if [[ -z "$remote_url" ]]; then echo "error: not a git repository or no origin remote" >&2 return 1 fi but that message was UNREACHABLE. Every wrapper in this directory runs `set -e`, and outside a git repo the assignment itself returns git's 128, terminating the caller before the -z check. The diagnostic was written and could never print. Observed cost: `pr-review.sh -n N -r owner/repo -H host -a comment -c "..."` invoked from a non-repo cwd exits 128 with NO stdout and NO stderr -- including when -r and -H are supplied, the flags whose help text says "skips git-remote inference". The inference runs first regardless. Two reviewer seats hit this tonight and correctly reported `blocked` with no diagnostic available to report; one of them lost a full review. Reproduced: cd /tmp && pr-review.sh -n 1086 -r mosaicstack/stack -H git.mosaicstack.dev \ -a comment -c "x" -> rc=128, 0 bytes of output same command from inside a repo -> rc=1, "Error: Comment required" `\|\| true` matches the idiom already used at the other two call sites in this same file (one `\|\| return 1`, one `\|\| true`), so the file was internally inconsistent: two guarded sites, two unguarded. test-detect-platform-outside-repo.sh asserts the MESSAGE, not merely a non-zero rc -- "rc != 0" passes on the broken build too, since 128 is also non-zero. RED on main (128:no for both functions), GREEN here (1:yes), with an in-repo control proving the functions still resolve normally. Enumerated on test:framework-shell. Found-by: be-coder-08, rev0 (both reported `blocked` per Gate 8 rather than working around it)	2026-08-06 18:56:03 -05:00

Author

SHA1

Message

Date

Mos

8e677d1f21

fix: stop re-escaping non-ASCII in package.json when enumerating a test

ci/woodpecker/pr/ci Pipeline was successful

Details

CI step `format` failed (pipeline 2255, exit 1) -- not on the test, on formatting.

I enumerated the new test by loading packages/mosaic/package.json with python's
json module and writing it back. json.dumps defaults to ensure_ascii=True, so the
em-dash in

    "description": "Mosaic agent framework — installation wizard and meta package"

was rewritten as —. Valid JSON, identical when parsed, and rejected by
`pnpm format:check`.

Enumeration is a TEXT edit now: read the file, splice the test into the
test:framework-shell string, write it back. A JSON round-trip rewrites the whole
document to serialiser defaults; the only safe edit to a formatted file is one
that touches the bytes it means to touch.

diff vs main is now exactly one line in each branch.

2026-08-06 19:11:44 -05:00

Mos

92a63e7027

test(tools/git): a skip that exits 0 is a pass -- refuse instead

ci/woodpecker/pr/ci Pipeline was canceled

Details

be-coder-07 (#1089 review 131) showed the precondition guard exited 0 when the
scratch dir turned out to be inside a git worktree:

    ( cd "$TMP" && git rev-parse --git-dir ) && { echo "SKIP"; exit 0; }

so pointing TMPDIR beneath a git worktree made this test PASS against unchanged
main. A skip that exits 0 is indistinguishable from a pass -- the same defect
this suite exists to catch, in the suite itself.

Now: set GIT_CEILING_DIRECTORIES to the PHYSICAL path (it is matched physically,
and /tmp is commonly a symlink, so a logical path silently disables the ceiling),
and if the outside-a-repo precondition still cannot be established, FAIL with an
explicit message rather than skipping.

Replaying be-coder-07's attack (TMPDIR beneath a worktree):
    before: fix rc=0, main rc=0   <- both "pass", the vulnerability
    after:  fix rc=1, main rc=1   <- both refuse; no false pass either way
Normal conditions are unchanged and still discriminate: fix rc=0, main rc=1.

The test refuses to report a result it cannot establish. That is weaker than
running under a hostile TMPDIR and strictly better than lying about it.

Reported-by: be-coder-07

2026-08-06 19:02:29 -05:00

Mos

b4b70675b6

fix(tools/git): detect-platform died silently outside a repo, taking every wrapper with it

ci/woodpecker/pr/ci Pipeline was canceled

Details

detect_platform() and get_repo_info() both already contained the correct error path:

    remote_url=$(git remote get-url origin 2>/dev/null)
    if [[ -z "$remote_url" ]]; then
        echo "error: not a git repository or no origin remote" >&2
        return 1
    fi

but that message was UNREACHABLE. Every wrapper in this directory runs `set -e`, and
outside a git repo the assignment itself returns git's 128, terminating the caller
before the -z check. The diagnostic was written and could never print.

Observed cost: `pr-review.sh -n N -r owner/repo -H host -a comment -c "..."` invoked
from a non-repo cwd exits 128 with NO stdout and NO stderr -- including when -r and -H
are supplied, the flags whose help text says "skips git-remote inference". The inference
runs first regardless. Two reviewer seats hit this tonight and correctly reported
`blocked` with no diagnostic available to report; one of them lost a full review.

Reproduced:
    cd /tmp && pr-review.sh -n 1086 -r mosaicstack/stack -H git.mosaicstack.dev \
        -a comment -c "x"          -> rc=128, 0 bytes of output
    same command from inside a repo -> rc=1, "Error: Comment required"

`|| true` matches the idiom already used at the other two call sites in this same file
(one `|| return 1`, one `|| true`), so the file was internally inconsistent: two guarded
sites, two unguarded.

test-detect-platform-outside-repo.sh asserts the MESSAGE, not merely a non-zero rc --
"rc != 0" passes on the broken build too, since 128 is also non-zero. RED on main
(128:no for both functions), GREEN here (1:yes), with an in-repo control proving the
functions still resolve normally. Enumerated on test:framework-shell.

Found-by: be-coder-08, rev0 (both reported `blocked` per Gate 8 rather than working around it)

2026-08-06 18:56:03 -05:00

fix(tools/git): detect-platform died silently outside a repo, taking every wrapper with it #1089