check_system advises on key shape, and the shape rules design is closed out (D64, step 5)

[twilde]

What and why

Step 5, the last, of the key shape rules design (D64). Two
commits, each readable on its own.

1. Refactor, no behavior change. Config.master_keys ("the keys in the masters of any
   core") and ShapeRules.is_off() replace the solver's own copies, because the checker needs
   both. The solver's output with the defaults on is identical to main's on 12 seeded runs of
   four fake systems.
2. The checker, and the close-out.
   • check_system prints a "Key shape" section: a SHAPE line per rule a key or control key
     breaks, with pin positions and never cuts, using the file's shape settings. It is advice:
     the exit status and the counts of problems and warnings do not change. Decoded unit keys
     and retired keys are not checked, and the section is left out when every rule is off.
   • The design document is marked Implemented. Its open questions are answered under
     "What building it showed": a thin pinnable set can leave no key that follows the rules
     (reported, not repaired); the search never got stuck in 100 seeded solves; and the
     objective's other bias is still there (a change key shares a master's cut at 26.8% of
     positions against 24.7% before and 20% by chance), which the rules do not touch.
   • D65: a key with no bitting that follows the rules is left null, said so in NOT SOLVED, and the full check is skipped. That was decided while building step 4 and had no
     log entry yet.
   • README, docs/design.md and TODO.md (the objective's holding bias; exact reachability
     bounds) are updated.

Route: a pull request, because main takes changes by pull request and the code should pass CI.

Never include real key data

None: tests and docs use the fake fixtures and invented keys; the measurements are of fake systems.

Checklist

• I have not included real bittings, real system files, or anything that identifies a real building.
• One logical change (refactors are in their own commits).
• Tests added or updated, and pytest passes. A bug fix has a regression test.
• README updated if behavior changed; a design decision gets a short entry in docs/decisions.md and, if it changes how things are, an edit to docs/design.md (and the feature's document in docs/designs/, if it has one).
• No new runtime dependencies; randomness defaults (secrets, random.SystemRandom) unchanged.

Behavior changes and AI assistance

check_system output gains the "Key shape" section when any rule is on, which is by default.
I compared it with main on system.example.json and the clean, pinning and five-pin
fixtures: with every rule set to null the output (stdout, stderr, exit code) is identical, and
with the defaults on it is identical once the new section is removed. Exit codes are unchanged.
The solver's scoring and algorithms are untouched (D6).

Tests run: ci_changes.py says scanner=false, so the whole suite in a venv with only
.[test]: 1042 passed, 16 skipped.

Written with Claude Code (Co-Authored-By trailer on the commits).

🤖 Generated with Claude Code

[twilde]

Changes requested

Reviewed head 60ae97e, two commits on main at b3772a1 (so the merge into current main is trivial). One Should fix (a docs claim that contradicts the checker) and two Optional notes. The code itself is sound.

What I ran

• The whole suite in a venv with only .[test] (ci_changes.py says scanner=false): 1042 passed, 16 skipped. The scanner's tests that need the extra were not run, and this PR does not touch the scanner.
• scripts/check_no_stray_data.py --history: clean. CI on the head is green (12 checks, the scanner job skipped).
• check_system on main and on the head, with system.example.json: the output differs only by the new "Key shape" section, and stderr and the exit code (0) are the same.
• The same file with every shape rule set to null: stdout, stderr and exit code identical to main, and no section is printed.
• A fake file with a master named unit:8, only max_run set and a core that makes unit: keys the masters: exit status 1 on both main and the head, with the same problem counts, and the new section is the only difference.
• Privacy scan of the diff and commit messages: no real-looking data (the seven-digit strings are in tests and are obviously fake), and both commits have the noreply author and committer and a Co-Authored-By trailer. Design prose speaks in general scenarios.

Verdict on the code and behaviour: Config.master_keys and ShapeRules.is_off() replace the solver's copies faithfully, the SHAPE lines show positions only, and the exit status and the counts are untouched, as the description says. The tests cover the exemptions, the cap, the rules-off case and the file's own settings.

What blocks: the new text in docs/design.md (and the existing line it repeats in docs/designs/key-shape-rules.md) says decoded unit keys are exempt from the shape advice "as for parity and MACS in D11". D11 exempts only retired keys, and check_system does check unit keys for parity and MACS. See the inline comment.

Review written with AI assistance (Claude).

---

Generated by Claude Code

[twilde]

Approved

Re-review of head 53c6421 (two new commits since my review of 60ae97e). main is still at b3772a1, so the merge into current main is trivial.

Earlier threads

• Should fix, D11 cited wrongly for the unit-key exemption: closed. docs/design.md and docs/designs/key-shape-rules.md now say retired keys are history (which is also why D11 exempts them from parity and MACS), and that decoded unit keys are exempt from the SHAPE lines only, with parity and MACS still applying, plus the reason. That matches what check_system does. The new test test_decoded_unit_keys_are_exempt_from_shape_advice_but_not_from_parity pins both halves (a PARITY line and no SHAPE line for a wrong-parity unit key).
• Optional, nested comprehension: done. checked = {...} is named first and the comprehension reads over it. The output for system.example.json is byte-identical to the previous head's.
• Optional, D65 in its own commit: declined with a reason, accepted. The entry's wording was already fine, and I would not rewrite and force-push a reviewed branch for a history tidy-up. The PR description already says it records a step-4 decision. The author plans a note in the merge commit body if it is merged that way.

What I ran on the new head

• The whole suite in a venv with only .[test] (ci_changes.py says scanner=false): 1043 passed, 16 skipped. The scanner's tests that need the extra were not run, and the PR does not touch the scanner.
• scripts/check_no_stray_data.py --history: clean. CI on 53c6421 is green (12 checks, the scanner job skipped).
• The two new commits touch only two docs, one readability change in check_system.py and one test. Both have the noreply author and committer and a Co-Authored-By trailer. There is no real-looking data, and the new test uses an obviously fake bitting.

Nothing blocks. The author or the maintainer resolves the threads; all three already show as resolved.

Review written with AI assistance (Claude).

---

Generated by Claude Code