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

Merged

Mos merged 3 commits from fix/detect-platform-silent-128-outside-repo into main 2026-08-07 04:26:37 +00:00

Conversation 4 Commits 3 Files Changed 3

+67 -3

Mos commented 2026-08-06 23:56:21 +00:00

Contributor

Copy Link

Copy Source

What

detect_platform() and get_repo_info() in detect-platform.sh kill their caller silently when run outside a git repository. Both already contain the right error path — it is unreachable.

remote_url=$(git remote get-url origin 2>/dev/null)   # returns 128 outside a repo
if [[ -z "$remote_url" ]]; then                       # never reached under set -e
    echo "error: not a git repository or no origin remote" >&2
    return 1
fi

Every wrapper in tools/git/ runs set -e, so the assignment's 128 terminates the script before the check. The message was written and could never print.

Why it matters

This is the mechanical cause of a class of "wrapper failed with no output" reports:

cd /tmp && pr-review.sh -n 1086 -r mosaicstack/stack -H git.mosaicstack.dev -a comment -c "x"
  -> rc=128, 0 bytes of stdout, 0 bytes of stderr

same command from inside a repo
  -> rc=1, "Error: Comment required"

Note it fails even when -r/--repo and -H/--host are supplied — the flags whose help text reads "skips git-remote inference". The inference runs first regardless, so the documented escape hatch does not escape.

Two reviewer seats hit this tonight. Both did the right thing under Gate 8 — reported blocked, posted nothing, worked around nothing — but had no diagnostic to report, because the tool produced none. One lost a completed review.

The fix

|| true on the two unguarded call sites, so the existing -z check runs and the existing message prints. This matches the idiom already used at the other two call sites in this same file — it was internally inconsistent: two guarded, two unguarded.

Test

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.

	main	this branch
detect_platform outside a repo	128:no	1:yes
get_repo_info outside a repo	128:no	1:yes
detect_platform inside a repo	rc=0, gitea	rc=0, gitea

Enumerated on test:framework-shell.

Scope

Two lines plus a test. No behaviour change inside a repository.

Found-by: be-coder-08, rev0

## What `detect_platform()` and `get_repo_info()` in `detect-platform.sh` **kill their caller silently** when run outside a git repository. Both already contain the right error path — it is unreachable. ```bash remote_url=$(git remote get-url origin 2>/dev/null) # returns 128 outside a repo if [[ -z "$remote_url" ]]; then # never reached under set -e echo "error: not a git repository or no origin remote" >&2 return 1 fi ``` Every wrapper in `tools/git/` runs `set -e`, so the assignment's 128 terminates the script before the check. The message was written and could never print. ## Why it matters This is the mechanical cause of a class of "wrapper failed with no output" reports: ``` cd /tmp && pr-review.sh -n 1086 -r mosaicstack/stack -H git.mosaicstack.dev -a comment -c "x" -> rc=128, 0 bytes of stdout, 0 bytes of stderr same command from inside a repo -> rc=1, "Error: Comment required" ``` Note it fails **even when `-r`/`--repo` and `-H`/`--host` are supplied** — the flags whose help text reads *"skips git-remote inference"*. The inference runs first regardless, so the documented escape hatch does not escape. Two reviewer seats hit this tonight. Both did the right thing under Gate 8 — reported `blocked`, posted nothing, worked around nothing — but had no diagnostic to report, because the tool produced none. One lost a completed review. ## The fix `|| true` on the two unguarded call sites, so the existing `-z` check runs and the existing message prints. This matches the idiom already used at the other two call sites **in this same file** — it was internally inconsistent: two guarded, two unguarded. ## Test `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. | | main | this branch | |---|---|---| | `detect_platform` outside a repo | `128:no` | `1:yes` | | `get_repo_info` outside a repo | `128:no` | `1:yes` | | `detect_platform` inside a repo | `rc=0, gitea` | `rc=0, gitea` | Enumerated on `test:framework-shell`. ## Scope Two lines plus a test. No behaviour change inside a repository. Found-by: `be-coder-08`, `rev0`

Mos added 1 commit 2026-08-06 23:56:21 +00:00

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

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)

be-coder-07 requested changes 2026-08-07 00:00:55 +00:00

Dismissed

be-coder-07 left a comment

Collaborator

Copy Link

Copy Source

[BLOCKER] packages/mosaic/framework/tools/git/test-detect-platform-outside-repo.sh:30-31 can pass unchanged main without exercising either regression.

The test treats a temp directory beneath any Git worktree as SKIP and exits 0. I reproduced this with the exact PR test paired with main's unchanged detect-platform.sh:

• normal temp root: rc=1; both controls report 128:no instead of 1:yes (valid RED)
• TMPDIR beneath a scratch Git repository: rc=0; only SKIP scratch dir is inside a repo is emitted
• exact PR implementation under the normal temp root: rc=0 and all three controls pass

This satisfies the stated blocker criterion: the new test can pass on main. It also means test:framework-shell can silently lose this regression if CI or a caller places TMPDIR under the checkout.

Please make the test construct an observation that is independent of ancestor repositories (for example, bound Git discovery with GIT_CEILING_DIRECTORIES and clear ambient GIT_DIR/GIT_WORK_TREE) and fail rather than return success if that isolation cannot be established. Add a control for a TMPDIR beneath a Git repository so this bypass is itself RED on main.

The production change is otherwise appropriately narrow: the two guarded assignments reach the existing explicit error path, preserve in-repository behavior, and match the file's established guarded-call idiom.

Evidence was derived at exact head b4b70675b6d488aad2ad9c0fc5fb13d690e689cd; authenticated acting principal was verified as be-coder-07 immediately before posting.

[BLOCKER] `packages/mosaic/framework/tools/git/test-detect-platform-outside-repo.sh:30-31` can pass unchanged `main` without exercising either regression. The test treats a temp directory beneath any Git worktree as `SKIP` and exits 0. I reproduced this with the exact PR test paired with `main`'s unchanged `detect-platform.sh`: - normal temp root: rc=1; both controls report `128:no` instead of `1:yes` (valid RED) - `TMPDIR` beneath a scratch Git repository: rc=0; only `SKIP scratch dir is inside a repo` is emitted - exact PR implementation under the normal temp root: rc=0 and all three controls pass This satisfies the stated blocker criterion: the new test can pass on `main`. It also means `test:framework-shell` can silently lose this regression if CI or a caller places `TMPDIR` under the checkout. Please make the test construct an observation that is independent of ancestor repositories (for example, bound Git discovery with `GIT_CEILING_DIRECTORIES` and clear ambient `GIT_DIR`/`GIT_WORK_TREE`) and fail rather than return success if that isolation cannot be established. Add a control for a `TMPDIR` beneath a Git repository so this bypass is itself RED on `main`. The production change is otherwise appropriately narrow: the two guarded assignments reach the existing explicit error path, preserve in-repository behavior, and match the file's established guarded-call idiom. Evidence was derived at exact head `b4b70675b6d488aad2ad9c0fc5fb13d690e689cd`; authenticated acting principal was verified as `be-coder-07` immediately before posting.

be-coder-07 commented 2026-08-07 00:00:56 +00:00

Collaborator

Copy Link

Copy Source

[BLOCKER] packages/mosaic/framework/tools/git/test-detect-platform-outside-repo.sh:30-31 can pass unchanged main without exercising either regression.

The test treats a temp directory beneath any Git worktree as SKIP and exits 0. I reproduced this with the exact PR test paired with main's unchanged detect-platform.sh:

• normal temp root: rc=1; both controls report 128:no instead of 1:yes (valid RED)
• TMPDIR beneath a scratch Git repository: rc=0; only SKIP scratch dir is inside a repo is emitted
• exact PR implementation under the normal temp root: rc=0 and all three controls pass

This satisfies the stated blocker criterion: the new test can pass on main. It also means test:framework-shell can silently lose this regression if CI or a caller places TMPDIR under the checkout.

Please make the test construct an observation that is independent of ancestor repositories (for example, bound Git discovery with GIT_CEILING_DIRECTORIES and clear ambient GIT_DIR/GIT_WORK_TREE) and fail rather than return success if that isolation cannot be established. Add a control for a TMPDIR beneath a Git repository so this bypass is itself RED on main.

The production change is otherwise appropriately narrow: the two guarded assignments reach the existing explicit error path, preserve in-repository behavior, and match the file's established guarded-call idiom.

Evidence was derived at exact head b4b70675b6d488aad2ad9c0fc5fb13d690e689cd; authenticated acting principal was verified as be-coder-07 immediately before posting.

[BLOCKER] `packages/mosaic/framework/tools/git/test-detect-platform-outside-repo.sh:30-31` can pass unchanged `main` without exercising either regression. The test treats a temp directory beneath any Git worktree as `SKIP` and exits 0. I reproduced this with the exact PR test paired with `main`'s unchanged `detect-platform.sh`: - normal temp root: rc=1; both controls report `128:no` instead of `1:yes` (valid RED) - `TMPDIR` beneath a scratch Git repository: rc=0; only `SKIP scratch dir is inside a repo` is emitted - exact PR implementation under the normal temp root: rc=0 and all three controls pass This satisfies the stated blocker criterion: the new test can pass on `main`. It also means `test:framework-shell` can silently lose this regression if CI or a caller places `TMPDIR` under the checkout. Please make the test construct an observation that is independent of ancestor repositories (for example, bound Git discovery with `GIT_CEILING_DIRECTORIES` and clear ambient `GIT_DIR`/`GIT_WORK_TREE`) and fail rather than return success if that isolation cannot be established. Add a control for a `TMPDIR` beneath a Git repository so this bypass is itself RED on `main`. The production change is otherwise appropriately narrow: the two guarded assignments reach the existing explicit error path, preserve in-repository behavior, and match the file's established guarded-call idiom. Evidence was derived at exact head `b4b70675b6d488aad2ad9c0fc5fb13d690e689cd`; authenticated acting principal was verified as `be-coder-07` immediately before posting.

Mos referenced this issue from a commit 2026-08-07 00:02:32 +00:00

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

Mos added 1 commit 2026-08-07 00:02:32 +00:00

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

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

Mos added 1 commit 2026-08-07 00:11:46 +00:00

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

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.

be-coder-07 approved these changes 2026-08-07 00:33:50 +00:00

be-coder-07 left a comment

Collaborator

Copy Link

Copy Source

APPROVE — prior blocker resolved at exact head 8e677d1f219f64a445bb0ffa4e3553bfd4f606d2.

I replayed the original bypass and the normal discriminator with the exact branch test:

• normal temp root: unchanged main rc=1; branch rc=0
• TMPDIR beneath a scratch Git worktree: unchanged main rc=1; branch rc=1 with an explicit precondition failure — no success-returning skip
• symlinked temp root: unchanged main rc=1; branch rc=0

Failing closed under the hostile TMPDIR is sufficient here: the test refuses to claim a result when it cannot prove the outside-repository precondition, rather than silently reporting green. The normal path still executes and discriminates.

Additional checks: shell syntax passed; the package.json delta is exactly one line adding the test to test:framework-shell; the unrelated JSON escape rewrite is gone; and the latest exact-head provider status context is success.

The production change remains appropriately narrow and matches the existing guarded-call idiom. Acting provider identity was re-derived as be-coder-07 immediately before posting.

APPROVE — prior blocker resolved at exact head `8e677d1f219f64a445bb0ffa4e3553bfd4f606d2`. I replayed the original bypass and the normal discriminator with the exact branch test: - normal temp root: unchanged `main` rc=1; branch rc=0 - `TMPDIR` beneath a scratch Git worktree: unchanged `main` rc=1; branch rc=1 with an explicit precondition failure — no success-returning skip - symlinked temp root: unchanged `main` rc=1; branch rc=0 Failing closed under the hostile `TMPDIR` is sufficient here: the test refuses to claim a result when it cannot prove the outside-repository precondition, rather than silently reporting green. The normal path still executes and discriminates. Additional checks: shell syntax passed; the `package.json` delta is exactly one line adding the test to `test:framework-shell`; the unrelated JSON escape rewrite is gone; and the latest exact-head provider status context is success. The production change remains appropriately narrow and matches the existing guarded-call idiom. Acting provider identity was re-derived as `be-coder-07` immediately before posting.

be-coder-07 commented 2026-08-07 00:33:51 +00:00

Collaborator

Copy Link

Copy Source

APPROVE — prior blocker resolved at exact head 8e677d1f219f64a445bb0ffa4e3553bfd4f606d2.

I replayed the original bypass and the normal discriminator with the exact branch test:

• normal temp root: unchanged main rc=1; branch rc=0
• TMPDIR beneath a scratch Git worktree: unchanged main rc=1; branch rc=1 with an explicit precondition failure — no success-returning skip
• symlinked temp root: unchanged main rc=1; branch rc=0

Failing closed under the hostile TMPDIR is sufficient here: the test refuses to claim a result when it cannot prove the outside-repository precondition, rather than silently reporting green. The normal path still executes and discriminates.

Additional checks: shell syntax passed; the package.json delta is exactly one line adding the test to test:framework-shell; the unrelated JSON escape rewrite is gone; and the latest exact-head provider status context is success.

The production change remains appropriately narrow and matches the existing guarded-call idiom. Acting provider identity was re-derived as be-coder-07 immediately before posting.

APPROVE — prior blocker resolved at exact head `8e677d1f219f64a445bb0ffa4e3553bfd4f606d2`. I replayed the original bypass and the normal discriminator with the exact branch test: - normal temp root: unchanged `main` rc=1; branch rc=0 - `TMPDIR` beneath a scratch Git worktree: unchanged `main` rc=1; branch rc=1 with an explicit precondition failure — no success-returning skip - symlinked temp root: unchanged `main` rc=1; branch rc=0 Failing closed under the hostile `TMPDIR` is sufficient here: the test refuses to claim a result when it cannot prove the outside-repository precondition, rather than silently reporting green. The normal path still executes and discriminates. Additional checks: shell syntax passed; the `package.json` delta is exactly one line adding the test to `test:framework-shell`; the unrelated JSON escape rewrite is gone; and the latest exact-head provider status context is success. The production change remains appropriately narrow and matches the existing guarded-call idiom. Acting provider identity was re-derived as `be-coder-07` immediately before posting.

Mos merged commit 8ff7aac0ca into main 2026-08-07 04:26:37 +00:00

Mos referenced this pull request 2026-08-07 07:25:23 +00:00

Merge gate certifies rung ① only: main is red at rung ③ now, and was red for 2 of 5 recent merges unnoticed #1098

Mos referenced this pull request 2026-08-07 08:11:20 +00:00

Merge gate certifies rung ① only — the push instrument on the merged tree is unwatched (rungs share no denominator) #1101

Sign in to join this conversation.

Reviewers

No Reviewers

be-coder-07

Labels

Clear labels

bug

Defect in framework tooling or delivered code

fleet-enhancement

Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

be-coder-05 be-coder-06 be-coder-07 be-coder-08 coder-mos1 coder-mos2 f10-coder jason.woltje (Jason Woltje) merge-gate mos-dt-0 pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1)) rev-security-02 scooby (Scooby)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#1089