docs(amd1213-d): record measured defect state, residuals, and remaining D3 environment half
Replaces notes that stopped on 2026-08-13 and understated progress by roughly two defects. Every row was re-measured against the tree rather than inherited: D1, D2, D4 and D6 read as substantially implemented; D3 and D5 closed this pass. Records the three things most likely to be lost or undone: that D2's alias defence is strictness rather than normalization and breaks if someone normalizes the link first; that D3 leaves two named residuals (the validation-to-exec window Node cannot close, and launch-runtime.py re-resolving the binary for claude/pi); and that the remaining D3 work is the measured child environment, not the executable resolution already done. Also records why the full suite needs the sanitized lease env, and the identity blocker that keeps this commit-only regardless of when the hold lifts.
This commit is contained in:
@@ -0,0 +1,125 @@
|
||||
# AMD1213-D — transaction and helper trust remediation
|
||||
|
||||
- **Task:** AMD1213-D (issue #1213 amendment; controlling packet `comms/20260813T212447Z__from-scrappy__dc43de.md`)
|
||||
- **Objective:** Address D1–D6 on local `feat/wf-fleet-mvp`, commit-only. Never push or re-author.
|
||||
- **Scope:** Existing C-fence production/tests only. No provider calls.
|
||||
- **Standing constraint:** AMEND/HOLD. Do not push, do not update PR #1213, do not merge, do not
|
||||
re-author. PR #1216 remains independently held for Jason.
|
||||
|
||||
## Where this actually stands (measured 2026-08-15, not inherited from notes)
|
||||
|
||||
Everything below was re-measured against the tree rather than trusted from the previous entries,
|
||||
which understated progress by roughly two defects. Branch head `b91b702a`.
|
||||
|
||||
| Defect | State | Evidence |
|
||||
|---|---|---|
|
||||
| D1 transactional rollback | Substantially implemented | `fleet-launch-command.ts` snapshots links before mutating, prepares the manifest, and has `injectFailure` seams at `prepare-manifest`, `write-manifest`, `close-manifest`, `rename-manifest`; restores `manifestLinksBefore` on failure |
|
||||
| D2 exact managed-link classes | Substantially implemented | classifier at `fleet-launch-command.ts:598-663`: exact resolved credential, direct one-component plugin/skill only, symlink target and ancestor rejected, `realpath` containment, duplicates rejected |
|
||||
| D3 ambient-PATH executable resolution | **Closed this pass** (commit `585dac7a`) | see below |
|
||||
| D4 config check/apply safety | Substantially implemented | `secure_dir` ancestor checks, `read_private` with `O_NOFOLLOW` + fstat, apply via mkstemp + fchmod 0600 + fsync + dev/ino re-check before `os.replace`, compatibility path separated |
|
||||
| D5 bounded test seam | **Closed this pass** (commit `b91b702a`) | see below |
|
||||
| D6 validate-by-path then exec-by-path | Substantially implemented | helper runs as a verified snapshot piped to `bash -s`, not executed by pathname; the same binding applied to the runtime in D3 |
|
||||
|
||||
### D2 — one thing worth recording so it is not "fixed" later
|
||||
|
||||
The alias concern in the packet (`duplicates/normalization aliases`) is closed by strictness, not
|
||||
by normalization. `dirname(link)` is compared literally against the seat root, so `/s/plugins//foo`
|
||||
(`dirname` → `/s/plugins/`), `/s/plugins/./foo` and `/s/plugins/bar/../foo` all fail the comparison
|
||||
and are rejected. Verified by direct measurement of `path.dirname` on each form. Anyone who
|
||||
"improves" this by normalizing the link first would open the alias hole the strict comparison
|
||||
currently closes.
|
||||
|
||||
### D3 — what was wrong and what was done
|
||||
|
||||
The launcher asked `which` whether a runtime was reachable and then spawned the **bare name**,
|
||||
letting the OS resolve it a second time against an ambient PATH at a later moment. Two independent
|
||||
resolutions of an attacker-influenced name with a gap between them.
|
||||
|
||||
Demonstrated against the old code before changing it — a world-writable `codex` shim prepended to
|
||||
PATH:
|
||||
|
||||
```
|
||||
OLD checkRuntime -> PASSED (which found it)
|
||||
OLD execRuntime -> "SHIM EXECUTED — this is not the real runtime"
|
||||
```
|
||||
|
||||
Three exposed call sites, not one: `checkRuntime`'s `which`; `execRuntime` spawning
|
||||
`codex`/`opencode` by name; and `execLeaseGatedRuntime` spawning `python3` by name — the
|
||||
interpreter that starts the lease gate, where a shim replaces the process that enforces every other
|
||||
check. `minimalLaunchEnv` copies ambient PATH straight through.
|
||||
|
||||
Fix: `resolveExecutableFromPath` searches only the PATH the child will actually receive, validates
|
||||
what the search lands on (regular file, executable, not group/other-writable, owned by the
|
||||
launching user or root, no group/world-writable non-sticky directory and no foreign-owned directory
|
||||
on the resolved path), and returns that path pinned to dev/ino. Callers execute the returned path
|
||||
and never the name again. The fleet lease-gate interpreter comes from the root-owned
|
||||
`trustedCapability('python3')`. `checkRuntime` is deliberately kept on the operator path, where
|
||||
"is it reachable from my shell" is the right question.
|
||||
|
||||
**Residuals, stated not engineered around:**
|
||||
|
||||
1. `assertUnchangedSinceValidation` re-confirms dev/ino immediately before spawn. That narrows the
|
||||
validation→exec window; it does not close it. Closing it means exec by held descriptor, which
|
||||
Node cannot do portably. Same accepted boundary already documented for the fleet helper.
|
||||
2. For claude and pi the runtime binary is still re-resolved inside `launch-runtime.py` after the
|
||||
trusted interpreter starts it. **This change does not cover that path.**
|
||||
|
||||
12 tests, one per hole. One was written wrong first and is worth remembering:
|
||||
`mkdirSync(path, { mode: 0o777 })` is masked by the umask to 0o755, so the world-writable-directory
|
||||
case passed while testing nothing. Create at 0o755, then `chmodSync`.
|
||||
|
||||
### D5 — what remained and what was done
|
||||
|
||||
Most of D5 was already closed: `launchFleetRuntimeForTest` is gone, specs enter through the real
|
||||
`registerFleetLaunchCommand → apply → launchFleetRuntime → launchRuntime` route on a fixture seat,
|
||||
the ledger points at the fixture and **is** asserted, and the seat-seeded/HOME-empty pass plus
|
||||
HOME-seeded/seat-empty fail pair both exist.
|
||||
|
||||
What remained was the dead `recordLaunch?: boolean` context field. Nothing in the package set it;
|
||||
its only effect was to let a caller silently disable recording on the claude branch while codex,
|
||||
opencode and pi recorded unconditionally. Removed.
|
||||
|
||||
## Not part of D1–D6, fixed because it blocked the required evidence
|
||||
|
||||
The amend requires a green full-package Vitest run.
|
||||
`install-ordering-guard.spec.ts > defaults to the real leaseEnforcementActivatable()` made that
|
||||
non-reproducible. `defaultCapabilityProbe` executes `dist/cli.js` out-of-process with a **2000 ms
|
||||
timeout**; in a full run with 86 spec files scheduled at once, one observation beats the timeout and
|
||||
the next does not, so the test's two observations of the same predicate disagree and it fails —
|
||||
reporting machine load as a wiring defect. Passed 3/3 in isolation, failed in three consecutive
|
||||
full runs.
|
||||
|
||||
Ruled out my own change by reverting only the `recordLaunch` edit and re-running: still failed.
|
||||
The guard call is now bracketed by two observations, only an agreeing pair is used as ground truth,
|
||||
a disagreeing pair is retried up to three times, and never holding still is a failure rather than a
|
||||
skip. Falsified by inverting the guard's default to `!leaseEnforcementActivatable()` → red
|
||||
(1 failed / 18 passed), then reverted.
|
||||
|
||||
## Verification state
|
||||
|
||||
- typecheck RC=0.
|
||||
- Full package Vitest, sanitized lease env (`MOSAIC_LEASE_*` + `MOSAIC_RUNTIME_GENERATION`
|
||||
stripped): **86 files / 1619 tests passed, 0 failed**, three consecutive runs, RC=0.
|
||||
- Without that sanitization the suite shows 4 failures in `mutator-gate.acceptance.spec.ts`. Those
|
||||
are the known host lease-identity leak into spawned hooks, **not** a product defect — the same
|
||||
spec re-run with only those five variables stripped and no code change is 20/20. The standing fix
|
||||
is the unpushed `fix/lease-test-env-isolation` branch (blocked on the identity blocker below).
|
||||
|
||||
## Still open
|
||||
|
||||
- **Remaining D3 surface:** the packet also asks for a capability-minimal *measured child
|
||||
environment* — construct child env from scratch, fixed locale, exact vars only, and add
|
||||
`BASH_ENV`/`ENV`, `PYTHON*`, `NODE_*`, `NPM_CONFIG_*`, unexpected-env-name killers. The
|
||||
executable-resolution half is closed; the environment half is not.
|
||||
- **D1/D2/D4/D6 need confirmation, not assumption.** They read as substantially implemented but I
|
||||
have not run the packet's full RED→GREEN control matrix against each seam.
|
||||
- Required next evidence per the packet: all D1–D6 observed RED→GREEN controls, framework-shell,
|
||||
build/lint/Prettier/bash -n, fresh current-next merge-tree.
|
||||
|
||||
## Blocker not solvable inside this branch
|
||||
|
||||
No `fred` principal exists (`tea login list` has no entry; `MOSAIC_GIT_IDENTITY` never reaches the
|
||||
pane). The only push path on this host is the **retired** mos-dt-0 token. That is why this work is
|
||||
commit-only beyond scrappy's instruction — even after the hold lifts, the truthful authenticated
|
||||
push the packet requires cannot be made under a correct identity yet. Raised with mos-claude and
|
||||
with Jason; awaiting a mint decision.
|
||||
Reference in New Issue
Block a user