[wdolinar]

Fixes the ten CRITICAL items (C1–C10) in docs/reviews/2026-09-25-repo-review.md. C1–C8 each come with a regression test. C9 and C10 live in the pybind-only sources, which no test target builds, so they were checked by hand from Python (see follow-ups). The Doxygen for the changed functions is updated in the same commits.

#	Fix	Regression test
C1	Observer's default handlers no longer call themselves forever. They are empty, like the C++ base handlers.	observer_pyt.py::TestObserverDefaultHandlers
C2	ProgressListener.on_begin_operation_string no longer calls itself. It reports and returns the operation's 1-based stack index, the same one Progress passes to the later events.	progress_listener_pyt.py
C3	DaStreamReader::ReadBinaryBytes checks the lengths it reads from the file before using them, sizes its decode buffer for the longest decode, and fails on truncated data. iBase64Decode rejects input it can't decode safely (see below).	testReadBinaryBytesRejectsBadBlocks, testReadBinaryBytesReadsGoodBlock
C4	ReadBinaryBytes rejects a block bigger than the destination	testReadBinaryBytesRejectsBadBlocks
C5	SetBinaryBlockSize rejects a non-positive size, which made WriteBinaryBytes loop forever	testSetBinaryBlockSizeRejectsNonPositive
C6	stSplit without compression gives n + 1 fields for n delimiters; an empty source gives one empty field	new cases in testSplit
C7	XM_ENSURE_FALSE_T_5 expands to XM_ENSURE_TRUE_T_5, so it compiles	XmErrorUnitTests::test1
C8	Pt4's two operator< overloads compare z in the right direction	new cases in PtUnitTests::testIt
C9	IntPairFromPyIter returns the pair's values instead of (1, 1)	none, checked by hand from Python
C10	PublicProgressListener uninstalls its global listener when destroyed, so the listener can't call back into a deleted object	none, checked by hand from Python

Notes for reviewers

• Base64 decoding (C3). Two bugs in the old iBase64Decode sat under the C3 fix.
  • boost's transform_width compares only its underlying positions, and reads ahead before each output byte. When the input (with = padding stripped) is 1 mod 4 characters long, the decode loop never matches end and reads past the buffer.
  • A non-base64 character makes boost throw dataflow_exception out of a function that returns bool.
  • Both now log an error and make ReadBinaryBytes return false. Each bad input in the test must log the error from the check meant to reject it, so deleting a check fails the test.
• Behaviour changes callers can see:
  • ReadBinaryBytes returns false on a malformed block instead of overrunning its buffers or throwing. It logs an error for every rejection, and the header must start with BINARY_BLOCK.
  • ReadVecInt, ReadVecDbl and ReadVecPt3d return false when their binary block is bad. They used to return true with a zeroed or partly filled vector.
  • ProgressListener's begin_operation callback reports the operation's stack index (1 for the outermost) instead of 0.
  • PublicProgressListener and Listener can no longer be copied. A copy would have deleted their impl twice.
  • SetBinaryBlockSize(<= 0) asserts in debug and keeps the current size.
  • stSplit(..., false) now returns the last field. It used to drop it, e.g. "A,B" gave {"A"}.
• CHANGELOG isn't touched here. Happy to add an entry if wanted.

Testing

Run locally on Windows with MSVC 194 (the testing-dynamic-debug and python-py314-dynamic-release presets):

• CxxTest (Debug): Running cxxtest tests (73 tests)...OK!
• Python 3.14: Ran 30 tests ... OK
• xmsconan job lint: flake8 _package reports 0
• Mutation checks:
  • Reverting the C7 fix gives a compile error.
  • Reverting the C8 fix fails the new pt.cpp assertions.
  • The old C1/C2 code raises RecursionError.
  • The unfixed decoder crashes the test runner.
  • Deleting the 1-mod-4 length check, or the destination-size check, fails testReadBinaryBytesRejectsBadBlocks.
  • Making ReadVecInt ignore the ReadBinaryBytes result again, or dropping the BINARY_BLOCK check, fails the new tests.
  • Dropping the floor on the Python listener's depth fails test_unmatched_end_operation.

Follow-ups (not in this PR)

• C9/C10 tests: IntPairFromPyIter and PublicProgressListener are built only into the Python module, so neither CxxTest nor the Python suite can see the installed listener. Covering them needs a CxxTest target that builds the pybind sources, or a test-only binding for ProgressListener::GetListener.
• C10: a copy taken through GetListener(), or a listener destroyed from inside its own callback, can still be left dangling. Fixing that needs a way to detach the listener (overlaps M23).
• M12: update_message always reports stack index -1.
• Test helper: testSetBinaryBlockSizeRejectsNonPositive saves and restores xmAsserting() by hand. Sharing XmAssertScopedDisable would mean adding it to the shipped TestTools.h.
• Doc gaps: iUncompress has an empty \brief and a closing comment that says iCompress.

[wdolinar]

Review of PR #136 — 19 findings

Review: PR #136, fixes for C1–C10 from the 2026-09-25 repo review (fix/review-critical-items)

Verdict: REQUEST CHANGES (C=0 M=3 m=16)

What changed. 15 files, all on the one commit 9be33afb. The PR makes ten separate small fixes, each a few lines with tests alongside:

• Python bindings: observer.py and progress_listener.py now call super() instead of recursing.
• Stream I/O: in daStreamIo.cpp, iBase64Decode gets length and character checks, ReadBinaryBytes gets length and buffer checks, and SetBinaryBlockSize rejects non-positive sizes.
• Smaller changes: the stSplit rewrite, a single-token macro fix in XmError.h, a one-token comparison fix in pt.h, IntPairFromPyIter, and a new PublicProgressListener destructor.

The riskiest part is ReadBinaryBytes/iBase64Decode, which read lengths from untrusted files and use them with fixed-size buffers. The diff changes only Pt4's two operator< overloads, but the description says "Pt3d/Pt4d".

Notably, xmscore/dataio/daStreamIo.cpp:712-713 checks both lengths read from the file before any read, decode or decompress touches a buffer: the encoded length against the encode buffer, and the block length against the destination.

Validation dropped two MAJOR candidates: a refactor of the Pt4::operator< nesting, which predates this PR, and the missing test for the C9 IntPairFromPyIter fix. The missing C9 test still appears under Minor items.

Paths below are relative to C:/Users/bdolinar/dev/xms/xmscore/.claude/worktrees/repo-review-2026-09-25-5c1d82.

Major items

M1 [validated]: _package/xms/core/misc/progress_listener.py:61 — begin_operation callback always reports stack index 0, not the real one

        stack_index = super().on_begin_operation_string(operation)
        self.call_back(('begin_operation', stack_index, operation))
        return stack_index

Change: Report the same index that Progress uses for the later events. Two ways to do it:

• Change Listener::OnBeginOperationString (xmscore/python/misc/detail/Listener.cpp:122-124) to pass its computed m_operations.size() to the Python parent instead of throwing away the parent's return value.
• Or keep a depth counter in the Python class: add 1 on begin, subtract 1 on end.

Then make progress_listener_pyt.py check against a fixed expected index instead of echoing the method's own return value.

Why: super() reaches the C++ stub { return 0; } (xmscore/python/misc/PublicProgressListener.h:37), but Progress tags every later progress and end event with the real 1-based index (xmscore/misc/Progress.cpp:65,75,123). A begin event therefore can't be matched to its progress and end events, which breaks the documented stack_index contract (pydocs/source/getting_started.rst:81-83). Callers can hit this for the first time now, because the old code raised RecursionError.

M2 [validated]: xmscore/dataio/daStreamIo.cpp:201 — invalid base64 now makes ReadVecInt/Dbl/Pt3d silently return success

    try
    {
      for (auto it = base64Begin; it != base64End; ++it)
        *destIterator++ = *it;
    }
    catch (const dataflow_exception&)
    {
      XM_LOG(xmlog::error, "Unable to read file. Invalid base64 character.")
      return -1;
    }

Change: Return false from ReadVecInt (line 499), ReadVecDbl (521) and ReadVecPt3d (543) when ReadBinaryBytes fails, instead of ignoring its result. This is the PR's deferred M17 follow-up, and this change makes it necessary now rather than later.

Why: Before this PR, an invalid base64 character threw dataflow_exception out of the public readers. Now the catch returns -1, so ReadBinaryBytes returns false (725-726), and the readers still return true with a zeroed or partly filled vector. A corrupt file now loads "successfully". The validator confirmed this only for the invalid-character path. The other new rejections (708, 716, 722) used to overrun buffers rather than fail loudly, so they are no worse than before.

M3 [validated]: xmscore/python/misc/detail/PublicProgressListener.cpp:58 — C10 destructor fix has no automated test of either branch (severity disagreement: aquaveo-workflow:pr-coverage-reviewer rated MAJOR, aquaveo-workflow:writer-reviewer rated MINOR, xunit-patterns:test-smell-reviewer rated MINOR)

  ~impl()
  {
    // m_child points back at the parent being destroyed, so it must not stay
    // installed. Leave a listener installed by someone else alone.
    if (ProgressListener::GetListener() == m_child)
      ProgressListener::SetListener(BSHP<ProgressListener>());
  }

Change: Add a test that can see which listener is installed. Either bind a test hook for ProgressListener::GetListener to Python, or build PublicProgressListener into a CxxTest target. Create two listeners, destroy one, and check both branches: the destroyed listener is uninstalled, and a listener someone else installed stays in place. If no test is added, change the PR description, which claims every fix has a regression test but lists C10 as "checked from Python".

Why: This destructor is what stops the global listener from calling into a freed PublicProgressListener. Nothing checks it, so the use-after-free could come back and CI would still pass. The "leave another listener installed" branch never runs, because the Python suite creates only one ProgressListener. The validator judged the destructor logic itself correct; the gap is coverage.

Minor items

• _package/tests/unit_tests/observer_pyt.py:222 — test_observer_without_overrides has no assertion. It reaches 3 of the 6 handlers only because 0.5 clears the 0.02 threshold; fix: assertTrue(observer.progress_status(0.5)).
• _package/tests/unit_tests/observer_pyt.py:233 — observer.progress[-1] checks only the last value, so an extra dispatch would still pass; fix: assertEqual([0.5], observer.progress).
• _package/tests/unit_tests/progress_listener_pyt.py:9 — no committed test for C9 or C10, despite "each fix comes with a regression test"; fix: add a C++ test or soften the PR claim.
• _package/tests/unit_tests/progress_listener_pyt.py:19 — the expected tuple reuses the returned stack_index, so the index check always passes; fix: assert a fixed index. The specialist suggested 0, the base default, but per M1 that value is itself wrong.
• _package/xms/core/misc/observer.py:24 — the six super() pass-throughs avoid recursion only because pybind11's get_override checks the caller's frame, which is disabled on PyPy and has varied across CPython versions; fix: replace the pass-through bodies with pass.
• xmscore/dataio/daStreamIo.cpp:169 — iBase64Decode's destination-capacity rule is stated only in the Doxygen comment, unlike iCompress/iUncompress; fix: add a_destCapacity and return -1 if exceeded.
• xmscore/dataio/daStreamIo.cpp:705 — the header-read failure (705-709) and the short data read (721-722) return false without logging, so a truncated file leaves no trace; fix: XM_LOG an error before each return false.
• xmscore/dataio/daStreamIo.cpp:705 — blockString is never compared to "BINARY_BLOCK", so any word followed by two ints is accepted as a header; fix: reject when blockString != "BINARY_BLOCK".
• xmscore/dataio/daStreamIo.cpp:715 — XM_LOG(...); has a trailing semicolon, unlike the other XM_LOG calls in this file; fix: drop the semicolon.
• xmscore/dataio/daStreamIo.cpp:721 — no test reaches the new !NextLine() failure arms on their own. A valid final block at EOF with no trailing newline now returns false where it used to return true, and nothing tests that; fix: add EOF-without-newline cases to the bad-block test.
• xmscore/dataio/daStreamIo.cpp:2327 — testReadBinaryBytesRejectsBadBlocks mixes the bad-input table, the oversize-destination case and the good-read case in one test; fix: split the good-read case into its own test.
• xmscore/dataio/daStreamIo.cpp:2339 — magic numbers (74, 120, the goodData blob) repeat across about 13 table rows; fix: name constants and derive 74 from goodData.size().
• xmscore/dataio/daStreamIo.cpp:2368 — errors.find("") is always true, so the rows marked "rejected without logging" never check that nothing was logged; fix: assert errors.empty() when the expected error is empty.
• xmscore/dataio/daStreamIo.cpp:2406 — saving and restoring xmAsserting() by hand is not exception-safe and can leak to later tests; fix: share XmAssertScopedDisable through a test header and use it here.
• xmscore/misc/XmError.cpp:253 — iTest_XM_ENSURE_FALSE_T has conditional test logic (try/catch and if (a_assert)) copied from its sibling helpers; fix: none needed, it matches the existing pattern.
• xmscore/python/misc/detail/PublicProgressListener.cpp:54 — impl now has a destructor with a global side effect but still has the implicit copy operations; fix: = delete the copy constructor and copy assignment.

Additional 2 MINOR findings already covered above by higher-severity entries.

[wdolinar]

Pushed b3c4db4, which responds to the review above. I've updated the PR description to match it.

Fixed

• M1, m4: begin stack index. ProgressListener now tracks its own depth. on_begin_operation_string reports and returns 1 for the outermost operation and 2 for one nested inside it. on_end_operation steps the depth back. That is the index Progress passes to the later progress, end and update events. I kept the fix on the Python side so the bound virtual's signature stays the same. Listener already returns its own m_operations.size() to Progress, and nothing reads the parent's return value. The tests assert fixed indices: 1, then 1, 2, end 2, 2 for nested operations. The four "0 for first" Doxygen lines in Listener.cpp and getting_started.rst now say 1 for the outermost.
• M2: vector readers. ReadVecInt, ReadVecDbl and ReadVecPt3d return the result of ReadBinaryBytes. The new testReadBinaryVecRejectsBadBlock gives each one a block with * in the base64, and expects false plus the logged error. If the result is ignored again, that test fails. This also closes the M17 follow-up, which moves out of the follow-up list.
• m1, m2. assertTrue(observer.progress_status(0.5)) and assertEqual([0.5], observer.progress).
• m5. The six Observer defaults are now pass. The C++ base handlers are empty (PublicObserver.h:33-38), so this is equivalent and doesn't depend on the frame check. One correction to the premise: pybind11 doesn't disable the check on PyPy. It emulates it there in Python.
• m7, m8, m9. A header that fails to read or doesn't start with BINARY_BLOCK now logs "Invalid binary block header." A short data read logs "Binary block data is truncated." I also dropped the stray ;.
• m11, m12, m13.
  • The good-block case moved to its own test, testReadBinaryBytesReadsGoodBlock.
  • Rows that only change the data share a goodHeader constant. Rows that vary a header field still spell the header out, because that field is the input under test.
  • Every row now names the error it expects, so the find("") case is gone.
  • The oversize-destination case checks its log too.
• m16. PublicProgressListener::impl deletes its copy constructor and copy assignment.
• Summary: the C8 row now says Pt4's two operator< overloads.

Not changed

• M3, m3: C9/C10 tests. I agree there's no automated test. IntPairFromPyIter and PublicProgressListener are compiled only into the pybind module. No CxxTest target builds them, and nothing in Python can see which listener is installed. Adding either harness is bigger than this PR, so I corrected the description instead: C9 and C10 now say they were checked by hand, and the harness is listed as a follow-up.
• m6: a capacity argument for iBase64Decode. Its only caller bounds the input (daStreamIo.cpp:712) and sizes the buffer for the longest decode (:695-696) before calling it. iBase64Encode has the same contract. iCompress/iUncompress take a capacity because zlib does, not as a house rule.
• m10: a final block without a newline. That case still reads. At end of file, ReadLine sets only eofbit, and NextLine returns !fail(). testReadBinaryBytesReadsGoodBlock now pins this with an "at end of file" case. The header's !NextLine() arm is covered by a new row, "header ends the file". The arm after the data can't fail once the read has returned every byte, because the stream is still good and ReadLine at end of file sets only eofbit (daStreamIo.cpp:441-446). A short read is caught first by the gcount() check, and the truncated-data row covers that.
• m14: sharing XmAssertScopedDisable. The manual save/restore matches the idiom the downstream repos use. Sharing the class would mean adding it to the shipped TestTools.h, so I listed it as a follow-up instead.
• m15. No change, as the finding says.

Local runs:

• CxxTest (Debug): 73 tests OK.
• Python 3.14: 29 tests OK.
• xmsconan job lint: 0.
• Mutation check: making ReadVecInt ignore the ReadBinaryBytes result again, and dropping the BINARY_BLOCK check, fails 2 of the 73 C++ tests.

[wdolinar]

Pushed b962563, a follow-up to b3c4db4. A second self-review of that round found gaps:

• Stack-index docs. The public ProgressListener docs in Progress.h, and the test mock in Progress.cpp, still said the index is 0 for the first operation. They now say 1 for the outermost, which matches Listener, the mock and getting_started.rst. A class note adds that an operation begun before a listener was installed reports 0. The OnUpdateMessage briefs no longer say "operation ends".
• m16, corrected. Deleting the copy operations on PublicProgressListener::impl didn't cover the real hazard. PublicProgressListener and Listener each own a raw impl* and delete it in their destructors, so copying either one would delete it twice. Both now use XM_DISALLOW_COPY_AND_ASSIGN. Nothing in the repo copies them, and the pybind module still builds.
• Python listener. progress_listener.py documents that overrides of the begin and end handlers must call super(), and that the C++ listener ignores the begin return value. A new test, test_unmatched_end_operation, pins that an end with no matching begin leaves the next operation at 1. A Progress made before the listener was installed sends that unmatched end, with index 0.
• Vector-reader test. testReadBinaryVecRejectsBadBlock now checks the log after each reader. Before, one reader's message could hide another that failed for a different reason.

Local runs:

• CxxTest (Debug): 73 tests OK.
• Python 3.14: 30 tests OK.
• xmsconan job lint: 0.
• Mutation check: dropping the depth floor fails the new test.