394 lines
26 KiB
Markdown
394 lines
26 KiB
Markdown
---
|
||
type: trekreview
|
||
review_version: "1.0"
|
||
created: 2026-06-17
|
||
task: "Marketplace polyrepo-migration tooling — 11-step extraction/validation/cutover harness"
|
||
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
|
||
---
|
||
|
||
# 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.
|
||
|
||
## 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 | 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 |
|
||
|
||
## Findings
|
||
|
||
### BLOCKER
|
||
|
||
#### `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.
|
||
|
||
### MAJOR
|
||
|
||
#### `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`.
|
||
|
||
#### `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`.
|
||
|
||
#### `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`.
|
||
|
||
#### `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/<key>` 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/<lf> 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.
|
||
|
||
## 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**
|
||
|
||
**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).
|
||
|
||
```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/<lf> 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."
|
||
}
|
||
]
|
||
}
|
||
```
|