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.