feat(ci): deterministic craftsmanship gate for CLAUDE.md § Code Style

[cdeust]

Summary

• Adds scripts/check_craftsmanship.py (+ craftsmanship_rules.py/craftsmanship_imports.py/craftsmanship_constants.py/craftsmanship_baseline.py): a deterministic, AST-based gate for the four rules CLAUDE.md § Code Style states but nothing checked — file size (300), method size (40, via ast.FunctionDef/end_lineno), layer-boundary imports (docs/module-inventory.md § Dependency Rules, restricted to shared/core/infrastructure/server), and module-scope numeric constants missing a # source: comment.
• Runs on a diff's changed files only (never the whole repo), against a versioned .craftsmanship-baseline.json ratchet: pre-existing debt (1307 entries, generated 2026-08-10 via --write-baseline over the full tracked tree) does not retroactively block; a genuinely new violation does; a baseline entry whose violation no longer reproduces also blocks (forces pruning rather than silent drift).
• Wired into .github/workflows/ci.yml as a new craftsmanship job, gated by CI Green (ci-green.needs, verified via scripts/check_ci_gate_complete.py).
• Updates CLAUDE.md § Code Style to describe the gate instead of admitting it doesn't exist yet; updates SECURITY.md § Change control to note the new job and that a repo admin must add it to GitHub's required-checks list (this PR does not and cannot do that itself).

Test plan

• scripts/check_craftsmanship.py, pointed at mcp_server/infrastructure/pg_store_stats.py and the three wiki_* core modules named in the task, detects the known violations (file-size, method-size, pathlib/os layer violations) before baselining.
• python scripts/check_craftsmanship.py --base origin/main on this branch's real committed diff: clean (0).
• uv run --no-sync pytest tests_py/scripts/test_craftsmanship_rules.py tests_py/scripts/test_craftsmanship_imports.py tests_py/scripts/test_craftsmanship_constants.py tests_py/scripts/test_craftsmanship_baseline.py tests_py/scripts/test_check_craftsmanship.py -q: 59 passed, order-independent (verified both forward and reverse file order).
• ruff format --check / ruff check on all touched files: clean.
• pyright (standalone) on all touched files: 0 errors/warnings/informations.
• scripts/check_ci_gate_complete.py, scripts/check_doc_claims.py, scripts/check_version_surfaces.py, scripts/generate_repo_badges.py --check: all pass.
• actionlint -color .github/workflows/ci.yml: clean.
• CI (this PR) — gh run watch after push.

🤖 Generated with Claude Code

[cdeust]

ZETETIC-REVIEW: REQUEST_CHANGES

Summary

Bien conçu et bien testé (59/59 tests réels, pas de paraphrase), mais deux failles vérifiées empiriquement défont la promesse centrale de la gate : (1) le baseline peut être régénéré dans la même PR pour blanchir une violation qu'on vient d'introduire — CI ne s'en aperçoit pas ; (2) la règle 3 (import inter-couches) implémente une liste noire, pas la liste blanche du tableau qu'elle prétend appliquer — core/ peut importer requests/boto3/psycopg2 (n'importe quelle lib tierce faisant de l'I/O) sans être détecté, alors que docs/module-inventory.md décrit core/ comme « pure business logic, zero I/O » et « May Import: shared/ only ». Une gate qui rate exactement le cas qu'elle est censée bloquer est un faux négatif au sens où tu le crains.

Toutes les commandes ci-dessous ont été exécutées sur un clone jetable, en quelques secondes, jamais sur la suite complète.

1. Contournement AST — ce qui échappe, vérifié par exécution directe des détecteurs

Confirmé par exécution (craftsmanship_rules.scan_source, craftsmanship_imports.check_layer_violation) :

• core/ important une lib tierce quelconque échappe entièrement. import numpy, import requests, import scripts.legacy_bridge dans un fichier mcp_server/core/x.py → [] (aucune violation). Le détecteur (_import_violates_layer) n'interdit que os/pathlib (stdlib) et mcp_server.{infrastructure,handlers,server}.* — c'est une liste noire de composants nommés, pas la liste blanche « shared/ uniquement » que docs/module-inventory.md et la description prose de core/ (« pure business logic, zero I/O ») promettent. C'est le cas le plus probable et le plus dommageable en pratique — ajouter une dépendance I/O tierce à core/ — et il est totalement invisible pour la gate.
• Import via un module non préfixé mcp_server (importlib.import_module("mcp_server.infrastructure..."), ou un import statique ordinaire d'un module comme scripts.legacy_bridge qui lui-même fait de l'I/O) échappe à la règle 3 pour la même raison : _import_violates_layer ne matche que top == "mcp_server". Un intermédiaire de blanchiment hors du package mcp_server — même un import parfaitement statique, sans réflexion — passe.
• Constante calculée (TIMEOUT = 60 * 60) échappe à la règle 4 : _numeric_literal ne gère que Constant et UnaryOp, pas BinOp. Vérifié : scan_source sur TIMEOUT_SECONDS = 60 * 60 → [].
• Constante définie dans une classe (class Config: TIMEOUT = 3600) échappe : _module_level_numeric_assignments ne parcourt que tree.body (niveau module), jamais l'intérieur d'un ClassDef. Vérifié : [].
• Valeur par défaut d'argument (def foo(timeout=3600):) échappe pour la même raison — jamais un Assign/AnnAssign de module. Vérifié : [].
• Import fonction-locale : correctement détecté (le NodeVisitor descend dans les corps de fonction) — pas un trou.
• if TYPE_CHECKING: : exemption délibérée et raisonnable (import de type, jamais exécuté à l'exécution) — acceptable.

Verdict sur ces échappatoires : les trois premières (lib tierce dans core/, intermédiaire hors mcp_server, constante calculée) sont fatales parce qu'elles couvrent le cas d'usage normal, pas un contournement exotique. Les deux dernières (classe, argument par défaut) sont réelles mais mineures — non testées, non documentées comme limitation connue, à corriger ou au moins à noter explicitly comme hors-scope de la règle 4 (aujourd'hui la portée « module-scope » n'est même pas mentionnée dans le docstring public, seulement déductible du code).

2. Le baseline : cliquet ou passoire — vérifié par démonstration

Passoire, démontré. _run_gate (check_craftsmanship.py) charge known_baseline depuis le fichier .craftsmanship-baseline.json de l'arbre de travail — c'est-à-dire la copie que la PR elle-même peut modifier. Reproduction :

echo 'SNEAKY_LIMIT = 12345' >> mcp_server/shared/text.py
python3 scripts/check_craftsmanship.py mcp_server/shared/text.py
# → exit 1, violation NEW détectée (correct)

python3 scripts/check_craftsmanship.py --write-baseline
python3 scripts/check_craftsmanship.py mcp_server/shared/text.py
# → exit 0, "Craftsmanship gate: OK"

Le champ $comment dans le JSON généré (« Do NOT add an entry by hand... that is gaming the ratchet ») est un contrat social, pas technique — rien dans ci.yml ne diffuse le baseline contre origin/main avant de le charger, rien ne vérifie qu'une entrée nouvelle du baseline correspond à un fichier hors du diff de la PR. Avec 1305 entrées déjà grandfathered (568 method-size, 466 unsourced-constant, 230 file-size, 41 layer-violation), un JSON de cette taille est structurellement illisible à l'œil (au-delà du seuil des 400 lignes où Cohen 2006 dit qu'une revue humaine cesse d'être fiable) — la seule ligne de défense qui reste (revue humaine du diff du baseline) est précisément celle que la gate a été construite pour ne plus exiger.

Correctif concret : charger known_baseline via git show {base_ref}:.craftsmanship-baseline.json (référence immuable dans la PR) plutôt que le fichier de l'arbre de travail, ou — alternative plus simple — faire échouer la gate si une entrée neuve du baseline correspond à un fichier que la PR modifie elle-même. Le dépôt a déjà le bon patron pour ce genre de problème : scripts/check_ci_gate_complete.py refuse exactement ce type de dérive silencieuse (job ajouté à ci.yml sans être dans needs:) en comparant contre une source de vérité versionnée plutôt qu'en faisant confiance à la déclaration. Applique la même discipline ici.

3. Table des couches : dupliquée en dur, pas lue depuis la doc — et incomplète

FORBIDDEN_SECOND_COMPONENT dans craftsmanship_imports.py est un dict Python recopié à la main depuis docs/module-inventory.md § Dependency Rules — jamais parsé depuis le fichier. Violation DRY dans l'outil même qui prétend faire respecter les règles métier ; toute future édition de module-inventory.md divergera silencieusement du code, sans qu'aucun test ne le détecte.

Pire : la table couvre seulement 4 des 8 lignes documentées (shared, core, infrastructure, server — validation/, errors/, handlers/, hooks/ ne sont jamais scannées, confirmé par layer_of() + CHECKED_LAYERS et par le test test_unrelated_layer_is_not_checked). Or CLAUDE.md affirme après cette PR : « Full table: docs/module-inventory.md § Dependency Rules » — c'est trompeur : ce n'est pas la table complète qui est appliquée, c'est la moitié. Un lecteur qui prend cette phrase au mot croira handlers/ protégé alors qu'il ne l'est pas du tout.

Correctif minimal : soit corriger la prose de CLAUDE.md pour dire explicitement « 4 des 8 couches, voir CHECKED_LAYERS », soit écrire un test qui parse docs/module-inventory.md § Dependency Rules et affirme l'égalité avec FORBIDDEN_SECOND_COMPONENT/CHECKED_LAYERS, pour que toute divergence casse la CI plutôt que de dériver en silence.

4. Sourcing des seuils — la gate se soumet-elle à sa propre règle 4 ?

FILE_LINE_LIMIT = 300 et METHOD_LINE_LIMIT = 40 portent un # source: qui remonte correctement à CLAUDE.md § Code Style → coding-standards.md §4.1/§4.2 → Martin 2008 Clean Code — chaîne de citation légitime, pas fabriquée. Bien.

Mais deux constantes internes de la gate elle-même citent # source: task instruction (_POWERS_OF_TWO_USUELLES plafonné à 65536, AUTO_GENERATED_SCAN_LINES = 5) — ce n'est ni un papier, ni un benchmark commité, ni une mesure datée : c'est l'auteur qui se cite lui-même. Non-bloquant (ce sont des seuils d'exemption internes à l'outil, pas des constantes métier), mais à noter : au sens strict de coding-standards.md §8, ce n'est pas une source.

Auto-application vérifiée : le gate tourne sur ses 5 propres fichiers (check_craftsmanship.py 190 l., craftsmanship_rules.py 151 l., craftsmanship_imports.py 125 l., craftsmanship_constants.py 101 l., craftsmanship_baseline.py 76 l.) → OK. Aucune méthode >40 lignes, aucun fichier >300. Cohérent, pas d'auto-exemption cachée.

5. Les 59 tests testent-ils la gate ou sa paraphrase ?

Testent réellement la gate. _craftsmanship_support.py charge une seule fois les modules via importlib.util.spec_from_file_location et les partage — nécessaire à cause du dataclass Violation (sinon deux chargements séparés créent deux classes distinctes, égalité de dataclass cassée), mais chaque test construit sa source de façon indépendante, il n'y a pas de circularité « la même fonction calcule l'attendu et le résultat ».

Cas limites annoncés, tous présents et vérifiés par exécution locale (0.006s, 47+12=59 tests, tous verts) :

• Exactement 300 lignes passe, 301 échoue (test_exactly_at_cap_passes / test_one_over_cap_fails).
• Méthode décorée mesurée par son corps, pas le décorateur (test_decorated_method_measured_by_body_not_decorator).
• Méthode imbriquée avec nom qualifié en pointillés (test_nested_function_gets_dotted_qualified_name).
• async def dans une classe (test_async_method_in_class_gets_qualified_name).
• TYPE_CHECKING exempté (test_type_checking_import_is_exempt).
• Fichier auto-généré exempté, insensible à la casse (test_auto_generated_marker_case_insensitive).
• Fichier avec erreur de syntaxe → liste vide, ne lève pas (test_syntax_error_returns_empty_not_raises).

Absents (confirmés par grep, aucun test ne les couvre) : BinOp constante calculée, constante de portée classe, argument par défaut, lib tierce importée dans core/, import via module hors mcp_server. Ce sont exactement les échappatoires de §1 — les tests documentent fidèlement ce que la gate fait, mais ne documentent jamais ce qu'elle ne fait pas, alors que ce sont les cas les plus probables en pratique.

6. Job CI : gréé correctement, sur les bons événements

ci.yml déclenché sur push (branche main), pull_request (branche main), workflow_dispatch — standard. Le job craftsmanship fait fetch-depth: 0 (nécessaire pour que origin/main soit résolvable), tourne python scripts/check_craftsmanship.py sans dépendance (stdlib only, cohérent avec le docstring), et a été correctement ajouté à needs: du job ci-green — vérifié à la fois par lecture du diff et par le fait que le dépôt possède déjà scripts/check_ci_gate_complete.py, qui aurait fait échouer la CI si le job avait été oublié dans needs:. gh pr checks 413 confirme : Craftsmanship Gate pass, CI Green pass.

Un point mineur, non-bloquant, pas nouveau à cette PR : sur un push direct (post-merge), origin/main == le commit poussé au moment du run, donc git diff origin/main...HEAD est vide et le job ne scanne rien — comportement inoffensif ici (aucune régression possible après merge), mais partagé par tous les gates diff-based du dépôt (check_doc_claims.py etc.), pas spécifique à cette PR.

Hygiène du diff

git diff main...HEAD --name-only : 15 fichiers, tous strictement liés au craftsmanship gate (script + 4 modules + baseline + workflow + CLAUDE.md + SECURITY.md + 6 fichiers de test). Rien d'étranger — le git stash pop mentionné n'a laissé aucune trace dans le diff final ; vérifié directement, pas seulement pris pour acquis.

Ce qui bloque le merge

1. Faille de blanchiment du baseline (§2) — démontrée, sans contre-mesure CI. Corrige en diffant contre le baseline du base_ref, pas celui de l'arbre de travail.
2. core/ (et la règle 3 en général) n'implémente qu'une liste noire de composants nommés, jamais la liste blanche documentée (§1, §3) — core/ peut importer n'importe quelle lib tierce faisant de l'I/O sans être détecté, ce qui contredit directement sa propre définition (« pure business logic, zero I/O », docs/module-inventory.md). Corrige _import_violates_layer pour appliquer une vraie liste blanche par couche, pas une liste noire de composants mcp_server.* nommés.
3. CLAUDE.md affirme « Full table » alors que 4 des 8 couches documentées ne sont pas vérifiées (§3) — corrige la prose ou étends CHECKED_LAYERS, et ajoute un test qui lie le dict au fichier source pour empêcher toute dérive future silencieuse.

Le reste (constantes calculées/de classe, sourcing des seuils internes) est réel mais non-bloquant — à traiter en suivi documenté, pas en note volatile.

[cdeust]

ZETETIC-REVIEW: REQUEST_CHANGES

Summary

Round 2 closes both round-1 findings genuinely — verified live, not by re-reading the diff. But adversarial testing found two new, still-live gaps that let the ratchet be defeated with zero code fix. Judged at head 9ce395c0.

Round 1 findings — re-verified closed (live reproduction, throwaway repo)

• Baseline self-tamper: reproduced the exact round-1 attack (add SNEAKY_LIMIT = 12345, gate blocks; run --write-baseline in the same tree) — now the gate still blocks on the second run, because comparison uses git show <base-ref>:.craftsmanship-baseline.json (immutable to the PR's own commits), not the working tree. Confirmed.
• core/ denylist → whitelist: import numpy, import requests, import scripts.legacy_bridge inside core/ are all now caught by craftsmanship_imports.py's true whitelist, derived at runtime from all 8 rows of docs/module-inventory.md § Dependency Rules (not 4). Confirmed via the repo's own ReviewCounterExampleTests and my own independent AST check.
• Legitimate fix path: fix the violation + prune the matching baseline entry → gate passes cleanly (tested against both a violation that was never in the base baseline and one that was baselined on main before the branch existed). The ratchet does not block real corrections.

Two NEW blocking gaps (adversarial, live-reproduced, untested by the PR's own 90 tests)

1. Hand-pruning a baseline entry with no matching code fix is undetected

added_entries only catches additions (working_baseline - base_baseline). new_violations compares the current code scan against base_baseline, never against working_baseline. Reproduced in a throwaway repo: baselined PREEXISTING_DEBT = 999 left completely untouched in the source, its one entry hand-deleted from .craftsmanship-baseline.json, committed — gate output: Craftsmanship gate: OK, exit 0.

This defeats the ratchet's entire premise for any of the 1362 currently-baselined violations, at the cost of deleting one JSON block — no --write-baseline, no code change, no detection. The baseline's own $comment field ("every entry removed here must correspond to a violation actually fixed in the code") is, again, a social contract only — nothing checks it. Same failure category as the round-1 finding, just the subtraction direction instead of the addition direction; closing one without the other leaves the mechanism half-fixed. No test in test_check_craftsmanship.py or test_craftsmanship_baseline.py covers this (checked: test_entry_added_beyond_base_is_reported, test_pure_shrink_reports_nothing — both only exercise pruning matched by a real fix, never an unmatched prune).

Fix shape: _run_gate needs a third check — for every entry in base_baseline - working_baseline (a genuine prune), the corresponding violation must be absent from current (the fresh scan of the diff's changed files) before it's accepted as a legitimate shrink; if it's still present in current, treat it the same as a stale-but-never-pruned entry (block). Scope caveat: this only catches prunes of files this PR's diff actually touches (matching the gate's stated by-diff scope) — a prune of an untouched file's entry would need a rescan of that file specifically, same as the existing stale_entries rescan already does.

2. Layer-table parser silently truncates on a malformed mid-table row

parse_layer_rules raises ValueError only when the table header is entirely missing or zero rows parse (test_missing_header_raises, test_header_with_no_rows_raises — both tested). But the row loop does if match is None: break — a single row that doesn't match _ROW_RE silently ends parsing there, dropping every row after it with no exception, no signal.

Reproduced: inserted one row after validation/ missing its trailing | (an entirely plausible human edit — a copy-paste slip, a reformatted line). Result: errors/, handlers/, server/, hooks/ (4 of 8 documented layers) silently vanished from CHECKED_LAYERS. Since check_layer_violation does if rule is None: return [], every file under those four layers gets zero import enforcement, indistinguishable in gate output from "everything passed." This directly contradicts the module's own docstring: "failing loudly here matters exactly as much as the rule itself: a silently-unparsed table would silently under-enforce." The design intent is right; the implementation only delivers it for the two edge cases already tested, not for the case a real markdown edit is most likely to produce — a malformed row in the middle, not the first or only row.

Missing header / zero rows failing loudly, confirmed correct, is not sufficient — the loop needs to raise (not break) on any post-header line that looks like it was meant to be a row and isn't (e.g., starts with | **) but doesn't match, or — more robustly — continue scanning to end-of-table and raise if the row count doesn't match the number of | **... lines found, rather than trusting the first non-match as "table ended."

Debt-count reconciliation (requested)

Baseline totals match the PR's claim exactly: 226 file-size + 114 layer-violation + 556 method-size + 466 unsourced-constant = 1362. Your 122-file/416-method figures reconcile fully: the gate's --write-baseline scope is the whole tracked-.py tree (benchmarks/, tests_py/, video/, not just mcp_server/+scripts/) — my own full-repo wc -l count also gets exactly 226; scoped to mcp_server/+scripts/ only I get 121 (≈ your 122). Method-size: an independent fresh AST scan (not the gate's own code) of mcp_server/+scripts/ gives 397, matching the tool exactly — your 416 doesn't reconcile against two independent measurements, so I attribute it to counting methodology, not a gate defect; flag if you measured differently and want it chased further.

CLAUDE.md merge-conflict check

Diffed the PR head's § Code Style directly against pre-#413 origin/main (which already includes #409's "grep both directions" text). Nothing lost: the manual-grep instructions are explicitly superseded by prose describing the automated gate ("This replaces the former manual-grep verification step … the craftsmanship gate below runs it, in both directions, across all eight layers"), and the three historically-named violations (wiki_axis_registry.py, wiki_classifier.py, wiki_schema_loader.py) are preserved in the same paragraph, now folded into "now live in the baseline like any other pre-existing debt." Clean.

Verdict

REQUEST_CHANGES. Both round-1 findings are genuinely closed — good work, verified live rather than trusted. But this round's fix is incomplete in the same spirit the brief warned about: "une gate fausse est pire que pas de gate." Findings 1 and 2 above are each, independently, a way for the gate to report success while enforcing nothing — fix both, add a regression test for each (a hand-pruned-without-fix entry; a malformed row after the first genuinely-parsed one), and this is close to mergeable.

[cdeust]

ZETETIC-REVIEW: APPROVE

Summary

Round 3 at head 583fe2b3. Closes the round-2 finding cleanly: _git_path_exists_at_ref now distinguishes git's own "path does not exist at this ref" stderr from every other cat-file -e failure (TOCTOU on the ref, corrupt object, permission/disk error), raising RuntimeError instead of collapsing everything into a bare return False that load_baseline_from_ref would otherwise read as a legitimate bootstrap and fall back to the tamperable working-tree baseline — reopening round 1's laundering exploit through a third path. Verified this is a real fix, not a described one, by direct reproduction outside the delivered test suite. Mergeable.

Independent adversarial reproduction (throwaway repos, seconds each, no full suite/bench run)

All three attacks reproduced and confirmed blocked, all three legitimate paths confirmed pass:

Attack	Result
Addition-laundering (SNEAKY_LIMIT, block → --write-baseline in same tree → still block, reported ADDED)	Blocked ✓
Falsified removal (hand-delete baseline JSON entry, source untouched → reported REMOVED)	Blocked ✓
Ambiguous git error (git cat-file -e bogus-ref-xyz:... → fatal: invalid object name, doesn't match either absence marker)	RuntimeError raised, not silently False ✓
Legit: fix code then prune (regenerate is a pure shrink)	Passes ✓
Legit: bootstrap first-ever run (no baseline anywhere yet)	Passes ✓
Legit: reformatted-but-valid layer table (extra whitespace, all 8 rows)	Parses cleanly, 8/8 layers ✓

Full 103-test suite (test_check_craftsmanship*.py 25 + test_craftsmanship_*.py 78) run green in ~3s total.

Fourth-instance search (Move requested)

Swept all 7 gate modules for the fail-open family (except, break, .get() without default, bare return False/True). One additional instance found, judged not in the same exploitable class: craftsmanship_rules.scan_source's except SyntaxError: return [] — a file that fails to parse yields zero violations. Unlike the three closed bugs, this cannot be used to launder a real violation: a syntax-broken .py file cannot ship, run, or import regardless of what the gate says, so there is no path from "silently pass" to "bad code merged and works." No 4th blocking gap.

falsified_removals reliability (deleted/renamed file)

Confirmed by reading _run_gate's removed_rescanned = {f: scan_files([f]) for f in removed_files}: scan_files returns an empty set for a file that no longer exists (if not full_path.is_file(): continue), so v in rescanned.get(v.file, set()) is False for a deleted file's removed entry — pruning a baseline entry for genuinely-deleted code is not blocked. Correct; a renamed file where the violation persists under the new name surfaces separately as a new/added violation under the new path, which is also correct (a rename doesn't launder debt).

8-file split

All ≤300 lines (largest 217), self-application verified clean (check_craftsmanship.py run against its own new/changed files, exit 0). Grepped the whole repo for craftsmanship_git usage outside scripts/ and its own tests — no orphaned public symbols, no external caller broken by the split.

Mutation check on the central assertion

Inverted falsified_removals's if v in rescanned.get(v.file, set()) to not in: 6 tests fail (2 unit-level in test_craftsmanship_baseline.py, plus 3 end-to-end exploit/legitimate-path tests in test_check_craftsmanship_exploits.py, catching both the false-negative and false-positive directions) — mutation-strong, not a surface/apparence test. Separately inverted the absent-marker branch in _git_path_exists_at_ref (forced it to always raise): 1 test dies immediately (test_real_absent_path_returns_false).

Verdict

APPROVE. Three rounds, three distinct fail-open bugs in the same family (baseline-gaming by addition, baseline-gaming by removal, ambiguous-git-error read as absence), all independently reproduced and confirmed closed. Test suite is mutation-strong on the load-bearing conditions, not just present. Ship it.