Pr Review Community Overlay

PR Review — Community Overlay (ArcKit)

Reviews ArcKit community-overlay PRs (xx-* jurisdictional command bundles) against the integration checklist that catches the recurring failure classes. Encodes the checklist used on PR #441 (au-federal) which surfaced 4 blockers and 6 important issues that the author had missed.

When to Use

• PR adds new community overlay (e.g. au-federal, ca-federal-fitaa, future au-vic-*)
• PR extends an existing overlay (new xx-* command, new recipe target)
• PR ships a new build recipe alongside xx-* commands
• Author is contributor (not maintainer) and PR is large (10+ files, 5K+ lines)

Skip for: official-baseline command changes, plugin infrastructure, hooks, agents — these have their own review surfaces.

Output shape (always)

## Code Review

**Verdict:** [one-line conclusion]

### BLOCKERS (must fix before merge)
[numbered, file:line, fix instruction, comparison to working precedent]

### IMPORTANT (should fix)
### MINOR (nits)
### POSITIVE (worth calling out)
### Recommended pre-merge sequence
[ordered fix steps]

End with: gh pr comment <N> --body "$(cat <<'EOF'...EOF)" to post.

The 5 recurring blocker classes

These are the failure modes that recur across overlay PRs. Check each, in order:

B1 — Templates lack ## Document Control heading

Symptom: Templates have <!-- DOC-CONTROL-HEADER --> directly under the > **Template Origin**… blockquote with no ## Document Control heading above.

Why it matters: The partial (document-control-uk.md / document-control-uae.md) is just the table — it has no heading. After inlining, artefact has a Document Control table with no section heading.

Verification:

for f in plugins/arckit-claude/templates/<prefix>-*-template.md; do
  head -10 "$f" | grep -q "^## Document Control" || echo "MISSING heading: $f"
done

Working precedent: plugins/arckit-claude/templates/ca-pia-template.md:5-7 and uae-pdpl-template.md:5-7 both have ## Document Control\n\n<!-- DOC-CONTROL-HEADER -->\n<!-- Resolved at command-execution time per _partials/RENDERING.md. -->.

B2 — Commands don't override the UK classification line

Symptom: Overlay introduces a non-UK classification scheme (Canadian Protected/A/B/C, AU UNOFFICIAL/PROTECTED, etc.) but commands say only Resolve the <!-- DOC-CONTROL-HEADER --> marker per RENDERING.md. — no instruction to substitute the classification line.

Why it matters: plugins/arckit-claude/templates/_partials/RENDERING.md only routes governance_framework: UAE Federal or classification_scheme: UAE Smart Data to a non-UK partial. Everything else falls back to document-control-uk.md which renders [PUBLIC / OFFICIAL / OFFICIAL-SENSITIVE / SECRET]. Without an explicit override, the body content (which references the AU/CA/etc. ladder) becomes inconsistent with the header.

Verification:

grep -L "classification scheme\|UK line in the header" arckit-<prefix>/commands/<prefix>-*.md
# Files printed = files MISSING the override

Working precedent: plugins/arckit-ca/commands/ca-pia.md:32:

Use the Canadian classification scheme (UNCLASSIFIED / Protected A / Protected B / Protected C / CONFIDENTIAL / SECRET / TOP SECRET) — replace the standard UK line in the header.

Proper long-term fix (separate PR): ship a document-control-<regime>.md partial, extend RENDERING.md routing, extend plugin.json userConfig to accept the regime governance framework value.

B3 — generate-document-id.mjs mis-invocation

Symptom: Commands write node scripts/generate-document-id.mjs <TYPECODE> --filename (single positional arg).

Why it matters: The signature is PROJECT_ID DOC_TYPE [VERSION] [OPTIONS]. With one positional arg, TYPECODE is read as PROJECT_ID and DOC_TYPE is empty, so the call exits 1 and the model invents a filename.

Since #723 the generator itself catches this (PROJECT_ID must be numeric ... looks like a doc-type), so it now fails loudly at runtime rather than silently. Still worth catching at review: a loud runtime failure is a broken command.

Verification:

grep -n "generate-document-id.mjs" arckit-<prefix>/commands/<prefix>-*.md | grep -v "<PROJECT_ID>\|{P}"

Working precedent: plugins/arckit-ca/commands/ca-pia.md:32:

node scripts/generate-document-id.mjs <PROJECT_ID> PIA --filename

Note: all 12 uae-* commands had this bug; fixed in #722.

B4 — Converter outputs are stale

Symptom: Re-running python scripts/converter.py on the PR branch produces drift on files that were not the focus of the PR (commonly arckit-pages variants).

Why it matters: Author may have run the converter at branch creation and not re-run after rebasing main. Result: variants regress to an older version of canonical commands.

Verification:

git checkout pr-<N>
python scripts/converter.py
git status --porcelain | grep -v "memory/"
# Should be empty. Anything printed = stale converter output in the PR.

Cross-check: Run the same on main first to confirm main itself is clean (otherwise the drift isn't the PR's fault).

Fix: ask author to rebase on main, re-run converter, and amend.

B5 — Regime not registered in REGIMES / REGIME_LABELS

Symptom: New regime: '<XX>' declared on each new doc-type entry in plugins/arckit-claude/config/doc-types.mjs, but the <XX> code is missing from the exported REGIMES array and/or REGIME_LABELS object.

Why it matters: Per-record regime: controls which bucket each artefact falls into, but consumers iterating REGIMES (dashboards, group headers, navigator UI) silently skip the unregistered jurisdiction. The HIGH_SEVERITY_BY_REGIME derivation has a fallback so the local file still works — but downstream consumers don't. Symptom is invisible in unit tests.

Verification:

# Confirm the new regime code appears in BOTH the array and the labels object.
# Anchor on the export blocks (avoids matching every doc-type entry as noise).
grep -nA 1 "^export const REGIMES" plugins/arckit-claude/config/doc-types.mjs
grep -nA 12 "^export const REGIME_LABELS" plugins/arckit-claude/config/doc-types.mjs
# Then cross-check against the regime values declared on new doc-types
grep -oE "regime: '[A-Z]+'" plugins/arckit-claude/config/doc-types.mjs | sort -u

Working precedent: PR #441 (au-federal) added both 'AU' and 'CA' (the latter a corrective for an earlier omission), e.g. plugins/arckit-claude/config/doc-types.mjs:165-179. Note the REGIME_LABELS ordering convention is officially-maintained-first then community alphabetical.

Discovered in: Test review of merged PR #432 (ca-federal-fitaa) where 12 doc-types declared regime: 'CA' but the REGIMES array shipped without 'CA'. Bug shipped and was only fixed retroactively in #441.

Integration checklist

Beyond the 4 blockers, verify:

Per-command checklist

For each new xx-* command file:

• Frontmatter: description present. No invalid fields (name:, color:, permissionMode:, tools: are NOT valid plugin command frontmatter — name: in particular is a common contributor mistake; filename is source of truth).

• $ARGUMENTS placeholder present in body — tests/plugin/test_commands_structure.py::test_arguments_placeholder_present checks this. Existing ca-*/uae-* commands fail this test (32 known failures); new overlay commands should pass.

• create-project.sh lookup — only required for project-bootstrapping commands (principles, requirements, the first command someone runs). Most overlay commands assume the project already exists and skip this; that matches the FR/AT/EU/CA precedent. Not a defect when absent.

• generate-document-id.mjs invoked correctly (B3).

• <!-- DOC-CONTROL-HEADER --> resolution instruction (B2 — should override classification when not UK/UAE).

• Write tool used to save artefact (32K output token limit otherwise).

• Handoffs: every handoffs.command value resolves to a real file in plugins/arckit-claude/commands/. Common bug: pluralisation (risks vs risk); deps on commands shipping in sibling PRs. Validation needs YAML parsing (handoffs are nested under YAML frontmatter — raw grep misses), e.g.:

  import yaml, glob, pathlib
  cmds = {pathlib.Path(p).stem for p in glob.glob('plugins/arckit-claude/commands/*.md')}
  for f in glob.glob('arckit-<prefix>/commands/<prefix>-*.md'):
      fm = yaml.safe_load(open(f).read().split('---')[1])
      for h in (fm or {}).get('handoffs', []):
          if h['command'] not in cmds: print(f, '→', h['command'])
  

• Template path references ${CLAUDE_PLUGIN_ROOT}/templates/<name>-template.md with templates-custom then .arckit/templates/ fallback.

• Per-template footer: **Generated by**, **Generated on**, **ArcKit Version**, **Project**, **Model** all present.

UK leakage check

For non-UK overlays, grep for unintended UK terminology:

grep -rE '\b(NCSC|ICO|Cyber Essentials|GovS|UK GDPR|GDS|Cabinet Office|DPA 2018|DPIA)\b' arckit-<prefix>/commands/<prefix>-*.md plugins/arckit-claude/templates/<prefix>-*.md

A small number of intentional comparison references is fine (PR #441 had 2 in au-dss.md + au-pia.md) — author should call them out in the PR body. Anything else is leakage.

Tests + CI

• Run pytest tests/plugin/ and check for new failures specific to this PR's files.
• If PR adds .github/workflows/* (first-time CI), run the full suite locally and report pre-existing failure count to the author.
• If PR adds a recipe-validation test, verify it codifies the headline scorecard claims (target count, deps, doc-types registration, regime registration, pages allow-list).

Verification commands (one-shot)

# Setup
git fetch origin pull/<N>/head:pr-<N>
git checkout pr-<N>

# B1: missing Document Control heading
for f in plugins/arckit-claude/templates/*-template.md; do
  head -10 "$f" | grep -q "^## Document Control" || echo "B1 missing: $f"
done | grep -E "<prefix>-"

# B2: missing classification override
grep -L "classification scheme\|UK line in the header" arckit-<prefix>/commands/<prefix>-*.md

# B3: generate-document-id.mjs mis-invocation
grep -n "generate-document-id.mjs [A-Z]\+ --filename" arckit-<prefix>/commands/<prefix>-*.md

# B4: converter drift
python scripts/converter.py && git status --porcelain | grep -v "memory/"

# B5: regime registration (anchored on export blocks to avoid doc-type noise)
grep -nA 1 "^export const REGIMES" plugins/arckit-claude/config/doc-types.mjs
grep -nA 12 "^export const REGIME_LABELS" plugins/arckit-claude/config/doc-types.mjs
grep -oE "regime: '[A-Z]+'" plugins/arckit-claude/config/doc-types.mjs | sort -u

# Recipe schema
python3 -c "
import yaml
r = yaml.safe_load(open('plugins/arckit-claude/skills/arckit-build/recipes/<recipe>.yaml'))
ids = {t['id'] for t in r['targets']}
for t in r['targets']:
  for d in t.get('deps', []):
    d_clean = d.rstrip('*')
    if d_clean not in {i.rstrip('-') for i in ids} and not any(i.startswith(d_clean) for i in ids):
      print(f'BROKEN: {t[\"id\"]} -> {d}')
"

# Dual-template parity
diff -rq plugins/arckit-claude/templates/ .arckit/templates/ | grep -E "<prefix>-"

# Tests
pytest tests/plugin/ --tb=line -q 2>&1 | tail -10

# Command count check vs CHANGELOG
ls plugins/arckit-claude/commands/*.md | wc -l

Posting the review

Use the heredoc form so the markdown renders correctly:

gh pr comment <N> --body "$(cat <<'EOF'
## Code Review

**Verdict:** ...
EOF
)"

Verify with the URL the command returns.

Reference test cases

• PR #441 (au-federal, 2026-05-05) — canonical fixture for B1–B4. Exhibited all four (template heading, classification override, doc-id invocation, converter drift) plus 6 important issues. Posted review: https://github.com/tractorjuice/arc-kit/pull/441#issuecomment-4386152326
• PR #432 (ca-federal-fitaa, merged 2026-05-05) — fixture for B5 (regime registration). Shipped 12 doc-types with regime: 'CA' while 'CA' was missing from the REGIMES array. Bug only surfaced retroactively in #441. Validates the skill against an already-merged overlay where B1–B4 all PASS but a different blocker class exists.

Common mistakes by reviewers (self-checks)