docs(migration): trekreview re-review — ALLOW (all 12 arc findings resolved)
Re-review scope a44e37b..85a8ee3. BLOCK (1+6+3) → WARN (2 new MAJOR surfaced in 30-fix-references.mjs) → ALLOW (both remediated, re-verified by the independent code-correctness reviewer). 0 BLOCKER / 0 MAJOR / 0 MINOR / 0 SUGGESTION. Operator window (RUNBOOK.md) no longer gated by a review BLOCK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
85a8ee345e
commit
d577cadcf4
1 changed files with 67 additions and 367 deletions
|
|
@ -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/<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.
|
||||
_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/<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."
|
||||
}
|
||||
]
|
||||
"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": []
|
||||
}
|
||||
```
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue