Read key shape rules from an optional shape field in the system file (D64, step 2)

[twilde]

What and why

Step 2 of the key shape rules design (D64): a system file can
carry an optional shape object, and Config.shape is the resulting ShapeRules.

• A file with no shape gets the defaults for its depth count; {} is the same.
• null turns a rule off (forbid_monotone also accepts false), and a rule left out keeps
  its default. Fields starting with _ are free text, as elsewhere in the file.
• Every mistake is a ConfigError naming the rule, so the tools print one error: line:
  shape not an object, an unknown rule (an error, with a did-you-mean, because a
  misspelled rule would silently leave the default on, as the design says), a bad value, a
  master_min_span wider than any key can span (bounded by the depths, and by the pins and
  max_step; the review notes on Accept the key shape rules design, and add ShapeRules (D64, step 1) #35 and Read key shape rules from an optional shape field in the system file (D64, step 2) #36), and a
  min_total_variation that no key of that many pins and that max_step could reach.
  The last is a small addition to the design, for the same reason as the span check: a rule
  nothing can meet would end every solve that needs it in NOT SOLVED with nothing wrong in
  the file to point at.
• The solver writes the file back from its raw form, so shape survives a solve (tested).

Nothing applies the rules yet. The README row for shape is left to the step in which a tool
first uses it, so the README does not describe behavior that is not there. The depth count
cannot be set from a file today (it is 10), so the depth-dependent cases are tested on the
helper with a small key space.

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

Never include real key data

Nothing here has any: tests use the fake clean fixture.

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 (see above: no tool applies the rules yet); the decision is already D64 and the design document describes this step.
• No new runtime dependencies; randomness defaults (secrets, random.SystemRandom) unchanged.

Behavior changes and AI assistance

No command's output changes. A file that already has a top-level shape field used to get an
"unknown top-level field" warning and now has it read as the rules.

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

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

🤖 Generated with Claude Code

[twilde]

Changes requested

One Should fix, one Optional. Everything else checks out.

Should fix: the master_min_span check does not use max_step or the pin count (inline comment on config.py). A narrow file, for example 5 pins with max_step 1, gets a default span of 6 that no key can reach, and an explicit master_min_span: 6 is accepted too. That is the failure the PR says the new validation exists to prevent, and the min_total_variation check already uses the right bound.

What I ran, at head 74cf5d7, which is based on current main (785ce7a), so merging adds no new commits:

• Whole suite in a venv with only .[test], since ci_changes.py says scanner=false: 953 passed, 16 skipped. This matches the PR description.
• scripts/check_no_stray_data.py --history: clean.
• Privacy scan of the diff and commit message: no seven-digit strings, no key-history facts. The author and committer are the noreply address, and the commit has the Co-Authored-By trailer.
• The probes below, run against five_pin.json with small edits:
  • shape absent and shape: {} both give the defaults.
  • shape: null is an error, shape: {"max_run": null} turns that rule off, and forbid_monotone null or false both turn it off.
  • A bad value for any rule is a ConfigError naming that rule.
  • An unknown rule is an error with a did-you-mean hint, and an _-prefixed key is free text.
  • The master-span and total-variation bounds accept the limit value and reject one above it.
  • The span-with-max_step-1 case above, which is accepted and should not be.
• CI is green on the head, and the scanner jobs are correctly skipped.

Docs: no tool uses the rules yet, so leaving the README row for shape until the step that first applies it is reasonable. The min_total_variation reachability check is not in the design document (see the Optional note). The changed message in model.py and the TOP_LEVEL_FIELDS change are fine.

Review written with AI assistance (Claude).

---

Generated by Claude Code

[twilde]

Should fix — the span check only looks at the number of depths, but max_step and the pin count also limit how wide a key can be. A key of pins cuts can span at most (pins - 1) * max_step (and at most depths - 1). This is the same bound you use for min_total_variation a few lines down, and your own reason for that check (a rule nothing can meet ends every solve that needs it in NOT SOLVED with nothing in the file to point at) applies here too.

Reproduction, with tests/fixtures/five_pin.json and max_step set to 1 (5 pins, so no key spans more than 4):

• no shape at all: parses, and the default master_min_span is 6, which no key can meet;
• "shape": {"master_min_span": 6}: parses with no error.

Both end in NOT SOLVED for any master once the solver applies the rule. Suggested: reject an explicit value above min(depths - 1, (pins - 1) * max_step), and for the default case cap it the same way in ShapeRules.for_depths (or pass pins and max_step to it), so a narrow file does not start with an unmeetable default that it never wrote. Add a test with a small max_step.

---

Generated by Claude Code

[twilde]

Fixed in b5a577e, with your reproduction as tests (5 pins, max_step 1). KeySpace now has widest_span = min(depths - 1, (pins - 1) * max_step) and most_variation; ShapeRules.for_depths became for_space(space), so the default span is capped by the same bound (4 for that file, not 6), and an explicit master_min_span above it is a ConfigError naming the pins, the cuts and max_step. The total-variation check now reads its bound from most_variation too, so both use one source.

[twilde]

Optional — two small things on this check. (1) The bound ignores pattern: with OEOEO and max_step 4, adjacent cuts must differ by an odd amount, so a step is at most 3 and the most variation is 12, yet min_total_variation: 16 is accepted. The check is conservative (it never rejects a rule some key could meet), so this is fine to leave, but say so in the comment or the design. (2) This check is an addition to the design: docs/designs/key-shape-rules.md, plan step 2, names only the master_min_span check. CLAUDE.md asks that the design document is edited in place when the design changes, so add a sentence there (and a line in the D64 entry if you think it counts as a decision).

---

Generated by Claude Code

[twilde]

Fixed in b5a577e: (2) plan step 2 in the design document now names both checks, the bounds, and the reason; I did not add to D64, since it is a detail of validation and the entry is at its word limit. (1) Said in the _shape_rules docstring and in the design that both bounds ignore the parity pattern, so they never reject a rule some key could meet but may accept one none can. Left conservative rather than computing the exact reachable value, which would need a pass over the digit sets for little gain.

[twilde]

Approved

Re-review of b5a577e (one new commit on top of 74cf5d7). Nothing blocks.

Earlier threads

• Should fix: the span check ignored max_step and the pin count. Closed. KeySpace.widest_span is min(depths - 1, (pins - 1) * max_step), and ShapeRules.for_space caps the default span with it, so a narrow file no longer starts with a default it never wrote. An explicit master_min_span above the bound is a ConfigError that names the pins, the cuts and max_step. I reproduced the original case on five_pin.json with max_step 1:

  • no shape: the default span is 4, not 6;
  • an explicit span of 6: rejected;
  • span 4, or null: accepted.

  With max_step 2, span 8 is accepted and 9 is rejected, which is the right edge. min_total_variation now reads KeySpace.most_variation, so both bounds have one source. The tests use the 5-pin, max_step 1 case, and test_model.py covers the new properties and for_space, including 1 pin and 1 depth.

• Optional: the bound ignores pattern, and the design document did not mention the min_total_variation check. Addressed. The _shape_rules docstring and plan step 2 of the design document now name both checks, the bounds, why they exist, and that both ignore parity. Leaving D64 alone is fine, since a validation detail does not need a log entry and the entry is at its word limit.

What I ran

• Based on current main (785ce7a), so merging adds no new commits.
• Whole suite in a venv with only .[test], since ci_changes.py says scanner=false: 956 passed, 16 skipped.
• scripts/check_no_stray_data.py --history: clean.
• Privacy scan of the diff and both commit messages: no seven-digit strings, no key-history facts. Author and committer are the noreply address on both commits, and both have the Co-Authored-By trailer.
• No remaining for_depths references anywhere in the repo, since the rename to for_space touched every caller.
• CI is green on the head, and the scanner jobs are correctly skipped.

Note, not blocking: the PR description still says "953 passed, 16 skipped"; the head now gives 956. Worth updating before merge.

Review written with AI assistance (Claude).

---

Generated by Claude Code