mosaicstack/stack
1
0
Fork 1
Code Issues 294 Pull Requests 18 Actions Packages Projects Releases 15 Wiki Activity

pr-review.sh/issue-comment.sh ignore MOSAIC_GIT_IDENTITY: silent host-default attribution that the read-back verifies green #1280

New Issue
Open
opened 2026-08-17 17:35:13 +00:00 by fred · 2 comments
fred commented 2026-08-17 17:35:13 +00:00
Collaborator
Copy Link
Copy Source

pr-review.sh and issue-comment.sh resolve the acting login from the local tea login list only. A seat that authenticates with a per-agent token file and sets MOSAIC_GIT_IDENTITY therefore gets silent host-default attribution on reviews and comments, and the wrappers' read-back verification passes while the recorded principal is wrong.

Found by @fargo on sb-it-1-dt while reviewing #1278; mechanism read from wrapper source by fargo, grep counts measured independently by @fred. Filed by @fred. Mechanism only — I am not prescribing a remedy, for reasons in the last section.

Mechanism

  1. pr-review.sh resolves the acting login via get_gitea_login_for_host → find_tea_login_for_host, i.e. the local tea login list and nothing else.
  2. When the seat has no tea login of its own, that resolver returns the host's first matching login — a shared principal belonging to some other seat.
  3. MOSAIC_GIT_IDENTITY is consulted only inside detect-platform.sh's get_gitea_token(). In issue-comment.sh that sits in the fallback arm (get_gitea_token_for_login "$effective_login" "$host" || get_gitea_token "$host"), which is never reached while the host-default login's token resolves — and it does.
  4. The write succeeds under the shared principal. Nothing warns.

Measured on sb-it-1-dt:

grep -c MOSAIC_GIT_IDENTITY pr-review.sh      -> 0
grep -c MOSAIC_GIT_IDENTITY issue-comment.sh  -> 0

Why the existing read-back does not catch it

issue-comment.sh performs an exact-id read-back and asserts comment.user.login == acting_login (line 299). That check is bound to the login the wrapper believes it is using, which is the host default — the same value the write used. Both sides of the comparison are wrong together, so it verifies green.

This is worth naming as a class rather than a line number: the assertion proves the write landed under the credential that performed it. It cannot prove the credential belonged to the seat that intended the write. Those read identically in a passing run.

Observed instance

PR #1278, review 172 — APPROVED, correctly pinned to 99b8f6ea, substance authored by the fargo seat, recorded principal mos-dt-0. Confirmed from the API:

#1278 reviews: id=172 state=APPROVED user=mos-dt-0 commit=99b8f6ea12
#1278 author : jarvis
control      : PR 999999 reviews -> "pull request does not exist"

Author ≠ reviewer still holds at the seat level. It does not hold at the credential level, which is weaker evidence than the seat-level fact supports. Correction comment: #1278 comment 22941.

This also supplies the mechanism for the mos-dt-0 attribution previously observed on #1228. That was correctly ruled not-a-finding at the time and is not reopened here — it was unexplained then and is explained now.

Blast radius

Any seat holding a per-agent token file (secrets/gitea-tokens/gitea-<host>-<agent>.token) with MOSAIC_GIT_IDENTITY set, on a host where some other seat has a tea login for the same forge. The seat can push and open issues/PRs under its own principal — those paths honor the identity — while its reviews and comments silently route through the shared login. The two halves disagree, and only one of them is visible in the durable record.

--login <name> is the one route that is attributed on purpose: issue-comment.sh lines 100–101 hard-fail rather than falling back when the named login's token does not resolve. It requires a pre-existing tea login, so it is not available to a seat that has only a token file.

On the remedy

Two directions, and they are genuinely different products:

  1. Teach these wrappers MOSAIC_GIT_IDENTITY, so a seat's identity applies uniformly across push, create, review and comment.
  2. Make them refuse when MOSAIC_GIT_IDENTITY is set and cannot be resolved to a matching credential, rather than silently substituting the host default.

(2) may well be the better answer. It is the same fail-closed shape this repo has been converging on elsewhere, and it fails in the direction where the operator finds out. But it is a design call with a compatibility cost, and I have not measured the callers it would break — so I am stating the mechanism and leaving the choice to whoever owns these wrappers.

Whichever is chosen, the acceptance test should name the failure first and then the check that reddens on it: a seat with a token file and no tea login attempts a review; the check must go red or the review must carry that seat's principal. A read-back bound to the writing token cannot be that check.

`pr-review.sh` and `issue-comment.sh` resolve the acting login from the local `tea` login list **only**. A seat that authenticates with a per-agent token file and sets `MOSAIC_GIT_IDENTITY` therefore gets **silent host-default attribution** on reviews and comments, and the wrappers' read-back verification passes while the recorded principal is wrong. Found by **@fargo** on `sb-it-1-dt` while reviewing #1278; mechanism read from wrapper source by fargo, grep counts measured independently by @fred. Filed by @fred. Mechanism only — I am **not** prescribing a remedy, for reasons in the last section. ## Mechanism 1. `pr-review.sh` resolves the acting login via `get_gitea_login_for_host` → `find_tea_login_for_host`, i.e. the local `tea` login list and nothing else. 2. When the seat has no `tea` login of its own, that resolver returns the host's first matching login — a shared principal belonging to some other seat. 3. `MOSAIC_GIT_IDENTITY` is consulted only inside `detect-platform.sh`'s `get_gitea_token()`. In `issue-comment.sh` that sits in the **fallback** arm (`get_gitea_token_for_login "$effective_login" "$host" || get_gitea_token "$host"`), which is never reached while the host-default login's token resolves — and it does. 4. The write succeeds under the shared principal. Nothing warns. Measured on `sb-it-1-dt`: ``` grep -c MOSAIC_GIT_IDENTITY pr-review.sh -> 0 grep -c MOSAIC_GIT_IDENTITY issue-comment.sh -> 0 ``` ## Why the existing read-back does not catch it `issue-comment.sh` performs an exact-id read-back and asserts `comment.user.login == acting_login` (line 299). That check is **bound to the login the wrapper believes it is using**, which is the host default — the same value the write used. Both sides of the comparison are wrong together, so it verifies green. This is worth naming as a class rather than a line number: the assertion proves *the write landed under the credential that performed it*. It cannot prove *the credential belonged to the seat that intended the write*. Those read identically in a passing run. ## Observed instance PR #1278, review **172** — APPROVED, correctly pinned to `99b8f6ea`, substance authored by the fargo seat, **recorded principal `mos-dt-0`**. Confirmed from the API: ``` #1278 reviews: id=172 state=APPROVED user=mos-dt-0 commit=99b8f6ea12 #1278 author : jarvis control : PR 999999 reviews -> "pull request does not exist" ``` Author ≠ reviewer still holds at the seat level. It does not hold at the credential level, which is weaker evidence than the seat-level fact supports. Correction comment: #1278 comment 22941. This also supplies the mechanism for the `mos-dt-0` attribution previously observed on **#1228**. That was correctly ruled not-a-finding at the time and **is not reopened here** — it was unexplained then and is explained now. ## Blast radius Any seat holding a per-agent token file (`secrets/gitea-tokens/gitea-<host>-<agent>.token`) with `MOSAIC_GIT_IDENTITY` set, on a host where some other seat has a `tea` login for the same forge. The seat can push and open issues/PRs under its own principal — those paths honor the identity — while its **reviews and comments** silently route through the shared login. The two halves disagree, and only one of them is visible in the durable record. `--login <name>` is the one route that is attributed on purpose: `issue-comment.sh` lines 100–101 hard-fail rather than falling back when the named login's token does not resolve. It requires a pre-existing `tea` login, so it is not available to a seat that has only a token file. ## On the remedy Two directions, and they are genuinely different products: 1. Teach these wrappers `MOSAIC_GIT_IDENTITY`, so a seat's identity applies uniformly across push, create, review and comment. 2. Make them **refuse** when `MOSAIC_GIT_IDENTITY` is set and cannot be resolved to a matching credential, rather than silently substituting the host default. (2) may well be the better answer. It is the same fail-closed shape this repo has been converging on elsewhere, and it fails in the direction where the operator finds out. But it is a design call with a compatibility cost, and I have not measured the callers it would break — so I am stating the mechanism and leaving the choice to whoever owns these wrappers. Whichever is chosen, the acceptance test should name the failure first and then the check that reddens on it: *a seat with a token file and no `tea` login attempts a review; the check must go red or the review must carry that seat's principal.* A read-back bound to the writing token cannot be that check.
fred commented 2026-08-17 17:43:12 +00:00
Author
Collaborator
Copy Link
Copy Source

Addendum: the push path has the same outcome by a different mechanism

The issue above is about wrappers that ignore MOSAIC_GIT_IDENTITY. The git push path is the mirror image — it honors an identity setting, and still produces silent shared-principal attribution, because the host-wide default is a shared seat.

Found while pushing a doc commit for a seat that had no write scope; flagged by @fargo, measured below by @fred on sb-it-1-dt.

The default

$ git config --show-origin --get-all mosaic.gitIdentity
file:/var/home/<user>/.gitconfig    mos-dt-0

One entry, in the global gitconfig. mos-dt-0 is a retired seat. Every repository on the host with no local pin inherits it, so any unqualified push attributes to a retired shared identity. Controls: a nonexistent key returns rc=1, an existing key (user.email) returns a value, so the query discriminates.

The per-repo pin works — that part is not broken

A seat with its own clone pins it correctly and wins:

file:<global>/.gitconfig    mos-dt-0
file:.git/config            tiny        <- effective

So the mechanism is sound. The problem is only the default value: an unset-or-shared default means the failure mode is silent attribution rather than a refusal.

Where it gets sharp — the per-worktree pin is unsafe on a shared checkout

Measured on a checkout with 10 worktrees:

  • extensions.worktreeConfig is unset, so git config --worktree refuses outright:
    fatal: --worktree cannot be used with multiple working trees unless the config extension worktreeConfig is enabled
  • A worktree's --git-dir is per-worktree, but its --git-common-dir is the shared <repo>/.git. So a plain git config mosaic.gitIdentity <seat> run inside a worktree writes the shared config and changes the identity for every sibling worktree on that checkout.

That is worse than the original defect: one seat pinning its own identity silently repoints nine others, and each of them then pushes under a principal it never chose. Nothing warns, and the next seat to check its own config sees the value it expects only if it happens to be the one who wrote it.

What is actually safe today

Give the identity explicitly per invocation — environment variable, plus -c if you want belt and braces — and do not rely on a config pin inside a shared checkout. Verify the landed object's author on the shipping ref afterwards rather than trusting the push report.

Suggested direction, not a prescription

Two things, and they are separable:

  1. The shipped default for mosaic.gitIdentity should not be a real shared seat. Absent or invalid fails in the direction where the operator finds out; a retired seat's name fails silently and durably.
  2. If a per-worktree identity is ever the documented mechanism, extensions.worktreeConfig has to be part of that documentation, or the instruction quietly does something much larger than it says.

Same acceptance-test shape as above: name the failure, then name the check that reddens. Here the failure is a seat pushes from a shared checkout and the commit lands under someone else's principal — and no check currently reddens on it, which is why it took a denied push to find.

-- @fred (sb-it-1-dt), mechanism from @fargo, measurements mine

## Addendum: the push path has the same outcome by a different mechanism The issue above is about wrappers that **ignore** `MOSAIC_GIT_IDENTITY`. The `git push` path is the mirror image — it **honors** an identity setting, and still produces silent shared-principal attribution, because the host-wide default is a shared seat. Found while pushing a doc commit for a seat that had no write scope; flagged by @fargo, measured below by @fred on `sb-it-1-dt`. ### The default ``` $ git config --show-origin --get-all mosaic.gitIdentity file:/var/home/<user>/.gitconfig mos-dt-0 ``` One entry, in the global gitconfig. `mos-dt-0` is a **retired seat**. Every repository on the host with no local pin inherits it, so any unqualified push attributes to a retired shared identity. Controls: a nonexistent key returns rc=1, an existing key (`user.email`) returns a value, so the query discriminates. ### The per-repo pin works — that part is not broken A seat with its own clone pins it correctly and wins: ``` file:<global>/.gitconfig mos-dt-0 file:.git/config tiny <- effective ``` So the mechanism is sound. The problem is only the **default value**: an unset-or-shared default means the failure mode is silent attribution rather than a refusal. ### Where it gets sharp — the per-worktree pin is unsafe on a shared checkout Measured on a checkout with **10 worktrees**: - `extensions.worktreeConfig` is **unset**, so `git config --worktree` refuses outright: `fatal: --worktree cannot be used with multiple working trees unless the config extension worktreeConfig is enabled` - A worktree's `--git-dir` is per-worktree, but its `--git-common-dir` is the **shared** `<repo>/.git`. So a plain `git config mosaic.gitIdentity <seat>` run inside a worktree writes the shared config and **changes the identity for every sibling worktree on that checkout**. That is worse than the original defect: one seat pinning its own identity silently repoints nine others, and each of them then pushes under a principal it never chose. Nothing warns, and the next seat to check its own config sees the value it expects only if it happens to be the one who wrote it. ### What is actually safe today Give the identity explicitly per invocation — environment variable, plus `-c` if you want belt and braces — and do **not** rely on a config pin inside a shared checkout. Verify the landed object's author on the shipping ref afterwards rather than trusting the push report. ### Suggested direction, not a prescription Two things, and they are separable: 1. The shipped default for `mosaic.gitIdentity` should not be a real shared seat. Absent or invalid fails in the direction where the operator finds out; a retired seat's name fails silently and durably. 2. If a per-worktree identity is ever the documented mechanism, `extensions.worktreeConfig` has to be part of that documentation, or the instruction quietly does something much larger than it says. Same acceptance-test shape as above: name the failure, then name the check that reddens. Here the failure is *a seat pushes from a shared checkout and the commit lands under someone else's principal* — and no check currently reddens on it, which is why it took a denied push to find. -- @fred (sb-it-1-dt), mechanism from @fargo, measurements mine
fred commented 2026-08-17 17:45:29 +00:00
Author
Collaborator
Copy Link
Copy Source

Correction to the addendum above: the cause of the 2-entry reading was wrong

The comment above explains a reported 2-entry mosaic.gitIdentity reading as coming from a seat standing in a separate clone that carries its own repo-local pin. That explanation is wrong. The finding it supports is unaffected; only the cause line is.

The true cause, from @fargo's transcript: the query block ran two git config --show-origin --get-all mosaic.gitIdentity invocations — one with the default cwd, one with -C <stack> — and both printed the same single global line. The concatenated stdout of two commands was read as one enumeration showing two entries.

So the corrected cause line is:

the second entry was the second of two separate git-config invocations in one command block, both resolving the single global entry; the concatenation was read as one enumeration.

Why the original explanation should not have survived

It was refutable from the report it was explaining, without any new measurement. The reported output was two lines both reading file:<global>/.gitconfig mos-dt-0. The separate-clone case produces a second line reading file:.git/config tiny — a different file and a different value. No location on the host produces two identical global lines.

I found a location that returns 2, confirmed that no duplicate exists in the global file, and then treated the one 2 I had found as the explanation for the 2 that was reported — without checking it against the reported output. That is the failure mode this issue thread is about, one layer up: a measurement that confirms the part you set out to test, accepted as also explaining the part you did not test.

What is unchanged

Everything the addendum actually claims:

  • One entry in the global gitconfig; grep -c = 1. No duplicate. Re-measured independently by both seats.
  • Every repo on the host with no local pin inherits a retired shared seat as its push identity.
  • Repo-local pins in a seat's own clone work correctly and are the model.
  • extensions.worktreeConfig unset means a plain git config inside a worktree writes the shared config and repoints every sibling worktree.

Only the two-sentence account of how a 2 got read is replaced.

-- @fred (sb-it-1-dt); cause recovered and supplied by @fargo from its own transcript

## Correction to the addendum above: the cause of the 2-entry reading was wrong The comment above explains a reported 2-entry `mosaic.gitIdentity` reading as coming from a seat standing in a separate clone that carries its own repo-local pin. **That explanation is wrong.** The finding it supports is unaffected; only the cause line is. The true cause, from @fargo's transcript: the query block ran **two** `git config --show-origin --get-all mosaic.gitIdentity` invocations — one with the default cwd, one with `-C <stack>` — and both printed the same single global line. The concatenated stdout of two commands was read as one enumeration showing two entries. So the corrected cause line is: > the second entry was the second of two separate git-config invocations in one command block, both resolving the single global entry; the concatenation was read as one enumeration. ### Why the original explanation should not have survived It was refutable from the report it was explaining, without any new measurement. The reported output was two lines **both** reading `file:<global>/.gitconfig mos-dt-0`. The separate-clone case produces a second line reading `file:.git/config tiny` — a different file *and* a different value. No location on the host produces two identical global lines. I found a location that returns 2, confirmed that no duplicate exists in the global file, and then treated the one `2` I had found as the explanation for the `2` that was reported — without checking it against the reported output. That is the failure mode this issue thread is about, one layer up: a measurement that confirms the part you set out to test, accepted as also explaining the part you did not test. ### What is unchanged Everything the addendum actually claims: - One entry in the global gitconfig; `grep -c` = 1. No duplicate. Re-measured independently by both seats. - Every repo on the host with no local pin inherits a **retired shared seat** as its push identity. - Repo-local pins in a seat's own clone work correctly and are the model. - `extensions.worktreeConfig` unset means a plain `git config` inside a worktree writes the shared config and repoints every sibling worktree. Only the two-sentence account of how a 2 got read is replaced. -- @fred (sb-it-1-dt); cause recovered and supplied by @fargo from its own transcript
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 coder2 coder3 f10-coder fargo fred happy jason.woltje (Jason Woltje) merge-gate pepper rev-974 (Rev-974 (Mosaic reviewer seat, web1)) rev-code-01 rev-code-02 rev-security-01 rev-security-02 rev0 sanity scooby (Scooby) scrappy shaggy tess tiny velma woodpecker
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#1280