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

guides: address rev-code-01's review of #1313 (B1, B2, S1, S2)
ci/woodpecker/pr/ci Pipeline was successful
Details

Browse Source 
All four findings reproduced before fixing. rev-code-01 was right on each.

B2 (blocker, mine). SEAT-IDENTITY provisioning step 4 said to symlink the
framework store entry to the seat slot, while the same file says those bridges
must not be recreated. The same bridge, told both ways, in one document. I
rewrote the resolution and token-location sections when the deploy made them
stale and did not carry the change into the numbered steps. Step 4 is gone and
the file now says explicitly that no provisioning step links the store to the
slot, so the omission cannot read as an oversight.

S1 (mine). The guide claimed the helper "attempts a fleet notification" on
refusal. The shipped helper does no such thing — its only reference to
notification is a comment saying an alert built on the record is best-effort, and
there is no send or wake call anywhere in the file. Now: it writes a durable
record, the record is what exists, and nobody should wait for a notification that
nothing sends. A guide that promises an alert is worse than one that promises
nothing.

S2. Estate-local content removed from files that ship to every estate: the
~/.mosaic/fleet/bin script paths (dead paths elsewhere) and the 2026-08-18 dates,
which dated a specific host's migration rather than describing behavior. The
bridge-removal passage now states the ORDERING that matters — remove bridges only
after a seat-aware helper can reach the slot, never before — which is the part
that transfers.

B1. prettier reformatted all three files. Reproduced the pipeline 2515 failure
locally before and confirmed clean after; the other three guides prettier flags
are untouched by this branch (0 changes vs origin/next) and are pre-existing.

Sanitization gate re-run and passing.

Verified for the record, since I could not verify my own work: rev-code-01
confirmed the no-fallback claim TRUE against helper content on origin/next, and
judged the evidence rules actionable on the grounds that each names an executable
replacement.
This commit is contained in:
fred
2026-08-18 18:51:15 -05:00
parent a3c50d91ca
commit 3884f2de4d
3 changed files with 35 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/mosaic/framework/guides/CODE-REVIEW.md
+5 -6
View File
@@ -48,7 +48,7 @@ you saw. Every rule here was earned by a wrong conclusion that reached a report.
### Measuring a shell suite
Earned on mosaicstack#1311, 2026-08-18. Each of these produced a wrong conclusion first.
Each of these produced a wrong conclusion before it was written down.
9. **`cmd | tail; echo rc=$?` reports `tail`'s exit code, not `cmd`'s.** It reads as a pass when
the command failed. Redirect to a file and check `rc` directly, or use `${PIPESTATUS[0]}`.
@@ -78,16 +78,15 @@ Reviewer seats split this checklist by class rather than duplicating it. A seat
sections in full and may raise anything it notices outside them as a Suggestion, never as a
Blocker on someone else's ground.
| Reviewer class | Owns |
|---|---|
| `rev-code-*` | 1 Correctness, 3 Testing, 4 Code Quality, 4a TypeScript, 5 Documentation, 6 Performance, 7 Dependencies |
| `rev-security-*` | 2 Security, 2a OWASP |
| Reviewer class | Owns |
| ---------------- | ------------------------------------------------------------------------------------------------------- |
| `rev-code-*` | 1 Correctness, 3 Testing, 4 Code Quality, 4a TypeScript, 5 Documentation, 6 Performance, 7 Dependencies |
| `rev-security-*` | 2 Security, 2a OWASP |
Where two seats of the same class review the same change, they review independently and compare
after. A second seat that reads the first seat's findings before measuring is a proofreader, not
a second opinion.
### 1. Correctness
- [ ] Code does what the issue/PR description says
packages/mosaic/framework/guides/FLEET-COMMS.md
+14 -14
View File
@@ -10,7 +10,7 @@ is unavailable, or where the roster is stale, this guide is the substitute.
`fleet/roster.yaml` is a declaration of intent, not an observation. It routinely names a socket
that was never created, lists seats that are not running, and omits seats that are — this was
measured on a live host on 2026-08-18 and all three were true at once. Find out what is actually
all three have been observed true at once on a live host. Find out what is actually
up before addressing anyone:
```bash
@@ -35,13 +35,13 @@ Use the **default socket**. Do not pass `-L mosaic-fleet` on the strength of the
`-C` takes exactly one of these. Anything else exits 3.
| Class | Use for |
|---|---|
| `terminal-log` | log only; never needs the agent's attention |
| `actionable` | a decision, blocker, gate, or question needing an answer |
| `human` | relayed from a human operator |
| `reaction` | an ack or acknowledgement token |
| `digest` | machine wake, coalescible |
| Class | Use for |
| -------------- | -------------------------------------------------------- |
| `terminal-log` | log only; never needs the agent's attention |
| `actionable` | a decision, blocker, gate, or question needing an answer |
| `human` | relayed from a human operator |
| `reaction` | an ack or acknowledgement token |
| `digest` | machine wake, coalescible |
An absent class is treated as `actionable` by consumers, which is the fail-safe direction. Prefer
naming it anyway.
@@ -53,12 +53,12 @@ sends, it does not auto-reply.
### Exit codes
| rc | Meaning |
|---|---|
| 0 | delivered or queued |
| 1 | target session not found |
| 2 | text reached the pane but is **still a draft** |
| 3 | usage error (bad class, missing `-s`) |
| rc | Meaning |
| --- | ---------------------------------------------- |
| 0 | delivered or queued |
| 1 | target session not found |
| 2 | text reached the pane but is **still a draft** |
| 3 | usage error (bad class, missing `-s`) |
**Never retry on rc=2.** The message is in the target pane; retrying double-sends it. Confirm
instead:
packages/mosaic/framework/guides/SEAT-IDENTITY.md
+16 -10
View File
@@ -14,7 +14,7 @@ copy is drift, and drift surfaces as the stale copy returning 401 — which read
token and sends whoever debugs it somewhere else entirely.
A credential refusal is correct behavior, not a bug to route around. If git refuses with a
fail-closed diagnostic, the fix is to provision or correct *your* identity. Escalate; do not
fail-closed diagnostic, the fix is to provision or correct _your_ identity. Escalate; do not
substitute.
## How a credential is resolved
@@ -22,7 +22,7 @@ substitute.
Find the helper the way **git** does, not with `command -v`. Git runs whatever
`credential.helper` names, and on a Mosaic host that is an absolute path — so a PATH lookup
answers a different question and the two disagree the moment the PATH copy is removed. It was
removed here on 2026-08-18.
removed on hosts that have completed that migration.
```bash
git config --get-all credential.helper # every helper, in the order git tries them
@@ -31,8 +31,8 @@ git config --get-all credential.helper # every helper, in the order git trie
Git tries **each** configured helper in turn until one supplies a credential. A fail-closed
helper supplies nothing, so a second helper configured behind it silently becomes the one that
answers. When you care which binary serves a credential, read the whole list.
`~/.mosaic/fleet/bin/lib-credential-helper.sh` does this correctly and handles all three forms
git accepts (absolute path, `!command`, bare name resolved on PATH).
Resolve all three forms git accepts — absolute path, `!command`, and a bare name looked up on
PATH — not just the one your host happens to use.
The helper resolves the identity in this order:
@@ -60,8 +60,10 @@ of the seat directory decides it. A seat that has a directory and an empty slot
does not reach the service store. That is the intended behavior — the alternative is an agent
silently acting as somebody else.
If the file is unreadable the helper **fails closed**: it refuses, spools a record, and attempts
a fleet notification. It does not fall back to a shared account. That fallback is what made
If the file is unreadable the helper **fails closed**: it refuses and writes a durable record to
the escalation spool. It does not fall back to a shared account. The record is what exists — any
alerting built on top of it is a separate, best-effort concern and is not performed by the helper,
so do not wait for a notification that nothing sends. That fallback is what made
`usc/uconnect#3084` unattributable, and it was removed deliberately.
Verify the helper you actually have:
@@ -84,8 +86,9 @@ The framework store at `~/.config/mosaic/secrets/gitea-tokens/` holds tokens for
identities only** — identities with no seat directory. A seat's token does not belong there.
Before mosaicstack#1311 the deployed helper knew only the service store, and seats were bridged
with a symlink from the store into the slot. **Those bridges are gone** (removed 2026-08-18 by
`~/.mosaic/fleet/bin/migrate-credentials-to-seat-slots.sh`) and must not be recreated. A symlink
with a symlink from the store into the slot. **Those bridges must be removed once a seat-aware helper is deployed, and must not be
recreated.** Remove them only after the helper can reach the slot without them; the reverse order
takes every seat offline. A symlink
is not how a system finds a credential; the helper resolving the right store is.
`.principal` and `.scopes` beside the token are grant records, not secrets. They are tracked. The
@@ -97,10 +100,13 @@ is not how a system finds a credential; the helper resolving the right store is.
2. Write `.principal` (the Gitea login) and `.scopes` (the granted scopes), mode 600.
3. The estate operator mints the token into the seat slot, mode 600. Agents do not mint their
own, and do not ask another agent to mint one for them.
4. Symlink the framework store entry to the seat slot.
5. Verify with an authenticated `GET /user` and confirm the returned login is the seat, **not the
4. Verify with an authenticated `GET /user` and confirm the returned login is the seat, **not the
minting account**. Record the date in `ENTITY.md`. Never record the value.
There is no step that links the framework store to the slot. A seat-aware helper reads the slot
directly; a store entry pointing at a slot is the bridge described below, and it is not part of
provisioning.
Until step 3, the seat is unminted and its git writes fail closed. That is the designed state and
is safe to launch in — the seat is told at launch so it does not discover it mid-task.