refactor: VisualizerContext backed by transaction wire data, eliminate instruction skipping (#228) by shahan-khatchadourian-anchorage · Pull Request #255 · anchorageoss/visualsign-parser

shahan-khatchadourian-anchorage · 2026-04-16T20:29:23Z

Summary

Addresses all review feedback from #230 by refactoring VisualizerContext to work directly with transaction wire data (&CompiledInstruction + &[Pubkey]), eliminating instruction skipping/filtering, and simplifying the diagnostic model.

Supersedes #230 — that PR has the original review discussion and should be closed once this merges.

What changed

VisualizerContext holds &CompiledInstruction + &[Pubkey] instead of an index into a pre-resolved Vec<Instruction>. Resolution of indices to pubkeys happens lazily via ProgramRef/AccountRef enums that distinguish resolved from unresolved.
No instructions are ever skipped. Every compiled instruction flows through the visualizer pipeline. unknown_program catches instructions with unresolvable program IDs.
Unified diagnostic model: 2 rules (oob_program_id, oob_account_index) replace the old 3-rule model. oob_account_index_in_skipped_instruction is eliminated — there is no concept of "skipped instruction."
Shared scan_instruction_diagnostics function used by both legacy and v0 paths, eliminating code duplication.
No data copies: visualizers access instruction data via context methods (context.data(), context.program_id(), context.account(n)) — no Instruction struct is constructed.

Review comments addressed (#230)

#	Comment	Resolution
Critical #1, #2	Index mismatch in instructions.rs/v0.rs	Eliminated — no filtered vec, no index to mismatch
High #3	Misleading ok-diagnostic for 0 skipped	Rule removed entirely
High #4	V0 "Instruction Decoding Note" change	Verified — no dependents
High #5	Text/human output untested	Restored solana-text fixture
High #6	Shallow diagnostic assertions	Checks rule, level, and instruction_index
Medium #7	.unwrap() in Diagnostic serialize	Direct serialize_entry calls
Medium #8	&str instead of Severity enum	Accepts Severity
Medium #9	Unregistered decode::visualizer_error	Documented as always-on
Medium #10	Code duplication legacy/v0	Shared scan function
Low #11	LintConfig::default() twice	Single construction, threaded through
Low #12	Doc comment placement	Fixed

Review comments addressed (this PR, @prasanna-anchorage)

Concern	Resolution	Commit
Drop `docs/superpowers/plans/2026-04-13-pr230-review-fixes.md` (1362-line internal planning artifact)	Removed; added `docs/superpowers/` to `.gitignore`	`0baec56`
Gate diagnostics behind a Cargo feature, default-on for CLI only	Added `diagnostics` feature on `visualsign` / `visualsign-solana` / `parser_cli`; `parser_app` and `grpc-server` build without it. CI/Makefile split per-package to avoid Cargo feature unification across the workspace. Integration test now strictly asserts `parser_app` emits no `diagnostic` fields.	`e468f86`
Forward-looking nit: `LintConfig` / `Severity` lack `DeterministicOrdering`	Added TODO at `lint.rs` for when lint config lands in `metadata_digest`	`0baec56`
Optional nit: explicit `assert_deterministic_ordering(&diag)` next to siblings	Added in the compile-time ordering test batch	`0baec56`

Branch synced with main (commit `80b076b`)

main advanced with 14 new IDL-based Solana presets (Jupiter Borrow/Earn/Perpetuals, Kamino Borrow/Vault/Farms, MetaDAO Conditional Vault/Futarchy, Meteora DAMM V2/DLMM, Neutral Trade, Onre App, Orca Whirlpool, DFlow Aggregator) authored against the pre-#228 current_instruction() API. Migrated each to the wire-data API.

Added instruction_index: usize to VisualizerContext (cheap, threaded from the dispatcher's enumerate()); new presets use it for "Instruction N" labels.
Added two convenience methods: resolve_program_id() and resolve_accounts() (Err on first unresolved index). IDL-based presets call these; visualizers wanting partial rendering pattern-match on program_id() / account(n) directly. Doc comment on VisualizerContext contrasts the two patterns and points at unknown_program as the canonical partial-rendering example.
Reconciled jupiter_swap's create_jupiter_swap_expanded_fields signature drift (now takes &VisualizerContext instead of separate program_id+data).
Dropped redundant Some(hex::encode(data)) in create_raw_data_field calls — the helper's None branch already emits the same lowercase hex.

Stacked follow-up

#281 — refresh the solana-add-idl skill template to match the post-#228 API (broken squads_multisig template reference, stale imports, redundant hex encoding).

Test plan

make build, make lint, make test clean in both feature configs (diagnostics on/off)
visualsign-solana: 135 lib tests pass with diagnostics on, 128 without
visualsign: 56 lib tests pass with diagnostics on, 48 without
parser_cli: 28 fixture-based tests pass
Pipeline integration tests pass (9)
Fuzz and proptest workflows green on CI; both fuzz targets verified locally
Integration e2e: parser_app strictly emits no diagnostic fields (locks in the production payload shape)
Clippy clean with -D warnings, cargo fmt --check clean

🤖 Generated with Claude Code

Add SignablePayloadFieldDiagnostic struct and Diagnostic variant to the SignablePayloadField enum. Diagnostics carry structured lint data (rule, domain, level, message, instruction_index) and are attested alongside display fields in the signed payload. Custom Serialize impl ensures alphabetical field ordering for deterministic serialization. Follows the same pattern as AmountV2. Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Builder function for constructing Diagnostic fields with rule, domain, level, message, and optional instruction_index. Sets fallback_text to "{level}: {message}" and label to the rule ID for backwards compat. Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Alphabetical key ordering at top-level and nested Diagnostic object - Optional InstructionIndex omitted when None - JSON serialize/deserialize roundtrip - Diagnostic field in SignablePayload passes deterministic ordering Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Replace silent filter_map drops with Diagnostic field emission when legacy transaction instructions have out-of-bounds program_id_index or account indices. Two rules: - transaction::oob_program_id: instruction skipped entirely - transaction::oob_account_index: individual accounts omitted Diagnostics are appended to the instruction fields vec, so they appear in the signed payload alongside display fields. Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Tests for decode_instructions with manually crafted transactions: - OOB program_id_index emits transaction::oob_program_id diagnostic - OOB account index emits transaction::oob_account_index diagnostic - Valid transactions produce no diagnostics Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Mirror the legacy transaction diagnostic emission for v0 transactions. Diagnostic messages note that OOB indices reference lookup table accounts that are unresolvable without on-chain data, distinguishing from corruption in legacy transactions. Also moves the empty_account_keys check before the instruction loop so it fails early. Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…dling - Add LintConfig with configurable severity overrides and report_all_rules flag for boot-metric-style attestation - Every rule always reports (ok or warn), so the attester can verify which rules ran and what they found - decode_instructions and decode_v0_instructions never panic or abort: - Data quality issues (OOB indices, empty accounts) become diagnostics - Per-instruction visualizer failures collected in errors vec - Function always returns DecodeInstructionsResult - empty_account_keys converted from Err to diagnostic - Replaced panic! in visualizer lookup with VisualSignError::DecodeError - Severity::Ok replaces "pass" for unambiguous naming - report_all_rules replaces emit_pass_diagnostics - Updated all fixtures and integration tests Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- field-types.mdx: document the diagnostic field type for wallet integrators, including properties, handling, and current rules - contributor-guides/lint-diagnostics.mdx: guide for contributors adding lint rules -- architecture, builder usage, LintConfig, naming conventions, testing patterns - docs.json: add lint-diagnostics page to Chain Development nav Part of #228. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…e lint rule Add a new `transaction::oob_account_index_in_skipped_instruction` rule that scans account indices in instructions already being skipped due to an OOB `program_id_index`. This ensures: - The `oob_account_index` ok diagnostic accurately reflects only the instructions that were actually processed (instructions.len() instead of message.instructions.len()) - The new rule provides boot-metric attestation that even skipped instructions were examined for OOB account indices Applied to both legacy (instructions.rs) and v0 (txtypes/v0.rs) paths. Updated swig_wallet tests (5->6, 7->8 fields) and added new tests covering the combined OOB case in both paths. Agent-Logs-Url: https://github.com/anchorageoss/visualsign-parser/sessions/2bb47e94-7f50-403f-a026-d71a37c4fd5f Co-authored-by: shahan-khatchadourian-anchorage <263420032+shahan-khatchadourian-anchorage@users.noreply.github.com>

- Check account indices on all instructions, including those with OOB program_id_index, so ok-level diagnostics are accurate - Preserve original instruction indices through the visualizer loop for consistent labeling and error reporting - Surface per-instruction visualizer errors as decode::visualizer_error diagnostics instead of silently discarding them - Fix lint.rs doc comment: "ok" not "pass" - Update design spec to reflect v0 coverage, LintConfig, correct return types, and error categorization Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

oob_account_index fires only for processed instructions (valid program_id), while oob_account_index_in_skipped_instruction fires for skipped instructions. This prevents double-counting when an instruction has both OOB program_id and OOB account indices. Also regenerates CLI fixtures and updates integration test expected output for the third rule. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Document the steps contributors must follow when adding new rules: regenerate CLI fixtures, update integration test JSON, update field count assertions, and run fuzz/proptest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Rename solana-json.expected to solana-json.display.expected (contains display fields only, no diagnostics) - Add solana-json.diagnostics.expected with compact rule/level pairs - Delete text fixtures (solana-text.expected, solana-text.input) - CLI test splits output: display fields compared against .display.expected, diagnostics validated by rule/level against .diagnostics.expected - Integration test filters diagnostics before display comparison, validates diagnostics separately by rule/level - Swig wallet tests count display fields only (excludes diagnostics) Adding a new lint rule now requires only adding a line to .diagnostics.expected -- display fixtures are unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Add tracing to visualsign core: create_diagnostic_field now emits tracing::warn! for warn/error-level diagnostics, giving operators production log visibility without per-chain boilerplate - Fix doc comments: replace "pass" with "ok" to match actual level strings in lint.rs, instructions.rs, and v0.rs - Update design spec: add oob_account_index_in_skipped_instruction rule to the rules table - Rename ethereum-json fixture to .display.expected to match the refactored test convention - Fix clippy needless_borrow warning in cli_test.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Add oob_account_index_in_skipped_instruction rule to contributor guide and field-types.mdx - Document tracing::warn! behavior in create_diagnostic_field across design spec and contributor guide - Rename test functions from "pass" to "ok" terminology in lint.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Add ProgramRef and AccountRef enums for typed resolution status. VisualizerContext holds references to the transaction's wire data instead of an index into a pre-resolved Vec<Instruction>. Resolution of compiled instruction indices to pubkeys happens lazily via helper methods (program_id, account, data). SolanaIntegrationConfig::can_handle simplified to (program_id: &str). InstructionVisualizer::can_handle uses ProgramRef pattern matching. The crate does not compile at this commit — downstream presets and instruction processing still reference the old API. Next commits update them.

All 8 visualizer presets updated: - system, compute_budget, associated_token_account, stakepool: use context.data(), context.program_id(), context.account(n) - jupiter_swap: builds Vec<String> accounts from context for parser - token_2022, swig_wallet: builds Vec<AccountMeta> shim from context - unknown_program: overrides can_handle to catch unresolved program_ids, uses context methods for IDL parsing and account resolution ProgramRef and AccountRef enums used explicitly at match sites. Instruction labels use operation names instead of instruction_index. Per-preset resolve helpers kept where they reduce repetition. Crate does not compile at this commit — instructions.rs and v0.rs still use the old VisualizerContext constructor.

No instructions are ever skipped. VisualizerContext is created for every compiled instruction. Diagnostics for inaccessible indices are emitted by scan_instruction_diagnostics (shared between legacy and v0). Eliminates: filtered instruction vec, oob_account_index_in_skipped_instruction rule, code duplication between legacy and v0 diagnostic logic. Tests not yet updated — 9 failures from changed diagnostic model and label format.

No instructions are ever skipped. Unified diagnostic scan with two rules (oob_program_id, oob_account_index) replaces the old three-rule model. shared scan_instruction_diagnostics function used by both legacy and v0. Test updates: - Remove oob_account_index_in_skipped_instruction assertions - Update ok-diagnostic counts from 3 to 2 - Update label assertions (operation names instead of "Instruction N") - Update fixture test helpers for new VisualizerContext constructor 70/70 lib tests pass. Pipeline integration tests need fixture updates.

Update instruction_fields() helper to identify instruction fields by excluding known non-instruction labels (Network, Accounts) rather than matching "Instruction N" prefix. All solana crate tests pass: lib (70), pipeline (9), fuzz, semantic.

- Replace .unwrap() in Diagnostic serialize impl with direct serialize_entry calls (review #7) - Accept Severity enum in create_diagnostic_field instead of &str, preventing arbitrary strings in attested payload (review #8) - Thread LintConfig through convert functions from single construction point in to_visual_sign_payload (review #11) - Document decode::visualizer_error as intentionally always-on, not routed through LintConfig (review #9) - Update spec to reflect three rules (removed oob_account_index_in_skipped_instruction), document ProgramRef/AccountRef types and no-skip behavior

- Integration test: update expected label and diagnostic rules - CLI fixtures: regenerate solana-json display expected output - CLI fixtures: update diagnostics to 2 rules (removed oob_account_index_in_skipped_instruction) - Fix clippy len_zero warning in test All tests pass: fmt, clippy, full test suite.

- Remove oob_account_index_in_skipped_instruction from rule lists - Add decode::visualizer_error as always-on rule - Update create_diagnostic_field examples to use Severity enum - Update diagnostic message examples (no more "skipped" wording) - Update field-types.mdx and contributor guide

…tic assertions - Restore solana-text.input and solana-text.display.expected fixtures - CLI test loop handles non-JSON output (text/human) with string comparison - JSON fixtures use membership checking (assert_json_contains) instead of byte-for-byte string comparison, avoiding false failures from key ordering - Diagnostic assertions check rule, level, and instruction_index - Regenerate solana-json and ethereum-json display fixtures

mintlify · 2026-04-16T20:29:29Z

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
visualsign-parser	🟢 Ready	View Preview	Apr 16, 2026, 8:30 PM

- Derive Copy on Severity (fieldless enum, remove .clone() calls) - Unify DecodeInstructionsResult / DecodeV0InstructionsResult into one type - Extract append_diagnostics helper in visualsign.rs (was duplicated) - Bounds-check u8 cast in swig_wallet visualize_inner_instruction

BTreeMap is consistent with project convention for deterministic ordering. .copied() is idiomatic for Copy types.

prasanna-anchorage

Marking as Request Changes — two blocking concerns plus smaller observations.

1. Drop `docs/superpowers/plans/2026-04-13-pr230-review-fixes.md`

This file (+1,362 lines, 36% of the PR additions) looks like Claude Code's internal planning artifact, and it'd introduce a brand-new docs/superpowers/ tree that doesn't exist on main. Could you drop it from this PR? If it's useful locally, a one-line .gitignore of docs/superpowers/ in a tiny follow-up would prevent this from recurring.

2. Gate diagnostics behind a Cargo feature, default-on for CLI only

We had wanted to land this CLI-first behind a diagnostics feature so production payload shapes stay stable while tooling iterates. Right now SignablePayloadFieldDiagnostic, the Diagnostic variant, the lint module, and create_diagnostic_field ship unconditionally to CLI, parser_app, and the gRPC server.

Signals this gating is needed:

No [features] entry for diagnostics in visualsign, visualsign-solana, parser/cli, or parser/app
Consumer-side .filter(field_type != "diagnostic") calls at chain_parsers/visualsign-solana/src/core/instructions.rs:433, presets/swig_wallet/mod.rs:2331,2469, and parser/cli/tests/cli_test.rs:127,154 look like workarounds that go away with a proper gate
Without gating, every HSM/wallet that derives a metadata digest from the SignablePayload sees a hash shift on day one — even if not ready to consume diagnostics

Suggested shape:

# src/visualsign/Cargo.toml
[features]
diagnostics = []

# src/chain_parsers/visualsign-solana/Cargo.toml
[features]
diagnostics = ["visualsign/diagnostics"]

# src/parser/cli/Cargo.toml
[features]
default = ["solana", "ethereum", "diagnostics"]
diagnostics = ["visualsign/diagnostics", "visualsign-solana/diagnostics"]

# src/parser/app/Cargo.toml and src/parser/grpc-server/Cargo.toml — do not enable yet

Then #[cfg(feature = "diagnostics")] gates the variant, lint module, builder, emission paths, and the parser/cli/tests/fixtures/solana-json.diagnostics.expected fixture. The consumer-side .filter workarounds become obsolete.

What looks good

DeterministicOrdering is properly applied to the new types: SignablePayloadFieldDiagnostic has the trait impl at src/visualsign/src/lib.rs:658, the custom Serialize emits alphabetically (Domain → InstructionIndex → Level → Message → Rule), the variant is wired through serialize_to_map/get_expected_fields, and there's both runtime coverage (verify_deterministic_ordering at lib.rs:2517,2557) and compile-time coverage (assert_deterministic::<SignablePayloadField>() in src/visualsign/examples/compile_time_check.rs).

Smaller observations (non-blocking)

Forward-looking: LintConfig and Severity in src/visualsign/src/lint.rs don't implement DeterministicOrdering / Serialize. Fine for this PR since lint config isn't in metadata_digest yet, but worth a TODO so it's not missed when the request-side plumbing lands.
Optional nit: an assert_deterministic_ordering(&diag) for SignablePayloadFieldDiagnostic directly (next to the existing batch at lib.rs:1990+) would give it the same compile-time anchor as siblings.

Happy to re-review once the gate is in and the planning doc is dropped.

- Remove docs/superpowers/plans/2026-04-13-pr230-review-fixes.md (Claude internal planning artifact, +1362 lines, ~36% of PR additions) and add docs/superpowers/ to .gitignore so it doesn't recur. - Add TODO on Severity/LintConfig in src/visualsign/src/lint.rs to implement DeterministicOrdering / deterministic Serialize when lint config gets folded into metadata_digest. - Add explicit assert_deterministic_ordering(&diag) for SignablePayloadFieldDiagnostic next to siblings in the compile-time ordering test, so it has the same anchor as other field types. Feature-gating diagnostics (the second blocking concern) is split out to a follow-up.

Addresses the second blocking concern in PR review: parser_app and parser_grpc_server now build without the diagnostics machinery, so wallets/HSMs hashing the SignablePayload see the same shape as before the lint refactor. - visualsign: cfg-gate `lint` module, `Diagnostic` variant on `SignablePayloadField`, `SignablePayloadFieldDiagnostic`, and `create_diagnostic_field` behind `feature = "diagnostics"`. - visualsign-solana: same feature is propagated. Provide two `decode_instructions` / `decode_v0_instructions` impls -- the diagnostics-on variant collects errors and emits diagnostic fields; the diagnostics-off variant returns `Result<Vec<_>, VisualSignError>` and propagates empty-account-keys + visualizer errors as `Err` (the pre-refactor abort-on-bad-tx semantics). - parser_cli: enables `diagnostics` by default; uses the weak `?` syntax on `visualsign-solana?/diagnostics` so non-default builds still work. - parser/app, grpc-server: no `diagnostics` feature -- production stays on the stable shape. - Makefile: split workspace builds/tests/clippy so feature unification can't sneak diagnostics on for parser_app via `cargo build --all`. Adds explicit feature-on test+clippy passes for the gated lib code. - Integration test now strictly asserts parser_app emits no diagnostic fields, validating the gate. Per-site treatment of the `.filter(field_type != "diagnostic")` workarounds the reviewer flagged: in cli_test.rs they're cfg-gated so the OFF build is filter-free; in swig_wallet tests they remain as a harmless no-op when the feature is off (the assertion counts still hold). True elimination would require moving diagnostics out of `payload.fields` into a separate top-level array -- noted as a possible follow-up rather than done here.

…-diagnostics-refactor

…ntext API main moved forward with 14 new IDL-based Solana presets while this PR was in review. All of them were authored against the pre-#228 VisualizerContext that exposed `current_instruction()` and `instruction_index()`. This PR's wire-data refactor removed those, so the merged tree fails to compile. Changes: - VisualizerContext now carries `instruction_index: usize` (cheap, threaded from the dispatcher's `enumerate()`). New presets use it to label rows like "Instruction N". - Two new convenience methods on VisualizerContext: - `resolve_program_id()` — returns Err(Decode) on out-of-bounds. - `resolve_accounts()` — returns Err(Decode) on the first unresolved index. Both keep IDL-based presets one-liner-clean while preserving the explicit error path the reviewer asked for in #230. - Doc comment on VisualizerContext contrasts the all-or-nothing pattern (these helpers) with partial rendering (the unknown_program preset's `unresolved(N)` placeholders), pointing readers at a real example. - All 14 new presets ported: dflow_aggregator, jupiter_borrow, jupiter_earn, jupiter_perps, kamino_borrow, kamino_farms, kamino_vault, metadao_conditional_vault, metadao_futarchy, meteora_damm_v2, meteora_dlmm, neutral_trade, onre_app, orca_whirlpool. Same shape: pull program_id, data, and accounts from the context up front, then use them throughout. - jupiter_swap signature drift: `create_jupiter_swap_expanded_fields` now takes &VisualizerContext (not separate program_id+data); test updated. - meteora_dlmm and neutral_trade `build_named_accounts` updated to take data + accounts separately instead of a constructed Instruction. - Drop redundant `Some(hex::encode(...))` second arg on every `create_raw_data_field` call site -- the helper's `None` branch already emits the same lowercase hex via `default_hex_representation`. Touching every preset in this commit anyway, so cleaner to land together. - Rename ethereum-from-file fixture to *.display.expected to match the *.display.expected / *.diagnostics.expected scheme this PR introduced. Verified: make build, make lint, make test all green in both feature configs (diagnostics on for parser_cli, off for parser_app/grpc-server).

shahan-khatchadourian-anchorage · 2026-05-06T15:56:27Z

@prasanna-anchorage status of the items from your review:

Blocking:

Planning doc dropped, docs/superpowers/ added to .gitignore (commit 0baec56)
diagnostics Cargo feature added to visualsign / visualsign-solana / parser_cli (default-on for CLI). parser_app and grpc-server build without it. CI/Makefile invokes cargo build/test/clippy --workspace --exclude parser_cli and cargo build/test/clippy -p parser_cli separately to defeat Cargo feature unification, plus explicit --features diagnostics --lib runs to cover the gated lib tests. Integration test strictly asserts parser_app emits no diagnostic fields, locking the production payload shape (commit e468f86).

Non-blocking nits:

LintConfig / Severity DeterministicOrdering TODO added in src/visualsign/src/lint.rs (commit 0baec56)
Explicit assert_deterministic_ordering(&diag) for SignablePayloadFieldDiagnostic added next to siblings in the compile-time ordering test batch (commit 0baec56)

Per-site filter treatment: with the gate in place, filters are either compiled out (cli_test diagnostic-fixture branch is #[cfg(feature = "diagnostics")]-gated and disappears in OFF builds), inside an entirely cfg-gated test (instructions.rs:433), or no-ops because no diagnostic-typed field can exist (swig_wallet display-count assertions). Removing them entirely in ON builds would require moving diagnostics out of payload.fields into a separate top-level array on SignablePayload, which is a payload-shape change.

Other: merge from main brought in 14 new IDL-based presets authored against the pre-#228 current_instruction() API. Ported all of them to the wire-data accessors (commit 80b076b).

Stacked follow-up #281 refreshes the solana-add-idl skill so newly-scaffolded presets match the new API.

Code-review feedback on the feature gate: the OFF-path implementations of `decode_instructions` and `decode_v0_instructions` had zero direct unit-test coverage because both test modules were entirely gated `#[cfg(all(test, feature = "diagnostics"))]`. Their semantics differ from the ON path (errors propagate as Err instead of being collected as diagnostics), so they need explicit coverage. - New `mod off_tests` in `core/instructions.rs` and `core/txtypes/v0.rs`, gated `#[cfg(all(test, not(feature = "diagnostics")))]`. Covers: - empty `account_keys` -> `Err(VisualSignError::DecodeError)` - OOB program_id flows through to `unknown_program` as `Ok` - OOB account index flows through as `Ok` - Makefile `make test` now invokes `cargo test -p visualsign-solana --no-default-features --lib` so the new OFF-path tests actually run. - Make the `Severity` match in `create_diagnostic_field` exhaustive (`Severity::Ok | Severity::Allow` instead of `_`) so the compiler flags missing arms when a new variant is added. - Document the `diagnostics` Cargo feature in `docs/contributor-guides/lint-diagnostics.mdx`: which artifacts ship with it, how the OFF path differs, and how the Makefile defeats Cargo feature unification.

shahan-khatchadourian-anchorage · 2026-05-06T16:42:20Z

Security review

/security-review ran on the branch tip (a2d9589). No high-confidence security vulnerabilities identified.

The reviewer agent examined the diagnostic feature gate, dual decode_instructions semantics, OOB index handling, resolve_accounts() writable/signer bit discard, string-formatting in the new presets, LintConfig deserialization paths, decode::visualizer_error content, and the unknown_program IDL handling. All concerns either pre-date this PR, are adequately mitigated, or have no concrete attack path.

Key checks that passed:

Feature gate for HSM-stable payload shape: parser_app / parser_grpc_server correctly omit diagnostics. The Makefile split (cargo build --workspace --exclude parser_cli then cargo build -p parser_cli) defeats Cargo feature unification. The integration test at src/integration/tests/parser.rs:376-385 runtime-asserts that no diagnostic-typed field appears in parser_app output, providing a backstop if the build split is ever bypassed.
OOB program_id / account indices: Diagnostics-OFF flows them through to unknown_program's unresolved(N) placeholders, which are visually distinct from real 44-char base58 pubkeys.
resolve_accounts() discards is_signer/is_writable: Pre-existing on origin/main (AccountMeta::new_readonly(..., false)); not new in this PR.
String-formatting in the 14 ported presets: Inputs are Pubkey (base58 ASCII), hex::encode(...) (hex ASCII), or compile-time-vetted IDL JSON. The existing ASCII charset assertion enforces this.
LintConfig: Always LintConfig::default(); no untrusted-input deserialization path.
decode::visualizer_error: Built from public transaction bytes and integer indices; no secrets/PII leak.
Dual decode semantics (ON/OFF): Behavioral divergence is intentional and documented; OFF output is strictly a subset of ON output, never more permissive.

shahan-khatchadourian-anchorage · 2026-05-06T16:45:22Z

@prasanna-anchorage ready for re-review.

@prasanna-anchorage

Addresses #230 review comment from @prasanna-anchorage on the original swig_wallet/mod.rs:934. Replaces the |-grouped fallback (`ListLayout | Divider | Unknown`) with one arm per variant in enum declaration order, matching the style used in src/visualsign/src/lib.rs. Eliminates the ordering ambiguity that prompted the review comment. Behavior is unchanged -- all four arms call fallback_summary(common).

prasanna-anchorage

other than Label everything else looks fine, I'd like to keep the user visible outputs unchanged. We have lot of changes that are user visible going out in upcoming release

prasanna-anchorage · 2026-05-07T23:27:57Z

                common,
                preview_layout,
            } => {
-                assert_eq!(common.label, "Instruction 1");


this is unexpected for this PR, can we keep user visible fields same instead of changing the behavior in a refactor PR?

shahan-khatchadourian-anchorage and others added 27 commits April 16, 2026 12:30

style: apply rustfmt to diagnostic changes

a6694be

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

style: apply cargo fmt

296407d

fix: add missing clippy allow attributes on test modules after rebase

ff92a53

Copilot AI review requested due to automatic review settings April 16, 2026 20:29

Copilot started reviewing on behalf of shahan-khatchadourian-anchorage April 16, 2026 20:29 View session

shahan-khatchadourian-anchorage added 3 commits April 17, 2026 10:27

fix: route empty_account_keys through LintConfig severity

e7c597e

refactor: use BTreeMap for LintConfig overrides, .copied() for Severity

ed1f75f

BTreeMap is consistent with project convention for deterministic ordering. .copied() is idiomatic for Copy types.

shahan-khatchadourian-anchorage added fuzz proptest stagex Trigger stagex OCI image build labels Apr 17, 2026

prasanna-anchorage requested changes Apr 29, 2026

View reviewed changes

mintlify Bot deployed to staging - docs May 6, 2026 03:58 View deployment

github-actions Bot added proptest-failure Property tests found a failure fuzz-failure Fuzz testing found a crash labels May 6, 2026

shahan-khatchadourian-anchorage added 3 commits May 6, 2026 01:31

Merge remote-tracking branch 'origin/main' into shahankhatch/228-lint…

6dfa155

…-diagnostics-refactor

mintlify Bot deployed to staging - docs May 6, 2026 15:38 View deployment

github-actions Bot removed proptest-failure Property tests found a failure fuzz-failure Fuzz testing found a crash labels May 6, 2026

shahan-khatchadourian-anchorage mentioned this pull request May 6, 2026

chore(skill): refresh solana-add-idl for post-#228 VisualizerContext API #281

Open

2 tasks

shahan-khatchadourian-anchorage marked this pull request as draft May 6, 2026 16:08

shahan-khatchadourian-anchorage marked this pull request as ready for review May 6, 2026 16:45

mintlify Bot deployed to staging - docs May 6, 2026 16:45 View deployment

shahan-khatchadourian-anchorage force-pushed the shahankhatch/228-lint-diagnostics-refactor branch from 1b8e15a to 3f28e5d Compare May 7, 2026 04:37

mintlify Bot deployed to staging - docs May 7, 2026 04:38 View deployment

shahan-khatchadourian-anchorage mentioned this pull request May 7, 2026

deps: bump solana-parser rev to clean tkhq-base branch #283

Merged

3 tasks

prasanna-anchorage requested changes May 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: VisualizerContext backed by transaction wire data, eliminate instruction skipping (#228)#255

refactor: VisualizerContext backed by transaction wire data, eliminate instruction skipping (#228)#255
shahan-khatchadourian-anchorage wants to merge 39 commits into
mainfrom
shahankhatch/228-lint-diagnostics-refactor

shahan-khatchadourian-anchorage commented Apr 16, 2026 •

edited

Loading

Uh oh!

mintlify Bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

prasanna-anchorage left a comment

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026 •

edited

Loading

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026

Uh oh!

prasanna-anchorage left a comment •

edited

Loading

Uh oh!

prasanna-anchorage May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

shahan-khatchadourian-anchorage commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Review comments addressed (#230)

Review comments addressed (this PR, @prasanna-anchorage)

Branch synced with main (commit 80b076b)

Stacked follow-up

Test plan

Uh oh!

mintlify Bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prasanna-anchorage left a comment

Choose a reason for hiding this comment

1. Drop docs/superpowers/plans/2026-04-13-pr230-review-fixes.md

2. Gate diagnostics behind a Cargo feature, default-on for CLI only

What looks good

Smaller observations (non-blocking)

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026

Security review

Uh oh!

shahan-khatchadourian-anchorage commented May 6, 2026

Uh oh!

prasanna-anchorage left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prasanna-anchorage May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shahan-khatchadourian-anchorage commented Apr 16, 2026 •

edited

Loading

Branch synced with main (commit `80b076b`)

mintlify Bot commented Apr 16, 2026 •

edited

Loading

1. Drop `docs/superpowers/plans/2026-04-13-pr230-review-fixes.md`

shahan-khatchadourian-anchorage commented May 6, 2026 •

edited

Loading

prasanna-anchorage left a comment •

edited

Loading