fix(warp-core): preserve the legacy operation_result_id hash for updates

operation_result_id routed previous_value_digest through
hash_optional_id, which prepends a discriminant tag for both Some
and None. This silently changed EchoOperationResultIdV1 (and
everything chained from it -- preparation_id, receipt identities)
for the update path too, not only the new create path, contradicting
this branch's own "byte-for-byte unchanged" claims. A prior commit
corrected the ADR's wording to document this honestly rather than
overclaim; this commit fixes it for real instead, per Codex's
automated review on PR #686 and my own self-review's finding #1.

Some(digest) now hashes exactly digest -- the same raw 32 bytes the
pre-ADR-0024 code always wrote, no tag -- so the update path's
result identity is truly unchanged, not merely behaviorally
equivalent. None uses a new domain-separated marker
(RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN) whose byte length differs
from every possible 32-byte digest, so it cannot collide with
Some(digest) for any digest value -- unlike
current_application_basis's Option<Hash> field (which needed
hash_optional_id's shared tag scheme because that field's encoding
is otherwise fixed-width), this field's own two encodings already
differ in length, so no tag is needed here.

RED: operation_result_id_preserves_the_legacy_hash_for_updates (new
unit test in echo_operation.rs's own mod tests) asserted Some(...)
matches a hand-reconstructed reference hash and failed against the
tagged encoding. GREEN: same test passes after the fix. Updated ADR
0024's Verification Evidence Grade and Consequences sections to state
the digests are unchanged, not merely judged acceptable to differ.
Full warp-core suite (81 binaries, 687 library tests, doctests)
green; clippy identical pre-existing error set.

Author: flyingrobots

crates/warp-core/src/echo_operation.rs

@@ -73,6 +73,8 @@ const ATOM_VALUE_DOMAIN: &[u8] = b"echo:operation-atom-value:v1\0";
 const APPLICATION_BASIS_VALUE_DOMAIN: &[u8] = b"echo:operation-anchored-node-alpha-basis:v1\0";
 const APPLICATION_BASIS_ABSENT_DOMAIN: &[u8] =
     b"echo:operation-anchored-node-alpha-basis-absent:v1\0";
+const RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN: &[u8] =
+    b"echo:operation-result-absent-previous-value:v1\0";
 const FOOTPRINT_DIGEST_DOMAIN: &[u8] = b"echo:operation-footprint:v1\0";
 const RECEIPT_DIGEST_DOMAIN: &[u8] = b"echo:operation-receipt:v1\0";
 const COMPOSITION_DIGEST_DOMAIN: &[u8] = b"echo:operation-singleton-composition:v1\0";
@@ -3814,7 +3816,19 @@ fn operation_result_id(
     hasher.update(&profile_digest(RESULT_SCHEMA));
     hasher.update(invocation.node.warp_id.as_bytes());
     hasher.update(invocation.node.local_id.as_bytes());
-    hash_optional_id(&mut hasher, previous_value_digest);
+    // PR #686 review finding #5 (Codex + self-review): preserve the exact
+    // pre-ADR-0024 hash input for Some(...) -- a raw 32-byte digest, no
+    // discriminant tag -- so the update path's EchoOperationResultIdV1 is
+    // truly unchanged, not merely documented as an acceptable change.
+    // RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN's length differs from every
+    // possible digest's 32 bytes, so it cannot collide with any Some(digest)
+    // regardless of that digest's value -- no tag byte is needed here the
+    // way current_application_basis needs one for a fixed-width Option<Hash>
+    // field; this field's own two encodings already differ in length.
+    match previous_value_digest {
+        Some(digest) => hasher.update(&digest),
+        None => hasher.update(RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN),
+    };
     hasher.update(&replacement_value_digest);
     hasher.update(&patch_digest);
     EchoOperationResultIdV1(hasher.finalize().into())
@@ -4700,4 +4714,82 @@ mod tests {
             EchoOperationArtifactErrorKindV1::InvalidStructure
         );
     }
+
+    #[test]
+    fn operation_result_id_preserves_the_legacy_hash_for_updates() {
+        // PR #686 review finding #5 (Codex + self-review): `Some(...)` must
+        // hash exactly like the pre-ADR-0024 code did -- a raw 32-byte
+        // digest, no discriminant tag -- so EchoOperationResultIdV1 (and
+        // everything chained from it) is truly unchanged for the update
+        // path this PR did not intend to touch. `None` must be
+        // domain-separated by construction, not merely by convention: its
+        // marker has a different byte length than any 32-byte digest, so no
+        // `Some(digest)` can ever produce the same input stream as `None`
+        // regardless of which digest value is chosen.
+        let installed = retained_fixture_installation();
+        let node = node_key_from_bytes([20; 64]);
+        let invocation = EchoOperationInvocationV1::anchored_node_attachment_compare_and_set(
+            installed.package_id(),
+            installed.operation_coordinate(),
+            EchoOperationEvaluationBasisV1::new(
+                WriterHeadKey {
+                    worldline_id: crate::WorldlineId::from_bytes(digest(21)),
+                    head_id: crate::HeadId::from_bytes(digest(22)),
+                },
+                WorldlineTick::ZERO,
+                None,
+                digest(23),
+                digest(24),
+                EchoOperationApplicationBasisV1::new(digest(25), digest(26)),
+            ),
+            digest(27),
+            EchoOperationBudgetV1::new(8, 512, 512),
+            node,
+            Some(digest(28)),
+            Vec::new(),
+        );
+        let replacement_value_digest = digest(29);
+        let patch_digest = digest(30);
+        let previous = digest(28);
+
+        let mut expected = Hasher::new();
+        expected.update(RESULT_ID_DOMAIN);
+        expected.update(&installed.installed_operation_id.as_hash());
+        expected.update(&profile_digest(RESULT_SCHEMA));
+        expected.update(invocation.node.warp_id.as_bytes());
+        expected.update(invocation.node.local_id.as_bytes());
+        expected.update(&previous);
+        expected.update(&replacement_value_digest);
+        expected.update(&patch_digest);
+        let expected_some = EchoOperationResultIdV1(expected.finalize().into());
+
+        assert_eq!(
+            operation_result_id(
+                &installed,
+                &invocation,
+                Some(previous),
+                replacement_value_digest,
+                patch_digest,
+            ),
+            expected_some,
+            "Some(...) must hash identically to a plain untagged digest write"
+        );
+
+        assert_ne!(
+            RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN.len(),
+            32,
+            "the absent marker's length must differ from every possible digest length"
+        );
+        let none_result = operation_result_id(
+            &installed,
+            &invocation,
+            None,
+            replacement_value_digest,
+            patch_digest,
+        );
+        assert_ne!(
+            none_result, expected_some,
+            "None must not collide with any Some(digest) result"
+        );
+    }
 }

docs/adr/0024-anchored-node-creation-from-absence.md

@@ -176,18 +176,21 @@ All 13 pre-existing tests in that file pass unmodified in behavior (only
 mechanically rewrapped in `Some(...)` at call sites, never changed in intent).
 This evidences that `EchoOperationInvocationV1`'s canonical byte encoding and
 `prepare_operation_v1`'s/`current_application_basis`'s update-path _behavior_
-are unchanged for `Some(...)`. It does **not** evidence that every internal
-digest is byte-identical: `operation_result_id`'s hash input now goes through
-`hash_optional_id`, which adds a one-byte discriminant tag ahead of the
-32-byte digest for _both_ `Some` and `None`, not only `None`. This changes
-`EchoOperationResultIdV1` -- and everything chained from it, including
-`preparation_id` and receipt identities -- for the update path too, not only
-the new create path. This is judged acceptable, not a correctness defect: per
-the CHANGELOG.md entry for the executable-operation-runtime slice, no real
-committed result or receipt identity exists yet for this corridor to break.
-A future change that needs `operation_result_id`'s update-path output to stay
-byte-identical across a wire revision is a separate, explicit decision, not
-something this ADR's "unchanged" claim should be read to already cover.
+are unchanged for `Some(...)`. `operation_result_id` preserves this precisely
+at the digest level too: `Some(digest)` hashes exactly `digest` -- the same
+raw 32 bytes the pre-ADR-0024 code always wrote, no discriminant tag -- so
+`EchoOperationResultIdV1` and everything chained from it (`preparation_id`,
+receipt identities) are truly unchanged for the update path, not merely
+behaviorally equivalent. `None` is domain-separated by construction rather
+than by a shared tag scheme: its marker
+(`RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN`) has a different byte length than
+any 32-byte digest, so it cannot collide with `Some(digest)` for any digest
+value, without needing to add a tag to the `Some` case the way
+`current_application_basis`'s fixed-width `EchoOperationApplicationBasisV1`
+field does. `operation_result_id_preserves_the_legacy_hash_for_updates`
+(in `echo_operation.rs`'s own `mod tests`) asserts this directly against a
+hand-reconstructed reference hash, not only indirectly through pipeline
+tests passing.
 
 The full `warp-core` suite (81 test binaries, 686 library tests, doctests
 included) passes with this change; `cargo clippy --all-targets` reports the
@@ -256,17 +259,13 @@ generated client.
   dead code, not defense in depth.
 - The update path's _behavior_ -- which preconditions succeed, which refuse,
   and with which obstruction -- is unchanged, evidenced by the pre-existing
-  test suite passing without behavioral modification. Its digests are not
-  entirely unchanged, though: `operation_result_id` now hashes
-  `previous_value_digest` through `hash_optional_id`, which prepends a
-  discriminant tag for both `Some` and `None`. `EchoOperationResultIdV1` (and
-  everything chained from it) therefore differs from what it would have been
-  under the prior code, for the update path as well as the new create path.
-  `EchoOperationInvocationV1`'s own canonical bytes are unaffected for
-  `Some(...)` -- only this one internal hash's input layout changed. Judged
-  acceptable for the same no-real-cutover-yet reason as the invocation
-  encoding change above; a future decision that needs this specific digest to
-  stay byte-identical across the change is separate and not yet made.
+  test suite passing without behavioral modification. Its digests are
+  unchanged too: `operation_result_id` hashes `Some(digest)` as exactly
+  `digest`, the same raw 32 bytes the prior code always wrote, so
+  `EchoOperationResultIdV1` (and everything chained from it) is identical to
+  what the prior code would have produced. `None` is domain-separated by a
+  distinctly-sized marker rather than a shared tag, so it never needed to
+  touch the `Some` encoding to stay unambiguous.
 - Echo still creates nodes of only the package-declared, static
   `required_node_type`; it still contains no application-specific intrinsic
   and no knowledge of what any caller is building.