let-fate-decide: improved 12-card spread (100+ bits of entropy)#166
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
- Compute ENTROPY_BITS from math.log2(math.comb(...)) so the dict cannot silently drift if the spread shape changes; test now verifies the formula and asserts the 100-bit floor. - Remove the ambiguous draw() wrapper (list-or-dict return); callers use explicit draw_zodiac_spread() / draw_cards() instead. - Extract _build_house_record() helper to bring draw_zodiac_spread() under the 50-line guideline. - Require --legacy explicitly for the positional count CLI; remove the implicit legacy flip when a bare integer was passed. - Generalize the rank-format test to all four minor suits. - Document test_reviewed_cards_avoid_unsafe_shortcuts as an exact-string regression guard rather than a semantic check. - SKILL.md: reframe example session as an explicit house-level fragment and note the draw agent's bullet output format. - README.md: correct the deck-shuffle description (two independent decks, not one 78-card shuffle); compute entropy text from formulas. - marketplace.json + plugin.json: sync description to mention the 12 Houses spread and the 100+ bit entropy budget. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@claude review |
…aces PR review flagged the SKILL.md frontmatter said "Draws a 12 Houses..." while plugin.json, marketplace.json, and agents/draw.md all say "Draws the 12 Houses...". SPREAD_NAME is a single fixed spread, so "the" is correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR review flagged inconsistent naming in the entropy_bits dict: major_arcana (deck-named) vs minor_shuffle (operation-named). The value is also log2(C(56,24)), an unordered selection — not log2(56!), a full shuffle — and every prose consumer (SKILL.md, README.md, the entropy_note f-string) already calls it "Minor Arcana selection". Rename for parallel structure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1. agents/draw.md: tighten portent template prose from "3-5 concise bullets" to "3 concise bullets" — the fenced template has exactly 3 placeholders, and a Haiku-class model will follow the template literally rather than the looser range. SKILL.md's example session synced to match. 2. references/INTERPRETATION_GUIDE.md: rewrite the Special Patterns section for the 12-house spread. The previous patterns were carried over from the 4-card hand and were broken in the new geometry: "Multiple Major Arcana" was guaranteed (12 Majors per draw), "All One Suit" was mathematically impossible (each Minor suit has only 14 cards vs. 24 Minor draws), "All Reversed" dropped from 2^-4 to 2^-36, and "Court Card Progression in sequence" had no meaning across houses. Replaced with four patterns calibrated to the new baseline frequencies: heavy reversal count (>= 24 of 36), heavy single-suit concentration (>= 10 of 24 Minors), weighty Majors in critical houses (1/8/12), and court cards clustering in people houses (3/7/11). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…heading - Correct "Heavy Reversal Count": P(X>=24) for Binomial(36, 0.5) is ~3.26%, so "6th percentile" was wrong in both magnitude and direction. Reword to "roughly the top 3% of draws". - Align each house focus string to its house file **Domain** line (Houses 6, 7, 8, 9, 12 differed in content; House 8 dropped "secrets"). Both surfaces reach the interpreter on the default --content path. - Add test_zodiac_focus_matches_house_domain to pin the two surfaces together. - Fix Step 1 heading to "Read Each House and Card File" to match its body. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| --- | ||
| name: draw | ||
| description: Draw 4 Tarot cards and return a 1-2 sentence reading. Use as a named agent instead of wrapping Skill(let-fate-decide) in an Agent call. Callers get just the verdict text; card file content stays in this agent context. | ||
| description: Draw the 12 Houses of the Zodiac Tarot spread and return a concise structured reading. Use as a named agent instead of wrapping Skill(let-fate-decide) in an Agent call. Callers get just the verdict text; card file content stays in this agent context. |
There was a problem hiding this comment.
🟡 The agent description on line 3 starts with the imperative 'Draw the 12 Houses...' while the three sibling description surfaces touched by this PR all use the third-person 'Draws...' (plugin.json line 4, marketplace.json line 350, and SKILL.md line 3). The repo's CLAUDE.md line 133 explicitly mandates third-person voice for skill/agent descriptions, and every other agent description across plugins/*/agents/*.md follows that convention. One-character fix: change 'Draw' → 'Draws' on agents/draw.md line 3 to align all four trigger surfaces.
Extended reasoning...
What the bug is. plugins/let-fate-decide/agents/draw.md line 3 reads:
description: Draw the 12 Houses of the Zodiac Tarot spread and return a concise structured reading. ...
The description starts with the imperative verb form Draw. The three sibling description surfaces touched by this same PR all begin with the third-person Draws:
.claude-plugin/marketplace.jsonline 350 →"Draws the 12 Houses of the Zodiac Tarot spread ..."plugins/let-fate-decide/.claude-plugin/plugin.jsonline 4 →"Draws the 12 Houses of the Zodiac Tarot spread ..."plugins/let-fate-decide/skills/let-fate-decide/SKILL.mdline 3 →"Draws the 12 Houses of the Zodiac Tarot spread ..."
Why it matters. The repo's own CLAUDE.md at line 133 explicitly mandates: "Third-person voice: 'Analyzes X' not 'I help with X'" (line 77 reinforces the same rule in the skill frontmatter template). A spot-check of every other agent description under plugins/*/agents/*.md confirms third-person is universal across the ~20 agents in this repo — verbs like Performs, Analyzes, Reviews, Verifies, Creates, Scans, Models, Identifies, Resolves, Propagates, Discovers, Validates, Executes. agents/draw.md is the lone outlier.
How it manifests. These four strings are trigger/description surfaces — they drive how Claude and the marketplace UI describe and decide to invoke the skill. Drift between them erodes trigger consistency (search/keyword matches and human readers may register the imperative form as a different concept than the third-person sibling versions). No functional behavior changes; this is the same shape of pure copy-edit drift the PR already accepted a fix for in commit e4ff011 (a 12 Houses → the 12 Houses article alignment across the same four surfaces).
Impact. Cosmetic/style consistency only. No functional regression.
Step-by-step proof.
- Open
plugins/let-fate-decide/agents/draw.mdline 3 →description: Draw the 12 Houses of the Zodiac Tarot spread ...(imperative). - Open
.claude-plugin/marketplace.jsonline 350 →"Draws the 12 Houses of the Zodiac Tarot spread ..."(third-person). - Open
plugins/let-fate-decide/.claude-plugin/plugin.jsonline 4 →"Draws the 12 Houses of the Zodiac Tarot spread ..."(third-person). - Open
plugins/let-fate-decide/skills/let-fate-decide/SKILL.mdline 3 →description: "Draws the 12 Houses of the Zodiac Tarot spread ..."(third-person). - Open
CLAUDE.mdline 133 → explicit third-person mandate. - Conclude:
agents/draw.mdline 3 is the lone imperative outlier across the four parallel surfaces and against the repo-wide convention.
How to fix. One-character change on plugins/let-fate-decide/agents/draw.md line 3: change Draw the 12 Houses to Draws the 12 Houses. Aligns all four trigger surfaces and matches the third-person convention used by every other agent in the repo.
| "entropy_note": ( | ||
| "Assumes secrets.randbelow() provides cryptographically secure " | ||
| "bounded draws. This is a conservative unordered-card budget: " | ||
| f"{bits['major_arcana']} bits from Major Arcana selection, " | ||
| f"{bits['minor_arcana']} bits from Minor Arcana selection, and " | ||
| f"{bits['reversals']} reversal bits. Ordered house assignment " | ||
| "contains more entropy." | ||
| ), |
There was a problem hiding this comment.
🟡 The entropy_note string in draw_zodiac_spread() says "Assumes secrets.randbelow() provides cryptographically secure bounded draws" as a sweeping caveat before enumerating Major Arcana selection + Minor Arcana selection + 36 reversal bits — but the 36 reversal bits come from is_reversed() at line 165, which uses secrets.randbits(1), not randbelow. Both are CSPRNG-backed in Python's secrets module so the budget is correct, but the note credits one primitive and silently leaves the other unstated. Suggest rewording to "Assumes secrets.randbelow() and secrets.randbits() provide cryptographically secure draws", and updating the matching caveat sentences in SKILL.md and README.md (both attach the randbelow-only caveat to a budget that includes the 36 reversal bits).
Extended reasoning...
What the issue is. The entropy_note literal built by draw_zodiac_spread() at plugins/let-fate-decide/skills/let-fate-decide/scripts/draw_cards.py:270-277 opens with "Assumes secrets.randbelow() provides cryptographically secure bounded draws" and then enumerates the three budget components, including {bits['reversals']} reversal bits. But the reversal bits do not come from secrets.randbelow() — they come from is_reversed() at draw_cards.py:165 (secrets.randbits(1) == 1). The note credits one CSPRNG primitive while the budget it justifies depends on two.
The specific code path. draw_zodiac_spread() calls fisher_yates_shuffle() on the Major and Minor decks, which uses secrets.randbelow(). It also calls card_record() which calls is_reversed() which uses secrets.randbits(1). The 39.28 bits of Major Arcana selection and 51.95 bits of Minor Arcana selection rest on randbelow being uniform; the 36 reversal bits rest on randbits being uniform. Both are guaranteed by Python's secrets module — test_uses_secrets_module at test_draw_cards.py:316-321 even asserts that *both* are imported — but the prose attached to the budget mentions only randbelow.
Why this is a documentation drift, not a security defect. The refuting verifier is correct on the cryptographic substance: randbelow and randbits share the same underlying os.urandom source, and extracting 1 unbiased bit from a CSPRNG has no subtle bounded-draw assumption. The 100-bit floor still holds. The concern is narrower: this is the same shape of doc/code drift the PR already accepted fixes for — minor_shuffle → minor_arcana, "a 12 Houses" → "the 12 Houses", "focus" vs "Domain", "6th percentile" vs "top 3%". The README's parallel sentence (line 38) is actually more careful: it attaches the randbelow caveat only to the two selection components and describes the 36 reversal bits separately. The entropy_note string and SKILL.md lines 83-84 both pull the caveat out as a sweeping precondition over the whole budget.
Step-by-step proof.
- Read
scripts/draw_cards.py:270-277—entropy_notesays "Assumes secrets.randbelow() provides cryptographically secure bounded draws" then enumerates{bits['reversals']} reversal bits. - Read
scripts/draw_cards.py:163-165—is_reversed()issecrets.randbits(1) == 1. The reversal bits do not pass throughrandbelow. - Read
scripts/draw_cards.py:255-256—card_record()callsis_reversed()for every card, so the 36 reversal bits in the entropy budget come fromrandbits. - Read
SKILL.mdlines 83-84 — same single-function caveat applied to the same three-component budget. - Read
README.mdline 38 — caveat is attached *only* to selection bits; reversal bits are described separately. So the README is internally consistent and the entropy_note and SKILL.md are the outliers.
Impact. Documentation accuracy only — no functional or security regression. entropy_note is informational JSON read by the interpreting agent; a reader auditing the budget will see the note assert one CSPRNG primitive while the code uses two. SKILL.md is read by Claude when deciding whether/how to use the skill. Same as prior accepted nits, the cost is the next reader reconciling a small mismatch.
How to fix. Three one-line edits:
scripts/draw_cards.pyentropy_note: "Assumes secrets.randbelow() and secrets.randbits() provide cryptographically secure draws."SKILL.mdlines 83-84: same rewording (or split into selection-vs-reversal sentences like the README).README.mdline 38: optional — current wording is already the most careful of the three, but could be aligned for symmetry.
Severity: nit. Same shape of documentation drift this PR has already accepted fixes for; no behavior change.
dguido
left a comment
dguido
left a comment
There was a problem hiding this comment.
Reviewed and addressed the remaining nits (reversal percentile correction, focus/Domain alignment with regression test, Step 1 heading). All checks green.
2 participants
Summary
Expands
let-fate-decidefrom the previous 4-card draw into a default 12 Houses of the Zodiac Tarot spread.Each house now receives:
The previous 4-card behavior is still available via the explicit legacy path:
uv run --no-config {baseDir}/scripts/draw_cards.py --legacyPassing a count also keeps legacy behavior:
uv run --no-config {baseDir}/scripts/draw_cards.py 4Changes
houses/reference files describing each zodiac house across:references/TECHNICAL_CONTEXT_LENSES.mdfor translating readings into audit, evidence, domain, failure-class, and stakeholder lenses.SKILL.md, README, interpretation guide, and draw agent instructions.--legacy.let-fate-decideto1.2.0.Entropy
The default spread reports a conservative unordered-card entropy budget of ** 271.71 bits**:
log2(20! / 10!) = 39.28log2(56! / 20!) = 187.57536The actual ordered assignment of cards to houses contains more entropy.
Validation
uv run --no-config plugins/let-fate-decide/skills/let-fate-decide/scripts/test_draw_cards.pyuv run --with ruff --no-project ruff check plugins/let-fate-decide/skills/let-fate-decide/scriptspython3 .github/scripts/validate_codex_skills.pygit diff --check -- plugins/let-fate-decideAlso reviewed with Claude for content quality and accidental disclosure of internal workflow details; no blocking findings remained.
(This was authored by Codex.)