trailmark: update skills to use v0.4.0 and v0.5.0 features - #183
trailmark: update skills to use v0.4.0 and v0.5.0 features#183tob-scott-a wants to merge 8 commits into
Conversation
|
@claude review once |
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 <noreply@anthropic.com>
Review SummaryReview ran four passes (pr-review-toolkit code-reviewer + comment-analyzer, Codex The core issue: the PR drew the v0.2/v0.4 boundary in the wrong place. Several APIs labeled v0.4+ have existed since v0.2.0, and one API labeled v0.2-safe doesn't exist until 0.3.x. Runtime behavior was mostly protected by the Findings
Claims verified as correct and kept: proxy node format Verification
Commitbe058c7 |
|
Re-verified after be058c7. The review feedback's version-boundary fixes are present on |
Interesting failure mode with the newlines there, Codex. |
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
* trailmark: add assurance workflow skills * fix: drop hasattr gates on v0.2-baseline query APIs entrypoint_paths_to() and reachable_from() are in the v0.2-safe baseline, so the hasattr fallbacks were dead code — and the entrypoint_paths_to fallback indexed attack_surface() entries with entry["name"] instead of node_id, which would raise KeyError. Addresses PR #187 review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… skills The trailmark-summary and trailmark-structural skills claimed v0.2-safe workflows but unconditionally imported trailmark.parse, a 0.3+ module. detect_languages() has existed in trailmark.query.api since v0.2.0 (kept as a deprecated alias in 0.3+), so gate the import with a fallback instead of relabeling the skills as 0.3+. Version Gate docs updated to document the v0.2-safe import path; supported_languages() remains 0.3+ with no 0.2.x equivalent. Verified against trailmark v0.2.0 source (fallback branch) and the current 0.4 line (canonical branch). Addresses review feedback on PR #183. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Trailmark 0.5.0 adds a PostgreSQL-oriented sql parser (node kinds
schema/table/view/procedure), the stable .trailmark/links.toml
configuration for cross-language/FFI/RPC/external links, repository
links/proxies/type_uses edges on single-language parses, Solidity
entrypoints from parser metadata (visibility/mutability/overridden-by
attributes), node attributes in attack_surface() entries, TypeScript
constructed-receiver resolution, and C# file-scoped namespaces.
Updates the Version Gate with a v0.5+ section and a structural probe
('SCHEMA' in NodeKind.__members__ — 0.5.0 adds no new QueryEngine
methods, so hasattr() cannot detect it), adds a Repository Links
section and cross-boundary query recipe, extends the parser list and
graph model docs, notes the reachability-vs-taint limitation, and
threads the 0.5 output additions through trailmark-structural and
audit-augmentation. Plugin version 0.9.0 -> 0.10.0.
All version claims verified against trailofbits/trailmark v0.2.0 and
v0.5.0 builds, including a live links.toml materialization test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tob-scott-a
changed the title
|
@claude review once |
There was a problem hiding this comment.
LGTM. My previous finding (the trailmark.parse ModuleNotFoundError contradiction in trailmark-summary/trailmark-structural) is now fixed with a try/except ModuleNotFoundError fallback to trailmark.query.api. The one issue found this round is a documentation nit (dict-vs-string-id mixup in two new reference files) that doesn't block, since it's non-executable LLM-guidance content likely to self-correct.
Extended reasoning...
Overview
This round re-reviews a large, mostly-settled documentation PR that updates the trailmark plugin's skills to version-gate v0.4.0/v0.5.0 features and adds four new skills (trailmark-finding-triage, trailmark-review-gate, trailmark-variant-neighborhood, plus updates to trailmark-summary/trailmark-structural). No scripts or executable code paths change; all edits are to SKILL.md/reference Markdown files that guide an LLM agent's behavior.
Security risks
None directly — this is prompt/documentation content, not code with a runtime attack surface. The prior substantive risk (agents following incorrect version-gating guidance and either crashing or skipping available security-analysis features) was the subject of the extensive earlier review rounds (dguido's 4-pass review, and my own 2026-06-29 finding) and has been resolved in the current diff: trailmark-summary/SKILL.md and trailmark-structural/SKILL.md now wrap the trailmark.parse import in try/except ModuleNotFoundError, falling back to trailmark.query.api, so the documented workflow no longer contradicts the v0.2-safe claim on 0.2.x installs.
Level of scrutiny
Low-to-moderate. This is guidance text for an LLM agent to interpret, not unattended production code — incorrect snippets tend to be self-correcting (a or at runtime prompts the agent to adjust), which lowers the blast radius of documentation bugs relative to shipped executable code. The one finding surfaced this round is explicitly a Nit (dict vs. string-id confusion in two new reference files) and doesn't rise to the level of the previously-fixed P1 contradiction.
Other factors
The PR has already been through multiple thorough review passes (human + multiple bots) with all P1/P2 findings resolved or dismissed with justification, and the version-gating boundaries have been cross-checked against actual trailmark release tags. Nothing has changed since my last review except the fix to the ModuleNotFoundError issue I flagged, which is now confirmed present in the diff.
| ## Taint And Privilege Boundaries | ||
|
|
||
| ```python | ||
| tainted = node_id in set(engine.subgraph("tainted")) | ||
| boundary = node_id in set(engine.subgraph("privilege_boundary")) | ||
| entry_reachable = node_id in set(engine.subgraph("entrypoint_reachable")) | ||
| ``` | ||
|
|
||
| When `connect_subgraphs()` exists, use it to find paths from tainted nodes to | ||
| privilege-boundary nodes and check whether the candidate sits on or near those | ||
| paths. | ||
|
|
||
| ## Blast Radius And Neighborhood | ||
|
|
||
| ```python | ||
| callers = engine.callers_of(node_id) | ||
| callees = engine.callees_of(node_id) | ||
| high_blast = node_id in set(engine.subgraph("high_blast_radius")) | ||
|
|
||
| downstream = engine.reachable_from(node_id) | ||
| ``` |
There was a problem hiding this comment.
🟡 Two new reference files in this PR pass dict node objects where the Trailmark API expects a string node ID (or vice versa), so the documented snippets crash if executed literally. In query-recipes.md, node_id in set(engine.subgraph(...)) wraps a list of node dicts in set() (dicts are unhashable → TypeError) on the taint/privilege_boundary/entrypoint_reachable/high_blast_radius lines; the correct form, {node["id"] for node in engine.subgraph(name)}, is already used two files over in this same PR's query-patterns.md. In neighborhood-patterns.md, engine.callers_of(seed)/callees_of(seed) return node dicts, but the sketch then feeds those dicts straight into engine.callees_of(caller)/callers_of(callee)/is_sensitive_sink(callee), which expect a string id (caller["id"]/callee["id"]).
Extended reasoning...
What the bug is. Both new reference files misuse the Trailmark QueryEngine API's dict-vs-string-id convention that the rest of this same PR establishes correctly.
In query-recipes.md (Taint And Privilege Boundaries / Blast Radius And Neighborhood sections):
tainted = node_id in set(engine.subgraph("tainted"))
boundary = node_id in set(engine.subgraph("privilege_boundary"))
entry_reachable = node_id in set(engine.subgraph("entrypoint_reachable"))
...
high_blast = node_id in set(engine.subgraph("high_blast_radius"))engine.subgraph(name) returns a list of node dict objects, not id strings — this is consistent everywhere else in the plugin: trailmark/SKILL.md iterates for node in high: print(node['id']...), preanalysis-passes.md does for node in tainted: engine.annotations_of(node['id'], ...), trailmark-structural/SKILL.md's summarize_subgraph() does [node['id'] for node in nodes[:limit]], and — most tellingly — this very PR's own query-patterns.md builds the membership set correctly with member_ids = {node['id'] for node in engine.subgraph('tainted')}. Because Python dicts are unhashable, set([{...}, {...}]) raises TypeError: unhashable type: 'dict' immediately, so the snippet cannot execute at all as written.
In neighborhood-patterns.md's "Query Sketches" block:
callers = engine.callers_of(seed)
callees = engine.callees_of(seed)
for caller in callers:
same_caller_candidates.extend(engine.callees_of(caller))
for callee in callees:
if is_sensitive_sink(callee):
same_sink_candidates.extend(engine.callers_of(callee))callers_of()/callees_of() return lists of node dicts (per query-patterns.md: callers = engine.callers_of("execute_query"); for caller in callers: caller['id']...) but are always called with a string node id (engine.callees_of("function_name"), engine.callers_of("execute_query")). Passing caller/callee (a dict) into engine.callees_of()/engine.callers_of() violates that convention and will fail (TypeError or a lookup miss) instead of expanding the neighborhood; is_sensitive_sink(callee) gets a dict instead of the id/name it presumably expects.
Why existing code doesn't prevent it. This is documentation/reference content for an LLM-driven skill, not unit-tested executable code, so nothing catches the type mismatch before an agent runs it. The PR's own query-patterns.md, added in this same diff, shows the correct idiom right next to the broken one, which is what makes the inconsistency clear rather than a matter of interpretation.
Step-by-step proof (query-recipes.md).
- An agent using
trailmark-finding-triagereaches Step 4, which explicitly instructs it to "Run the query recipe" in this file. - It builds the graph, runs
engine.preanalysis(), and executes the Taint And Privilege Boundaries block verbatim. engine.subgraph("tainted")returns e.g.[{"id": "foo", ...}, {"id": "bar", ...}].set([{"id": "foo", ...}, {"id": "bar", ...}])raisesTypeError: unhashable type: 'dict'before the membership check ever runs.- The same failure recurs on the
privilege_boundary,entrypoint_reachable, andhigh_blast_radiuslines. - The correct fix, already present two files away:
node_id in {n["id"] for n in engine.subgraph("tainted")}.
Impact. Low-to-moderate: these are reference snippets an LLM agent interprets rather than shipped, unattended executable code, so an agent would likely notice the TypeError/lookup miss and self-correct by indexing ["id"]. But it is a genuine, unambiguous defect in brand-new documentation that contradicts the corrected pattern the same PR ships two files over, and it teaches the wrong API-usage pattern to anyone who copies it verbatim.
Fix. Build the id set/lookup explicitly wherever a dict list meets a membership test or another API call expecting a string id:
tainted = node_id in {n["id"] for n in engine.subgraph("tainted")}
...
same_caller_candidates.extend(engine.callees_of(caller["id"]))
...
if is_sensitive_sink(callee["id"]):
same_sink_candidates.extend(engine.callers_of(callee["id"]))2 participants
Summary
Updates the
trailmarkplugin skills to document and use Trailmark v0.4.0 features while remaining backward-compatible with v0.2.x installs. Bumps the plugin version 0.8.2 → 0.8.3.This is a documentation-only change to the skill content; no scripts or executable behavior change.
What changed
trailmark --version) or runtimehasattr()probe, with a documented v0.2-safe fallback. The skills never assume v0.4 APIs exist.trailmark: Adds a "Version Gate" section listing the v0.2-safe baseline vs. v0.4+ features (proxy nodes, node origins, transitive slices,entrypoint_paths_to(),subgraph_edges(),type_references(), binary augmentation, nativeversion/entrypoints/diff/diagramCLI). Expanded graph model docs (proxy nodes, new origins and edge kinds, expanded parser list).audit-augmentation: Documentsengine.augment_binary()for importing external binary-analysis graph exports (v0.4+), behind a version gate, with the expected JSON shape.diagramming-code: Notes the nativetrailmark diagramcommand (v0.4+) with a version check, falling back to the bundled script otherwise.graph-evolution: Gates nativetrailmark diffbehind a version check; falls back tograph_diff.pyover JSON exports on v0.2.x.trailmark-structural/trailmark-summary: Optional version recording andhasattr()-probed v0.4 fields (proxy counts, subgraph edges, type/reference summaries).marketplace.json,plugin.json, and README to reflect the expanded (binary + source) capabilities.Verification
uv run python3 .github/scripts/check_claude_loadability.py✅uv run python3 .github/scripts/check_codex_loadability.py✅plugins/trailmark/.claude-plugin/plugin.jsonand the rootmarketplace.json(both 0.8.3).🤖 Generated with Claude Code