Critical Patterns — Recurring Review Findings

Patterns that have appeared 3+ times across /vt-d-4-review runs and require active awareness during development. Each entry documents the anti-pattern, the correct fix, and where it has been found.

---

P-001: Hardcoded Airtable Infrastructure IDs

Status: Fixed in airtable_client.py (2026-03-05) Recurrence: 4 times across reviews (first flagged by pattern-recognition-specialist)

Anti-pattern

# BAD — hardcoded table IDs
CONTACT_TABLE_ID = "tblGYmwykHGUu9r3k"
COMPANY_TABLE_ID = "tbl4U3xdsw3wTdWmI"

Airtable table IDs (tbl...) and base IDs (app...) are infrastructure identifiers that differ between production, staging, and test bases. Hardcoding them makes environment switching impossible without source changes, and leaks internal schema identifiers into version control.

Correct pattern

# GOOD — env var with production default documented inline
CONTACT_TABLE_ID = os.environ.get("AIRTABLE_TABLE_CONTACT", "tblGYmwykHGUu9r3k")
COMPANY_TABLE_ID = os.environ.get("AIRTABLE_TABLE_COMPANY", "tbl4U3xdsw3wTdWmI")

• Use AIRTABLE_TABLE_<NAME> env vars for table IDs
• Use AIRTABLE_BASE_ID for base IDs (already parameterised in function signatures)
• Keep verified production defaults as the fallback so existing deployments keep working
• Comment the verification date so staleness is detectable

Affected files

File	Fix applied
plugins/ufi/src/ufi_pipeline/airtable_client.py:27-30	2026-03-05

Detection signal

Code review tools consistently flag constants matching ^tbl[A-Za-z0-9]{14}$ or ^app[A-Za-z0-9]{14}$ as hardcoded Airtable infrastructure IDs.

---

P-002: Hardcoded User Paths in Shared Scripts

Status: Fixed in session-start.sh (2026-03-05) Recurrence: Flagged as S-01 in review

Anti-pattern

# BAD — breaks for any user other than rolf
TOOLKIT_ROOT="/Users/rolf/01-repositories/V025-claude-toolkit"

Correct pattern

# GOOD — derives root from script location, works for any user/path.
# THREE levels, because this script lives at plugins/<plugin>/scripts/:
#   scripts -> <plugin> -> plugins -> <repo root>
TOOLKIT_ROOT="$(cd "$(dirname "$(realpath "$0")")/../../.." && pwd)"

⚠ Count the levels for YOUR script's location; do not copy the number. This block published ../../../.. — four levels — until 2026-08-27 (BUG-070), while the working script it credits, session-start.sh:22, has always used three. Copied verbatim from plugins/<plugin>/scripts/ it resolves to the repository's PARENT, after which the block it feeds is guarded by a [ -f ... ] that is simply false: no error, no output, nothing. That is worse than the anti-pattern above, which at least fails loudly for other users.

When a script is a peer of its containing project (installed into a known relative path), derive the project root from $0 rather than hardcoding it.

Affected files

File	Fix applied
plugins/vt-base/scripts/session-start.sh:22	2026-03-05

---

P-003: Stale components.* Counts in plugin.json (Added from review retrospective 2026-05-14)

Status: Auto-fixed in SPEC-133 review (2026-05-14) Recurrence: 3 times across reviews Signal verified: 2026-08-13, 8/8 plugins — and pinned by scripts/tests/setup/57-plugin-component-counts-agree.sh, which executes this checklist's derivation and fails if this section ever republishes a hardcoded namespace prefix (BUG-067)

Anti-pattern

"description": "... 54 agents, 118 skills ...",
"components": {
    "agents": 53,
    "skills": 117
}

Version bump tasks (MINOR bump for new skill/agent) update version, description, and the skills/agents count inside description — but miss the separate components.* block. The two counts diverge silently.

Correct pattern

When adding a new skill: update ALL of the following in plugin.json: 1. "version": bump MINOR (e.g., 3.36.0 → 3.37.0) 2. "description": update N skills count 3. "components": { "skills": N }: update to match

When adding a new agent: also update: 1. "description": update N agents count 2. "components": { "agents": N }: update to match

Checklist for version bump tasks

[ ] version field bumped
[ ] description string count(s) updated
[ ] components.skills matches actual manifest line count: grep -v '^#' .claude-plugin/skill-symlinks.manifest | grep -c ' -> '
[ ] components.agents matches actual manifest line count: grep -c "^[a-zA-Z]" .claude-plugin/agent-symlinks.manifest

Detection signal

components.skills !== grep -v '^#' .claude-plugin/skill-symlinks.manifest | grep -c ' -> '

Two traps, both already paid for. Derive per plugin — each manifest carries its own vt-{x}- prefix, so no prefix literal belongs in this command; one hardcoded here returned 0 for 7 of 8 plugins and read as drift-free (BUG-067). And strip comments first — every manifest header contains a literal ->, so a naive grep -c ' -> ' overcounts by one per file.

The agents line needs no such treatment: agent names were never prefixed, and on the three plugins with no agent manifest it fails loudly (warning, exit 2) rather than returning a plausible 0.

Affected instances

Branch	Fix applied
feature/spec-133-quick-fix-intake-path	2026-05-14 — components.skills 117→118

---

P-004: A Repo-Scoped Fact Leaking into a Diff-Scoped Decision (Added from review retrospective 2026-08-09)

Status: All three variants Fixed in SPEC-168 (v3 in 4d399ac6, verified by four independent main-vs-HEAD sweeps: 0 drops, every delta exactly +php-reviewer) Recurrence: 3 times, all inside a single spec (SPEC-168), and the test suite was green for each

Anti-pattern

Two different kinds of fact get collapsed into one boolean:

• a repo-scoped fact — "this project contains PHP", evidenced by a composer.json marker;
• a diff-scoped fact — "this changeset contains PHP", evidenced by *.php in the changed-file list.

# BAD — one boolean, two kinds of evidence
has_php=false
if $diff_known && files_match '\.php$'; then has_php=true; fi
if [ -f "$PROJECT_ROOT/composer.json" ]; then has_php=true; fi   # repo-scoped, same variable
...
if $has_php; then add php-reviewer; swapped=true; fi              # now drives a diff-scoped decision

"This project is PHP" is not evidence that "this diff is not TypeScript". Once the two are merged, the repo-scoped marker starts moving decisions that only the diff can support — and it does so in both directions:

• suppression — the marker drops a reviewer the diff never argued against (a silent drop, the BUG-004 class);
• conjuring — the marker summons a reviewer onto a changeset proven to contain none of its language, because the marker opened an enclosing if that an unguarded default sits inside.

SPEC-168 hit all three variants in sequence: v1 suppressed, v2 (the fix for v1) conjured the default TypeScript reviewer, and v3 (the fix for v2) closed both for TypeScript and Python while leaving the angular.json / nest-cli.json branches — three lines above, and newly reachable because the PHP marker now opens the same enclosing condition — unguarded.

Correct pattern

Keep the two signals in separate variables, and let only the diff-scoped one move a diff-scoped decision:

# GOOD — separate signals; the marker selects its OWN reviewer and nothing else
has_php_diff=false
if $diff_known && files_match '\.php$'; then has_php_diff=true; fi
has_php_repo=false
if [ -n "$(find "$PROJECT_ROOT" -maxdepth 2 -name composer.json ...)" ]; then has_php_repo=true; fi

if $has_php_diff || $has_php_repo; then add php-reviewer; fi        # either signal SELECTS
if $has_php_diff && ! $has_ts; then swapped=true; fi                # only the DIFF may suppress

# v3 — the branches missed twice. angular.json says WHICH TypeScript reviewer, never WHETHER the
# diff is TypeScript, so they need the same guard. Note it is the enclosing condition MINUS the
# newly-added signal, not the narrower "! $diff_known || $has_ts" that reads more principled:
# the narrow form drops angular-reviewer from a .py diff where it previously fired, trading an
# over-selection for a silent drop. Measured, not reasoned — 108 of 408 cells.
want_lang_pre_php=false
if ! $diff_known || $has_ts || $has_py; then want_lang_pre_php=true; fi
if [ -f "$PROJECT_ROOT/angular.json" ] && $want_lang_pre_php; then add angular-reviewer; swapped=true; fi

if ! $swapped && { ! $diff_known || $has_ts; }; then add kieran-typescript-reviewer; fi

The generalisable guard, which is the part that was missed twice: when a fix establishes a rule, enumerate every sibling branch the rule governs and assert the rule on each. A rule applied to three of five branches is a rule that will be rediscovered as a bug.

Detection signal

Grep for a variable that is assigned from both a files_match-style diff probe and a [ -f ... ] marker probe, then read by anything other than the add for its own reviewer. Any such variable is this defect waiting to happen.

Empirically: run the selector against a fixture whose root carries a marker and whose diff is README.md only. Every reviewer in the output must have diff-scoped grounds, except the marker's own.

Two caveats, both learned by executing this test against the fixed code rather than assuming it passed.

It reports one known exception. With angular.json and requirements.txt at the root and a README.md-only diff, angular-reviewer is selected — its grounds are "a Python marker exists," which is neither diff-scoped nor its own marker. That is main's pre-existing behaviour, retained deliberately: removing it here would have been a silent drop against main, which SC-4 forbids. Whether a framework reviewer should fire on a pure-Python diff is a real question, deferred to SPEC-179. Treat this cell as a known exception, not a fresh instance.

Do not compare against an idealised rule; compare against main. The acceptance test that actually caught the v3 fix's own defect was a differential sweep — run the previous revision of the selector and the new one over the same repo × diff matrix and require zero drops. That is what distinguishes "removed an over-selection" from "introduced a silent drop," and it is the one check that a rule stated in prose cannot give you.

The test pathology that let it recur

The suite was green all three times, because the assertion (test-persona-selection.sh case T8(f)) was written to match whatever the code currently did. It asserted php-reviewer alone in v1 (encoding the suppression defect), php-reviewer + kieran-typescript-reviewer in v2 (encoding the conjure defect), and php-reviewer alone again in v3 — the same expected set as v1, reached for the opposite reason.

A green suite is not evidence when the assertion was derived from the behaviour rather than from the rule. Two guards, both cheap:

1. State the rule once, in prose, and write the assertion from the rule — not from a run.
2. Mutation-test in both directions. In SPEC-168 the conjure direction bit and the suppression direction did not, and nothing revealed the asymmetry until a reviewer restored v1's collapsed form and watched the suite stay at 44/0.

Related: docs/solutions/patterns/suppression-mirrors-detection.md (a suppressor must be anchored as tightly as the detection it cancels) — this is its diff-scope sibling.

Affected instances

Branch	Variant	Status
feature/spec-168-project-stack-descriptor	v1 — marker suppresses the TS reviewer	Fixed (RCI F2, 986aa701)
feature/spec-168-project-stack-descriptor	v2 — marker conjures the TS reviewer	Fixed (review pass 1, H-B/H-C)
feature/spec-168-project-stack-descriptor	v3 — marker conjures angular-reviewer / nestjs-reviewer	Fixed (review pass 2, H-E, 4d399ac6)

---

P-005: A Verdict Without a Denominator (Added from BUG-077 review, 2026-08-17)

A verification step reports success without reporting its scope. PASS (all checksums verified) is true when 26 files were checked and equally true when zero were, because the sentence never names a number. Every bypass works the same way: don't defeat the comparison, remove the thing from the set being compared.

Measured cost in BUG-077: the hook-integrity verifier had been checking nothing in the toolkit repo for 144 days, printing a full-verification claim on every run, while nine pins drifted.

# BAD — the same line for 26 checks and for zero
echo "  PASS (all checksums verified)"

# BETTER — but still self-consistent, so entry removal is invisible:
#   deleting a whole entry lowers checked AND declared together
echo "  checked=$checked"

# GOOD — denominator, every outcome bucket, and a CLOSURE check whose
#        population comes from OUTSIDE the list being verified
for f in $(approved_location_hooks); do
  grep -qxF "$f" <(cut -f1 "$pairs") || { echo "  CRITICAL: unpinned: $f"; unpinned=$((unpinned+1)); }
done
echo "  checked=$checked mismatching=$m missing=$k unparsed=$u unpinned=$unpinned"

Three rules: a zero denominator is a finding, never a pass; the expected count must be anchored outside the source under test; and the set must be closed — match on the property ("a thing this repo executes"), never on a naming convention.

⚠ Two traps this pattern has, both hit on the branch that fixed it.

1. The fix carries the bug into the new code. Reintroduced three times in one branch: comment-stripping rescued a trailing-comment key and erased a leading-comment one; the new count was derived comment-blind while the code strips comments; the closure scan globbed *.sh and missed the one extensionless pin. Each fix reasoned about the case in front of it rather than the property.
2. Existing tests start passing for the wrong reason. Adding closure made two regression cases stop testing what they named — their assertion was "a CRITICAL appeared, exit != 0", and closure supplies both for an unrelated reason. Mutating the parser to reintroduce the original bypass left the suite green. Each case must assert its own signature (which counter moved), not merely that something complained.

Full write-up: docs/solutions/patterns/verdict-needs-a-denominator.md — includes the self-reference confound (mutating a verifier that is itself pinned invalidates its own entry, so the run goes red for the wrong reason) and the over-loose-matcher floor (case-insensitive critical also matches RESULT: 0 critical, 0 warnings).

Related: suppression-mirrors-detection.md (a suppressor must be anchored as tightly as the detection it cancels) — the denominator case is the same rule applied to a counter.

Affected instances

Where	Variant	Status
repo-audit-ci.sh Check 4 → Check 0	PASS (all checksums verified), no denominator, unreachable for 144 days	Fixed (BUG-077)
repo-audit-ci.sh closure	entry removal invisible; count had no anchor outside the manifest	Fixed (BUG-077)
60-hook-integrity-cannot-fail-open.sh	own denominator satisfiable by 1 of 26	Fixed (BUG-077 review)
60-hook-integrity-cannot-fail-open.sh	C-1/C-3 passing via closure, not their named mechanism	Fixed (BUG-077 review)
BUG-066	a working detector decayed into a no-op	Pre-existing, recorded

P-006: A Pattern Doc Whose Exemplar Fails Its Own Invariant (Added from review retrospective 2026-09-22)

A pattern document states a rule and cites an in-tree example as the thing to copy. When the cited example does not satisfy the rule the same page states, every reader who copies the example copies the defect, and the page's authority makes it look settled. Third instance in review history (SPEC-157 P2-C1/P2-C2, twice; SPEC-191 PAT-4).

Anti-pattern

negative-control-shapes.md states the invariant "a labelled row that can only pass if a deliberately-defective subject WAS detected AND a deliberately-clean twin was NOT", then names guard 93's NC1 as the S2 exemplar. That row asserted only the first half: the injected registry row appears on the mutated page. Nothing asserted the unmutated page lacks it.

And this entry shipped self-violating, in the same commit as the unfixed exemplar, while its own Correct pattern below says to fix the example first or name the clause it lacks. Writing a finding down is not closing it: a pattern entry that records an open defect in the present tense reads as settled guidance. Fixed 2026-09-22 — guard 93's NC1 now asserts both halves and is mutation-proven, so the citation is true and this entry is kept for the class, not the instance.

Correct pattern

Before citing an exemplar, check it against the page's own rule, clause by clause, and cite the line that satisfies each clause. If no in-tree example satisfies every clause, fix the example first or say which clause it lacks. An exemplar is a test fixture for the pattern: it has to pass. And if the fix cannot land in the same commit as the finding, the citation carries the caveat until it does — a reader copies the example, not the review record.

Source: Review findings on feature/spec-191-guards-ship-with-a-negative-control — pattern-recognition-specialist (PAT-4) and tdd-compliance-reviewer (TDD-4) found it independently.

P-007: A Review Isolation That Isolates the Wrong Commit (Added from review retrospective 2026-09-15)

⚠ Recorded as P-006 in SPEC-192's pass-11, pass-12 and pass-13 review records, in review-triage.md, and in state.yaml's gate_history. Both this branch and main appended a different P-006; main's kept the number at the merge and this one moved. Those records are dated and are deliberately NOT rewritten, so a reader following P-006 from them lands on the pattern above — look here instead.

❌ WRONG: Dispatch a measuring reviewer or the test-runner with isolation: "worktree" and trust that the worktree holds the commit under review. On SPEC-192 it never did, four passes running: the worktree came up at origin/main (pass 1), 2b103878 (pass 2), 52428958, the HEAD of an unrelated branch (pass 3), and 188265c0 (pass 4). Pass 2's corrective git checkout was blocked by the guard hook and the test gate ran nothing; pass 4's test-runner made no progress for 30 minutes and never reported. The isolation reports as granted either way, so a measurement of the wrong tree reads exactly like a measurement of the right one.

✅ CORRECT: Before dispatch, create one detached worktree per measuring agent at the exact commit — git worktree add --detach .worktrees/rpN/<name> <sha> — confirm git rev-parse --short HEAD and an empty git status --porcelain for each, then dispatch without isolation and name the absolute worktree path and the target SHA in the prompt. Require the agent to report the SHA it measured. Check every worktree's porcelain again at grading time.

Source: Review findings on feature/spec-192-no-verdict-without-measurement — vt-d-4-review orchestrator detected 4 times (passes 1-4); the pre-created form held in passes 2 and 4. Related: v025-multi-worktree-workflow.md; /vt-d-4-review Step 2 "How a mutating reviewer is dispatched" (BUG-204), which names the wrong-repository variant of the same failure.