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
TOOLKIT_ROOT="$(cd "$(dirname "$(realpath "$0")")/../../../.." && pwd)"

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:

State the rule once, in prose, and write the assertion from the rule — not from a run.
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`)

Critical Patterns — Recurring Review Findings¶

P-001: Hardcoded Airtable Infrastructure IDs¶

Anti-pattern¶

Correct pattern¶

Affected files¶

Detection signal¶

P-002: Hardcoded User Paths in Shared Scripts¶

Anti-pattern¶

Correct pattern¶

Affected files¶

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

Anti-pattern¶

Correct pattern¶

Checklist for version bump tasks¶

Detection signal¶

Affected instances¶

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

Anti-pattern¶

Correct pattern¶

Detection signal¶

The test pathology that let it recur¶

Affected instances¶

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