Make stream representation deterministic

[ccamel]

Fixes #14 : make stream representation deterministic by replacing pointer-based output with IDs, provided by an atomic counter that assigns unique IDs to every stream at creation time.

Before: <stream>(0x140001d2210) (pointer address - non-deterministic - very bad)
After: <stream>(0x1) (deterministic - very good)

This is of course critical in the Axone blockchain context where deterministic behavior is essential for consensus.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes introduce a global stream ID counter that assigns unique numeric identifiers to each Stream instance, replacing pointer-based identity. Stream comparison and WriteTerm output now use these IDs, with a new exported ResetStreamIDCounter() function for test determinism. Tests are updated across three files to accommodate the new ID system and refactor error handling patterns.

Changes

Cohort / File(s)	Summary
Stream ID Counter Infrastructure engine/stream.go	Introduces global streamIDCounter with nextStreamID() helper and exported ResetStreamIDCounter() function. Each Stream gets a unique numeric id field. WriteTerm output prefers alias, otherwise prints id in hex. Stream comparison uses id field instead of pointer identity. Stream constructors initialize id field with unique values.
Stream Tests engine/stream_test.go	Tests updated to call ResetStreamIDCounter() for deterministic IDs starting from 1. Test expectations now verify id and streamType fields. TestStream_WriteTerm refactored with prepare callback for VM state setup. TestStream_Compare initializes stream array with explicit IDs. Test structure improved to handle dynamic stream aliasing via VM registry insertion.
PutChar Test Error Handling engine/builtin_test.go	Test table refactored from static error values to dynamic error factory functions (func(Term) error). Error factories invoked with runtime stream/alias term (sOrA) during validation. Stream ID counter reset added at TestPutChar start. All test cases updated to provide error factories instead of plain errors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25–30 minutes

• engine/stream.go: Global state management (streamIDCounter) and changes to core Stream behavior (comparison, ID assignment, output formatting) require verification of determinism and pointer-identity replacement implications
• engine/stream_test.go: Widespread test updates following consistent patterns; verify ResetStreamIDCounter() is called appropriately for test isolation and that dynamic VM setup doesn't introduce flakiness
• engine/builtin_test.go: Error factory pattern applied across multiple test cases; ensure error comparison logic correctly handles the new closure-based approach

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Make stream representation deterministic' directly reflects the main objective of the changeset, which replaces pointer-based stream output with deterministic ID-based representation.
Description check	✅ Passed	The PR description clearly describes the changeset: replacing pointer-based stream output with deterministic IDs using an atomic counter.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   98.05%   98.27%   +0.21%     
==========================================
  Files          24       24              
  Lines        7917     7053     -864     
==========================================
- Hits         7763     6931     -832     
+ Misses        122       90      -32     
  Partials       32       32              

Files with missing lines	Coverage Δ
engine/stream.go	100.00% <100.00%> (ø)

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

engine/stream.go (2)

13-31: Global stream ID counter and Reset API look sound; consider constraining Reset to tests

The atomic counter and nextStreamID()/ResetStreamIDCounter() implementation are correct and safe for concurrent stream creation. The only concern is that ResetStreamIDCounter mutates global ordering and seems to exist purely for test determinism; exporting it makes it easy for callers to reset IDs in production and silently change comparison/log ordering.

If you want to keep the public surface minimal and avoid surprising global state changes, consider either:

• Making ResetStreamIDCounter unexported and calling it only from tests in the same package, or
• Leaving it exported but documenting explicitly that it is intended for testing and should not be used in normal code paths.

Also applies to: 33-37

---

52-62: Deterministic IDs in constructors, WriteTerm, and Compare align with the PR goal

Assigning id via nextStreamID() in all New*Stream constructors, printing <stream>(alias) when alias is set, and ordering streams by id in Compare give you stable, deterministic behavior and remove pointer-address dependence. This matches the stated objective and keeps CompareAtomic semantics intact.

One nuance: Streams constructed manually as &Stream{...} (without going through New*Stream) will keep the default id == 0 and thus all compare equal among themselves and sort before any constructor-created stream. That’s still deterministic and consistent, but if you ever rely on uniqueness for such manually created streams, you may want to either:

• Route all production code through the constructors, or
• Lazily assign an ID in WriteTerm/Compare when id == 0.

Given current usage, this can remain as-is, just something to be aware of.

Also applies to: 64-98, 100-107, 110-122

engine/builtin_test.go (1)

4454-4529: PutChar tests correctly handle stream-dependent errors with the new factory pattern

The refactor of TestPutChar to:

• Reset the global stream ID counter once at the start, and
• Use streamOrAlias func() (Term, func(*testing.T)) plus err func(Term) error

is a solid way to keep tests deterministic while still building expected errors from the actual sOrA term passed to PutChar. This avoids brittle coupling to internal details of how streams are represented while still verifying the exact Exception structure.

If you want to make intent slightly clearer, you could name the Term parameter of error factories _ Term in cases where it’s ignored (e.g., simple InstantiationError(nil)), but the current implementation is functionally fine.

engine/stream_test.go (2)

16-26: Constructor tests updated for deterministic IDs and stream types look correct

Calling ResetStreamIDCounter() at the start of each constructor test and asserting id: 1 plus the appropriate mode, eofAction, and streamType matches the new contract of the New*Stream functions. This gives strong coverage that each constructor wires fields correctly and no longer relies on pointer addresses.

If you want slightly stronger guards, you could also assert reposition: false explicitly in these expected structs, but that’s optional since the zero value already enforces it.

Also applies to: 28-38, 40-62

---

64-97: Stream.WriteTerm tests validate ID and alias behavior; consider simplifying the “registered” setup

The updated TestStream_WriteTerm:

• Resets the ID counter so the first two NewInputTextStream(nil) calls get IDs 1 and 2, and
• Checks that streams without aliases render as <stream>(0x1)/<stream>(0x2) while an aliased stream renders as <stream>(foo),

accurately captures the new WriteTerm behavior.

Note that in the "registered" case, prepare adds the stream to a local VM’s streams set but never assigns s.vm, and WriteTerm does not consult vm at all. As a result, that registration work has no observable effect on the assertion; the case effectively just verifies the second auto-assigned ID.

You could either:

• Drop the VM registration from "registered" to keep the test minimal, or
• If the intent is to cover VM/stream integration, also set s.vm = &vm and assert on VM state separately.

Functionally it’s fine as-is; this is mostly about avoiding implied dependencies that aren’t actually exercised.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 051a282 and 70ab2eb.

📒 Files selected for processing (3)

• engine/builtin_test.go (3 hunks)
• engine/stream.go (7 hunks)
• engine/stream_test.go (5 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2024-11-24T08:47:15.195Z

Learnt from: ccamel
Repo: axone-protocol/prolog PR: 21
File: engine/builtin_test.go:40-45
Timestamp: 2024-11-24T08:47:15.195Z
Learning: In `engine/builtin_test.go`, the function `do_not_call_misc_error` intentionally panics with a non-string, non-error value (e.g., `panic(42)`) as part of the test. Do not suggest changing this code to panic with an error value.

Applied to files:

• engine/builtin_test.go

🧬 Code graph analysis (2)

engine/stream.go (4)

engine/vm.go (1)

• VM (122-152)

engine/builtin.go (1)

• Compare (883-906)

engine/term.go (2)

• Term (12-15)
• CompareAtomic (95-106)

engine/env.go (1)

• Env (27-32)

engine/builtin_test.go (4)

engine/stream.go (2)

• ResetStreamIDCounter (29-31)
• NewOutputTextStream (77-86)

engine/atom.go (1)

• NewAtom (204-206)

engine/exception.go (1)

• InstantiationError (33-35)

engine/variable.go (1)

• NewVariable (30-38)

🔇 Additional comments (1)

engine/stream_test.go (1)

99-121: Stream.Compare tests now align with id-based ordering

Initializing ss with explicit id values {1, 2, 3} and using those in the comparison table ties the expectations directly to the new Stream.Compare semantics (ordering by id via CompareAtomic). The test matrix (s > X, s = s, s < s, etc.) still exercises the same ordering cases and remains valid under the new implementation.