feat(solana): core::arg_rendering + Marginfi preset; bootstrap-only solana-add-idl skill

[shahan-khatchadourian-anchorage]

Summary

Single, additive PR consolidating the solana-add-idl skill work (supersedes #401, closed into this).

• core::arg_rendering — adds format_arg_value (+ charset_safe, bytes_as_hex): a charset-safe, single-field-per-arg renderer (byte arrays -> 0x hex). New presets import it from crate::core; existing presets are left untouched.
• Marginfi preset — new IDL-based preset, auto-registered via build.rs, as a live validation of the skill.
• SKILL.md — Sonnet-subagent dispatch (model enforced at the API layer) + compare mode; import format_arg_value from crate::core; bootstrap-only scope (scaffold a new preset, never regenerate an existing one); subtitle convention; surfpool live-fuzz validation; and an "Improving this skill" section with two non-destructive validation loops (corpus diff + model compare).

No existing preset is modified — clean on main.

Test plan

• cargo clippy -p visualsign-solana --all-targets -- -D warnings
• cargo clippy -p visualsign-solana --features diagnostics --all-targets -- -D warnings
• cargo test -p visualsign-solana — 270 passed
• cargo test -p visualsign-solana --features diagnostics
• Add surfpool label to run live fuzz CI (incl. Marginfi)

Closes #397

[shahan-khatchadourian-anchorage]

Finding from commit 3 (regeneration test): config.rs template prescribes HashMap but SolanaIntegrationConfigData uses BTreeMap

The skill's config.rs template currently has:

use std::collections::HashMap;
// ...
let mut programs = HashMap::new();
let mut instructions = HashMap::new();

But SolanaIntegrationConfigData is defined as:

pub struct SolanaIntegrationConfigData {
    pub programs: BTreeMap<&'static str, BTreeMap<&'static str, Vec<&'static str>>>,
}

A contributor following the template verbatim would get a type mismatch compile error. The template needs to be updated to BTreeMap throughout — both the use line and both ::new() calls.

[shahan-khatchadourian-anchorage]

Finding from commit 3 (regeneration test): Step 4 of the skill is stale — presets/mod.rs is auto-generated, do not edit

Step 4 currently says:

Add pub mod {snake_name}; to src/chain_parsers/visualsign-solana/src/presets/mod.rs. Keep entries in alphabetical order.

But presets/mod.rs contains only:

//! ... Do NOT edit this file — it has no hand-maintained content.
include!(concat!(env!(\OUT_DIR\), \/generated_presets_mod.rs\));

build.rs auto-discovers any {PascalName}Visualizer struct under src/presets/. Step 4 should be replaced with a note explaining this — creating the directory with mod.rs + config.rs is all that's needed; no manual module registration.

[shahan-khatchadourian-anchorage]

Finding from commit 3 (regeneration test): skill requires InstructionView in crate::core, which only lands via PR #367

The updated skill prescribes:

use crate::core::{
    InstructionView, ...
};
// ...
let view = InstructionView::from_context(context);

InstructionView does not exist on main yet — it's introduced in PR #367 (fix/362-idl-preset-alt-accounts). Any contributor running the skill against main today will get a compile error. This PR is stacked on #367 for that reason.

The skill should document this precondition — either a note at the top ("requires PR #367 / the InstructionView struct in core") or, once #367 merges, this becomes a non-issue. If the skill is intended to always reflect merged state, add a note that it tracks main and should not be used on branches predating the InstructionView introduction.

[shahan-khatchadourian-anchorage]

Comparison: generated vs prior neutral_trade and onre_app implementation (template gap fix)

Finding that triggered this commit: the generated template was missing Program: {display_name} as the first field in expanded_fields for both build_parsed_fields and build_fallback_fields. The condensed view already had it correctly. The prior hand-written implementations had this field in the expanded view.

Fix applied:

• neutral_trade/mod.rs: added create_text_field("Program", "Neutral Trade") as first expanded field in both functions
• onre_app/mod.rs: added create_text_field("Program", "Onre App") as first expanded field in both functions
• SKILL.md: template note added to Step 3 prescribing this pattern for all future presets

After fix, the generated expanded view is now consistent with the prior implementations on this point, while still being strictly better on: OnceLock caching, recursive push_arg_fields, BTreeMap for named_accounts, Remaining Account N handling, and InstructionView pattern.

[Copilot]

Pull request overview

Validates and updates the solana-add-idl scaffolding skill by regenerating multiple Solana IDL-based presets to match the newer InstructionView::from_context() pattern (graceful handling for v0 + ALT / unresolved accounts), plus aligning config generation and test-helper conventions.

Changes:

• Regenerated onre_app, neutral_trade, and dflow_aggregator preset implementations to use InstructionView, IDL caching via OnceLock, BTreeMap named accounts, and dummy_account_strings test helpers.
• Improved expanded/fallback field rendering consistency (e.g., ensuring expanded views include a human-readable Program field).
• Updated .claude/skills/solana-add-idl/SKILL.md to document the new patterns (BTreeMap config, no manual presets/mod.rs registration, additional tests, etc.).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/chain_parsers/visualsign-solana/src/presets/onre_app/mod.rs	Regenerated IDL visualizer using InstructionView, cached IDL, fallback rendering, recursive arg field rendering, and expanded “Remaining Account N” support + tests.
src/chain_parsers/visualsign-solana/src/presets/onre_app/config.rs	Simplifies config map construction (type inference) while keeping BTreeMap backing.
src/chain_parsers/visualsign-solana/src/presets/neutral_trade/mod.rs	Regenerated visualizer and tests in the same InstructionView/fallback/recursive-args pattern.
src/chain_parsers/visualsign-solana/src/presets/dflow_aggregator/mod.rs	Small regeneration deltas: formatting, expanded/fallback “Program” field, and test helper conversion to dummy_account_strings.
.claude/skills/solana-add-idl/SKILL.md	Updates skill instructions to match current preset patterns (InstructionView, BTreeMap config, no manual module registration, extended required tests).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[shahan-khatchadourian-anchorage]

Fixed in 76e32a3 — replaced the single "{subtitle_text}".to_string() placeholder with two explicit forms: one showing a custom subtitle as a string literal with .to_string(), and one showing the empty-default case as String::new() directly. This matches what the generated presets actually emit.

[shahan-khatchadourian-anchorage]

Review summary

Stacked on: #367 (fix/362-idl-preset-alt-accounts — introduces InstructionView)

What this PR does

Validates the solana-add-idl scaffolding skill by running it in Validation Mode: delete three existing preset implementations (dflow_aggregator, neutral_trade, onre_app), regenerate them from scratch using the skill as the spec, compare against the prior implementations, and fold every gap back into the skill template.

Skill changes (SKILL.md)

• Two-mode orientation at the top — "Adding a new program" vs "Validation Mode" are now named explicitly so first-time readers don't have to reach the end to discover the distinction
• Step 1 now asks for a subtitle (default: empty, matching the corpus-wide convention for IDL presets; the prior hand-written neutral_trade/onre_app used the display name as subtitle, which was an inconsistency — the title already contains the program name)
• config.rs template corrected from HashMap to BTreeMap — SolanaIntegrationConfigData.programs requires BTreeMap
• Step 3 now documents the exact build_named_accounts signature (data, idl, accounts order) and the subtitle two-form template (custom string vs String::new() — fixes a Copilot finding where the prior placeholder would have generated literal text)
• Step 4 corrected — presets/mod.rs is fully auto-generated by build.rs; no manual pub mod line is needed
• Step 6 added InstructionView::from_context as the prescribed account-resolution pattern, with security-asymmetry note explaining why resolve_accounts()? is kept only in token_2022
• Step 7 (new) — compare generated output against prior implementation, post a structured PR comment; skip for new programs with no prior implementation
• Required tests expanded from 4 to 10: added push_arg_fields (scalars, arrays, objects, empty collections), build_named_accounts_surfaces_extra_accounts, and remaining_account_label_is_human_readable, plus field_label_value and serde_json/IdlSource import prescriptions
• Validation Mode section (formerly "When validating…") — iterative loop diagram, per-finding PR comment guidance, "the skill is the artifact" principle

Generated preset changes vs prior implementations

All three presets are strictly better than the hand-written versions they replace:

	Prior	Generated
IDL caching	Per-call decode_idl_data	OnceLock<Option<Idl>> (once at startup)
Account resolution	resolve_accounts()? (fails on ALT)	InstructionView::from_context (infallible)
Extra accounts	Silently dropped	Surfaced as "Remaining Account N"
Arg rendering	Flat format_arg_value	Recursive push_arg_fields (handles nested objects/arrays)
Named accounts map	Vec<(String,String)> or HashMap	BTreeMap (deterministic order)
Test helpers	Vec<AccountMeta>	Vec<String> via dummy_account_strings
Tests per preset	4	10

Strengths

• The delete→regenerate→compare loop is a rigorous validation methodology — findings surface as PR comments immediately rather than being batched, and each one closes with a skill fix + re-regeneration confirmation
• The skill is now self-referential: the Validation Mode section describes how to improve the skill itself, so the next round of preset additions will benefit from all lessons learned here
• The corpus subtitle survey (24/26 presets use String::new()) resolved an ambiguity that would otherwise have required a judgement call on every new preset

[prasanna-anchorage]

Rebased onto main (in a worktree) now that #367 is merged. Because #367 was squash-merged, its commits still appeared on this branch and inflated the diff to 21 files with 3 conflicting presets. Rebased with git rebase --onto main 7936534 to drop the now-merged #367 commits and replay only this PR's 13 commits; the delete-then-regenerate sequence resolved the 3 preset conflicts cleanly. Force-pushed as 41c82e4.

The diff is now just the real change — 5 files: SKILL.md + the 3 regenerated presets (dflow_aggregator, neutral_trade, onre_app) + onre_app/config.rs.

Verified locally against the merged baseline:

• cargo clippy -p visualsign-solana --all-targets -- -D warnings — clean
• cargo clippy -p visualsign-solana --features diagnostics --all-targets -- -D warnings — clean
• cargo test -p visualsign-solana — 266 lib + 59 integration tests pass, 0 failures

Ready for re-review.

[shahan-khatchadourian-anchorage]

Consolidating the solana-add-idl skill work into this single PR and making it purely additive — heads-up since you rebased this branch.

What changed in approach: we're dropping the regenerate-existing-presets validation (the delete+regenerate of dflow/neutral_trade/onre_app) in favor of bootstrap-then-own: the skill scaffolds a new preset and never regenerates an owned one. Net effect here is additive only — core::arg_rendering (your #387 renderer, lifted verbatim into crate::core) + a new Marginfi preset + the skill doc. Your #387 dflow is untouched, so the #394 nested renderer still stacks on top exactly as planned.

Preserved from the prior branch: the regeneration-loop framing you'd added (reframed non-destructive — regenerate to a scratch dir, diff vs committed, fix the skill), plus the subtitle convention and surfpool live-fuzz docs. Added the Sonnet-subagent dispatch + compare mode (closes #397).

Follow-up (separate issue, not this PR): neutral_trade + onre_app still use the buggy push_arg_fields on main — same charset/field-explosion bug #387 fixed for dflow; tracked for a focused fix.

Tests: 270/270, clippy clean (default + diagnostics). Requesting your review given the merge-approval policy.

[prasanna-anchorage]

Follow-up on the bytes_as_hex finding (src/chain_parsers/visualsign-solana/src/core/arg_rendering.rs:31-41):

Best fix — make it type-driven, not value-driven. format_arg_value only ever sees a serde_json::Value, but every call site already has (or can get) the arg's declared IdlType from idl_instruction.args: Vec<IdlField> (matched by field name — the same lookup build_named_accounts already does for accounts). Thread that type through instead of guessing from the decoded values:

pub fn format_arg_value(value: &serde_json::Value, ty: Option<&IdlType>) -> String {
    match (value, ty) {
        (serde_json::Value::Array(items), Some(IdlType::Array(inner, _) | IdlType::Vec(inner)))
            if matches!(**inner, IdlType::U8) =>
        {
            bytes_as_hex(items).unwrap_or_else(|| "[]".to_string())
        }
        (serde_json::Value::Array(items), _) => {
            let inner: Vec<String> = items.iter().map(|v| format_arg_value(v, None)).collect();
            format!("[{}]", inner.join(","))
        }
        // ... other arms unchanged, ty ignored for non-array cases
    }
}

This makes [u8; 32]/Vec<u8> collapse to hex exactly when the IDL says so, and a Vec<u16>/[u16; N] of small values (bps, weights, small indices) always renders as a readable list regardless of what the values happen to be — no more guessing.

If threading the type isn't worth the churn right now, a strictly safer heuristic than "all elements ≤255" is to also gate on length, since deliberate byte-blobs (seeds, hashes, pubkey-shaped fields) are essentially always ≥16 bytes, while meaningful small-integer parameter arrays (bps, weights, fee tiers) are almost always shorter than that in practice:

fn bytes_as_hex(items: &[serde_json::Value]) -> Option<String> {
    if items.len() < 16 {   // short arrays are far more likely semantic values than byte blobs
        return None;
    }
    ...
}

That's still a heuristic (a legitimate 16+ element Vec<u16> of small values would still misrender), so I'd treat it as a stop-gap rather than the real fix — but it closes the most likely false-positive case (short bps/weight arrays) without touching call sites.

[prasanna-anchorage]

Fleshed out the type-driven version as a concrete suggestion (not applying it myself — this is for you to evaluate/apply).

src/chain_parsers/visualsign-solana/src/core/arg_rendering.rs — thread Option<&IdlType> through instead of guessing from decoded values:

use solana_parser::solana::structs::IdlType;

/// `ty` is the arg's declared IDL type, when known — it decides whether an array
/// collapses to hex or renders as a bracketed list. Byte classification is
/// type-driven: an array only becomes hex when the IDL says its elements are
/// `u8`, never merely because every decoded value happens to be `<= 255`.
///
/// `ty` is `None` for values reached through a struct/enum field (resolving
/// `IdlType::Defined` against the IDL's `types` registry isn't done here).
/// That's safe by construction: without positive type evidence, arrays never
/// collapse to hex, they just render as a (possibly longer) bracketed list.
pub fn format_arg_value(value: &serde_json::Value, ty: Option<&IdlType>) -> String {
    match value {
        serde_json::Value::String(s) => charset_safe(s),
        serde_json::Value::Number(n) => n.to_string(),
        serde_json::Value::Bool(b) => b.to_string(),
        serde_json::Value::Null => "null".to_string(),
        serde_json::Value::Array(items) => {
            let element_type = array_element_type(ty);
            if is_byte_element_type(element_type) {
                if let Some(hex) = bytes_as_hex(items) {
                    return hex;
                }
            }
            let inner: Vec<String> = items
                .iter()
                .map(|v| format_arg_value(v, element_type))
                .collect();
            format!("[{}]", inner.join(","))
        }
        serde_json::Value::Object(map) => {
            let inner: Vec<String> = map
                .iter()
                .map(|(k, v)| format!("{}:{}", charset_safe(k), format_arg_value(v, None)))
                .collect();
            format!("{{{}}}", inner.join(","))
        }
    }
}

fn array_element_type(ty: Option<&IdlType>) -> Option<&IdlType> {
    match ty {
        Some(IdlType::Array(inner, _) | IdlType::Vec(inner)) => Some(inner.as_ref()),
        _ => None,
    }
}

fn is_byte_element_type(ty: Option<&IdlType>) -> bool {
    matches!(ty, Some(IdlType::U8))
}

fn charset_safe(text: &str) -> String {
    text.chars()
        .filter(|&c| c == ' ' || (c.is_ascii_graphic() && c != '"' && c != '\\'))
        .collect()
}

/// Encodes `items` as `0x`-prefixed hex. Returns `None` only if an element
/// doesn't actually fit in a `u8` (defensive: shouldn't happen once the caller
/// has confirmed the IDL element type is `u8`).
fn bytes_as_hex(items: &[serde_json::Value]) -> Option<String> {
    let mut bytes = Vec::with_capacity(items.len());
    for item in items {
        let byte = item.as_u64().filter(|n| *n <= u8::MAX as u64)? as u8;
        bytes.push(byte);
    }
    Some(format!("0x{}", hex::encode(bytes)))
}

Notes on that sketch:

• bytes_as_hex no longer special-cases empty arrays — once the type gate confirms u8, an empty array is unambiguously 0x (empty hex), not [].
• Recursion now threads the element type one level down (array_element_type), so a Vec<Vec<u8>>-shaped arg still classifies its inner arrays correctly instead of losing type info at the first level.
• Struct/enum (Object) fields still pass None down rather than resolving IdlType::Defined against idl.types — doing that fully would mean carrying a TypeResolver-like registry into this function, which is a bigger change. This partial version already eliminates the actual bug (false hex-collapse of semantic top-level args); it just means a byte-array nested inside a struct won't get hex-compacted, it'll render as a plain number list — strictly safer than today, just less compact in that one case.

src/chain_parsers/visualsign-solana/src/presets/marginfi/mod.rs — the caller needs each arg's IdlType by name, from idl_instruction.args: Vec<IdlField>. Sketch:

use solana_parser::solana::structs::{IdlInstruction, IdlType};

fn parse_marginfi_instruction(
    data: &[u8],
    accounts: &[String],
) -> Result<MarginfiParsedInstruction, Box<dyn std::error::Error>> {
    if data.len() < 8 {
        return Err("Invalid instruction data length".into());
    }

    let idl = get_marginfi_idl().ok_or("Marginfi IDL not available")?;
    let parsed = parse_instruction_with_idl(data, MARGINFI_PROGRAM_ID, idl)?;
    let idl_instruction = find_idl_instruction(data, idl);

    let (named_accounts, extra_accounts) = build_named_accounts(idl_instruction, accounts);
    let arg_types = idl_instruction
        .map(|inst| {
            inst.args
                .iter()
                .map(|field| (field.name.clone(), field.r#type.clone()))
                .collect()
        })
        .unwrap_or_default();

    Ok(MarginfiParsedInstruction { parsed, named_accounts, extra_accounts, arg_types })
}

fn find_idl_instruction<'a>(data: &[u8], idl: &'a Idl) -> Option<&'a IdlInstruction> {
    idl.instructions.iter().find(|inst| {
        inst.discriminator
            .as_ref()
            .is_some_and(|disc| data.get(..disc.len()) == Some(disc.as_slice()))
    })
}

// build_named_accounts takes `Option<&IdlInstruction>` directly now (reuses the
// lookup above instead of re-finding it), and MarginfiParsedInstruction gains
// `arg_types: BTreeMap<String, IdlType>`.

Then in build_parsed_fields, look up each arg's type before formatting:

for (key, value) in &parsed.program_call_args {
    let ty = instruction.arg_types.get(key);
    condensed_fields.push(create_text_field(key, &format_arg_value(value, ty))?);
}

(same for the expanded_fields loop). This does touch build_named_accounts's signature and the one direct-call test (test_build_named_accounts_surfaces_extra_accounts), and the arg_rendering unit tests need updating to pass None/Some(&IdlType::U8) for the new second param — flagging that so the diff size is clear before you decide whether to take this now or as a fast-follow.

Checked marginfi's own IDL: this repo's only array-typed instruction arg is [EmodeEntry; 10] (a struct array, unaffected either way), so nothing here is a live bug for Marginfi today — it only matters for whatever the next preset scaffolded from this skill happens to declare.

[prasanna-anchorage]

One more generalization on the type-driven suggestion above: move the instruction/arg-type lookup into core itself rather than having each preset (starting with marginfi/mod.rs) hand-roll it. The lookup pattern — find the matching IdlInstruction by discriminator, then build a name -> IdlType map from its args — is identical for any IDL-driven preset, not marginfi-specific:

// src/chain_parsers/visualsign-solana/src/core/arg_rendering.rs (or core/mod.rs)
use solana_parser::solana::structs::{Idl, IdlInstruction, IdlType};
use std::collections::BTreeMap;

pub fn find_idl_instruction<'a>(data: &[u8], idl: &'a Idl) -> Option<&'a IdlInstruction> {
    idl.instructions.iter().find(|inst| {
        inst.discriminator
            .as_ref()
            .is_some_and(|disc| data.get(..disc.len()) == Some(disc.as_slice()))
    })
}

pub fn arg_types_for(idl_instruction: Option<&IdlInstruction>) -> BTreeMap<String, IdlType> {
    idl_instruction
        .map(|inst| {
            inst.args
                .iter()
                .map(|field| (field.name.clone(), field.r#type.clone()))
                .collect()
        })
        .unwrap_or_default()
}

Then marginfi/mod.rs (and the SKILL.md template for future presets) just calls these instead of re-deriving the lookup per preset:

let idl_instruction = find_idl_instruction(data, idl);
let (named_accounts, extra_accounts) = build_named_accounts(idl_instruction, accounts);
let arg_types = arg_types_for(idl_instruction);

Worth folding the call pattern into the SKILL.md scaffolding prompt too (Step C / "Arg rendering" section), so every future solana-add-idl-generated preset gets type-driven, charset-safe rendering without hand-rolling the instruction lookup — one shared entry point instead of copy-pasted boilerplate per preset.

(Opened a separate tracking issue for migrating the 17 existing presets, which each have their own local format_arg_value with a different, charset-safety gap — out of scope for this PR.)