stack/docs/plans/reviews/2026-09-10_m20-i2-charter-verdict.md

M20-I2-CHARTER-FILBERT-1 (#1500) — charter and prerequisite verdict

Reviewer: Filbert. Role disclosure: I was the increment 1 independent reviewer; that approval covered the fifteen tests' assertions, not comprehensive registry safety, and this review relies on fresh reproduction only. Baseline b3fa2210…; frozen draft /tmp/m20-i2-charter-review-fga9lki3 at manifest SHA-256 78981e71521e5df2b62649fe11eda6c241029c4fe41ef5dfe06fff3776f44401 (both hashes verified). All reproduction used a disposable git archive tree and synthetic fixtures; no real credentials, Pi, timers, Docker, packages, source edits, or shared logs.

Verdict: APPROVED (charter), with prerequisite correction confirmed as mandatory stage 1

The charter is well-formed and its dependency ordering is right: nothing may build on the validator until the reproduced defects are closed and independently verified. The factual findings below are separately recorded from the proposed design, per the request.

Factual prerequisite findings (all reproduced on the clean baseline)

• D1 — fail-open root (the coordinator's defect, independently reproduced): a nonexistent registry root and an empty root both return valid, exit 0; list exits 0 with an empty projection. safeList converts every directory error into an absent directory.
• D2 — symlink/containment failure: a provider file symlinked from outside the root with a matching id returns valid exit 0; an account directory symlinked from outside the root is followed, its records validated and listed. The loader uses stat/readFile, both of which follow symlinks; there is no containment check anywhere.
• D3 — version schema inconsistency: numeric providerVersion: 2 is rejected (invalid-id) while the governing plan's examples use numeric 1; arbitrary strings such as "banana-schema" are accepted. Wrong in both directions.
• D4 — nested unknown keys pass: unknown fields inside harnesses.pi (e.g. injected secretExfil) validate clean; harness entries are only checked to be objects.
• D5 — enrollment not enforced: a profile default account absent from allowedAccounts validates clean.
• D6 — credential-type match not enforced: an oauth account under a provider whose credentialTypes is ["api_key"] validates clean.
• D7 — non-pi endpoints unvalidated: a custom-endpoint provider carrying a plain-http baseUrl on a non-pi harness produces no insecure-transport error (I1-F3, now confirmed as a prerequisite defect rather than later hardening).
• D8 — URL credentials accepted: https://user:secret@host/v1 as a pi baseUrl returns valid.
• D9 — bounded content echo in errors: malformed JSON surfaces V8's parse message including a truncated excerpt of the file's first bytes ("FIXTURE-MA"...). Bounded to a short prefix, but a credential-shaped file misnamed .json would leak its opening characters.
• D10 — partial trusted output: list prints its projection even when validation errors exist (exit 1, output still emitted). Also observed in passing: a missing account.json inside a listed account directory reports as invalid-json: ENOENT, a code mislabel.

D1–D10 confirm every concern in the charter's prerequisite gate. None were covered by the increment 1 suite's assertions; two (D7, partially D4) were flagged in my I1 notes as hardening and now stand as blocking prerequisites.

Charter assessment

• Dependency ordering: correct. Stage 1 (close verified defects, independent approval, clean-tree regression) precedes resolution, materialization, and the refresh adapter. Given D1/D2, building a materializer on this validator would generate inputs from absent or out-of-root sources while reporting success.
• Scope: confined to packages/mosaic plus assignment records; scripts/auth.sh, launchers, native homes, dataRoot, services, images, releases untouched; no new dependencies without reviewed need. The stage-2 boundary against implementing #53's full runtime model is stated.
• No-secret guarantees: fake-marker disclosure tests for stdout/stderr/errors, no credential hashes in manifests, token-bearing output never logged, fake-Pi refresh simulation with isolated auth location, --credentials/print-* exclusion. Adequate, and now demonstrably necessary (D9).
• Rollback: source-level revert; tests clean synthetic temp directories; no runtime migration performed. Consistent.
• The closing self-correction (prior valid responses were labeled acceptance too strongly) is accurate and appropriately does not waive anything.

Recommended corrective scope (the separate narrow prerequisite increment)

1. Fail-closed roots: refuse missing, inaccessible, or non-directory registry roots and required subdirectories before any record processing.
2. Containment: lstat-based regular-file checks and no symlink following for files or directories anywhere under the supplied root.
3. Version fields: numeric versions per the governing plan, with unknown-version refusal; migrate fixtures accordingly in the same patch.
4. Nested shape validation for every harness entry (unknown keys, per-kind requireds), applied to all harnesses, not only pi; reject URLs carrying user:pass@ and confine http to allowInsecureTransport.
5. Cross-record enforcement: default accounts enrolled in allowedAccounts; account type within the provider's credentialTypes.
6. Bounded secret-free errors: fixed invalid-JSON code without parse-message excerpts; correct the ENOENT mislabel; list must not emit trusted projections on an invalid tree.
7. Negative controls in the package suite for each item above, including fake-marker disclosure assertions.

This correction is small, independently reviewable, and should land before any stage-2 work. The charter already mandates exactly that ordering; approving the charter does not approve any implementation.