Skip to content

feat(surfpool,skill): auto-discover preset IDLs and fix solana-add-idl template gaps#289

Draft
shahan-khatchadourian-anchorage wants to merge 3 commits into
shahankhatch/surfpool-rust-runnerfrom
shahankhatch/surfpool-helpers-for-skill
Draft

feat(surfpool,skill): auto-discover preset IDLs and fix solana-add-idl template gaps#289
shahan-khatchadourian-anchorage wants to merge 3 commits into
shahankhatch/surfpool-rust-runnerfrom
shahankhatch/surfpool-helpers-for-skill

Conversation

@shahan-khatchadourian-anchorage
Copy link
Copy Markdown
Contributor

Summary

Wires the surfpool harness to cover skill-generated presets via reflection, and fixes three correctness gaps in the solana-add-idl skill that would have produced code that does not compile.

Stacked on top of #285 (shahankhatch/surfpool-rust-runner).

Reflection — surfpool now auto-discovers preset IDLs

  • build.rs scans src/presets/<name>/<name>.json and emits pub const PRESET_IDLS: &[(&str, &str)] to OUT_DIR; lib.rs re-exports it. Dropping a JSON file at the matching path makes it appear on the next build — no manual registration.
  • tests/surfpool_fuzz.rs::surfpool_preset_idls iterates PRESET_IDLS and runs each through the same roundtrip used for the named upstream-IDL tests. Currently empty (no preset ships an embedded IDL today); will pick up the first skill-generated preset.
  • run_idl_roundtrip + idl_test! macro hoisted from surfpool_fuzz.rs into tests/common/mod.rs so future per-preset test files can reuse them. #[macro_export] + $crate::common::run_idl_roundtrip path makes the macro work from any test that does mod common;.

Skill fixes — .claude/skills/solana-add-idl/SKILL.md

  • Removed the squads_multisig template reference — that preset does not exist in this repo. Replaced with unknown_program, which is the actual working IDL-parsing reference.
  • Removed references to five non-existent solana_parser helpers (build_named_accounts, build_parsed_fields, build_fallback_fields, append_raw_data, format_arg_value). Documented the manual named_accounts loop pattern that unknown_program uses.
  • Added explicit step for declaring pub(crate) const {SCREAMING_SNAKE}_PROGRAM_ID: &str in the preset's mod.rs — the generated config.rs references it via use super:: but the previous skill never told contributors where to put it.
  • Removed the idl_test! registration step (replaced by PRESET_IDLS auto-discovery) and added a verification command that greps the generated preset_idls.rs.
  • Documented what's already auto-covered (proptest is generative, cargo-fuzz is generative, surfpool roundtrip is reflective) vs. what still needs hand-written assertions (semantic_pipeline.rs).

Test plan

  • cargo fmt -p visualsign-solana
  • cargo clippy -p visualsign-solana --all-targets -- -D warnings
  • cargo test -p visualsign-solana — 111 unit/integration tests pass; 15 surfpool tests #[ignore] (the new surfpool_preset_idls discovers as the 15th)
  • cargo test -p visualsign-solana --test surfpool_fuzz -- --list lists surfpool_preset_idls alongside the existing 14 named upstream-IDL tests
  • OUT_DIR/preset_idls.rs is generated with pub const PRESET_IDLS: &[(&str, &str)] = &[ ]; (empty — no preset has an embedded IDL JSON yet, which is the expected initial state)
  • Surfpool label run on this PR — exercises surfpool_preset_idls (will be a no-op until a preset ships an IDL JSON, but confirms the test discovers cleanly)

🤖 Generated with Claude Code

…l template gaps

Wires the surfpool harness to cover skill-generated presets via
reflection, and fixes three correctness gaps in the solana-add-idl
skill that would have produced code that does not compile.

Reflection:
- build.rs scans `src/presets/<name>/<name>.json` and emits
  `pub const PRESET_IDLS: &[(&str, &str)]` to OUT_DIR; lib.rs
  re-exports it. Drop a JSON file at the matching path and it
  appears here on the next build with no manual registration.
- tests/surfpool_fuzz.rs::surfpool_preset_idls iterates PRESET_IDLS
  and runs each through the same roundtrip used for the named
  upstream-IDL tests. Currently empty (no preset ships an embedded
  IDL today); will pick up the first skill-generated preset.
- run_idl_roundtrip and the idl_test! macro hoisted from
  surfpool_fuzz.rs into tests/common/mod.rs so future per-preset
  test files can reuse them. #[macro_export] + $crate::common::...
  path makes the macro work for any test that does `mod common;`.

Skill template fixes (.claude/skills/solana-add-idl/SKILL.md):
- Removed the squads_multisig template reference -- that preset
  does not exist in this repo. Replaced with unknown_program,
  which is the actual working IDL-parsing reference.
- Removed references to five solana_parser helpers
  (build_named_accounts, build_parsed_fields, build_fallback_fields,
  append_raw_data, format_arg_value) that do not exist. Documented
  the manual named_accounts loop pattern that unknown_program uses.
- Added explicit step for declaring
  `pub(crate) const {SCREAMING_SNAKE}_PROGRAM_ID: &str` in the
  preset's mod.rs -- the generated config.rs references it via
  `use super::` but the previous skill never told contributors
  where to put it.
- Removed the obsolete `idl_test!` registration step (replaced by
  PRESET_IDLS auto-discovery) and added a verification command that
  greps the generated preset_idls.rs.
- Documented what's already auto-covered (proptest is generative,
  cargo-fuzz is generative, surfpool roundtrip is reflective) vs.
  what still needs hand-written assertions (semantic_pipeline.rs).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the last manual touchpoint outside the preset directory:
build.rs now scans `src/presets/<name>/` for `mod.rs` and emits one
`#[path = "..."] pub mod <name>;` per match into
`OUT_DIR/presets_mod.rs`. `presets/mod.rs` becomes a single
`include!`. Drop a preset directory in place and it wires up on the
next build with no edit.

The `#[path]` attribute is required because `include!` substitutes
the generated text at the OUT_DIR file's location, so the bare
`pub mod foo;` would otherwise resolve to `OUT_DIR/foo.rs`. Pinning
the absolute source path fixes resolution and keeps spans correct.
A `.join("mod.rs").exists()` guard skips any subdirectory that
isn't a real module.

SKILL.md scope refresh:
- Frontmatter description: drop "registers the preset" (now
  reflective), state explicitly that this scaffolds a structural
  decoder and that semantic refinement is a follow-up workflow.
- New "Scope" section after the H1 enumerates what the skill
  produces (decoded args, named accounts, auto-registration,
  auto-test-coverage) and what it deliberately does not (domain
  labels, token resolution, per-instruction dispatch, semantic
  assertions). Points at `presets/jupiter_swap/mod.rs` as the
  fully-semantic exemplar.
- Step 4 rewritten: registration is now automatic; nothing to edit.
- Step 5 promotes the `tests/semantic_pipeline.rs` mention from a
  bullet to its own subsection clarifying the auto-roundtrip
  asserts no semantic correctness -- by design.
- New Step 8: "What's next -- semantic refinement (optional)"
  spells out the three contributor paths (ship as-is, hand-extend
  modelled on jupiter_swap, wait for `solana-refine-idl-preset`)
  and lists the patterns to copy from jupiter_swap.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four fixes prompted by review:

- SKILL.md scope section pointed at "Step 7: What's next" but the
  actual section is Step 8 (Step 7 is "Verify"). Now correct.
- SKILL.md Step 5 referenced a "JUPITER_IDL block" in
  tests/semantic_pipeline.rs that does not exist -- Jupiter has its
  own dedicated preset and is not exercised in semantic_pipeline.rs.
  Replaced with RAYDIUM_IDL / ORCA_IDL, which are real.
- The idl_test! macro docstring claimed test files "must declare the
  module with `#[macro_use] mod common;`". With #[macro_export] that
  is unnecessary and surfpool_fuzz.rs uses plain `mod common;`.
  Rewrote the docstring to reflect that any sibling test file gets
  the macro at the test binary's crate root for free.
- surfpool_preset_idls started a fresh SurfpoolManager for every
  preset in the loop, paying the fork-startup cost N times. Split
  run_idl_roundtrip into a setup-free run_idl_roundtrip_inner so the
  iterating test can share one manager across all presets. Per-IDL
  named tests via idl_test! still get their own manager, which is
  fine for isolated runs.

Plus, three new build.rs unit tests that lock in the reflection
contract: collect_preset_mods emits #[path]-pinned `pub mod` lines,
collect_preset_mods skips dirs without mod.rs, collect_preset_idls
output is tuple-shaped and sorted. These mirror the existing
test_collect_visualizers_* tests in the same file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shahan-khatchadourian-anchorage
Copy link
Copy Markdown
Contributor Author

Coordination notes — overlap with #278 and #281

Surfaced during a follow-up triage. Posting here so reviewers and downstream rebases know what to expect.

Overlap with PR #278 — auto-generate presets/mod.rs

#278 (build(solana,sui): auto-generate presets/mod.rs from filesystem) solves the same problem this PR's commit a45e514 solves, with the same #[path] mechanism, but with broader and better-engineered scope:

  • Covers both visualsign-solana and visualsign-sui (this PR only covers solana)
  • Rewrites build.rs to be Result-returning (no unwrap/expect, no #[allow(clippy::unwrap_used)] dodge — properly aligned with the workspace lint policy in CLAUDE.md)
  • Motivated by quantified pain: 12 in-flight preset PRs colliding on the alphabetical pub mod list during rebase

Suggested resolution: drop commit a45e514 from this PR (revert the auto-mod portion) and rebase on top of #278 once it lands. The PRESET_IDLS reflection (pub const PRESET_IDLS: &[(&str, &str)]) and the SKILL.md scope refresh stay; only the presets/mod.rs auto-generation work goes back to #278.

Overlap with PR #281 — skill refresh for post-#228 API

#281 (chore(skill): refresh solana-add-idl for post-#228 VisualizerContext API) is a parallel refresh of the same skill, against a different stack:

  • It's stacked on shahankhatch/228-lint-diagnostics-refactor, where context.current_instruction() is being removed in favor of context.resolve_program_id()? / context.resolve_accounts()? / context.data().
  • It points at dflow_aggregator as the real working template (replacing the dead squads_multisig reference) — the same fix this PR makes by pointing at unknown_program.
  • This PR's SKILL.md tells contributors to copy the named_accounts loop from unknown_program::try_parse_with_idl — that loop uses context.current_instruction() and will compile-rot the moment feat: attested transaction lint diagnostics in SignablePayload #228 lands.

Suggested resolution: #281 lands first, then this PR's SKILL.md changes rebase on top of it. The pieces of this PR's SKILL.md edits worth carrying forward post-rebase:

  • The new "Scope" section (lines 13-36) framing the skill as structural-not-semantic
  • Step 5's promotion of the tests/semantic_pipeline.rs note to its own subsection
  • Step 8 "What's next — semantic refinement (optional, follow-up)"
  • The {SCREAMING_SNAKE}_PROGRAM_ID declaration step

Most of the import list and parse_instruction_with_idl-pattern guidance overlaps with #281 and should be reconciled at rebase time.

Out of scope here

tl;dr

This PR ships in two halves: a build.rs auto-mod commit that duplicates #278, and a SKILL.md rewrite that conflicts with #281. The clean path is to land #278 and #281 first, then this PR carries only PRESET_IDLS reflection + the SKILL.md pieces #281 doesn't already do (Scope section, Step 8, semantic-refinement handoff).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant