Skip to content

fix: preserve explicit discriminant_size values in IDL snapshot #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adiman9 merged 2 commits into from
Mar 19, 2026
+18 −7
Merged

fix: preserve explicit discriminant_size values in IDL snapshot #64

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ccdd91
fix: preserve explicit discriminant_size values in IDL snapshot
adiman9 Mar 19, 2026
b373cde
fix: handle null discriminant in snapshot deserialization
adiman9 Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions hyperstack-idl/src/snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,26 @@ impl<'de> Deserialize<'de> for IdlSnapshot {
.map(|a| a.len())
.unwrap_or(0);

// Steel-style variant 1: has explicit discriminant field, no discriminator
let has_discriminant = ix.get("discriminant").is_some();
// Treat discriminant as present only if the value is non-null.
// ix.get("discriminant").is_some() returns true even for `null`,
// which causes misclassification when the AST serializer writes
// `discriminant: null` explicitly (as the ore AST does).
let has_discriminant = ix
.get("discriminant")
.map(|v| !v.is_null())
.unwrap_or(false);
let has_discriminator = discriminator
.map(|d| {
!d.is_null() && d.as_array().map(|a| !a.is_empty()).unwrap_or(true)
})
.unwrap_or(false);

// Steel-style variant 1: explicit discriminant object, no discriminator array
let is_steel_discriminant = has_discriminant && !has_discriminator;

// Steel-style variant 2: discriminator is stored as a 1-byte array (no
// discriminant field). This happens when the AST was generated from a
// Steel IDL that serialised its u8 discriminant directly into the
// discriminator field.
// Steel-style variant 2: discriminator is stored as a 1-byte array with no
// discriminant value. This happens when the AST serializer flattens the
// Steel u8 discriminant directly into the discriminator field.
let is_steel_short_discriminator = !has_discriminant && disc_len == 1;

is_steel_discriminant || is_steel_short_discriminator
Expand All @@ -68,7 +75,11 @@ impl<'de> Deserialize<'de> for IdlSnapshot {
// Now deserialize the full struct
let mut intermediate: IdlSnapshotIntermediate = serde_json::from_value(value)
.map_err(|e| DeError::custom(format!("Failed to deserialize IDL: {}", e)))?;
intermediate.discriminant_size = discriminant_size;
// Only use the heuristic if discriminant_size wasn't already present in the JSON
// (discriminant_size = 0 means it was absent / defaulted).
if intermediate.discriminant_size == 0 {
intermediate.discriminant_size = discriminant_size;
}
Comment on lines +78 to +82
Copy link

@greptile-apps greptile-apps bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 0 used as sentinel collides with a valid explicit value

The fix assumes intermediate.discriminant_size == 0 unambiguously means "the field was absent from the JSON". That holds because #[serde(default)] will produce 0 for a missing field, and 0 is not a meaningful discriminant size.

However, if a consumer ever serialises a snapshot with "discriminant_size": 0 (e.g. as an intentional sentinel they control, or simply by mistake), the heuristic will silently overwrite it — the same class of bug this PR is trying to fix.

A safer approach is to peek at the raw value (which is already available before the intermediate struct is constructed) to check whether the key is literally present:

let explicit_discriminant_size = value
    .get("discriminant_size")
    .and_then(|v| v.as_u64())
    .map(|n| n as usize);

// ...existing heuristic computation...

let mut intermediate: IdlSnapshotIntermediate = serde_json::from_value(value)
    .map_err(|e| DeError::custom(format!("Failed to deserialize IDL: {}", e)))?;

if explicit_discriminant_size.is_none() {
    intermediate.discriminant_size = discriminant_size;
}

This way the distinction between "key absent" and "key present with value 0" is unambiguous, and neither silently overwrites the other.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: hyperstack-idl/src/snapshot.rs
Line: 71-75

Comment:
**`0` used as sentinel collides with a valid explicit value**

The fix assumes `intermediate.discriminant_size == 0` unambiguously means "the field was absent from the JSON". That holds because `#[serde(default)]` will produce `0` for a missing field, and `0` is not a meaningful discriminant size.

However, if a consumer ever serialises a snapshot with `"discriminant_size": 0` (e.g. as an intentional sentinel they control, or simply by mistake), the heuristic will silently overwrite it — the same class of bug this PR is trying to fix.

A safer approach is to peek at the raw `value` (which is already available before the intermediate struct is constructed) to check whether the key is literally present:

```rust
let explicit_discriminant_size = value
    .get("discriminant_size")
    .and_then(|v| v.as_u64())
    .map(|n| n as usize);

// ...existing heuristic computation...

let mut intermediate: IdlSnapshotIntermediate = serde_json::from_value(value)
    .map_err(|e| DeError::custom(format!("Failed to deserialize IDL: {}", e)))?;

if explicit_discriminant_size.is_none() {
    intermediate.discriminant_size = discriminant_size;
}
```

This way the distinction between "key absent" and "key present with value 0" is unambiguous, and neither silently overwrites the other.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

Comment on lines +78 to +82
Copy link

@greptile-apps greptile-apps bot Mar 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 No test covers the fixed scenario

The existing test_ore_instructions_have_discriminators test only verifies the heuristic path (the ore.json fixture has no discriminant_size key, so the heuristic fires). There is no test that:

  1. Provides a JSON with Steel-style instructions (heuristic would yield 1) and an explicit "discriminant_size": 8.
  2. Asserts that 8 is preserved after deserialisation.

Without this, a future refactor could silently re-introduce the regression. Consider adding a unit test like:

#[test]
fn test_explicit_discriminant_size_is_preserved() {
    // Steel-style instructions (discriminant field, no discriminator),
    // but discriminant_size is explicitly overridden to 8.
    let json = r#"{
        "name": "my_program",
        "version": "0.1.0",
        "discriminant_size": 8,
        "accounts": [],
        "instructions": [{
            "name": "do_thing",
            "discriminant": {"value": 0},
            "accounts": [],
            "args": []
        }]
    }"#;

    let snapshot: IdlSnapshot = serde_json::from_str(json)
        .expect("should deserialize");

    assert_eq!(
        snapshot.discriminant_size, 8,
        "explicit discriminant_size should not be overridden by heuristic"
    );
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: hyperstack-idl/src/snapshot.rs
Line: 71-75

Comment:
**No test covers the fixed scenario**

The existing `test_ore_instructions_have_discriminators` test only verifies the heuristic path (the ore.json fixture has no `discriminant_size` key, so the heuristic fires). There is no test that:

1. Provides a JSON with Steel-style instructions (heuristic would yield `1`) **and** an explicit `"discriminant_size": 8`.
2. Asserts that `8` is preserved after deserialisation.

Without this, a future refactor could silently re-introduce the regression. Consider adding a unit test like:

```rust
#[test]
fn test_explicit_discriminant_size_is_preserved() {
    // Steel-style instructions (discriminant field, no discriminator),
    // but discriminant_size is explicitly overridden to 8.
    let json = r#"{
        "name": "my_program",
        "version": "0.1.0",
        "discriminant_size": 8,
        "accounts": [],
        "instructions": [{
            "name": "do_thing",
            "discriminant": {"value": 0},
            "accounts": [],
            "args": []
        }]
    }"#;

    let snapshot: IdlSnapshot = serde_json::from_str(json)
        .expect("should deserialize");

    assert_eq!(
        snapshot.discriminant_size, 8,
        "explicit discriminant_size should not be overridden by heuristic"
    );
}
```

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code


Ok(IdlSnapshot {
name: intermediate.name,
Expand Down
Loading