Address PR #133 review feedback

tob-scott-a · claude · tob-scott-a · commit 728614769e5d · 2026-03-23T15:38:12.000-04:00
- Add Rationalizations (Do Not Skip) sections to 5 security skills:
  trailmark, audit-augmentation, crypto-protocol-diagram,
  mermaid-to-proverif, graph-evolution
- Fix requires-python: diagram.py &gt;= 3.12 (was 3.13), protocol.py
  &gt;= 3.12 (was 3.11) to match trailmark's actual requirement
- Rename diagram/ to diagramming-code/ to match SKILL.md frontmatter
  name and all cross-skill references; update Codex symlink

Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/.codex/skills/diagram b/.codex/skills/diagram
diff --git a/.codex/skills/diagramming-code b/.codex/skills/diagramming-code
@@ -0,0 +1 @@
+../../plugins/trailmark/skills/diagramming-code
diff --git a/plugins/trailmark/skills/audit-augmentation/SKILL.md b/plugins/trailmark/skills/audit-augmentation/SKILL.md
@@ -29,6 +29,19 @@ onto Trailmark code graphs as annotations and subgraphs.
 - Building the code graph itself (use the `trailmark` skill)
 - Generating diagrams (use the `diagramming-code` skill after augmenting)
 
+## Rationalizations (Do Not Skip)
+
+| Rationalization | Why It's Wrong | Required Action |
+|-----------------|----------------|-----------------|
+| "The user only asked about SARIF, skip pre-analysis" | Without pre-analysis, you can't cross-reference findings with blast radius or taint | Always run `engine.preanalysis()` before augmenting |
+| "Unmatched findings don't matter" | Unmatched findings may indicate parsing gaps or out-of-scope files | Report unmatched count and investigate if high |
+| "One severity subgraph is enough" | Different severities need different triage workflows | Query all severity subgraphs, not just `error` |
+| "SARIF results speak for themselves" | Findings without graph context lack blast radius and taint reachability | Cross-reference with pre-analysis subgraphs |
+| "weAudit and SARIF overlap, pick one" | Human auditors and tools find different things | Import both when available |
+| "Tool isn't installed, I'll do it manually" | Manual analysis misses what tooling catches | Install trailmark first |
+
+---
+
 ## Installation
 
 **MANDATORY:** If `uv run trailmark` fails, install trailmark first:
diff --git a/plugins/trailmark/skills/crypto-protocol-diagram/SKILL.md b/plugins/trailmark/skills/crypto-protocol-diagram/SKILL.md
@@ -34,6 +34,17 @@ For call graphs, class hierarchies, or module dependency maps, use the
 - User wants to formally verify a protocol — use `mermaid-to-proverif` (after generating the diagram)
 - Input has no cryptographic protocol semantics (no parties, no message exchange)
 
+## Rationalizations (Do Not Skip)
+
+| Rationalization | Why It's Wrong | Required Action |
+|-----------------|----------------|-----------------|
+| "The protocol is simple, I can diagram from memory" | Memory-based diagrams miss steps and invert arrows | Read the source or spec systematically |
+| "I'll skip the spec path since code exists" | Code may diverge from the spec — both paths catch different bugs | When both exist, run spec workflow first, then annotate code divergences |
+| "Crypto annotations are optional decoration" | Without crypto annotations, the diagram is just a message flow — useless for security review | Annotate every cryptographic operation |
+| "The abort path is obvious, no need for alt blocks" | Implicit abort handling hides missing error checks | Show every abort/error path with `alt` blocks |
+| "I don't need to check the examples first" | The examples define the expected output quality bar | Study the relevant example before working on unfamiliar input |
+| "ProVerif/Tamarin models are code, not specs" | Formal models are specifications — they describe intended behavior, not implementation | Use the spec workflow (S1–S5) for `.pv` and `.spthy` files |
+
 ---
 
 ## Workflow
diff --git a/plugins/trailmark/skills/crypto-protocol-diagram/examples/simple-handshake/protocol.py b/plugins/trailmark/skills/crypto-protocol-diagram/examples/simple-handshake/protocol.py
@@ -1,5 +1,5 @@
 # /// script
-# requires-python = ">=3.11"
+# requires-python = ">=3.12"
 # dependencies = ["cryptography>=42.0"]
 # ///
 """
diff --git a/plugins/trailmark/skills/diagramming-code/SKILL.md b/plugins/trailmark/skills/diagramming-code/SKILL.md
diff --git a/plugins/trailmark/skills/diagramming-code/references/diagram-types.md b/plugins/trailmark/skills/diagramming-code/references/diagram-types.md
diff --git a/plugins/trailmark/skills/diagramming-code/references/mermaid-syntax.md b/plugins/trailmark/skills/diagramming-code/references/mermaid-syntax.md
diff --git a/plugins/trailmark/skills/diagramming-code/scripts/diagram.py b/plugins/trailmark/skills/diagramming-code/scripts/diagram.py
@@ -1,5 +1,5 @@
 # /// script
-# requires-python = ">=3.13"
+# requires-python = ">=3.12"
 # dependencies = ["trailmark"]
 # ///
 """Generate Mermaid diagrams from Trailmark code graphs.
diff --git a/plugins/trailmark/skills/graph-evolution/SKILL.md b/plugins/trailmark/skills/graph-evolution/SKILL.md
@@ -33,6 +33,19 @@ taint propagation changes, and privilege boundary modifications.
 - Diagram generation from a single snapshot (use the `diagramming-code` skill)
 - Mutation testing triage (use the `genotoxic` skill)
 
+## Rationalizations (Do Not Skip)
+
+| Rationalization | Why It's Wrong | Required Action |
+|-----------------|----------------|-----------------|
+| "We just need the structural diff, skip pre-analysis" | Without pre-analysis, you miss taint changes, blast radius growth, and privilege boundary shifts | Run `engine.preanalysis()` on both snapshots |
+| "Text diff covers what changed" | Text diffs miss new attack paths, transitive complexity shifts, and subgraph membership changes | Use structural diff to complement text diff |
+| "Only added nodes matter" | Removed security functions and shifted privilege boundaries are equally dangerous | Review removals and modifications, not just additions |
+| "Low-severity structural changes can be ignored" | INFO-level changes (dead code removal) can mask removed security checks | Classify every change, review removals for replaced functionality |
+| "One snapshot's graph is enough for comparison" | Single-snapshot analysis can't detect evolution — you need both before and after | Always build and export both graphs |
+| "Tool isn't installed, I'll compare manually" | Manual comparison misses what graph analysis catches | Install trailmark first |
+
+---
+
 ## Prerequisites
 
 **trailmark** must be installed. If `uv run trailmark` fails, run:
diff --git a/plugins/trailmark/skills/mermaid-to-proverif/SKILL.md b/plugins/trailmark/skills/mermaid-to-proverif/SKILL.md
@@ -28,6 +28,17 @@ Mermaid `sequenceDiagram` annotated with cryptographic operations (`Sign`,
 - User wants to verify properties of non-cryptographic systems (state machines, access control)
 - User wants to run ProVerif on an existing .pv file — just run `proverif model.pv` directly
 
+## Rationalizations (Do Not Skip)
+
+| Rationalization | Why It's Wrong | Required Action |
+|-----------------|----------------|-----------------|
+| "Reachability queries are just busywork" | If events aren't reachable, all other query results are meaningless | Always add reachability queries first as a sanity check |
+| "Public channels are fine for all messages" | Private channels for internal state prevent false attacks | Use private channels for intra-process state threading |
+| "I'll skip the forward secrecy test" | Ephemeral keys demand forward secrecy verification | Add the ForwardSecrecyTest process whenever the diagram shows ephemeral keys |
+| "Unused declarations are harmless" | ProVerif may report spurious results from orphan declarations | Clean up all unused types, functions, and events |
+| "The model compiles, so it's correct" | A compiling model can have dead receives, type mismatches, or impossible guards that make queries vacuously true | Validate reachability before trusting any security query |
+| "I don't need to check the example first" | The example defines the expected output quality bar | Study `examples/simple-handshake/` before working on unfamiliar protocols |
+
 ---
 
 ## Workflow
diff --git a/plugins/trailmark/skills/trailmark/SKILL.md b/plugins/trailmark/skills/trailmark/SKILL.md
@@ -25,6 +25,19 @@ semantic metadata for security analysis. Supports 16 languages.
 - Mutation testing triage (use the genotoxic skill, which calls trailmark internally)
 - Runtime behavior analysis (trailmark is static, not dynamic)
 
+## Rationalizations (Do Not Skip)
+
+| Rationalization | Why It's Wrong | Required Action |
+|-----------------|----------------|-----------------|
+| "I'll just read the source files manually" | Manual reading misses call paths, blast radius, and taint data | Install trailmark and use the API |
+| "Pre-analysis isn't needed for a quick query" | Blast radius, taint, and privilege data are only available after `preanalysis()` | Always run `engine.preanalysis()` before handing off to other skills |
+| "The graph is too large, I'll sample" | Sampling misses cross-module attack paths | Build the full graph; use subgraph queries to focus |
+| "Uncertain edges don't matter" | Dynamic dispatch is where type confusion bugs hide | Account for `uncertain` edges in security claims |
+| "Single-language analysis is enough" | Polyglot repos have FFI boundaries where bugs cluster | Use the correct `--language` flag per component |
+| "Complexity hotspots are the only thing worth checking" | Low-complexity functions on tainted paths are high-value targets | Combine complexity with taint and blast radius data |
+
+---
+
 ## Installation
 
 **MANDATORY:** If `uv run trailmark` fails (command not found, import error,

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+../../plugins/trailmark/skills/diagramming-code`