Skip to content

Conversation

@EvilBeaver
Copy link
Owner

@EvilBeaver EvilBeaver commented Dec 18, 2025

closes #1637

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Read-only streams now properly reject write operations and display appropriate error messages.
    • Stream resizing is now restricted for read-only stream instances.
  • Tests

    • Added comprehensive tests validating read-only stream functionality and write operation prevention.
    • Added tests ensuring proper data and position separation between read-only and writable stream instances.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 18, 2025

Walkthrough

The changes add runtime guards to prevent writing operations on read-only streams. The GenericStream.cs file now validates stream writability in Write and SetSize methods, throwing a RuntimeException when modifications are attempted on non-writable streams. Corresponding test procedures validate this read-only behavior and data isolation.

Changes

Cohort / File(s) Summary
Stream Write Protection
src/OneScript.StandardLibrary/Binary/GenericStream.cs
Added runtime guard clauses in Write and SetSize methods to prevent writing when stream is not writable; throws RuntimeException with Russian message and English description. Introduced using directive for OneScript.Exceptions.
Read-Only Stream Tests
tests/binary-objects.os
Added two exported test procedures: ТестДолжен_ПроверитьЧтоПолучитьПотокТолькоДляЧтенияЗапрещаетЗапись() and ТестДолжен_ПроверитьЧтоПолучитьПотокТолькоДляЧтенияРазделяетДанныеИПозицию() to validate read-only streams reject writes and maintain data isolation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Review the exception guard clauses in Write and SetSize for correct writability property checks
  • Verify no accidental duplication of test procedures in the test file
  • Validate consistency between Russian and English exception messages align with issue #1637 requirements

Poem

🐰 A stream stood too open, inviting writes through,
But now our guards block them—read-only proves true!
With exceptions thrown on each scripting attempt,
Protected from chaos, our data's well-kept! 📚✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly references issue #1637 and describes the main change: fixing a bug where read-only streams permitted write operations.
Linked Issues check ✅ Passed The PR adds runtime guards to prevent writes to read-only streams and includes comprehensive tests validating the expected exception behavior [#1637].
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the read-only stream write protection issue described in #1637; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/1637-readonlystream

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f46f06 and af962b1.

📒 Files selected for processing (2)
  • src/OneScript.StandardLibrary/Binary/GenericStream.cs (3 hunks)
  • tests/binary-objects.os (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)

Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8

Files:

  • src/OneScript.StandardLibrary/Binary/GenericStream.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1557
File: src/VSCode.DebugAdapter/Transport/RpcOperationException.cs:11-11
Timestamp: 2025-07-18T12:25:24.078Z
Learning: EvilBeaver prefers to preserve original namespaces for types involved in binary serialization or cross-version compatibility, even if the type is not directly serialized, to maintain overall system compatibility.
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1557
File: src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs:60-0
Timestamp: 2025-07-16T12:44:12.167Z
Learning: EvilBeaver prefers fail-fast error handling in debugging contexts - when stream operations fail in debug client factories, exceptions should be thrown to terminate the process rather than handled gracefully with fallback mechanisms.
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.

Applied to files:

  • src/OneScript.StandardLibrary/Binary/GenericStream.cs
📚 Learning: 2025-12-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Ignore errors from http.os test file and the test named 'Тест_ДолженПроверитьЧтоПеремещениеФайлаРаботаетПоHTTP' when reviewing BSL test results

Applied to files:

  • tests/binary-objects.os
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Binary/GenericStream.cs (2)
src/OneScript.Core/Contexts/Internal/IBuildableMember.cs (1)
  • CanWrite (32-32)
src/OneScript.StandardLibrary/Json/JSONWriter.cs (2)
  • RuntimeException (489-493)
  • RuntimeException (495-499)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: SonarQube
🔇 Additional comments (6)
src/OneScript.StandardLibrary/Binary/GenericStream.cs (3)

11-11: LGTM! Necessary import for RuntimeException.

The using directive is required for the RuntimeException instances thrown in the Write and SetSize guards below.


139-140: LGTM! Correct guard prevents writing to read-only streams.

The guard correctly checks CanWrite (which considers both _isReadOnly and underlying stream writability) and throws an appropriate exception matching the expected behavior from issue #1637. The bilingual error message follows the pattern established in other parts of the codebase.


277-278: LGTM! Consistent guard prevents resizing read-only streams.

The guard correctly prevents resizing read-only streams, which is conceptually a write operation. The implementation is consistent with the Write method guard and uses the same appropriate error message.

tests/binary-objects.os (3)

32-33: LGTM! Test registration follows conventions.

The two new tests are correctly registered in the test list with names that follow the existing naming convention.


330-360: LGTM! Comprehensive test validates write prohibition.

This test thoroughly validates the fix for issue #1637 by:

  • Verifying ДоступнаЗапись returns False for read-only streams
  • Confirming Записать (Write) throws the expected exception
  • Confirming УстановитьРазмер (SetSize) throws the expected exception

The test directly addresses the reported issue and validates the expected behavior.


362-434: LGTM! Excellent test validates data and position sharing.

This comprehensive test validates that read-only streams correctly share both data and position with the original stream:

  • Verifies position changes in one stream are reflected in the other
  • Confirms data written to the main stream is visible when reading from the read-only stream
  • Tests both reading and seeking operations affect shared position

This is critical for ensuring the read-only wrapper doesn't break the expected behavior of shared underlying data.


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

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

@sonar-openbsl-ru-qa-bot
Copy link

@EvilBeaver EvilBeaver merged commit 80e0162 into develop Dec 19, 2025
5 checks passed
@EvilBeaver EvilBeaver deleted the feature/1637-readonlystream branch December 19, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants