stack/agents/codex/work/queue-333/r1-repair/REVIEW-R1.md

# P5 row333 independent implementation review — Mos1966 / Mos1961

Verdict: **CHANGES**

Reviewer: independent Codex review sub-agent; not the implementation author.
Implementation author: Codex builder, isolated worktree `/home/jwoltje/.t3/worktrees/mosaic-stack/t3-79263d4f`.
Git-recorded author/committer: Jason Woltje; commit subject `feat: add optional proxy usage and human model-key quotas (row333)`.
Candidate: `503b91e56137482abef475bc3b1b72a64b0a329c`.
Base: `09a847edaa25b609bac7728ff83f224708a29399`.
Review time: 2026-10-26 23:03 UTC (2026-10-26 18:03 CDT).

Reviewed the exact candidate/base Git objects, all 31 changed package files, and supporting callers/contracts. The isolated worktree HEAD equals the full candidate and its status was clean. The parent's integrated row334 product was not substituted for this candidate. Requirements are accepted P5 in `docs/plans/2026-10-24_model-proxy-rows.md`, design D18–D20 with the mos-1600 per-key/no-raw-upstream correction, and the review assignment's compatibility and nonblocking constraints.

## Findings

### 1. P1 — Do not drain a listener after its instance identity was refused

Primary location: candidate `packages/bus/src/runtime.mjs:387–389`; destructive read at `packages/bus/src/proxy-usage.mjs:31–33`.

Source trigger: enable `plugins.usage: "cliproxy-queue"`, point the instance at a loopback management listener that accepts its management authentication but lacks this config directory's sentinel (for example a different instance with a reused management key). `ModelKeys.reconcile()` checks the sentinel before modifying the list (`model-keys.mjs:80–84`) and throws `proxy-open`; runtime intentionally catches that refusal to retain the control surface (`runtime.mjs:326–327`). Nevertheless the new collector receives `client: () => modelProxyClient` unconditionally and performs the queue GET at startup and on later ticks. It never consults the controller's identity/readiness refusal.

The GET consumes the other listener's queue. Resolving local owners *after* `module.read()` and dropping/logging unknown IDs cannot undo that consumption. The local Store writer lock prevents two local writers, but does not establish ownership of a remote loopback listener. This is a new P5 side effect after a fail-closed refusal; it is not a request to change P2's existing decision to keep human controls available.

Smallest remedy: fence the collector's management-client access behind verified identity for this instance, honoring boot and subsequent sentinel refusals before any queue GET. Keep an unavailable telemetry state and value-free receipt when verification is absent; do not retry around `proxy-open`. This must not require a particular agent to have an issued key merely to establish instance identity.

Required regression evidence: a fake listener accepts management auth and returns a valid foreign sentinel/list, while its queue records/drains calls. With telemetry enabled, assert `proxy-open`, zero queue requests/consumption across startup and scheduled ticks, and an available human/control surface. Add the matching-instance positive case and a refusal after a previously successful identity check. Current `p5-runtime.test.mjs` uses the correct sentinel and enables telemetry only after startup; `proxy-usage.test.mjs` bypasses this host identity boundary entirely.

### 2. P2 — Twelve-hex legacy account names lose their quota, reset, and no-check behavior

Primary location: candidate `packages/plugins/limit-check/token-limit/index.mjs:33–35`; related new gates at `packages/bus/src/limits.mjs:126–136` and `packages/business/src/organization.mjs:85–91`.

Concrete legacy input: `launch.account.name = "abcdefabcdef"`, an ordinary existing `tokens.keys.abcdefabcdef = { limit: 10, since: <canonical timestamp> }`, and native records/subject whose `account` is `abcdefabcdef`. No proxy or model record is needed by this previously valid configuration. `ID_PATTERN` accepts this account name (`business/src/vocabulary.mjs:206`; `launch.mjs:85–92`), and the base token-limit matched every keys leaf against `account` (`base token-limit/index.mjs:31–34`). Nothing in the accepted P5 brief reserves existing twelve-hex account names.

The candidate changes this plain leaf to match `modelKeyId` solely because its name matches a regex. Consequently:

- With `token-limit` enabled and no proxy/model record, `createLimits` returns `proxy-key-refused` even below the account limit, before asking the check. At the plug-in level, known account records at the limit are instead ignored and its count becomes zero.
- With no limit checks configured, `state().checks` is `[]`, so it passes the `!st.checks` guard. The same lookup still runs before the empty loop and returns `proxy-key-refused`. The base returned `allow:true` / `stop:false` here. Thus an unused legacy limit now blocks launches/stops turns even though enforcement is switched off.
- With proxy metadata present, the resolved model ID generally differs from the account name; the account quota silently stops matching its original records. The new organization fence also prevents the chief's previously permitted reset of this plain account leaf.

These outcomes follow directly from the branches above, independently of the builder's test counts. They are introduced regressions, not Mos1864 baseline defects. The same reasoning applies to the previously examined valid name `abcdef012345`; the parent's additional `abcdefabcdef` example confirms the ambiguity without relying on a digit-leading name.

Smallest contract-preserving remedy within accepted P5: distinguish the genuinely new daily model-key leaf by its validated `reset`/`timeZone` shape, retain ordinary `{limit,since}` keys leaves as legacy account limits, and use that same classification consistently in the host lookup, matching, reports and both organization fences (including old-to-new/removal transitions). Preserve the empty-check path before performing metadata lookup. P5 requires daily model-key quotas; newly added cumulative model-key support must not silently take over the existing account namespace. If cumulative model-key quotas are retained, they need an unambiguous separately authorized representation.

Required regression evidence: cover `abcdefabcdef` with plain account limits, no proxy/model record, below/at limit, original account report count and chief reset; repeat with proxy metadata whose ID differs. Cover no configured checks with that legacy leaf and with a new daily model-key leaf: preserve the established no-check result without resolving model metadata. Retain positive daily per-key enforcement, current/retiring native lookup, and human-only changes/removals/resets for actual model-key daily quotas. Existing P5 tests cover an invented cumulative model-key interpretation and a daily key, but not this legacy collision or the empty-check branch.

### 3. P2 — Optional telemetry adds a blocking startup dependency

Primary location: candidate `packages/bus/src/runtime.mjs:865–867`.

Source trigger: correct sentinel/reconciliation, telemetry enabled at boot, and a queue endpoint that stalls while other management operations work. The initial `await proxyUsage.tick()` waits for that queue read, up to the existing five-second client deadline (`model-proxy/src/client.mjs:10,45`). The process waits for `startBroker()` before sending its ready answer and launch capabilities (`bus/src/process.mjs:147–152`). Thus optional telemetry delays native readiness/launch admission even though it has no quota authority. Later timer ticks are independent of native requests; the startup await is the exception.

Smallest remedy: start the first poll in the same supervised background path as subsequent ticks, preserving single-flight handling, value-free errors and close cleanup. Do not increase any existing deadline.

Required regression evidence: hold a fake queue response open while allowing required reconcile requests, prove startup/native readiness completes before releasing the queue, and exercise native usage/limit work while a poll is outstanding. Then release/fail the response and verify availability, no duplicate drain and bounded cleanup. Current whole-file receipts contain no such slow-queue readiness test.

## Independent evidence checks

Performed read-only Git/filesystem/hash analysis, not product execution:

- `source-inputs.sha256` SHA-256 is exactly `b4662dcafb2aeb27a06e5dbd3b9d0b6d184e8624e344fed351ff20e3b255951b`. All **202** listed file hashes match candidate Git blobs and the pinned isolated tree, with zero mismatches. The 944 raw JSONL entries contain exactly 202 distinct path/hash pairs, exactly equal to the manifest; no path has multiple traced hashes.
- All **31** changed package paths exactly equal `candidate-sourcepaths.txt`; every entry in `candidate-sourcehashes.sha256` matches both sources. That manifest's SHA-256 is `b1457ec57c0fb695f05a08ba20b420820be5f32cac2b77ac5ea20b02115153e2`.
- All **25** entries in `receipts.sha256` match candidate blobs and isolated-tree bytes. Receipt-manifest SHA-256: `a8b91660ada571eab9c0328b74407517ec63e1ff56172302acec1d347253a810`. Raw source-trace SHA-256: `9c16aa4b8e6d81aa5b9867fec5e0890b22e90db5c307da55badfcfa9f240aef1`.
- The scoped runner explicitly names **24 whole test files**, uses `nice -n 19`, `--test-concurrency=1`, and private HOME/XDG_CONFIG_HOME/TMPDIR/MOSAIC_CONFIG with cleanup. Its SHA-256 is `c8a2bf4539bffe38f3198308fd60e9c1ad338841b406af160ab0739c15394290`. These are verified runner bytes and recorded output, not independent observation of the original processes' environment.
- Receipt24 records **174 tests, 174 pass, 0 fail/cancelled/skipped/todo**, SHA-256 `3358af061c35d42b00d5ee1e001939534c5e7563d4f5418ea85431605d0b9f6d`. This remains builder scoped evidence, not an independent test run, full gate or browser gate.

RED history is retained as RED. Receipt16 records the upstream-value mutant with **11/15 passing and four failing tests**: the client exposes the raw account value, while collector validation refuses it and downstream expectations fail. Receipt17 records restored **15/15**. Final client bytes hash to `f5f86cad661034f223a76352d877fa8d7883bfa921a7a01282347a9a03272441`, matching the documented restoration hash and final source pins. The receipts do not contain a separately captured pre/post backup or mutant patch hash, so byte-identical restoration at that historical instant is documented by the builder rather than independently replayed here.

Receipt18 is **169/170**, with the exact existing `REQ-LIMIT-4 a role run at its limit stops at the end of its turn: RESULT and read land first, the claim is revoked by limit-check` assertion failing at `limit-stop.test.mjs:61` because `run` was added to the original second subject argument. The candidate keeps that argument's shape and puts `{run}` in the private third context. The entire original limit-stop file is byte-identical to base. Receipt19 records **170/170** after that correction. Receipt20 is a genuine **2/3** RED for the supplied modelKeyId fallback; receipt21 is **15/15** after its removal. Receipt22 records **173/173**. Receipt23 is a genuine **3/4** RED for missing native context; receipt24 records **174/174** after the explicit context refusal. These were builder development REDs, not prior independent review findings, and are not relabelled GREEN. Receipt05 also remains a syntax-error RED despite its filename; receipts12/13 remain failed runtime-fixture runs.

All modified pre-existing test files other than the two authorized inventory updates retain their entire base bytes as an unchanged prefix. The kinds/loader edits are the strictly additive `usage`/`usage:[]` expected inventories; no old assertion or deadline was removed or weakened.

## Other source conclusions and limits

Telemetry source rows are excluded at the start of `turnRecords`, before native totals and quota input. The collector validates and whitelists persisted fields, uses hashed client/upstream identities, retains only safe failure metadata, serializes concurrent ticks, and routes current/retiring ownership from credential status. Malformed/unknown/oversize/non-finite/unsafe counts cannot flow through the new queue client as raw values into store, error, audit or Console fields: the queue operation reuses fixed loopback GET, count=100, original response cap/deadline and value-free errors/receipts. No second queue consumer was found in the product sources; finding 1 concerns foreign-listener identity rather than local double counting.

Native model-key authority comes from the host's run map and credential metadata, not a JSON-supplied key ID. Lookup rejects missing/duplicate owners, foreign business/instance/run, missing retained generations and ambiguous IDs. The broker's private context is derived from its authenticated session; the public subject drops supplied run/modelKeyId fields. The new usage and model-quota fences run before validation and again against serialized bytes, compare against the old human, and reject tester/agent identities even when names collide. Finding 2 concerns which leaves those otherwise fail-closed fences classify.

Polling is absent by default, defaults to 15 seconds when enabled, validates integer 1..59, and is exposed through the exact human `plugins.set` argument contract. Console native key rows and optional proxy-reported account counts are separated and labelled; telemetry is explicitly described as incomplete and non-enforcing. Broker imports are unchanged, kinds remains import-free, and no host management implementation was added to the pure Broker/sandbox import closure. No P3 launcher/session/harness/business-routing files changed.

No tests, Node/product runtime, HTTP, browser, provider, real instance config, credential files or live proxy paths were run/read. Hash checking used repository files only. No code, tests, helpers, queue, session/build logs, issues, posts, pushes, commits, merges or live state were changed; this report is the only authorized write. Static analysis establishes the reported branch behavior; the suggested regression cases remain to be executed by the authorized builder/gate owner. Hash equality verifies the captured inputs/receipts, not completeness of uninstrumented reads or independent execution provenance.

These three findings are in the candidate delta. No pre-existing issue is being promoted to a new landing blocker; any separately identified baseline defect belongs under Mos1864 unless a required gate is RED. #662 / 72f77815 and #657/#658 remain excluded/backlog; #267/#289 are outside the 10-27 slice but remain in full scope. The #660 full-gate target is not admitted by this review. No landing authority or full/browser/combined-gate approval is implied.