From be058c7596045fb58095ca77a0f34a118570880c Mon Sep 17 00:00:00 2001 From: Dan Guido Date: Thu, 11 Jun 2026 14:13:53 -0400 Subject: [PATCH] fix: resolve code review findings for PR #183 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Redraws the version boundary to match the actual Trailmark release history, verified against the v0.2.0/v0.2.1/v0.2.2/v0.3.1/v0.4.0 tags of trailofbits/trailmark: P1 (misclassified APIs, fixed): - ancestors_of(), reachable_from(), entrypoint_paths_to(), nodes_with_annotation(), clear_annotations() and the diff/entrypoints CLI exist since v0.2.0 — moved from the v0.4+ list to the v0.2-safe baseline; removed needless hasattr() gates and degraded fallbacks - supported_languages()/detect_languages() (trailmark.parse) are 0.3+ modules, not v0.2-safe — annotated as such - CLI --version/version were added in 0.2.2, not 0.4 — documented as 0.2.2+ and version-probe failure semantics clarified - graph-evolution/SKILL.md reverted to main: native diff has existed since v0.2.0 with identical args, so the 0.2.x-fallback rewrite was built on a false premise (also resolves the quality-checklist contradiction flagged in review) P3 (fixed): lexical version comparison hazard noted; stale diagramming-code checklist label; README baseline list now defers to the SKILL.md Version Gate instead of keeping a second divergent copy Dismissed: 'diff CLI signatures contradict' (both forms valid — before/ after positionals accept paths or git refs, --repo/--json exist since 0.2.0); 'subgraph_edges has no edge_kinds param' (v0.4.0 signature has edge_kinds keyword); 'diagram.py fallback broken on 0.2.x' (trailmark.diagram module exists in 0.2.x; only the CLI subcommand is new); 'phantom diff_against()' (real, v0.2-safe, now in baseline) Verified: check_claude_loadability.py, check_codex_loadability.py, pre-commit hooks pass; all SKILL.md files under 500 lines Co-Authored-By: Claude Fable 5 --- plugins/trailmark/README.md | 31 ++++---- .../skills/diagramming-code/SKILL.md | 2 +- .../trailmark/skills/graph-evolution/SKILL.md | 43 ++--------- plugins/trailmark/skills/trailmark/SKILL.md | 74 +++++++++++-------- .../trailmark/references/query-patterns.md | 58 +++++++-------- 5 files changed, 91 insertions(+), 117 deletions(-) diff --git a/plugins/trailmark/README.md b/plugins/trailmark/README.md index eb7ab64..3677e30 100644 --- a/plugins/trailmark/README.md +++ b/plugins/trailmark/README.md @@ -3,10 +3,10 @@ **Source code graph analysis for security auditing.** Parses code into queryable graphs of functions, classes, and calls, then uses that structure for diagram generation, mutation testing triage, protocol verification, and differential review. These skills support Trailmark 0.2.x through the 0.4.0 release line. Prefer -`--language auto`, `trailmark.parse.detect_languages()`, and -`QueryEngine.preanalysis()` for the v0.2-safe core workflow. Before using -features added for v0.4.0, check the installed Trailmark version or probe for -the method/CLI command first. +`--language auto`, `trailmark.parse.detect_languages()` (0.3+), and +`QueryEngine.preanalysis()` for the core workflow. Before using features added +for v0.4.0, check the installed Trailmark version or probe for the method/CLI +command first. ## Compatibility @@ -16,19 +16,18 @@ Use this guard before relying on v0.4-only features: trailmark --version 2>/dev/null || uv run trailmark --version 2>/dev/null ``` -If the command reports `trailmark 0.4.0` or newer, the expanded v0.4 feature -set is available. If the command is missing or reports an older version, stay -on the v0.2-safe surface: `analyze`, `--language auto`, `detect_languages()`, -`QueryEngine.from_directory()`, `summary()`, `complexity_hotspots()`, -`attack_surface()`, `preanalysis()`, annotations, and SARIF/weAudit -augmentation. +Compare the reported version numerically. If it is `0.4.0` or newer, the +expanded v0.4 feature set is available. If the command is missing or reports +an older version, stay on the v0.2-safe baseline — the `trailmark` skill's +Version Gate section has the authoritative list. (The version CLI itself was +added in 0.2.2, so a missing command can also mean trailmark is not installed +at all.) -v0.4.0 adds expanded parser coverage, explicit proxy nodes for unresolved calls, -node origins (`source`, `proxy`, `binary`, `synthetic`), new edge kinds -(`resolves_to`, `type_uses`, `specializes`, `corresponds_to`), transitive query -helpers, entrypoint path queries, subgraph edge queries, generic/type-reference -queries, CLI `version`/`entrypoints`/`diff`/`diagram`, and binary graph -augmentation via `augment_binary()`. +v0.4.0 adds expanded parser coverage, explicit proxy nodes for unresolved +calls, node origins (`source`, `proxy`, `binary`, `synthetic`), new edge kinds +(`resolves_to`, `type_uses`, `specializes`, `corresponds_to`), subgraph edge +and connection queries, generic/type-reference queries, the native +`trailmark diagram` CLI, and binary graph augmentation via `augment_binary()`. ## Prerequisites diff --git a/plugins/trailmark/skills/diagramming-code/SKILL.md b/plugins/trailmark/skills/diagramming-code/SKILL.md index bdb1b71..7d1158d 100644 --- a/plugins/trailmark/skills/diagramming-code/SKILL.md +++ b/plugins/trailmark/skills/diagramming-code/SKILL.md @@ -105,7 +105,7 @@ Diagram Progress: - [ ] Step 1: Verify trailmark is installed - [ ] Step 2: Identify diagram type from user request - [ ] Step 3: Determine focus node and parameters -- [ ] Step 4: Run diagram.py script +- [ ] Step 4: Run diagram.py script (or native trailmark diagram on v0.4+) - [ ] Step 5: Verify output is non-empty and well-formed - [ ] Step 6: Embed diagram in response ``` diff --git a/plugins/trailmark/skills/graph-evolution/SKILL.md b/plugins/trailmark/skills/graph-evolution/SKILL.md index d443c61..3a7c495 100644 --- a/plugins/trailmark/skills/graph-evolution/SKILL.md +++ b/plugins/trailmark/skills/graph-evolution/SKILL.md @@ -15,9 +15,7 @@ description: > Builds Trailmark code graphs at two source snapshots and computes a structural diff. Surfaces security-relevant changes that text-level diffs miss: new attack paths, complexity shifts, blast radius growth, -taint propagation changes, and privilege boundary modifications. Native -`trailmark diff` is a Trailmark 0.4.0+ feature; use the API/plugin fallback -when the installed build is older. +taint propagation changes, and privilege boundary modifications. ## When to Use @@ -45,7 +43,6 @@ when the installed build is older. | "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 | -| "Native diff is always available" | Trailmark 0.2.x installs may not have the `diff` CLI | Check `trailmark diff --help` before using it | --- @@ -61,17 +58,6 @@ uv pip install trailmark substitute for running trailmark. The tool must be installed and used programmatically. If installation fails, report the error. -Check for native v0.4 diff support before using `trailmark diff`: - -```bash -trailmark diff --help 2>/dev/null || uv run trailmark diff --help 2>/dev/null -``` - -If unavailable, skip the native diff command and rely on `engine.diff_against()` -when present plus `graph_diff.py` over exported JSON. If neither native diff nor -`diff_against()` exists, still compute the plugin's JSON diff from the two -exports and state that native entrypoint diff data was unavailable. - --- ## Quick Start @@ -98,7 +84,7 @@ exports and state that native entrypoint diff data was unavailable. │ └─ Read: references/report-format.md │ ├─ Already have two graph JSON exports? -│ └─ Jump to Phase 3 (run native diff if available + graph_diff.py) +│ └─ Jump to Phase 3 (run native diff + graph_diff.py) │ └─ Starting from two git refs? └─ Start at Phase 1 @@ -172,10 +158,7 @@ instead of `auto`. ### Phase 3: Compute Structural Diff -Run the native Trailmark diff only when the version gate above succeeds. Always -run the plugin's `graph_diff.py` helper for subgraph membership changes. - -When native diff is available, run **both**: +Run **both**: 1. Trailmark's native structural diff for nodes, edges, and entrypoints 2. The plugin's `graph_diff.py` helper for subgraph membership changes @@ -191,19 +174,8 @@ uv run {baseDir}/scripts/graph_diff.py \ --after "{after_json}" > "{work_dir}/subgraph_diff.json" ``` -If a diff command you chose to run fails or writes an empty JSON file, stop and -report the error instead of continuing to Phase 4. - -Fallback for Trailmark 0.2.x: - -```bash -uv run {baseDir}/scripts/graph_diff.py \ - --before "{before_json}" \ - --after "{after_json}" > "{work_dir}/subgraph_diff.json" -``` - -In the report, mark native `trailmark_diff.json` as unavailable and interpret -the `graph_diff.py` node/edge/subgraph output instead. +If either diff command fails or writes an empty JSON file, stop and report the +error instead of continuing to Phase 4. The native Trailmark diff contains: @@ -225,9 +197,8 @@ The subgraph diff contains: ### Phase 4: Interpret Diff and Generate Report -Read every diff JSON file produced in Phase 3 and generate a security-focused -markdown report. On Trailmark 0.2.x, this may be only `subgraph_diff.json`; -state that native entrypoint diff data was unavailable. +Read **both** diff JSON files and generate a security-focused markdown +report. See [references/report-format.md](references/report-format.md) for the full template. diff --git a/plugins/trailmark/skills/trailmark/SKILL.md b/plugins/trailmark/skills/trailmark/SKILL.md index fba6102..153aa11 100644 --- a/plugins/trailmark/skills/trailmark/SKILL.md +++ b/plugins/trailmark/skills/trailmark/SKILL.md @@ -64,31 +64,40 @@ feature listed as **v0.4+**, check the installed version: trailmark --version 2>/dev/null || uv run trailmark --version 2>/dev/null ``` -Accept `trailmark 0.4.0` or newer. If the version command is unavailable or -reports `< 0.4.0`, use only the v0.2-safe baseline below. When working -programmatically and version output is unavailable, probe with `hasattr()` and -fall back instead of assuming the method exists: +Compare the reported version numerically (not lexically). `0.4.0` or newer +means the full v0.4 surface is available. The version command itself was added +in 0.2.2, so a failure means either a pre-0.2.2 install or trailmark missing +entirely — distinguish with `trailmark analyze --help`. When working +programmatically, probe with `hasattr()` and fall back instead of assuming a +v0.4-only method exists: ```python -if hasattr(engine, "entrypoint_paths_to"): - paths = engine.entrypoint_paths_to("sensitive_sink") +if hasattr(engine, "subgraph_edges"): + edges = engine.subgraph_edges("tainted") else: - paths = [] # v0.2 fallback: use attack_surface() + paths_between() + # v0.2 fallback: filter engine.to_json() edges whose endpoints + # are both in engine.subgraph("tainted") + edges = [] ``` -**v0.2-safe baseline:** `analyze`, `--language auto`, -`detect_languages()`, `supported_languages()`, `QueryEngine.from_directory()`, -`callers_of()`, `callees_of()`, `paths_between()`, `complexity_hotspots()`, -`attack_surface()`, `summary()`, `to_json()`, `preanalysis()`, -`annotate()`, `annotations_of()`, `findings()`, `subgraph()`, -`subgraph_names()`, `augment_sarif()`, and `augment_weaudit()`. +**v0.2-safe baseline:** CLI `analyze`, `diff`, `entrypoints`, `augment`, and +`--language auto`; `QueryEngine.from_directory()`, `callers_of()`, +`callees_of()`, `paths_between()`, `ancestors_of()`, `reachable_from()`, +`entrypoint_paths_to()`, `complexity_hotspots()`, `attack_surface()`, +`summary()`, `to_json()`, `preanalysis()`, `annotate()`, `annotations_of()`, +`nodes_with_annotation()`, `clear_annotations()`, `findings()`, `subgraph()`, +`subgraph_names()`, `diff_against()`, `augment_sarif()`, and +`augment_weaudit()`. -**v0.4+ features:** CLI `--version`/`version`, native `entrypoints`, -`diff`, and `diagram` subcommands; expanded parser coverage; proxy nodes for -unresolved calls; node origins; binary graph augmentation; transitive -`ancestors_of()` / `reachable_from()`; `connect_subgraphs()`; -`entrypoint_paths_to()`; `nodes_with_annotation()`; `clear_annotations()`; -`subgraph_edges()`; `generic_parameters()`; and `type_references()`. +**Added in 0.2.2:** CLI `--version` flag and `version` subcommand. + +**Added in 0.3.x:** the `trailmark.parse` module with module-level +`detect_languages()` and `supported_languages()`. + +**v0.4+ features:** native `diagram` subcommand; expanded parser coverage; +proxy nodes for unresolved calls; node origins; binary graph augmentation via +`augment_binary()`; `connect_subgraphs()`; `subgraph_edges()`; +`generic_parameters()`; and `type_references()`. ## Quick Start @@ -103,16 +112,21 @@ uv run trailmark analyze --language python,rust {targetDir} # Complexity hotspots uv run trailmark analyze --language auto --complexity 10 {targetDir} -# v0.4+: version, entrypoint, diff, and native diagram commands -uv run trailmark --version +# Entrypoint inventory and structural diff (v0.2-safe) uv run trailmark entrypoints --language auto {targetDir} uv run trailmark diff --repo {repoDir} main HEAD --json + +# Version report (0.2.2+) +uv run trailmark --version + +# v0.4+: native diagram command uv run trailmark diagram -t {targetDir} -T call-graph -f main --depth 2 ``` ### Programmatic API ```python +# trailmark.parse is a 0.3+ module; on 0.2.x pass language="auto" instead from trailmark.parse import detect_languages, supported_languages from trailmark.query.api import QueryEngine @@ -132,12 +146,12 @@ engine.attack_surface() engine.summary() engine.to_json() -# v0.4+: transitive slices and entrypoint path queries -if hasattr(engine, "ancestors_of"): - engine.ancestors_of("sensitive_sink") - engine.reachable_from("entry_func") -if hasattr(engine, "entrypoint_paths_to"): - engine.entrypoint_paths_to("sensitive_sink") +# Transitive slices and entrypoint path queries (v0.2-safe) +engine.ancestors_of("sensitive_sink") +engine.reachable_from("entry_func") +engine.entrypoint_paths_to("sensitive_sink") + +# v0.4+: connect named subgraphs if hasattr(engine, "connect_subgraphs"): engine.connect_subgraphs("tainted", "privilege_boundary") @@ -166,10 +180,8 @@ engine.annotations_of("function_name", kind=AnnotationKind.BLAST_RADIUS) engine.annotations_of("function_name", kind=AnnotationKind.TAINT_PROPAGATION) -if hasattr(engine, "nodes_with_annotation"): - engine.nodes_with_annotation(AnnotationKind.FINDING) -if hasattr(engine, "clear_annotations"): - engine.clear_annotations("function_name", kind=AnnotationKind.ASSUMPTION) +engine.nodes_with_annotation(AnnotationKind.FINDING) +engine.clear_annotations("function_name", kind=AnnotationKind.ASSUMPTION) # v0.4+: generic/type-reference and binary augmentation APIs if hasattr(engine, "generic_parameters"): diff --git a/plugins/trailmark/skills/trailmark/references/query-patterns.md b/plugins/trailmark/skills/trailmark/references/query-patterns.md index e493574..2884eca 100644 --- a/plugins/trailmark/skills/trailmark/references/query-patterns.md +++ b/plugins/trailmark/skills/trailmark/references/query-patterns.md @@ -12,12 +12,17 @@ from trailmark.query.api import QueryEngine engine = QueryEngine.from_directory("{targetDir}", language="auto") -if hasattr(engine, "entrypoint_paths_to"): - paths = engine.entrypoint_paths_to("sensitive_sink") # v0.4+ +if hasattr(engine, "subgraph_edges"): + edges = engine.subgraph_edges("tainted") # v0.4+ else: - paths = [] - for ep in engine.attack_surface(): - paths.extend(engine.paths_between(ep["node_id"], "sensitive_sink")) + # v0.2 fallback: filter exported edges by subgraph membership + import json + graph = json.loads(engine.to_json()) + member_ids = {node["id"] for node in engine.subgraph("tainted")} + edges = [ + e for e in graph.get("edges", []) + if e["source"] in member_ids and e["target"] in member_ids + ] ``` ## 1. Mapping Attack Surface @@ -74,34 +79,20 @@ for caller in callers: Check if a function is reachable from any entrypoint: ```python -if hasattr(engine, "entrypoint_paths_to"): - paths = engine.entrypoint_paths_to("sensitive_function_id") -else: - paths = [] - for ep in engine.attack_surface(): - paths.extend(engine.paths_between(ep["node_id"], "sensitive_function_id")) - +paths = engine.entrypoint_paths_to("sensitive_function_id") if paths: print(f"Reachable via {len(paths)} path(s)") else: print("Not reachable from any entrypoint") ``` -For Trailmark 0.4.0+, prefer `entrypoint_paths_to()` because it searches all -detected entrypoints directly. On older versions, use `attack_surface()` plus -`paths_between()` as shown in the version-gated example above. - ## 6. Transitive Slices -Trailmark 0.4.0+ exposes upward and downward transitive slices: +Upward and downward transitive slices (v0.2-safe): ```python -if hasattr(engine, "ancestors_of"): - callers_to_sink = engine.ancestors_of("execute_query") - downstream = engine.reachable_from("handle_request") -else: - callers_to_sink = engine.callers_of("execute_query") - downstream = engine.callees_of("handle_request") +callers_to_sink = engine.ancestors_of("execute_query") +downstream = engine.reachable_from("handle_request") ``` Use `ancestors_of()` for "who could eventually reach this sink?" and @@ -164,6 +155,7 @@ Ask Trailmark which languages it supports, detect what exists under the target tree, then choose `auto` or an explicit list: ```python +# trailmark.parse is a 0.3+ module; on 0.2.x pass language="auto" instead from trailmark.parse import detect_languages, supported_languages from trailmark.query.api import QueryEngine @@ -178,13 +170,13 @@ As of Trailmark 0.4.0, supported parser names include `python`, `javascript`, `typescript`, `php`, `ruby`, `c`, `cpp`, `c_sharp`, `java`, `go`, `rust`, `solidity`, `cairo`, `circom`, `haskell`, `erlang`, `masm`, `swift`, `objc`, `kotlin`, `dart`, `move`, `tact`, `func`, `sway`, `rego`, `proto`, `thrift`, -and `graphql`. Always call `supported_languages()` on the installed build -before relying on this list. +and `graphql`. Treat this list as documentation, not a source of truth; on +0.3+ builds call `supported_languages()` before relying on it. ## 11. CLI Patterns ```bash -# Version check before v0.4-only commands +# Version check before v0.4-only commands (version CLI itself is 0.2.2+) uv run trailmark --version # Quick summary with auto-detection @@ -197,8 +189,10 @@ uv run trailmark analyze --language python,rust --complexity 8 {targetDir} # Entrypoint inventory uv run trailmark entrypoints --language auto {targetDir} -# v0.4+: native diff and native diagram +# Structural diff between two refs or directories uv run trailmark diff --repo {repoDir} main HEAD --json + +# v0.4+: native diagram uv run trailmark diagram -t {targetDir} -T call-graph -f main --depth 2 # Full JSON output for piping to other tools @@ -225,13 +219,11 @@ for ann in engine.annotations_of("handle_request"): assumptions = engine.annotations_of("handle_request", kind=AnnotationKind.ASSUMPTION) # Clear annotations (all, or by kind) -if hasattr(engine, "clear_annotations"): - engine.clear_annotations("handle_request", kind=AnnotationKind.ASSUMPTION) - engine.clear_annotations("handle_request") +engine.clear_annotations("handle_request", kind=AnnotationKind.ASSUMPTION) +engine.clear_annotations("handle_request") -# v0.4+: nodes with a given annotation -if hasattr(engine, "nodes_with_annotation"): - finding_nodes = engine.nodes_with_annotation(AnnotationKind.FINDING) +# Nodes with a given annotation +finding_nodes = engine.nodes_with_annotation(AnnotationKind.FINDING) ``` **Annotation kinds:** `ASSUMPTION`, `PRECONDITION`, `POSTCONDITION`, `INVARIANT`.