refactor(PR8/8 META-FINAL): distill LESSONS.md into CLAUDE.md + skill + lint hooks - #22
Merged
Merged
Conversation
…v dep Adds the lint primitives that PR8 will hook into CI + (optionally) pre-commit. Two artifacts: 1. scripts/check_ascii_print.py — stdlib-only AST visitor (zero deps, doubles as a pre-commit hook with no install footprint beyond Python). Walks every print() call and rejects non-ASCII string literals. Skips streamlit_app.py (Streamlit is UTF-8-safe in the browser; the cp1252 stdout-codec constraint is CLI-only). Emits file:line: msg violations on stderr; exits 1 when any are found. Catches LESSONS Theme 18 (ASCII-only CLI output on Windows) at commit-time instead of post-merge bug report. 2. ruff>=0.6 added to requirements-dev.txt for the F401 'unused import' check (LESSONS Themes 2 + 19). ### Pre-existing violations cleaned up The new tools immediately found 30 unused imports + 6 em-dash chars in print() literals, accumulated across 7 PRs of refactor: - Em-dashes: src/data_loader.py:315, src/efficient_frontier.py:294, fetch_data.py:99/114/126/179. Replaced with ASCII '--'. - Unused imports: 30 occurrences across 15 files (backtest.py, fetch_data.py, src/* and tests/*). Auto-fixed via 'ruff --fix'. After cleanup: 464 tests still pass, 0 warnings, byte-identical CLI output preserved. ### NOT in this commit (next ones) - CI 'lint' job in .github/workflows/tests.yml (commit #2). - .pre-commit-config.yaml for optional local pre-commit hook (commit #3). - CLAUDE.md + .claude/skills/pr-self-review/SKILL.md (commit #4).
New parallel job in .github/workflows/tests.yml:
- Single Ubuntu / Python 3.12 runner (lint output is OS/minor-version
invariant; matrixing would burn CI minutes for no signal).
- Step 1: ruff check --select F401 across src/, tests/, all root
Python entry points (backtest.py, fire.py, fetch_data.py, analyze.py,
streamlit_app.py). Catches LESSONS Themes 2 + 19 (unused imports +
orphan-after-cleanup).
- Step 2: python scripts/check_ascii_print.py over the CLI surface
(backtest.py, fire.py, fetch_data.py, analyze.py, src/). Catches
LESSONS Theme 18 (Windows cp1252 UnicodeEncodeError on emoji/em-dash
in print()).
streamlit_app.py is checked by F401 but NOT by the ASCII print check —
the Streamlit browser is UTF-8-safe and the CLI-codec constraint
doesn't apply to it. The script's internal exclusion list documents
this; the CI step explicitly omits streamlit_app.py from its arg list
as belt-and-suspenders.
The new job runs on every push to main and every pull_request to main
alongside the existing 6-cell matrix test job. Together they form a
2-gate guardrail: lint must pass AND tests must pass before merge.
A failing lint produces:
src/x.py:42:5: F401 [*] imported but unused
src/y.py:99: non-ASCII char(s) inside print() string literal: U+2014 ('--')
which surfaces in the GitHub Actions log + 'Files changed' annotations.
Mirrors the CI lint job for opt-in local enforcement. Setup is two
commands and pre-commit installs the hooks into .git/hooks/pre-commit:
pip install pre-commit
pre-commit install
After install every 'git commit' runs the hooks; failures block the
commit until fixed (or bypassed via --no-verify, in which case CI still
catches it before merge).
Hooks:
1. ruff F401 from astral-sh/ruff-pre-commit (pinned v0.6.9). Only
touches files under src/, tests/, and the 5 root entry points so
third-party files in .venv aren't scanned.
2. Local hook 'check-ascii-print' that runs scripts/check_ascii_print.py
over backtest.py, fire.py, fetch_data.py, analyze.py, src/. The
script's internal exclusion list takes care of streamlit_app.py
(UTF-8-safe in browser; CLI codec rule doesn't apply).
Why opt-in instead of mandatory: pre-commit isn't a runtime dependency
of the engine -- requiring it would force every contributor to install
it just to read the code. The CI lint job is the mandatory gate. This
file is a convenience for contributors who want fast local feedback.
…ntions section
Final distillation of LESSONS.md (23 themes) into operational artifacts
that Claude Code and human contributors load automatically.
### CLAUDE.md (project root, ~280 lines)
Imperative 'rules of the road' grouped by MOMENT OF CODE CHANGE rather
than by theme number — that's how you actually decide what to check
while you work. Sections:
- Project conventions (Python 3.11+, byte-identical guarantee, plan-mode
workflow, parallel-PR stacking)
- Before you commit any PR (5 universal checks)
- When you change a function signature (transitive callers,
user-controlled fields, type hints)
- When you parse external input (per-field coercion, allowlists,
reject-no-runtime-effect, catch all expected exceptions, ground truth
vs metadata cache)
- When you write Streamlit code (apply not just display, filter UI to
backend, snapshot run-time params, end-to-end wiring, no-mapping
fallback, no bare tracebacks)
- When you write CLI code (help-default alignment, exit codes)
- When you write tests (docstring honesty, runtime config not
hardcoded labels, wrong-type-per-allowed-field matrix)
- When you delete a feature (imports + tests + docs)
- Known tech debt (with rationale why deferred)
- Tooling reference
### .claude/skills/pr-self-review/SKILL.md
YAML-frontmatter Claude Code skill that runs the 23-theme checklist
against a staged diff. Trigger when about to push / open PR / address
Copilot review. 5 numbered steps:
1. Survey the diff (git diff --stat / log).
2. Run the local lint gate (ruff F401 + check_ascii_print.py).
3. Run pytest -q.
4. Walk the 23-theme checklist (organized by category, each item
with a per-theme PR# example for grounding).
5. Self-summary — what was verified, what was skipped (with reason),
byte-identical guarantee status if engine touched.
The skill complements CLAUDE.md: CLAUDE.md is the standing rule set,
the skill is the operational checklist with diagnostic commands per
item.
### README.md additions
- Step #6 in the development workflow: optional pre-commit install
(the new .pre-commit-config.yaml for fast local feedback)
- Step #7 mentions the new CI lint job
- New 'Repository conventions' subsection pointing at CLAUDE.md +
LESSONS.md + the pr-self-review skill + the lint scripts. Read once
before your first PR.
### Verification
- pytest -q -> 464 passed, 0 warnings (no engine change).
- python backtest.py --synthetic still byte-identical to main.
- ruff check --select F401 ... -> 0 errors.
- python scripts/check_ascii_print.py ... -> 0 violations.
PR8 is the LAST PR of the refactor. After merge, the repo has rules
permanent (CLAUDE.md), skill auto-invoked (pr-self-review), lint
automated (CI + optional pre-commit). Future PRs should land cleaner.
There was a problem hiding this comment.
Pull request overview
This PR turns prior review “lessons” into repo-enforced workflow artifacts: documentation/standing rules, an auto-invoked self-review checklist, and CI/local lint hooks. It also applies the new lint standards by removing unused imports and replacing a few non-ASCII CLI print() literals.
Changes:
- Add
scripts/check_ascii_print.pyand wire a new GitHub Actionslintjob to enforce ASCII-onlyprint()string literals plus Ruff F401 (unused imports). - Add optional local
.pre-commit-config.yamlhooks mirroring CI lint. - Add contributor-facing operational docs (
CLAUDE.md, PR self-review skill) and update README conventions; clean up unused imports and a few CLIprint()strings.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/check_ascii_print.py |
New AST-based checker to reject non-ASCII string literals inside print() calls. |
.github/workflows/tests.yml |
Adds a dedicated lint job running Ruff F401 + ASCII print() check. |
.pre-commit-config.yaml |
Optional local hooks mirroring the CI lint gate. |
requirements-dev.txt |
Adds Ruff as a dev dependency for CI/local lint. |
CLAUDE.md |
New standing rules/checklist distilled from prior PR review themes. |
.claude/skills/pr-self-review/SKILL.md |
Adds a “PR self-review” skill checklist and suggested local commands. |
README.md |
Documents the new conventions, pre-commit hooks, and CI lint job behavior. |
src/data_loader.py |
Removes unused imports; adjusts a CLI-facing print() message to ASCII. |
src/efficient_frontier.py |
Adjusts a CLI-facing print() message to ASCII (-- instead of em-dash). |
src/portfolio.py |
Removes unused dataclasses import. |
src/plots.py |
Removes unused typing imports. |
src/monte_carlo.py |
Removes unused typing import. |
backtest.py |
Removes unused imports and trims unused symbol import. |
fetch_data.py |
Removes unused import; replaces em-dash in CLI strings with ASCII --. |
streamlit_app.py |
Removes unused matplotlib import. |
tests/test_*.py (multiple) |
Removes unused imports across the test suite to satisfy Ruff F401. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+80
| # We only inspect literal string args. f-strings (JoinedStr) | ||
| # and computed exprs (Call/Name/...) get a pass — false-positive | ||
| # avoidance trumps theoretical completeness here. The whole | ||
| # point of the rule is to catch obvious authored Unicode in | ||
| # print literals, which is what the LESSONS theme documents. |
There was a problem hiding this comment.
_scan_file() skips f-strings (ast.JoinedStr), so non-ASCII characters in the literal portions of f-strings (e.g., currency symbols or arrows embedded before a {format}) will not be flagged even though they still reach stdout and can trigger UnicodeEncodeError on some Windows code pages. Consider scanning JoinedStr constant segments (and/or updating docs to clearly state the limitation) so the check matches the intended "ASCII-only CLI output" policy.
| ``` | ||
|
|
||
| **PRs must pass CI to be merged.** The CI runs on Ubuntu / macOS / Windows × Python 3.11 / 3.12. If your PR adds new behavior, please add corresponding unit tests — see [Testing](#testing) for conventions. | ||
| **PRs must pass CI to be merged.** The CI runs on Ubuntu / macOS / Windows × Python 3.11 / 3.12 (matrix tests) plus a single Ubuntu / 3.12 `lint` job that enforces no unused imports (ruff F401) and ASCII-only `print()` literals on the CLI surface. If your PR adds new behavior, please add corresponding unit tests — see [Testing](#testing) for conventions. |
There was a problem hiding this comment.
This section says the lint job enforces "ASCII-only print() literals" on the CLI surface, but scripts/check_ascii_print.py currently only checks plain string constants and skips f-strings/expressions. To avoid confusion for contributors, consider adjusting the wording (or enhancing the checker) so the README accurately describes what CI will reject.
Comment on lines
+59
to
+62
| char > 127 inside `print()` literals in `backtest.py`, `fire.py`, | ||
| `fetch_data.py`, `analyze.py`, or `src/*.py`. Run | ||
| `python scripts/check_ascii_print.py backtest.py fire.py fetch_data.py analyze.py src/`. | ||
| The CI `lint` job blocks merge if it fires. |
There was a problem hiding this comment.
The rule here says "char > 127 inside print() literals" and points to check_ascii_print.py, but that script currently only inspects plain string constants (it explicitly skips f-strings/JoinedStr). As written, this overstates what the CI gate actually enforces and may mislead contributors; either tighten the wording to match the actual check, or extend the checker to also validate the literal parts of f-strings.
Suggested change
| char > 127 inside `print()` literals in `backtest.py`, `fire.py`, | |
| `fetch_data.py`, `analyze.py`, or `src/*.py`. Run | |
| `python scripts/check_ascii_print.py backtest.py fire.py fetch_data.py analyze.py src/`. | |
| The CI `lint` job blocks merge if it fires. | |
| char > 127 in plain string constants passed to `print()` in | |
| `backtest.py`, `fire.py`, `fetch_data.py`, `analyze.py`, or `src/*.py`. | |
| Run `python scripts/check_ascii_print.py backtest.py fire.py fetch_data.py analyze.py src/`. | |
| The CI `lint` job blocks merge if it fires; this check currently covers | |
| plain string constants, not f-strings. |
Copilot flagged that scripts/check_ascii_print.py only inspected
ast.Constant (plain strings) and skipped ast.JoinedStr (f-strings),
while CLAUDE.md and README claimed broader coverage. The static text
segments of f-strings (e.g. "NAV: EUR" in f"NAV: EUR{x:,.0f}") were
silently bypassed, allowing 11 latent non-ASCII violations to remain
in backtest.py and fire.py despite the linter "passing".
Changes:
- scripts/check_ascii_print.py: walk JoinedStr.values, check each
Constant segment. Computed FormattedValue slots ({...}) remain
out of scope (their runtime value is unknowable at AST time —
documented in the script docstring).
- backtest.py (9 fixes): EUR substitutions in Monte Carlo output,
"x" for multiplication sign, "--" for em-dash separators.
- fire.py (2 fixes): EUR for legacy nominal/real lines.
- tests/test_check_ascii_print.py: new file, 16 tests covering the
violation matrix (plain literals, f-string segments, computed
values not inspected, print() boundaries, file-level edge cases).
- CLAUDE.md rule #5: now describes actual coverage accurately.
- README "Repository conventions": same correction.
- LESSONS.md: new Theme 24 ("A linter that's too narrow lies about
its coverage") to capture the meta-lesson for future audits.
Verified:
- pytest -q -> 480 passed
- ruff check --select F401 -> All checks passed
- python scripts/check_ascii_print.py backtest.py src/ fire.py
fetch_data.py analyze.py -> EXIT=0
- python backtest.py --synthetic -> byte-identical to main
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
Last PR of the refactor. Converts the 23 LESSONS.md themes accumulated across 7 PRs of Copilot reviews into permanent operational artifacts: standing rules, an auto-invoked skill, and CI-enforced lint.
Regola ferrea preservata:
python backtest.py --syntheticbyte-identical a main. Engine non toccato.Commits (4)
feat(PR8): scripts/check_ascii_print.py + ruff F401 cleanup + ruff dev depci(PR8): add lint job (ruff F401 + check_ascii_print.py)chore(PR8): .pre-commit-config.yaml -- optional local lint hooksdocs(PR8): CLAUDE.md + pr-self-review skill + README repository-conventions sectionArtifacts shipped
CLAUDE.md(~280 lines) — imperative standing rules, grouped by moment of code change (before-commit / signature-change / external-input / Streamlit / CLI / tests / cleanup). Includes Known tech debt section with rationale on what was deferred and why..claude/skills/pr-self-review/SKILL.md— Claude Code skill auto-invoked before pushing. 5 numbered steps, 23-theme checklist with per-item PR# example for grounding, diagnostic commands inline.scripts/check_ascii_print.py— stdlib-only AST visitor that rejects non-ASCII string literals insideprint()calls. Catches LESSONS Theme 18 (Windows cp1252 crashes on emoji/em-dash/U+2192)..github/workflows/tests.yml::lintjob — single Ubuntu/3.12 runner that gates merge onruff check --select F401+check_ascii_print.py. Runs in parallel to the existing 6-cell test matrix..pre-commit-config.yaml— opt-in local hooks for fast feedback (mirrors CI).Pre-existing debt cleaned up
The new lint tooling immediately found 30 unused imports (
F401) + 6 em-dash chars inprint()literals, accumulated across 7 PRs of refactor. All auto-fixed byruff --fixand a small set of manual ASCII-replacements. 464 tests still pass, 0 warnings, byte-identical CLI output preserved.Why no sensitivity cleanup
src/sensitivity.py::_snapshot_config/_restore_config/_apply_param_overrideprivate helpers stay. The legacyportfolio=Nonebranch inrun_sensitivity_sweepuses them and implements equity-internal absorption logic forput_write/nasdaq_top30/momentum/qualitysweeps that's semantically different from the generic-path "absorb from cash" logic. Refactoring would shift the numeric output ofpython backtest.py --sensitivity put_writeon the Four Umbrellas preset — violating the byte-identical guarantee that's been preserved across 7 PRs. Documented in CLAUDE.md "Known tech debt" with the rationale + two future-PR options for whoever picks it up.Verification
pytest -q→ 464 passed, 0 warningsruff check --select F401 ...→ All checks passed!python scripts/check_ascii_print.py ...→ 0 violationspython backtest.py --syntheticbyte-identical to mainlintjob passes on first push (will be visible after this PR opens)Repository state after merge
Portfoliodataclass +OptionsConfig+ TOML catalog + Streamlit Impostazioni tab + save/load + Frontier/FIRE/walk-forward integrated.🤖 Generated with Claude Code