feat(rules): bootstrap 3 Python doc families (architecture, ioc, logging)#19
Merged
Conversation
…ing) First Python-side bootstrap PR. Same multi-doc shape as PRs #17, #18. 3 Python docs (~500-820 lines), 6 rules, single bot review cycle. Expands rule coverage from Go-only to Python. Rules added (rules/index.json: 68 -> 74): python-architecture/* (owner: python-architecture-assistant) - constructor-injection-only (MUST) — deps via __init__; methods take only runtime data. Mixing the two breaks the dep-graph visibility that makes Python's typing useful. - main-py-composition-root (SHOULD) — wire all deps in main(), never at module scope. Module-level instantiation runs at import time, making tests order-dependent. python-ioc/* (owner: python-architecture-assistant) - protocol-not-abc-for-dependencies (MUST) — typing.Protocol for dependency interfaces; abc.ABC only when concrete impls share code via super(). Protocol is the right primitive for pure contracts; ABC is the right primitive for shared inheritance. - dependencies-as-private-fields (MUST) — store injected deps on self._foo (private), never self.foo (public). Public attribute storage invites external mutation that breaks the constructor- injection immutability contract. python-logging/* (owner: python-quality-assistant) - configure-once-in-main (MUST) — logging.basicConfig only at the application entry point. Libraries call logging.getLogger(__name__) and emit; they never configure. Library-side basicConfig produces three failure modes (first-import-wins, duplicate handlers, lost log-level control). - lazy-evaluation-for-debug (MUST) — use %s placeholders for DEBUG messages with expensive interpolated values, not f-strings. F-string interpolation runs every call even when DEBUG is filtered out; %s defers evaluation to the logging library. CLAUDE.md doc-agent table updated with all 3 new mappings. Generic examples throughout (User, Order, UserService, UserRepository). No personal vault paths, no trading-domain terms. Pre-emptive grep clean. make build-index regenerated; check-index passes.
![ben-s-pull-request-reviewer[bot]](https://avatars.githubusercontent.com/u/1193284?s=60&v=4){kind=small}
There was a problem hiding this comment.
Now let me compile the consolidated report and produce the JSON verdict.
PR Review: feat(rules): bootstrap 3 Python doc families (architecture, ioc, logging)
Branch: feat/bootstrap-python → master | Files changed: 5 | +239 / -0
Automated Checks
- LICENSE file: Present ✓
- make precommit: Could not run — sandbox blocked. Manual validation not performed.
Must Fix (Critical)
1. Self-contradiction in python-logging/configure-once-in-main
docs/python-logging-guide.md:41-79 + docs/python-logging-guide.md:180
The new RULE states it applies to logging.basicConfig() called outside the application entry point. The doc's own "Extract Logging Configuration to Dedicated Module" example (logging_setup.py → configure_logging() → logging.basicConfig(..., force=True)) is inside a helper module, not main.py. The ast-grep enforcement described would mechanically flag this recommended pattern as a violation. The doc contradicts itself.
2. Duplicate RULE blocks + free-form Constraint sections
docs/python-ioc-guide.md:56-61 + 63-85 (protocol rule); docs/python-ioc-guide.md:130-135 + 156-173 (private-fields rule); docs/python-architecture-patterns.md:15-56 + 58-86 (constructor-injection rule); docs/python-logging-guide.md:41-79 + 81-119 (configure-once rule); docs/python-logging-guide.md:420-444 + 446-465 (lazy-evaluation rule)
Every new RULE block is immediately followed by an un-numbered Constraint section with identical content. The doc structure now has each rule stated twice. This creates a maintenance burden (any update must be applied in two places) and violates the single-source-of-truth principle. The RULE blocks and the free-form versions cannot drift — one set must go.
Should Fix (Important)
3. constructor-injection-only covers three architecturally distinct anti-patterns
docs/python-architecture-patterns.md:18
The Applies when clause covers: (a) dependency through method parameter, (b) dependency from global module variable, (c) self.foo = bar.foo post-construction mutation. These are separate violations with different fixes and enforcement strategies. Collapsing them into one MUST rule increases judgment burden and reduces actionability.
4. dependencies-as-private-fields ast-grep recipe is not achievable as stated
docs/python-ioc-guide.md:134
The enforcement says the right-hand side must be "a parameter typed as a Protocol / ABC interface." ast-grep cannot resolve type annotations to their declarations. The rule is labeled judgment anyway, so the aspirational ast-grep follow-up label is misleading — the type-resolution condition is permanently unsolvable mechanically.
5. lazy-evaluation-for-debug enforcement is over-broad
docs/python-logging-guide.md:424
ast-grep detects all f"..." in logger.debug(...), but the Applies when restricts to f-strings with expensive operations. logger.debug(f"Count: {count}") would be flagged despite count being trivial. The rule text should list specific operation-name patterns to look for (e.g., function calls matching serialize|dump|fetch|load|tqdm) or explicitly state that ast-grep is a first-pass filter and semantic judgment is always required.
6. Good example in configure-once-in-main omits if __name__ guard
docs/python-logging-guide.md:63-69
The Good example shows logging.basicConfig(...) without if __name__ == "__main__":. The ast-grep enforcement relies on detecting that guard to classify a file as an entry point. A developer copying the example literally would produce a file not recognized as an entry point by the mechanical check.
Nice to Have (Optional)
7. main-py-composition-root is SHOULD but described as universally correct
docs/python-architecture-patterns.md:96-101
The rule is labeled SHOULD (correctly — one-off scripts can skip it), but the Why section and examples present it as an absolute requirement. No "when to skip" escape hatch is documented.
8. main-py-composition-root Bad example conflates two concerns
docs/python-architecture-patterns.md:107-110
The comment "imports trigger connection!" mixes import-side-effect concern with module-level-instantiation concern. These are separable issues; the example should focus on the module-level instantiation problem only.
9. lazy-evaluation-for-debug doesn't cover non-f-string lazy violations
docs/python-logging-guide.md:423
logger.debug(some_variable) or logger.debug(str(obj)) are also non-lazy (evaluated before the level check). The rule should note this or explicitly scope to the f-string case only.
{
"verdict": "request-changes",
"summary": "The PR adds 5 well-structured Python guide entries with solid rule rationale, but contains two critical self-contradictions: (1) the `configure-once-in-main` RULE would mechanically flag the doc's own `logging_setup.py` extraction pattern as a violation, and (2) every new RULE block is duplicated by an adjacent free-form Constraint section with identical content. These must be resolved before merge.",
"comments": [
{
"file": "docs/python-logging-guide.md",
"line": 41,
"severity": "critical",
"message": "Must Fix: Self-contradiction — RULE `python-logging/configure-once-in-main` states it applies to `basicConfig()` called outside entry points, but the doc's own \"Extract Logging Configuration to Dedicated Module\" example at line ~180 calls `basicConfig(..., force=True)` inside `logging_setup.py` (a helper module). The ast-grep enforcement described would flag this recommended pattern as a violation. Either refine the `Applies when` to permit `basicConfig` inside helper modules wired from entry-point code, or revise the `logging_setup.py` example to use manual handler setup."
},
{
"file": "docs/python-ioc-guide.md",
"line": 56,
"severity": "major",
"message": "Should Fix: Redundancy — RULE `python-ioc/protocol-not-abc-for-dependencies` (line 56) is immediately followed by an un-numbered `### Use Protocol for Dependency Interfaces` section (line 63) with nearly identical content. RULE `python-ioc/dependencies-as-private-fields` (line 130) is similarly duplicated at line 156. Every new RULE block in this PR is duplicated by a free-form section. Pick one format per rule and remove the duplicate."
},
{
"file": "docs/python-architecture-patterns.md",
"line": 15,
"severity": "major",
"message": "Should Fix: Redundancy — RULE `constructor-injection-only` (line 15) is followed by a \"Protocol Definition\" + \"Service Implementation\" subsection (lines 58-86) containing byte-for-byte identical GOOD/BAD examples. Same duplication pattern applies to `main-py-composition-root` (lines 96-135) which overlaps with the existing \"main.py Pattern (Composition Root)\" prose below it."
},
{
"file": "docs/python-logging-guide.md",
"line": 420,
"severity": "major",
"message": "Should Fix: Redundancy — RULE `lazy-evaluation-for-debug` (line 420) is immediately followed by a free-form `### Use Lazy Evaluation for Expensive DEBUG Operations` section (line 446) with nearly identical GOOD example. Remove the duplicate free-form section."
},
{
"file": "docs/python-architecture-patterns.md",
"line": 18,
"severity": "major",
"message": "Should Fix: `Applies when` clause for `python-architecture/constructor-injection-only` covers three architecturally distinct anti-patterns: (a) dependency through method parameter, (b) dependency from global module variable, (c) post-construction `self.foo = bar.foo` mutation. These have different fixes and enforcement strategies. Split into focused rules or lead with the primary violation (method-parameter DI) in the `Applies when` clause."
},
{
"file": "docs/python-ioc-guide.md",
"line": 134,
"severity": "major",
"message": "Should Fix: `dependencies-as-private-fields` ast-grep recipe requires resolving type annotations to their declarations — ast-grep cannot do this. The `ast-grep follow-up` label is misleading since the type-resolution condition is permanently unsolvable. Mark as permanently judgment-tier, or drop the type-resolution condition from the recipe and let ast-grep flag all `self.X` assignments in `__init__` (overinclusive but mechanically sound)."
},
{
"file": "docs/python-logging-guide.md",
"line": 424,
"severity": "major",
"message": "Should Fix: `lazy-evaluation-for-debug` enforcement is over-broad — ast-grep detects all `f\"...\"` in `logger.debug`, but the rule targets *expensive* operations specifically. `logger.debug(f\"Count: {count}\")` would be flagged despite `count` being trivial. Add specific operation-name patterns (e.g., function calls matching `serialize|dump|fetch|load|tqdm`) or explicitly state that ast-grep is a first-pass filter and semantic judgment is always required."
},
{
"file": "docs/python-logging-guide.md",
"line": 63,
"severity": "major",
"message": "Should Fix: `configure-once-in-main` Good example (line 63) shows `logging.basicConfig(...)` without `if __name__ == \"__main__\":` guard, but the ast-grep enforcement (line 45) relies on that guard to classify a file as an entry point. Add the guard to the example, or label it explicitly as a snippet (not a complete file)."
},
{
"file": "docs/python-architecture-patterns.md",
"line": 96,
"severity": "nit",
"message": "Nice to Have: `main-py-composition-root` is labeled SHOULD (correctly — one-off scripts can skip it) but the Why section and Bad/Good examples present it as universally required. Add a \"When to skip\" clause for small CLI tools or scripts under ~50 lines."
},
{
"file": "docs/python-architecture-patterns.md",
"line": 107,
"severity": "nit",
"message": "Nice to Have: `main-py-composition-root` Bad example comment \"imports trigger connection!\" conflates import side effects with module-level instantiation. These are separable concerns. Simplify to focus on the instantiation problem only."
},
{
"file": "docs/python-logging-guide.md",
"line": 423,
"severity": "nit",
"message": "Nice to Have: `lazy-evaluation-for-debug` only covers f-string cases. `logger.debug(some_variable)` and `logger.debug(str(obj))` are also non-lazy (evaluated before the level check). Mention this edge case or explicitly scope the rule to the f-string case only."
}
],
"concerns_addressed": [
"correctness: rules/index.json 5 new rules declared with 'enforcement: judgment (semantic)' — confirmed; all are judgment-tier, no corresponding ast-grep YAML files exist (not a regression; same pattern holds for many existing Go rules)",
"docs: CLAUDE.md references agents 'python-architecture-assistant' and 'python-quality-assistant' — confirmed they exist in agents/ directory",
"correctness: python-architecture/constructor-injection-only semantic enforcement edge cases — addressed: flagged as overly broad, should be split",
"correctness: python-logging/lazy-evaluation-for-debug ast-grep false positives — addressed: flagged as over-broad enforcement gap"
]
}Bot's deepest review of the session. Many real issues, mostly traceable to my Edits inserting RULE blocks ABOVE existing constraint sections instead of replacing them — produced duplicate free-form sections under each new rule. Cleaned up. CRITICAL: - configure-once-in-main would mechanically flag the doc's own logging_setup.py extraction pattern (basicConfig in a helper module wired from main.py). Refined applies_when to exempt application- private configuration helpers; enforcement now notes the helper-module exemption case. MAJOR — redundancy cleanup (4 of 7): - python-ioc: removed duplicate 'Use Protocol for Dependency Interfaces' free-form section (RULE block above covers same Bad/Good) - python-ioc: removed duplicate 'Store Dependencies as Private Fields' free-form section - python-architecture: replaced duplicate 'Protocol Definition' + 'Service Implementation' sections with a Key-points cross-reference list pointing at the canonical RULEs in python-ioc-guide.md - python-logging: removed duplicate 'Configure Logging Once at Entry Point' free-form section - python-logging: removed duplicate 'Use Lazy Evaluation for Expensive DEBUG Operations' free-form section MAJOR — scope/enforcement refinements (3 of 7): - constructor-injection-only applies_when narrowed to method-parameter DI as the primary trigger; global-var and post-construction-mutation noted as related anti-patterns covered by sibling rules (main-py-composition-root + dependencies-as-private-fields). - dependencies-as-private-fields enforcement reframed: ast-grep is a first-pass overinclusive filter (any non-_ self.X in __init__); type-annotation resolution acknowledged as impossible in ast-grep, agent makes final call. - lazy-evaluation-for-debug enforcement reframed similarly: ast-grep flags all f-strings in logger.debug as first-pass; agent decides expensive-vs-trivial after the filter. Also fixed configure-once-in-main Good example to include 'if __name__ == "__main__":' guard for consistency with the applies_when's entry-point definition. NITs (skipped — not blocking, can be addressed in future doc polish): - main-py-composition-root SHOULD vs universal-required framing - Bad example comment 'imports trigger connection!' conflates concerns - lazy-evaluation-for-debug doesn't cover non-f-string non-lazy cases make build-index regenerated; check-index passes.
ben-s-pull-request-reviewer
Bot
dismissed
their stale review
superseded by new automated review
There was a problem hiding this comment.
PR Review: feat(rules): bootstrap 3 Python doc families (architecture, ioc, logging)
Consolidated Report
Files Changed: CLAUDE.md, docs/python-architecture-patterns.md, docs/python-ioc-guide.md, docs/python-logging-guide.md, rules/index.json
Must Fix (Critical)
- None — The 6 new rule entries are schema-consistent with existing entries; anchors, IDs, doc_paths, and owner fields all align correctly.
Should Fix (Important)
- docs/python-architecture-patterns.md:20,71 — Non-standard "Why" field in RULE blocks: The rule blocks include a
**Why**:field, butrule-block-schema.mdonly definesOwner,Applies when, andEnforcementas the standard fields. If this is intentional,rule-block-schema.mdshould be updated to documentWhyas an optional supported field. If not intentional, the field should be removed from bothpython-architecture/constructor-injection-onlyandpython-architecture/main-py-composition-rootrule blocks.
Nice to Have (Optional)
- rules/index.json:618 — Overlong enforcement text: The
enforcementfield forpython-architecture/constructor-injection-onlyis verbose and includes a parenthetical that references other rules' enforcement paths. Consider shortening to focus on the specific mechanism. - docs/python-architecture-patterns.md —
Applies whencross-references: TheApplies whenfield forconstructor-injection-onlyis lengthy (includes references topython-architecture/main-py-composition-rootandpython-ioc/dependencies-as-private-fields). Consider moving cross-references to a separate "Related rules" section for better scannability.
Validation Summary
| Check | Result |
|---|---|
| Rule IDs unique (6 new) | ✅ Pass |
| anchor == id for all 6 | ✅ Pass |
| doc_path files exist | ✅ Pass |
| Schema fields complete (7/7) | ✅ Pass |
| Owner fields match CLAUDE.md agents | ✅ Pass |
| Rule levels match doc headers | ✅ Pass |
| Good/Bad examples correct | ✅ Pass |
| Generic examples (no trading domain) | ✅ Pass |
Verdict: APPROVE — No blocking issues. The optional improvements noted are stylistic and do not affect rule extraction or enforcement.
{
"verdict": "approve",
"summary": "PR adds 6 well-structured Python rule entries across 3 new doc families with correct anchors, unique IDs, and consistent schema. One Should Fix identified: the non-standard 'Why' field in python-architecture RULE blocks should either be documented in rule-block-schema.md or removed to maintain schema consistency.",
"comments": [
{
"file": "docs/python-architecture-patterns.md",
"line": 20,
"severity": "major",
"message": "Should Fix: Non-standard 'Why' field in RULE block — rule-block-schema.md defines only Owner, Applies when, and Enforcement as required fields. Either document 'Why' as optional in schema or remove from the rule block."
},
{
"file": "docs/python-architecture-patterns.md",
"line": 71,
"severity": "major",
"message": "Should Fix: Same 'Why' field issue in python-architecture/main-py-composition-root RULE block."
},
{
"file": "rules/index.json",
"line": 618,
"severity": "nit",
"message": "Nice to Have: enforcement field for python-architecture/constructor-injection-only is verbose — the parenthetical referencing other rules' enforcement paths adds length without actionable detail."
}
],
"concerns_addressed": [
"correctness: 6 new rule entries verified for schema consistency (anchor, applies_when, doc_path, enforcement, id, level, owner) — all 7 fields present, all match existing entry patterns",
"correctness: Rule IDs unique — python-architecture/constructor-injection-only, python-architecture/main-py-composition-root, python-ioc/protocol-not-abc-for-dependencies, python-ioc/dependencies-as-private-fields, python-logging/configure-once-in-main, python-logging/lazy-evaluation-for-debug",
"correctness: doc_path values correctly reference new Python doc files",
"correctness: CLAUDE.md agent rows match owner fields in index.json (python-architecture-assistant owns 4 rules, python-quality-assistant owns 2)",
"correctness: RULE anchors in docs match corresponding index.json rule IDs"
]
}
bborbe
added a commit
that referenced
this pull request
Jun 2, 2026
…hema PR #19's bot review flagged the 'Why' field in 2 of my python-architecture RULE blocks as 'non-standard' per rule-block-schema.md. Verified: every existing rule block in this repo (74 total across 17 families, including all 4 the original pilot author wrote) carries Why. Schema doc was the incomplete artifact, not the rule blocks. Updated rule-block-schema.md to: - Note Why as a recommended (not indexed, walker ignores it) field after the three required fields. Explains it's the canonical home for the rule's rationale — the failure mode it prevents — which makes the rule defensible during code review. - Note #### Bad / #### Good code-example sections as recommended for the same reason: unambiguous intent, concrete comparison surface for bot reviews. Both additions match what every rule block in the repo already does; schema doc now describes reality. Future bot reviews on new RULE blocks won't flag Why as non-standard — it's now documented as part of the recommended shape.
3 tasks
bborbe
added a commit
that referenced
this pull request
Jun 2, 2026
…ture, makefile) Second Python-side bootstrap. Same multi-doc-per-PR shape as PRs #17, #18, #19. 3 docs (~467-576 lines), 6 rules. This time the RULE blocks REPLACE the existing constraint sections (not insert above), avoiding the redundancy issue that hit PR #19. Rules added (rules/index.json: 74 -> 80): python-pydantic/* (owner: python-quality-assistant) - boundary-validation-only (MUST) — Pydantic at system boundaries only (API, queue ingestion, file parser); internal domain types use dataclass / plain types to avoid validation overhead on trusted data. - optional-needs-default (MUST) — Optional[T] alone is NOT omittable; it's a type-system 'T or None'. Pair with '= None' (or Field(default=...)) when the intent is 'may be omitted'. python-project-structure/* (owner: python-architecture-assistant) - src-layout-required (MUST) — packages live in src/, not at repo root. Root layout silently picks up the dev directory via sys.path; src/ forces 'install then import' which surfaces packaging bugs. - pyproject-toml-with-hatchling (MUST) — pyproject.toml + hatchling build backend, never setup.py. PEP 517/518 declarative manifest; hatchling is the recommended PyPA backend. python-makefile/* (owner: python-quality-assistant) - precommit-target-required (MUST) — every Python project's Makefile has a precommit target running format + test + check. Project- agnostic uniform entry point for CI scripts and pre-commit hooks. - uv-not-pip-or-poetry (SHOULD) — uv for dependency management (10-100x faster than pip/poetry/pipenv, deterministic uv.lock, reads standard PEP 621 metadata). SHOULD level acknowledges legacy projects mid-migration. CLAUDE.md doc-agent table updated with all 3 new mappings. Generic examples (User, Product, iphone_backup package, etc). No personal vault paths, no trading-domain terms. make build-index regenerated; check-index passes.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First Python-side bootstrap PR. Expands rule coverage from Go-only (14 families) to also include Python (3 new families). Same multi-doc shape as PRs #17, #18.
`rules/index.json`: 68 → 74 entries across 17 doc families total.
Rules added
Rule rationale
Three high-leverage MUST rules selected per doc; one SHOULD on architecture for the composition-root pattern. All are mechanically enforceable in principle — ast-grep YAMLs deferred to focused follow-ups (same approach as PR #8 / #10 / #11).
Picked Python's standard primitives where they differ from Go:
Pre-emptive checks
Test plan