Skip to content

[Reborn] Centralize snippet display hashing - #3507

Merged
serrrfirat merged 3 commits into
reborn-integrationfrom
reborn/3476-snippet-hash
May 13, 2026
Merged

serrrfirat merged 3 commits into
reborn-integrationfrom
reborn/3476-snippet-hash

Conversation

@serrrfirat

Copy link
Copy Markdown
Collaborator

Summary

  • add a shared run-profile helper for opaque model-visible snippet display hashes
  • use it for skill snippet model-message refs and memory snippet refs
  • remove duplicated FNV-1a hash loops from call sites

Scope

Follow-up to #3476/#3470. Addresses the #3492 comment item: snippet-ref hash derivation should not be duplicated across prompt/context paths.

Verification

  • CARGO_TARGET_DIR=/Users/firatsertgoz/Documents/ironclaw/target cargo test -p ironclaw_turns -p ironclaw_host_runtime
  • CARGO_TARGET_DIR=/Users/firatsertgoz/Documents/ironclaw/target cargo test -p ironclaw_architecture

Does not close #3492.

@github-actions github-actions Bot added size: M 50-199 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: core 20+ merged PRs labels May 11, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request centralizes the FNV-1a hashing logic for model-visible snippet references into a new snippet_ref module, replacing duplicate implementations in memory_context.rs and skill_context.rs. Feedback indicates that the new centralized helper introduces a subtle change in hash calculation by appending a trailing field separator, which may break backward compatibility for existing or persisted LoopMessageRef strings.

Comment on lines +409 to +410
let ordinal = ordinal.to_string();
let hash = stable_snippet_display_hash([snippet_ref, safe_summary, &ordinal]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The centralization of the hashing logic here introduces a subtle change in the resulting hash value for skill snippets. The previous implementation (stable_snippet_ref_hash) did not include a trailing field separator after the final ordinal field, whereas the new stable_snippet_display_hash appends a separator after every field in the iterator.

While these hashes are documented as opaque display identifiers, this change will break any existing or persisted LoopMessageRef strings that were generated using the old logic. If backward compatibility for transcript resolution or in-flight runs is required, this should be addressed by either adjusting the centralized helper or explicitly handling the trailing separator in the call site.

References
  1. Prioritize backward compatibility for existing data and logic when modifying centralized helpers or established formats.

@serrrfirat

Copy link
Copy Markdown
Collaborator Author

Paranoid review found a Medium issue at head 52d38d2181d6cf45d4d3ccc720ba5dfb966eba7c:

# Severity Confidence Category File:Line Finding
1 Medium Certain Correctness / compatibility crates/ironclaw_turns/src/run_profile/skill_context.rs:410 skill_snippet_model_message_ref now hashes [snippet_ref, safe_summary, ordinal] through stable_snippet_display_hash, whose implementation appends 0xFF after every field (snippet_ref.rs:16-21). Old skill refs used separators only between fields (snippet_ref, safe_summary, ordinal) with no trailing separator. Concrete vector: old skill_snippet_model_message_ref("skill:alpha", "summary", 0) hash was 6e54cb74d742607c; new hash is bc763a89c5c9fe99. Any persisted/precomputed LoopModelMessage ref from before this PR will still look like msg:snippet.* but HostManagedModelPort::instruction_snippet_messages_by_ref will recompute only new refs, miss old refs, then fail model resolution with model message reference is unavailable. Memory snippet refs keep old behavior because their previous helper already appended a separator after each field. Fix/test: preserve old skill hash semantics while still centralizing feed logic, e.g. shared private FNV feed plus explicit separator mode (BetweenFields for skill refs, AfterEachField for memory refs), or otherwise version/migrate snippet refs intentionally. Add exact compatibility tests for both skill_snippet_model_message_ref("skill:alpha", "summary", 0) and a memory-path hash vector so future centralization cannot silently change either protocol.

@serrrfirat
serrrfirat merged commit 90861e4 into reborn-integration May 13, 2026
15 checks passed
@serrrfirat
serrrfirat deleted the reborn/3476-snippet-hash branch May 13, 2026 10:42
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
* fix(reborn): centralize snippet display hashing

* fix(reborn): preserve skill snippet display refs

* fix(reborn): harden snippet display ref API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: low Changes to docs, tests, or low-risk modules size: M 50-199 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant