refactor: tighten check_no_magic_numbers for named module constants (#1856)#1866
Conversation
…1856) Recognise module-level `NAME: int|float|Final|Final[int]|Final[float] = literal` as "this IS the named constant"; the gate no longer flags typed protocol constants such as `JSONRPC_PARSE_ERROR: int = -32700`. Bare `NAME = literal` without an annotation still flags, preserving the rule for unsignalled assignments. Lock the existing carve-outs (`_HTTP_STATUS_KEYWORDS`, `_IO_KEYWORD_NAMES`, `_IO_ALLOWED_POWERS_OF_2`, `_FILE_ALLOWLIST_PREFIXES`) with parametrized tests so a future deletion from any of those sets surfaces as a failing row. Baseline shrinks 820 -> 513 entries (-37 %, pure removals). Phase 1 of the issue (gate refinement only); migrating the remaining literals in src/ to settings/definitions/ is phase 2 and blocked on #1857.
Pre-reviewed by 12 agents, 13 findings addressed: Critical - Remove `(#1856)` issue back-ref from test docstring (CLAUDE.md "no issue back-refs" rule). - CLAUDE.md "No Hardcoded Values" bullet now lists the annotated-named-constant skip as the 5th allowlist category so the one-liner contract matches the gate. - Add `typing.Final[int]` qualified-import test that locks the scope decision (still flags; switch to direct import to silence). - Add negative-value rows to the parametrized helper test (`JSONRPC_PARSE_ERROR: int = -32700`, `_FOO: Final[int] = -1`). Major - Remove "after this PR" migration framing from test docstring. - Tighten `_ScriptModule` Protocol: `ast.expr | None` (was `object | None`). - Extend non-numeric annotation parametrize with `Final[str]`, `Final[None]`. - Add `test_named_constant_allowlist_contents` locking exact frozenset membership. - Add negative-pow2 I/O lockdown (`buffering: int = -1024` flags). Medium - Tighten `_annotation_marks_as_named_constant` docstring and the `_NAMED_CONSTANT_*` comment block to reduce file-length drift. - Add "Protocol constants are not settings" section to configuration-precedence.md noting typed `Final[int]` is the right vehicle for wire-protocol numerics like JSON-RPC error codes. File still 57 lines over the 800-line soft guideline (pre-existing condition; full extraction of baseline I/O is scope creep). Net additions from this PR after tightening: ~62 lines on a base of 795.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🧰 Additional context used📓 Path-based instructions (1){README.md,docs/**/*.md}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-05-05T09:04:46.195ZApplied to files:
🔇 Additional comments (8)
WalkthroughThis pull request enhances the magic number detection system to recognize and exempt module-level annotated numeric constants. The policy is clarified in the coding guidelines and configuration precedence documentation, specifying that protocol numeric values must be implemented as typed constants with annotations such as 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the magic number enforcement policy to allow module-level numeric constants when they are explicitly annotated with types such as int, float, or Final. The changes include updates to the check_no_magic_numbers.py script to support AST-based annotation checking, documentation updates, and a significant reduction in the baseline of flagged numbers. Feedback suggests simplifying the classification logic in the script by removing redundant type checks already handled by helper functions.
| if not _is_numeric_constant(inner): | ||
| return None |
There was a problem hiding this comment.
The _is_numeric_constant(inner) check already guarantees that inner is an ast.Constant with a numeric value (int or float, excluding bool). This makes several checks later in this function redundant (specifically the isinstance(inner, ast.Constant) and the type check on raw at lines 585-589). Cleaning these up would simplify the logic and might allow removing the PLR0911 (too many return statements) suppression.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/configuration-precedence.md`:
- Around line 137-145: The example in the docs is missing the Final import and
doesn't state the linter only recognises direct Final[...] annotations; update
the example to import Final from typing and clarify that only declarations using
Final (e.g., _MAX_FRAME_SIZE: Final[int] = 16384) are recognised by
scripts/check_no_magic_numbers.py, and that using typing.Final or omitted Final
will not be accepted; reference the constants JSONRPC_PARSE_ERROR,
A2A_TASK_NOT_FOUND and _MAX_FRAME_SIZE and mention typing.Final explicitly so
the snippet is copy-paste-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f13c337-bc15-4c20-80f5-c6729fa38916
📒 Files selected for processing (5)
CLAUDE.mddocs/reference/configuration-precedence.mdscripts/check_no_magic_numbers.pyscripts/no_magic_numbers_baseline.txttests/unit/scripts/test_check_no_magic_numbers.py
💤 Files with no reviewable changes (1)
- scripts/no_magic_numbers_baseline.txt
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test (Python 3.14)
- GitHub Check: Lighthouse Site
- GitHub Check: Build Web Assets (melange)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,ts,tsx,md}
📄 CodeRabbit inference engine (CLAUDE.md)
No region/currency/locale privileged; use metric units; British English
Files:
CLAUDE.mdscripts/check_no_magic_numbers.pydocs/reference/configuration-precedence.mdtests/unit/scripts/test_check_no_magic_numbers.py
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Numerics in README and public docs must be sourced from
data/runtime_stats.yamlvia<!--RS:NAME-->markers (Doc Numeric Claims)
Files:
CLAUDE.mddocs/reference/configuration-precedence.md
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Read design spec from
docs/design/page before implementing; deviations need approvalPresent every plan for accept/deny before coding (MANDATORY Planning)
Ship every convention PR with its enforcement gate
Use
SettingsService/ConfigResolverfor configuration precedence: DB > env > YAML > code default; noos.environ.getoutside startupNumerics live in
settings/definitions/; allowlist only 0, 1, -1, HTTP codes, hex masks, powers-of-2, and module-level annotated named constants (NAME: int|float|Final|Final[int]|Final[float] = literal)Never edit
tests/baselines/unit_timing.jsonor anyscripts/*_baseline.{txt,json}/scripts/_*_baseline.py; timeout/slow failures indicate source-code regression (Test Regression)After issue: create branch + commit + push (no auto-PR); use
/pre-pr-reviewcommand; nogh pr createAfter PR created: use
/aurelio-review-prfor external feedback; fix EVERYTHING valid; no deferringComments must explain WHY only; no reviewer citations, issue back-refs, or migration framing
No
from __future__ import annotations(Python 3.14+ has PEP 649); PEP 758 except forexcept A, B:(no parens unless binding)Type hints on public functions; mypy strict mode; Google-style docstrings
Line length 88 characters; functions <50 lines; files <800 lines
Errors must follow
<Domain><Condition>Errorpattern inheriting fromDomainError, never fromException/RuntimeErrordirectly (enforced bycheck_domain_error_hierarchy.py)Pydantic v2 models: API DTOs (Request/Response/Snapshot/Result/Envelope/Status/Info/Summary suffixes) must be frozen with
extra="forbid"; use@computed_fieldfor derived properties; useNotBlankStrfor identifiersArgs models at every system boundary; use
parse_typed()for every external dict ingestion (enforced bycheck_boundary_typed.py)Immutability: use
model_copy(update=...)for Pydantic models orcopy.deepcopy()for other types; deepcopy at system boundariesAsync code: use `as...
Files:
scripts/check_no_magic_numbers.pytests/unit/scripts/test_check_no_magic_numbers.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use D2 for architecture and nested container diagrams; use mermaid for flowcharts, sequence, and pipelines; use Markdown tables for tabular data; D2 theme 200 (Dark Mauve), D2 CLI pinned to v0.7.1 in CI
Files:
docs/reference/configuration-precedence.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test markers: use
@pytest.mark.{unit,integration,e2e,slow}; async tests use auto mode; global timeout 30s; coverage min 80%xdist auto-applied:
-n 8 --dist=loadfile(loadfile prevents 3.14+Windows ProactorEventLoop leak); set via pyprojectaddoptsWindows: unit tests use
WindowsSelectorEventLoopPolicy(3.14 IOCP teardown race); subprocess tests override backTest doubles: use
FakeClockfor Clock seam,mock_of[T](**overrides)for typed-boundary substitutions,SimpleNamespacefor attribute-bags; bareMagicMockat typed boundaries forbidden (enforced bycheck_mock_spec.py)Import
FakeClockandmock_offromtests._shared; inject viaclock=parameter and mock helper's spec subscriptHypothesis: 10 deterministic CI examples; failures are real bugs (fix + add
@example(...)decorator); never skip/xfail flaky testsFlaky tests: never skip/xfail; fix fundamentally; use
asyncio.Event().wait()notsleep(large)for timing
Files:
tests/unit/scripts/test_check_no_magic_numbers.py
⚙️ CodeRabbit configuration file
Test files do not require Google-style docstrings on classes or functions -- ruff D rules are only enforced on src/. A bare
@settings() decorator with no arguments on Hypothesis property tests is a no-op and should not be suggested -- the HYPOTHESIS_PROFILE env var controls example counts via registered profiles, which@given() honors automatically.
Files:
tests/unit/scripts/test_check_no_magic_numbers.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: Git commits: format `<type>: <description>` (feat/fix/refactor/docs/test/chore/perf/ci); commitizen-enforced
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: Signed commits required on protected refs (GPG/SSH or GitHub App via `synthorg-repo-bot`)
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: Branches: `<type>/<slug>` naming from main branch
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: Squash merge only; PR body becomes squash commit; trailers (`Release-As`, `Closes `#N``) must be in PR body
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: GitHub queries: use `gh issue list` via Bash, NOT MCP `list_issues`
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: After every squash merge → run `/post-merge-cleanup` command
Learnt from: CR
Repo: Aureliolo/synthorg
Timestamp: 2026-05-11T10:21:39.338Z
Learning: CLI is Docker-only (init/start/stop/status); features go in dashboard + REST API
📚 Learning: 2026-05-05T09:04:46.195Z
Learnt from: Aureliolo
Repo: Aureliolo/synthorg PR: 1760
File: scripts/_dual_backend_parity_lib.py:215-216
Timestamp: 2026-05-05T09:04:46.195Z
Learning: This repository targets Python 3.14+ and follows PEP 758. Therefore, reviewer tooling should NOT treat unparenthesized multi-exception `except` clauses written without an `as` clause (e.g., `except MemoryError, RecursionError:`) as syntax errors. Only flag `except`-clause problems when they are genuinely invalid for Python 3.14+.
Applied to files:
scripts/check_no_magic_numbers.pytests/unit/scripts/test_check_no_magic_numbers.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1866 +/- ##
=======================================
Coverage 84.89% 84.89%
=======================================
Files 1806 1806
Lines 105211 105211
Branches 9214 9214
=======================================
+ Hits 89319 89321 +2
+ Misses 13659 13658 -1
+ Partials 2233 2232 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
Aureliolo
temporarily deployed
to
cloudflare-preview
GitHub Actions
Inactive
…1856 phase 2) (#1872) ## Summary Finishes off the work tracked in (closed) issue #1856 by draining `scripts/no_magic_numbers_baseline.txt` from 499 entries down to **0**, exceeding the issue's stretch goal. Phase 1 (#1866) tightened `scripts/check_no_magic_numbers.py` to recognise typed module-level constants as already-named. This phase migrates the remaining real literals into either: - module-level `Final` constants (`NAME: Final[int|float] = literal`) for code-level invariants and bootstrap defaults, or - references to existing centralised constants where the value was already named elsewhere. Inline `# lint-allow: magic-numbers` markers in `src/`: 50 → 0. ## What changed - 313 `.py` files under `src/synthorg/`: inline numeric literals hoisted to module-level `Final` constants - `scripts/no_magic_numbers_baseline.txt`: 499 entries → header-only (0 entries) - `tests/evals/prompt/test_memory_consolidation_prompt.py`: contract test relaxed to accept Final references for the consolidation temperature pin - Net diff: 1264 insertions / 1304 deletions (slightly negative) No behavioural changes. No SQL / schema / migration / API / config changes. Every hoisted value was preserved exactly (verified by sampling 40+ files across the diff). ## Test plan - `uv run ruff check src/ tests/`: clean - `uv run ruff format src/ tests/`: 3712 files unchanged - `uv run mypy src/ tests/` (strict): clean across 3712 files - `uv run python -m pytest tests/ -m unit`: 28400 passed, 18 skipped (platform-only) - `uv run python scripts/check_no_magic_numbers.py`: exit 0 (no violations against the now-empty baseline) ## Review coverage Pre-reviewed by 10 parallel agents. 8 returned clean. 2 surfaced nomenclature suggestions (`_DEFAULT_LIMIT` repetition across controllers; `_DEFAULT_LIST_LIMIT_50` value-in-name in 9 persistence files); reviewed with the author and consciously deferred since the constants are module-private and the patterns are deliberate. Verified clean: value preservation across 40+ files, security-sensitive constants (JWT TTLs, PKCE lengths, OAuth thresholds, webhook clock-skew, rate-limit caps, sandbox config) across 18 files, API contracts across 36 controllers, SQLite/Postgres parity in persistence, docs consistency (CLAUDE.md, convention-gates.md, design pages).
<!-- HIGHLIGHTS_START --> ## Highlights > _AI-generated summary (model: `openai/gpt-4.1-mini` via GitHub Models). Commit-based changelog below._ ### What you'll notice - Password and secret fields now include an eye-toggle for easier visibility control. - Containers running without probes are shown as healthy in the doctor command. - Unloaded and missing PR-review agents are restored and available again. ### What's new - Gate baseline protection is enhanced to block em-dashes during writing. ### Under the hood - Replaced Atlas with yoyo-migrations for persistence management. - Refactored codebase extensively, including context-bound user authentication and registry pattern for enums. - Improved linting by draining magic number usages and tightening mock and constant checks. - Updated CI to retry Docker pushes on network timeout errors. - Updated apko lockfiles for dependency management. <!-- HIGHLIGHTS_END --> :robot: I have created a release *beep* *boop* --- ## [0.8.3](v0.8.2...v0.8.3) (2026-05-12) ### Features * harden gate baseline protection + block em-dashes at write time ([#1860](#1860)) ([b41f151](b41f151)) * **web:** eye-toggle on every password / secret field ([#1873](#1873)) ([9070387](9070387)) ### Bug Fixes * **ci:** retry Docker push on Go net/http deadline + cancellation errors ([#1877](#1877)) ([23a0bfa](23a0bfa)) * **cli:** render running-no-probe containers as healthy in doctor ([#1870](#1870)) ([6263795](6263795)) * restore unloaded and missing PR-review agents ([#1875](#1875)) ([db004fd](db004fd)), closes [#1871](#1871) ### Refactoring * bind authenticated user via ContextVar ([#1858](#1858)) ([57ed0b4](57ed0b4)) * code-structure cleanup (sub-tasks D + F + G + H + I) ([#1859](#1859)) ([362e5c8](362e5c8)) * convert enum dispatch to registry pattern ([#1854](#1854)) ([e90550e](e90550e)) * drain no_magic_numbers baseline to zero via Final hoists ([#1856](#1856) phase 2) ([#1872](#1872)) ([ec8109e](ec8109e)) * drain pagination + loop-init + kill-switch baselines ([#1857](#1857)) ([#1868](#1868)) ([115c3c2](115c3c2)) * **persistence:** replace Atlas with yoyo-migrations ([#1876](#1876)) ([1b7e975](1b7e975)), closes [#1874](#1874) * protocols audit follow-up (REVIEW + fold pass) ([#1869](#1869)) ([af33ddb](af33ddb)) * protocols audit follow-up REMOVE pass ([#1867](#1867)) ([dd1eebc](dd1eebc)) * tighten check_mock_spec gate, add mock_of[T], drain baseline ([#1862](#1862)) ([240a253](240a253)) * tighten check_no_magic_numbers for named module constants ([#1856](#1856)) ([#1866](#1866)) ([90c933b](90c933b)) ### CI/CD * update apko lockfiles ([#1863](#1863)) ([2bd32e6](2bd32e6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: synthorg-repo-bot[bot] <279117679+synthorg-repo-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Closes #1856 (phase 1 only; phase 2 migration in
src/is blocked on #1857).Summary
scripts/no_magic_numbers_baseline.txtpreviously froze 820 sites insrc/synthorg/. Spot-checking showed the gate flagged already-named typed module-level constants such asJSONRPC_PARSE_ERROR: int = -32700insrc/synthorg/a2a/models.py:124. Those are exactly the named constants the rule wants to encourage, not debt.This PR teaches the gate to recognise typed module-level named constants and skip them.
Gate change
scripts/check_no_magic_numbers.pynow skips module-level assignments where the annotation is one of:NAME: int = literalNAME: float = literalNAME: Final = literalNAME: Final[int] = literalNAME: Final[float] = literalBare
NAME = literal(no annotation) still flags: the developer must opt in by typing the assignment, so the gate retains its discriminating power.Qualified
typing.Final[int]is deliberately not matched (directFinalimports are the project convention). A test locks that scope decision.Carve-out lockdown
Existing per-call carve-outs (
_HTTP_STATUS_KEYWORDS,_IO_KEYWORD_NAMES,_IO_ALLOWED_POWERS_OF_2,_FILE_ALLOWLIST_PREFIXES) are now exhaustively locked by parametrized tests so any deletion from those sets surfaces as a failing row. Each set's source comment cross-references the locking test (# Locked by: ...).Baseline impact
scripts/no_magic_numbers_baseline.txt: 820 → 513 entries (-37 %, pure removals, no additions).Sites like
src/synthorg/a2a/models.pyJSON-RPC error codes and similar typed constants throughoutsrc/evaporated automatically.Test plan
tests/unit/scripts/test_check_no_magic_numbers.pypass (was 149 before review-round additions).uv run python scripts/check_no_magic_numbers.pyexits 0 against the new baseline.block growth of gate suppression baselinesconfirms pure shrink).uv run ruff check,uv run ruff format,uv run mypyclean.Review coverage
Pre-reviewed by 12 agents (code-reviewer, python-reviewer, docs-consistency, comment-quality-rot, issue-resolution-verifier, conventions-enforcer, pr-test-analyzer x2, type-design-analyzer, comment-analyzer, mini-pass-missing-event-constants, mini-pass-race-conditions). 13 valid findings addressed in commit 84d3f23; 1 invalid finding (false "line over 88 chars") rejected; 3 debatable / covered findings skipped with documented rationale. Triage at
_audit/pre-pr-review/triage.md.Notes for the reviewer
scripts/check_no_magic_numbers.pyis 857 lines after docstring tightening (-15 from initial 872; pre-existing baseline was 795). Still 57 lines over the 800-line soft guideline. Extracting baseline I/O into a separate module would bring it under but is scope creep for this PR.src/tosettings/definitions/and auditing the 50 inline# lint-allow: magic-numbersmarkers. That work is blocked on [REFACTOR] Drain pagination + loop-bound-init + kill-switch baselines (37 + 35 + 29 -> 0) #1857 to avoid file-level conflicts across many lifecycle services.After this PR is reviewed, run
/aurelio-review-prfor external feedback triage.