diff --git a/docs/marketplace-polyrepo-migration/review.md b/docs/marketplace-polyrepo-migration/review.md index 8bf60ac..ab810a5 100644 --- a/docs/marketplace-polyrepo-migration/review.md +++ b/docs/marketplace-polyrepo-migration/review.md @@ -7,388 +7,88 @@ slug: marketplace-polyrepo-migration project_dir: docs/marketplace-polyrepo-migration brief_path: docs/marketplace-polyrepo-migration/brief.md scope_sha_start: a44e37b -scope_sha_end: 5e00f92 -reviewed_files_count: 24 -verdict: BLOCK -findings: - - 94b83adae4d42f4418033b723c41345304d4ba88 - - 9e97cd5f5abe96ad325b96a4311116b788db870a - - 1708e9046e56b0c923122a8f60f4674eafbb31a9 - - 4e494c8a16d9dca9fbf3c116b0f55e96b18c096f - - aeb6292d4c528ad4320c7b6e29621b9f96047558 - - 5d112cb49d23a4d23f6e45df4d79822eee01d17a - - 9e588ca48abe3104725dd90df62e2c5aad75c3de - - bc0f8a7df6d384d7288f533ca3485b5989c06b69 - - 8d649e966f4ae94dd2b11e7374982e6f96c8bc52 - - 4044c49e4a47cad56de6b56d9cb9d85c24443b86 +scope_sha_end: 85a8ee3 +reviewed_files_count: 29 +findings: [] --- # Review: Marketplace polyrepo-migration tooling — 11-step extraction/validation/cutover harness ## Executive Summary -**Verdict: BLOCK.** One BLOCKER survives and is decisive: `60-rewrite-marketplace.mjs:87` -emits a **flat** external-source shape (`{ name, source: 'url', url, ref, description }`) -where the brief §6 ratified literal — and the official Claude Code marketplace schema — -require the **nested** form (`source: { source: 'url', url, ref }`). A flat shape would not -resolve at install time, breaking SC1/SC3/SC8 for every externalised entry. The unit test -(`60-rewrite-marketplace.test.mjs:32-34`) asserts the flat shape, so the suite **passes the -defect** — only the Step 10 operator pilot or a direct schema check would catch it. This is -the single thing to verify and resolve before the cutover window. - -Beyond the BLOCKER, the review found a cluster of correctness/robustness gaps concentrated in -the **regression-relative SC2 / SC6 block of `99-dryrun.sh`** (the highest-risk region of the -harness — it is the deterministic detector for the very 87-file-drop defect class the -`plugin-map.json` correction exists to prevent), plus two declared-dependency / coverage gaps. -None of these block the local mechanics that the dry-run already proved green (11/11), but -several are operationally significant before the window. - -**Two disclosures bound this review:** - -1. **Brief has no YAML frontmatter (FM_MISSING — soft warning).** `brief.md` is a RATIFIED, - hand-written brief, so the absent frontmatter is expected and is *not* itself a finding. - Brief-ref traceability therefore rests on section anchors (e.g. SC1, §6) rather than - parser-extracted SC IDs. -2. **High-effort normalization applied (operator standing instruction: maximal discipline).** - The default/standard-effort coordinator path drops findings whose `rule_key` is not in the - 12-entry catalogue. Four substantive findings (the config-audit SC2-routing gap and the - three `99-dryrun.sh` regression-cluster fragilities) carried reviewer keys (`SC_PARTIAL`, - `FRAGILE_ASSUMPTION`, `CROSS_FILE_REGRESSION`) with no catalogue home and would have been - suppressed. Rather than drop substantive signal on a naming technicality, they are - **retained** here via the v5.1.1 high-effort normalization — `rule_key` substituted with - the catalogue's general `PLAN_EXECUTE_DRIFT`, the reviewer's original preserved in - `original_rule_key`. The verdict (BLOCK) is unchanged either way; this only affects audit - completeness and what flows into `/trekplan --brief review.md`. - -The delivered surface is the **Claude-run local/reversible half (Steps 1–11)**. The -window-only steps (Forgejo `auto_init:false` create, post-strip history push, HTTPS -`url`+`ref` external-source resolution) are operator-gated by design (D8 / NULL-push) and are -correctly absent from this code — they are not gaps. +Verdict: **ALLOW**. This is the final pass of a three-step remediation arc. The first review (commit 3065930) returned **BLOCK** (1 BLOCKER + 6 MAJOR + 3 MINOR). The second pass (after commits 86208da + fef4b33) returned **WARN** — all 10 original findings resolved (the BLOCKER's flat external-source shape replaced by the nested `{ source: { source:'url', url, ref } }` form per the official Claude Code marketplace schema, with `validate()` + asserting tests; the SC6/SC2 regression detectors extracted into `sc6-check.sh`/`sc2-regression.sh` and negatively tested; the single-tag assertion, `$dest`-sourced regression capture, mktemp guard, `sc2_gate` routing, `git filter-repo` re-assertion, path-hygiene gate, and `blob_strip_safe: null` all confirmed) — but the high-effort deep read surfaced 2 NEW MAJOR findings in `migration/30-fix-references.mjs` (an untested `package.json` rewrite branch + a stale monorepo-relative `repository.directory` surviving into standalone repos). Both were remediated: the rewriter now drops `repository.directory` in both the plugin.json and package.json branches (idempotency preserved), and a new test drives the previously-unguarded branch against a synthetic `llm-security` extract. The independent code-correctness reviewer re-verified both as RESOLVED with no new correctness issue. Every Success Criterion traces to delivered code; every Non-Goal (the window-only Forgejo create / push / HTTPS-source resolution) remains correctly unbuilt and operator-gated by D8/NULL-push. The full local dry-run is green (11/11 targets, 0 pushes). Two standing disclosures, unchanged: the brief has no YAML frontmatter (FM_MISSING — a soft warning, not a finding, because it is a hand-written RATIFIED brief), and high-effort normalization was applied per the operator's maximal-discipline standing instruction (Pass 3 Cloudflare reasonableness filtering skipped). ## Coverage -All 24 in-scope files are listed; **0 silently dropped, 0 `skip`, 0 `deep-review`.** All files -live under `docs/marketplace-polyrepo-migration/` (paths abbreviated to `migration/…` and -`scripts/…` for readability). The committed scope `a44e37b..5e00f92` is entirely new files, so -current content equals the full delivered diff. +| File | Treatment | Reason | +|------|-----------|--------| +| `migration/00-preflight.sh` | summary-only | Preflight; path-hygiene + filter-repo/python3 gates confirmed asserting | +| `migration/00-preflight.test.mjs` | summary-only | Preflight guard test; single-path lock added (836b8e9 alignment) | +| `migration/10-extract.sh` | summary-only | Extraction driver; single-path per 836b8e9; re-asserts git-filter-repo | +| `migration/10-extract.test.mjs` | summary-only | Extraction test; confirmed asserting | +| `migration/20-rehome-config.sh` | summary-only | Config rehome; covered by paired test | +| `migration/20-rehome-config.test.mjs` | summary-only | Rehome test; confirmed asserting | +| `migration/30-fix-references.mjs` | summary-only | Reference rewriter; 2 prior MAJOR (deep read) now RESOLVED | +| `migration/30-fix-references.test.mjs` | summary-only | Now covers the package.json + plugin.json branch (synthetic llm-security) | +| `migration/40-validate-standalone.sh` | summary-only | Standalone validator; sc2_gate routing added | +| `migration/40-validate-standalone.test.mjs` | summary-only | Validator test; confirmed asserting | +| `migration/templates/validate-plugin.generic.sh` | summary-only | Generic validation template; no dynamic logic regression | +| `migration/templates/gitignore.plugin.tmpl` | summary-only | Static template; no executable regression surface | +| `migration/50-config-audit-sc2.sh` | summary-only | SC2 audit gate; covered by paired test | +| `migration/50-config-audit-sc2.test.mjs` | summary-only | SC2 audit test; confirmed asserting | +| `migration/60-rewrite-marketplace.mjs` | summary-only | Prior BLOCKER fixed at :92 — nested source-object emitted + tested | +| `migration/60-rewrite-marketplace.test.mjs` | summary-only | Asserts the nested `{ source: {...} }` schema shape | +| `migration/70-thin-catalog.sh` | summary-only | Catalog thinning (3a5f558); covered by paired test | +| `migration/70-thin-catalog.test.mjs` | summary-only | Catalog thinning test; confirmed asserting | +| `migration/sc6-check.sh` | summary-only | SC6 DROP detector; negatively tested via sc-checks.test.mjs | +| `migration/sc2-regression.sh` | summary-only | SC2 regression detector; negatively tested | +| `migration/99-dryrun.sh` | summary-only | Dry-run harness (5e00f92); SC6/SC2 block confirmed correct | +| `migration/99-dryrun.test.mjs` | summary-only | Dry-run integration test; confirmed asserting | +| `migration/sc-checks.test.mjs` | summary-only | Negative tests for the sc6/sc2 detectors (both branches) | +| `migration/plugin-map.json` | summary-only | Extraction map; ms-ai-architect blob_strip_safe reset to null (preflight-computed) | +| `migration/RUNBOOK.md` | summary-only | Operator-window runbook; git-filter-repo + python3 preconditions added | +| `scripts/sync-design-system.mjs` | summary-only | Design-system sync; no migration-contract regression surface | +| `scripts/sync-design-system.test.mjs` | summary-only | DS-sync test; confirmed asserting | +| `docs/marketplace-polyrepo-migration/review.md` | skip | Review artifact itself — excluded from being reviewed as delivered code | -| File | Treatment | Note | -|------|-----------|------| -| migration/00-preflight.sh | summary-only | Deterministic preflight; no finding | -| migration/00-preflight.test.mjs | summary-only | Covers preflight | -| migration/10-extract.sh | summary-only | **MAJOR** — undeclared git-filter-repo / python3 runtime deps | -| migration/10-extract.test.mjs | summary-only | Covers extraction | -| migration/20-rehome-config.sh | summary-only | No finding | -| migration/20-rehome-config.test.mjs | summary-only | Covers rehome | -| migration/30-fix-references.mjs | summary-only | No finding | -| migration/30-fix-references.test.mjs | summary-only | Covers ref-fix | -| migration/40-validate-standalone.sh | summary-only | **MAJOR** — config-audit SC2 not routed to its gate | -| migration/40-validate-standalone.test.mjs | summary-only | SC7 planted-leak coverage present (good) | -| migration/50-config-audit-sc2.sh | summary-only | SC2 gate; no finding | -| migration/50-config-audit-sc2.test.mjs | summary-only | Covers SC2 gate | -| migration/60-rewrite-marketplace.mjs | summary-only | **BLOCKER** — flat vs nested external-source shape | -| migration/60-rewrite-marketplace.test.mjs | summary-only | Asserts flat shape — masks the BLOCKER | -| migration/70-thin-catalog.sh | summary-only | No finding | -| migration/70-thin-catalog.test.mjs | summary-only | Covers thinning | -| migration/99-dryrun.sh | summary-only | **Cluster** — 5 findings in the regression-relative block | -| migration/99-dryrun.test.mjs | summary-only | **MAJOR** — no negative DROP/regression test | -| migration/plugin-map.json | summary-only | **MINOR** — hardcoded `blob_strip_safe` for ms-ai-architect | -| migration/RUNBOOK.md | summary-only | Runtime preconditions incomplete (see 10-extract MAJOR) | -| migration/templates/gitignore.plugin.tmpl | summary-only | No finding | -| migration/templates/validate-plugin.generic.sh | summary-only | No finding | -| scripts/sync-design-system.mjs | summary-only | No finding | -| scripts/sync-design-system.test.mjs | summary-only | Covers sync | +Note: the brief §6 nested-shape criterion is FULLY met. 0 files silently dropped; 0 deep-review treatments (all summary-only); 1 skip (the review artifact), recorded above. -## Findings +## Findings (BLOCKER) -### BLOCKER +_None._ -#### `94b83ad` — External marketplace entry uses flat source shape, not the nested schema -- **file:line:** docs/marketplace-polyrepo-migration/migration/60-rewrite-marketplace.mjs:87 -- **rule_key:** BROKEN_SUCCESS_CRITERION -- **brief_ref:** SC1 (brief §6) -- **detail:** Line 87 returns `{ name, source: 'url', url, ref, description }` — a flat object - with `source`/`url`/`ref` as sibling top-level keys. Brief §6 ratifies the nested form - `{ name, source: { source: 'url', url, ref }, description }`, where the entry's top-level - `source` key holds an object and the inner `source` is the type discriminator. A flat - `source: 'url'` with sibling `url`/`ref` is not the documented external-source shape and - would not resolve at install, breaking SC1/SC3/SC8 for every externalised entry. The test - (`60-rewrite-marketplace.test.mjs:32-34`) asserts the flat shape, so the defect passes the - suite; only the Step 10 pilot catches it. -- **recommended_action:** Confirm flat-vs-nested against the official CC marketplace schema; if - nested, emit `source: { source: 'url', url, ref }` and update `validate()` + - `60-rewrite-marketplace.test.mjs` to assert the nested shape. +## Findings (MAJOR) -### MAJOR +_None._ -#### `9e97cd5` — config-audit SC2 not routed to its dedicated gate in the standalone harness -- **file:line:** docs/marketplace-polyrepo-migration/migration/40-validate-standalone.sh:95 -- **rule_key:** PLAN_EXECUTE_DRIFT *(original_rule_key: SC_PARTIAL)* -- **brief_ref:** SC2 -- **detail:** `40-validate-standalone.sh` runs the raw `test_cmd` from `plugin-map.json` for - every key with no branch for config-audit's `sc2_gate`. config-audit's `test_cmd` is the full - `node --test 'tests/**/*.test.mjs'`, which FAILs at a fresh-clone path on the 6 machine-locked - v5.0.0 byte-stability tests — the exact blocker Step 7 (`50-config-audit-sc2.sh`) exists to - resolve. The harness documents `--all` as supported usage, so `40-validate-standalone.sh --all` - would falsely report config-audit as a SC2 FAIL. Only `99-dryrun.sh` and the RUNBOOK per-repo - procedure special-case config-audit; the standalone harness does not, giving an inconsistent - SC2 verdict by entry point. -- **recommended_action:** Read each target's `sc2_gate` and delegate to that script when present - (mirroring `99-dryrun.sh`) instead of running the raw `test_cmd`. +## Findings (MINOR) -#### `1708e90` — Tag report uses `git tag | head -1`, relies on an unverified single-tag invariant -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.sh:113 -- **rule_key:** PLAN_EXECUTE_DRIFT *(original_rule_key: FRAGILE_ASSUMPTION)* -- **brief_ref:** NFR — code correctness (SC6) -- **detail:** `tag="$(git tag | head -1)"` is correct only because `10-extract.sh` strips all - carried-over tags and re-creates exactly one. If the extractor's tag-strip loop partially - fails, multiple tags survive and `head -1` silently reports the lexicographically-first wrong - tag (e.g. `v1.0.0` instead of `v5.1.1`) while the dry-run still passes — nothing asserts tag - count == 1. -- **recommended_action:** Assert exactly one tag survives (`[ "$(git tag | wc -l)" = "1" ]`) and - FAIL (`ok=0`) otherwise, instead of `head -1`. +_None._ -#### `4e494c8` — SC2 regression capture depends on a side-effect clean room, masking real regressions if absent -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.sh:145 -- **rule_key:** PLAN_EXECUTE_DRIFT *(original_rule_key: CROSS_FILE_REGRESSION)* -- **brief_ref:** NFR — code correctness (SC2) -- **detail:** `capture_fails "/tmp/claude-$key" …` reads a directory populated only as a side - effect of `40-validate-standalone.sh` (`rm -rf "$cr"; cp -R "$dest" "$cr"`). This cross-script - coupling is implicit and untested: if that path changes or the dir is absent, `grep` matches - nothing, `comm -23` yields empty, and the target is reported PASS with zero regressions — - masking a real one. The contract "standalone failing set ⊆ in-repo failing set" is only sound - if both sets come from the same prepped tree. -- **recommended_action:** Capture the standalone failing set from `$WORK/$key` (the tree the - dry-run itself preps), or assert `/tmp/claude-$key` exists before `capture_fails`. +## Findings (SUGGESTION) -#### `aeb6292` — `live_files`/`mappaths` assume whitespace-free map paths with no validation -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.sh:54 -- **rule_key:** PLAN_EXECUTE_DRIFT *(original_rule_key: FRAGILE_ASSUMPTION)* -- **brief_ref:** NFR — code correctness -- **detail:** `for p in $(mappaths "$key")` relies on space-joined paths and word-splitting; - correct only while every `plugin-map.json` path is whitespace/glob-free. No assertion enforces - this invariant, and `git ls-files "$p"` would mis-count a path with spaces. Latent fragility on - controlled input, but it is the exact "fragile assumption on file structure, unvalidated" - pattern. -- **recommended_action:** Document and assert the no-whitespace-in-paths invariant in - `00-preflight.sh`, or emit newline-separated paths consumed via `while IFS= read -r`. - -#### `5d112cb` — No negative test exercises the SC6 DROP or SC2-regression FAIL branch -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.test.mjs:73 -- **rule_key:** MISSING_TEST -- **brief_ref:** SC6 -- **detail:** The dry-run test only asserts the happy path (the live report contains no `DROP` - and no `regression:`). Nothing plants a dropped file or an injected failure to drive the DROP - or SC2-regression FAIL branch, so the two highest-stakes detectors — SC6 DROP (the - llm-security 87-file-drop class the whole map correction targets) and the SC2 regression FAIL — - are entirely unverified, unlike the SC7 planted-leak test at - `40-validate-standalone.test.mjs:31`. A regression that disables the DROP comparison would - pass every existing test. -- **recommended_action:** Add a negative test that constructs a `$WORK/` extract with a - deliberately removed tracked file (or a planted failing test) and asserts the dry-run reports - `DROP`/`regression:` and exits non-zero. - -#### `9e588ca` — git-filter-repo and python3 are undeclared hard runtime deps -- **file:line:** docs/marketplace-polyrepo-migration/migration/10-extract.sh:68 -- **rule_key:** UNDECLARED_DEPENDENCY *(original_rule_key: HIDDEN_DEPENDENCY)* -- **brief_ref:** RUNBOOK preconditions -- **detail:** `10-extract.sh:68` invokes `git filter-repo` (and the scripts use `python3` - pervasively via inline `python3 -c`) with no own presence check, relying on `00-preflight.sh`. - But the self-heal path runs preflight only when the mirror is absent, so a since-uninstalled - `git-filter-repo` dies with a raw git error rather than the actionable `brew install` message. - `git-filter-repo` is a non-default git extension and neither it nor `python3` appears in the - RUNBOOK preconditions — an operator reading only the RUNBOOK has no declared prerequisite for - the single most critical external tool the extraction depends on. -- **recommended_action:** Add `git-filter-repo` and `python3 >= 3.6` to the RUNBOOK - "Preconditions", and have `10-extract.sh` re-assert `git filter-repo --version` before line 68. - -### MINOR - -#### `bc0f8a7` — ms-ai-architect ships a hardcoded `blob_strip_safe` despite documenting it as preflight-computed -- **file:line:** docs/marketplace-polyrepo-migration/migration/plugin-map.json:96 -- **rule_key:** PLAN_EXECUTE_DRIFT *(original_rule_key: SC_PARTIAL)* -- **brief_ref:** SC6 -- **detail:** The map `_comment` says `blob_strip_safe` is (re)computed by `00-preflight.sh`, and - preflight only rewrites it on change. Yet ms-ai-architect already carries - `"blob_strip_safe": true` as committed data (every other target is `null`). If preflight is - skipped or a non-screenshot >1 MB blob enters history, `10-extract.sh` selects - `--strip-blobs-bigger-than 1M` off a stale `true` and could strip a non-screenshot blob, - corrupting the extracted ms-ai-architect history (SC6). -- **recommended_action:** Reset `targets["ms-ai-architect"].blob_strip_safe` to `null` and rely - on `00-preflight.sh` to populate it per run. - -#### `8d649e9` — SC6 DROP branch mislabels a failed extract as content DROP -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.sh:126 -- **rule_key:** MISSING_ERROR_HANDLING -- **brief_ref:** SC6 -- **detail:** When extraction fails, `ext_files` stays `0` but the SC6 block still runs and emits - `sc6=0/ DROP`, attributing an *extraction* failure to content loss. The `ok=0` path is - correct, but the label misdirects the operator reading the report. -- **recommended_action:** Gate the SC6 comparison behind the same `[ -d "$dest/.git" ]` success - that guards the extraction block, and emit a distinct `(extract failed)` label. - -#### `4044c49` — Unguarded mktemp under `set -uo` can yield a false SC2 PASS -- **file:line:** docs/marketplace-polyrepo-migration/migration/99-dryrun.sh:144 -- **rule_key:** MISSING_ERROR_HANDLING -- **brief_ref:** SC2 -- **detail:** The regression block runs under `set -uo pipefail` (no `-e`). If `mktemp` fails, - `sf`/`bf` are empty strings, `comm -23` operates on empty filenames, `regr` becomes empty, and - the target is reported `PASS (0 pre-existing)` — a false PASS. `-u` does not catch this because - the vars are defined-but-empty. -- **recommended_action:** Guard each `mktemp` (`sf="$(mktemp)" || fail`) and treat an empty - capture as an error (FAIL/abort), not as zero regressions. +_None._ ## Remediation Summary -- **BLOCKER: 1** — flat-vs-nested external source shape (`60-rewrite-marketplace.mjs:87`). - **Must be resolved before the cutover window**; verify against the official CC marketplace - schema, fix line 87 + the test that currently asserts the flat shape. -- **MAJOR: 6** — config-audit SC2 routing in the standalone harness; the `git tag | head -1` - single-tag assumption; the side-effect clean-room coupling in the regression capture; the - whitespace-free-path assumption; the missing negative DROP/regression test; the undeclared - `git-filter-repo`/`python3` runtime deps. -- **MINOR: 2** — hardcoded `blob_strip_safe`; the mislabeled DROP-on-failed-extract. - *(MINOR count here is 3 in the full findings list; two are listed above and `bc0f8a7` is the - third — the `blob_strip_safe` MINOR.)* -- **SUGGESTION: 0** +- BLOCKER: 0 +- MAJOR: 0 +- MINOR: 0 +- SUGGESTION: 0 -**Highest-risk region:** the regression-relative SC2/SC6 block of `99-dryrun.sh` (lines 54, 113, -126, 144, 145) accounts for 5 of the 10 findings. It is the deterministic detector for the -87-file-drop defect class that the entire `plugin-map.json` correction (commit `836b8e9`) exists -to prevent — and it currently has no negative test proving it fires. Harden this block + add the -negative test before trusting the dry-run as the batch gate. - -**Note on scope:** the dry-run proved the *local mechanics* green (11/11, 0 pushes). None of -these findings contradict that result — they are robustness/coverage gaps in the harness and one -schema-shape divergence in the externalisation output, all of which surface *before* the -operator window where they are cheap to fix. The BLOCKER is the one that would actually break a -user `/plugin install` if shipped as-is. - -`/trekplan --brief review.md` consumes the trailing JSON `findings[]` (full content) and the -frontmatter `findings:` list (ID set) to produce a remediation plan (Handover 6). +All 12 findings across the remediation arc are resolved: the 1 BLOCKER + 6 MAJOR + 3 MINOR from the first review (commits 86208da, fef4b33), plus the 2 MAJOR surfaced by the re-review's deep read of `30-fix-references.mjs` (this commit). The delivered Claude-run local/reversible half (Steps 1–11) is verified — full local dry-run 11/11 targets, 0 pushes; every paired unit suite green; the externalised marketplace.json emits the schema-correct nested source-object. The window-only steps (Forgejo `auto_init:false` create, post-strip history push, HTTPS `url`+`ref` resolution) remain correctly absent from this code — operator-gated by D8 / the RUNBOOK. The operator window (RUNBOOK.md) is no longer gated by a review BLOCK. ```json { - "verdict": "BLOCK", - "counts": { "BLOCKER": 1, "MAJOR": 6, "MINOR": 3, "SUGGESTION": 0 }, - "normalization": "high-effort (v5.1.1): unknown reviewer rule_keys substituted with PLAN_EXECUTE_DRIFT; original preserved in original_rule_key. Applied per operator standing instruction (maximal discipline) to retain substantive findings the standard-effort path would suppress.", - "findings": [ - { - "id": "94b83adae4d42f4418033b723c41345304d4ba88", - "severity": "BLOCKER", - "rule_key": "BROKEN_SUCCESS_CRITERION", - "file": "docs/marketplace-polyrepo-migration/migration/60-rewrite-marketplace.mjs", - "line": 87, - "brief_ref": "SC1 (brief §6)", - "title": "External marketplace entry uses flat source shape, not the nested schema", - "detail": "Line 87 returns { name, source: 'url', url, ref, description } (flat). Brief §6 and the official CC marketplace schema require nested { name, source: { source: 'url', url, ref }, description }. Flat would not resolve at install, breaking SC1/SC3/SC8. 60-rewrite-marketplace.test.mjs:32-34 asserts the flat shape, so the suite passes the defect; only the Step 10 pilot catches it.", - "recommended_action": "Confirm flat-vs-nested against the official CC marketplace schema; if nested, emit source: { source: 'url', url, ref } and update validate() + the test assertion." - }, - { - "id": "9e97cd5f5abe96ad325b96a4311116b788db870a", - "severity": "MAJOR", - "rule_key": "PLAN_EXECUTE_DRIFT", - "original_rule_key": "SC_PARTIAL", - "file": "docs/marketplace-polyrepo-migration/migration/40-validate-standalone.sh", - "line": 95, - "brief_ref": "SC2", - "title": "config-audit SC2 not routed to its dedicated gate in the standalone harness", - "detail": "40-validate-standalone.sh runs the raw test_cmd for every key with no sc2_gate branch. config-audit's test_cmd is the full node --test 'tests/**/*.test.mjs', which FAILs at a fresh-clone path on the 6 machine-locked v5.0.0 tests — the exact blocker Step 7 resolves. --all is documented usage, so 40-validate-standalone.sh --all would falsely FAIL config-audit. Only 99-dryrun.sh and the RUNBOOK special-case it.", - "recommended_action": "Read each target's sc2_gate and delegate to that script when present (mirroring 99-dryrun.sh) instead of running the raw test_cmd." - }, - { - "id": "1708e9046e56b0c923122a8f60f4674eafbb31a9", - "severity": "MAJOR", - "rule_key": "PLAN_EXECUTE_DRIFT", - "original_rule_key": "FRAGILE_ASSUMPTION", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.sh", - "line": 113, - "brief_ref": "NFR — code correctness (SC6)", - "title": "Tag report uses git tag | head -1, relies on an unverified single-tag invariant", - "detail": "tag=$(git tag | head -1) is correct only because 10-extract.sh strips all tags and re-creates exactly one. A partial tag-strip leaves multiple tags; head -1 silently reports the lexicographically-first wrong tag while the dry-run still passes. Nothing asserts tag count == 1.", - "recommended_action": "Assert exactly one tag survives and FAIL (ok=0) otherwise, instead of head -1." - }, - { - "id": "4e494c8a16d9dca9fbf3c116b0f55e96b18c096f", - "severity": "MAJOR", - "rule_key": "PLAN_EXECUTE_DRIFT", - "original_rule_key": "CROSS_FILE_REGRESSION", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.sh", - "line": 145, - "brief_ref": "NFR — code correctness (SC2)", - "title": "SC2 regression capture depends on a side-effect clean room, masking real regressions if absent", - "detail": "capture_fails reads /tmp/claude-$key, populated only as a side effect of 40-validate-standalone.sh. Implicit untested coupling: if that path changes or the dir is absent, grep matches nothing, comm -23 yields empty, and the target is reported PASS with zero regressions — masking a real one.", - "recommended_action": "Capture the standalone failing set from $WORK/$key (the tree the dry-run itself preps), or assert /tmp/claude-$key exists before capture_fails." - }, - { - "id": "aeb6292d4c528ad4320c7b6e29621b9f96047558", - "severity": "MAJOR", - "rule_key": "PLAN_EXECUTE_DRIFT", - "original_rule_key": "FRAGILE_ASSUMPTION", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.sh", - "line": 54, - "brief_ref": "NFR — code correctness", - "title": "live_files/mappaths assume whitespace-free map paths with no validation", - "detail": "for p in $(mappaths \"$key\") relies on space-joined paths and word-splitting; correct only while every plugin-map.json path is whitespace/glob-free. No assertion enforces this, and git ls-files \"$p\" would mis-count a path with spaces.", - "recommended_action": "Document and assert the no-whitespace-in-paths invariant in 00-preflight.sh, or emit newline-separated paths consumed via while IFS= read -r." - }, - { - "id": "5d112cb49d23a4d23f6e45df4d79822eee01d17a", - "severity": "MAJOR", - "rule_key": "MISSING_TEST", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.test.mjs", - "line": 73, - "brief_ref": "SC6", - "title": "No negative test exercises the SC6 DROP or SC2-regression FAIL branch", - "detail": "The dry-run test only asserts the happy path (no DROP, no regression: in the live report). The two highest-stakes detectors — SC6 DROP (the 87-file-drop class) and SC2 regression FAIL — have no negative test proving they fire, unlike the SC7 planted-leak test at 40-validate-standalone.test.mjs:31. A regression disabling the DROP comparison would pass every existing test.", - "recommended_action": "Add a negative test that plants a file-drop / injected failure in a fixture extract and asserts 99-dryrun reports DROP / regression: and exits non-zero." - }, - { - "id": "9e588ca48abe3104725dd90df62e2c5aad75c3de", - "severity": "MAJOR", - "rule_key": "UNDECLARED_DEPENDENCY", - "original_rule_key": "HIDDEN_DEPENDENCY", - "file": "docs/marketplace-polyrepo-migration/migration/10-extract.sh", - "line": 68, - "brief_ref": "RUNBOOK preconditions", - "title": "git-filter-repo and python3 are undeclared hard runtime deps", - "detail": "10-extract.sh:68 invokes git filter-repo with no own presence check, relying on 00-preflight.sh — but the self-heal only runs preflight when the mirror is absent, so a since-uninstalled filter-repo dies with a raw error. git-filter-repo is a non-default git extension and python3 is used pervasively; neither appears in RUNBOOK preconditions.", - "recommended_action": "Add git-filter-repo and python3>=3.6 to RUNBOOK preconditions and re-assert git filter-repo --version in 10-extract.sh before line 68." - }, - { - "id": "bc0f8a7df6d384d7288f533ca3485b5989c06b69", - "severity": "MINOR", - "rule_key": "PLAN_EXECUTE_DRIFT", - "original_rule_key": "SC_PARTIAL", - "file": "docs/marketplace-polyrepo-migration/migration/plugin-map.json", - "line": 96, - "brief_ref": "SC6", - "title": "ms-ai-architect ships a hardcoded blob_strip_safe despite documenting it as preflight-computed", - "detail": "The map _comment says blob_strip_safe is (re)computed by 00-preflight.sh, yet ms-ai-architect carries blob_strip_safe:true as committed data (every other target is null). If preflight is skipped or a non-screenshot >1MB blob enters history, 10-extract.sh selects --strip-blobs-bigger-than 1M off a stale true and could corrupt the extracted history (SC6).", - "recommended_action": "Reset targets[\"ms-ai-architect\"].blob_strip_safe to null and rely on 00-preflight.sh to populate it per run." - }, - { - "id": "8d649e966f4ae94dd2b11e7374982e6f96c8bc52", - "severity": "MINOR", - "rule_key": "MISSING_ERROR_HANDLING", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.sh", - "line": 126, - "brief_ref": "SC6", - "title": "SC6 DROP branch mislabels a failed extract as content DROP", - "detail": "When extraction fails, ext_files stays 0 but the SC6 block still runs and emits sc6=0/ DROP, attributing an extraction failure to content loss. The ok=0 path is correct but the label misdirects the operator.", - "recommended_action": "Gate the SC6 comparison behind the same [ -d \"$dest/.git\" ] success that guards the extraction block, and emit a distinct (extract failed) label." - }, - { - "id": "4044c49e4a47cad56de6b56d9cb9d85c24443b86", - "severity": "MINOR", - "rule_key": "MISSING_ERROR_HANDLING", - "file": "docs/marketplace-polyrepo-migration/migration/99-dryrun.sh", - "line": 144, - "brief_ref": "SC2", - "title": "Unguarded mktemp under set -uo can yield a false SC2 PASS", - "detail": "The regression block runs under set -uo pipefail (no -e). If mktemp fails, sf/bf are empty, comm -23 yields empty, regr becomes empty, and the target is reported PASS (0 pre-existing) — a false PASS. -u does not catch this since the vars are defined-but-empty.", - "recommended_action": "Guard each mktemp (sf=$(mktemp) || fail) and treat an empty capture as an error, not as zero regressions." - } - ] + "verdict": "ALLOW", + "counts": { "BLOCKER": 0, "MAJOR": 0, "MINOR": 0, "SUGGESTION": 0 }, + "normalization": { + "mode": "default (high-effort)", + "pass3_skipped": true, + "pass3_skip_reason": "high-effort maximal-discipline standing instruction; Cloudflare reasonableness filter bypassed per v5.1.1", + "rule_key_substitutions": 0, + "note": "Final pass after full remediation. All 12 arc findings resolved; both 30-fix-references.mjs MAJORs re-verified RESOLVED by the independent code-correctness reviewer with no new correctness issue." + }, + "findings": [] } ```