Review findings

[wdolinar]

CRITICAL findings (0)

(none)

MAJOR findings (11)

README.md:38 — unittest discover will discover zero tests as written

python -m unittest discover _package/tests

Change: All Python test files in this project use the *_pyt.py suffix (e.g. _package/tests/unit_tests/observer_pyt.py, filesystem_pyt.py), which unittest discover's default test*.py pattern will not match. Update the README to either python -m unittest discover -s _package/tests -p "*_pyt.py" or point at the actual CI invocation python build.py … from .github/workflows/XmsCore-CI.yaml:141.
Why: A new contributor follows the README, sees "Ran 0 tests", and concludes their environment works when no tests ran — exactly the silent-success failure mode the README is supposed to prevent.

xmscore/misc/XmLog.h:108 — ErrCount() \brief is factually wrong (severity disagreement: writer-reviewer rated MAJOR, silent-failure-reviewer rated MINOR)

/// \brief Number of error-level entries currently on the stackable error stack.
int ErrCount();

Change: Reword to "Number of stacked messages (info, warning, and error) currently held; debug entries are not stacked." XmLog.cpp:189-204 pushes every non-debug message onto m_stackedMessages, and ErrCount() returns m_stackedMessages.size() — info and warning are counted too.
Why: Callers writing if (log.ErrCount() == 0) thinking "no errors" will silently ignore real errors when warnings or info messages have been logged, because the documented contract doesn't match the implementation.

xmscore/misc/Observer.h:34 — ProgressStatus \brief mischaracterizes the return value (severity disagreement: writer-reviewer rated MAJOR, silent-failure-reviewer rated MINOR)

/// \brief Forward a percent-complete update to the listener; returns true to continue.
virtual bool ProgressStatus(double a_percentComplete);

Change: Reword to "Publish a percent-complete update; returns true if the underlying observer was actually notified (the value advanced enough since the last call), false when the call was suppressed." Observer.cpp:95-109 returns true only when the percent advanced more than 0.02 since the last update; false is not a stop signal and no caller treats it as one.
Why: A future caller reading "returns true to continue" will write if (!o.ProgressStatus(p)) abort(); and abort on every small step. The doc invents a cancel/continue contract the type does not have.

xmscore/misc/StringUtil.h:4 — \ingroup inconsistency splits StringUtil docs across two groups

// StringUtil.h moved to:
/// \ingroup misc
// but StringUtil.cpp:3 and StringUtil.t.h:5 still say:
/// \ingroup xmscore_misc

Change: Update xmscore/misc/StringUtil.cpp:3 and xmscore/misc/StringUtil.t.h:5 to \ingroup misc, then drop the new \defgroup xmscore_misc block from Doxygen/DoxygenMainpage.md. Audit the rest of xmscore/misc/* for the same pattern and migrate consistently.
Why: Doxygen will render declarations under misc and definitions/tests under xmscore_misc, splitting the rendered docs for one module across two groups. The mainpage's "see \ref misc for the public surface" workaround papers over the bug instead of fixing it.

Doxygen/DoxygenMainpage.md (Common Idioms section) — \ref XmscoreCommonIdioms is a dangling cross-reference (Flagged by: writer-reviewer, complexity-reviewer)

see [Common Idioms](@ref XmscoreCommonIdioms)

Change: Either add a Doxygen/CommonIdioms.md page with a {#XmscoreCommonIdioms} heading anchor, or remove the "Common Idioms" paragraph until that page lands. grep -rn XmscoreCommonIdioms returns no anchor anywhere in the repo.
Why: Doxygen will emit "unable to resolve reference" and the rendered link will be dead. Documentation that points at non-existent destinations is the doc analogue of dead code.

README.md:21-32 and Doxygen/DoxygenMainpage.md:32-49 — duplicated module-overview tables

| dataio | Stream IO helpers (daStreamIo) ... |   ← in both files, near-identical wording
| math   | ... |
| misc   | ... |

Change: Pick one source of truth. Keep the table in the Doxygen mainpage (since \ingroup taxonomy already lives there), and have README link to it (docs/index.html#XmscoreModules or similar) instead of re-stating the per-module descriptions. The README already links to the Doxygen site for C++ docs; extend that link.
Why: Two near-identical tables describing the same 7 modules will drift the moment any module's purpose is described differently in one place. Documentation that lies about itself is the smell this PR is supposed to prevent, not entrench.

pydocs/source/modules/filesystem/filesystem.rst:9-10 — documents silent-failure as a feature without enumerating it

their value is in normalizing behavior across the XMS suite (consistent
handling of relative paths, trailing-separator differences between Windows
and POSIX, and silent failure modes used by callers that cannot afford to raise).

Change: Drop the "silent failure modes used by callers that cannot afford to raise" clause. Replace with per-function docstrings in _package/xms/core/filesystem/filesystem.py that name the specific exception classes intentionally swallowed (PermissionError, OSError, FileNotFoundError, ValueError) and the sentinel each function returns. Better still, address the underlying broad-except Exception handlers (lines 35-36, 71-74, 83-86, 101-110, 125-130, 147-150) and stop swallowing.
Why: This PR newly publishes xms.core.filesystem.filesystem via automodule and tells future consumers that silent-failure is a designed feature without naming which functions silently fail or what they hide. Documentation that legitimizes broad-except patterns ensures the bugs become permanent.

xmscore/misc/Singleton.h:25-69 — Singleton<T> documented as CRTP base, but API conflates get/install/destroy and returns dangling reference

/// \brief CRTP Singleton base class used by xmscore singletons
template <class T>
class Singleton {
  static T& Instance(bool a_delete, T* a_new) {
    ...
    return *theSingleInstance.get();   // when a_delete=true: dereferences empty shared_ptr
  }
};

Change: Split into Instance() / SetInstance(std::unique_ptr<T>) / Reset(). The destruction path must return void or bool, never a dangling T&. Mark the legacy combined Instance(bool, T*) [[deprecated]]. The new \brief should not freeze "CRTP Singleton" as the documented contract until the type actually behaves like one.
Why: One Instance call returning a reference to a destroyed shared_ptr's payload is undefined behavior, and the new \brief invites future xmscore singletons to keep using the broken shape. The doc cements the bug.

xmscore/misc/Singleton.h:81-121 — SharedSingleton<T> same conflated API plus silent install-clobber and no thread-safety

/// \brief CRTP Singleton base class used by xmscore singletons
template <class T>
class SharedSingleton {
  static T& Instance(Ptr a_new = Ptr(), bool a_delete = false) {
    // commented-out assert — preconditions not enforced
  }
};

Change: Replace with explicit Get() / Install(Ptr) / Reset() methods. Document thread-safety expectations or guard with std::call_once. If two callers race to install, the second silently loses; that needs to either fail loudly or be made thread-safe.
Why: Log subsystems and other foundation singletons commonly have this race. The new doc names the type as the canonical CRTP base for xmscore singletons, encouraging more consumers to inherit a contract the type can't keep.

xmscore/misc/Progress.h:23-39 — Progress documented as RAII but rule-of-five is broken (default copy ctor causes double-pop)

/// \brief RAII helper that reports progress for a single named operation
class Progress {
public:
  Progress(...);
  ~Progress();   // pops the progress stack
private:
  int m_stackIndex;   // copyable by default
};

Change: Progress(const Progress&) = delete; Progress& operator=(const Progress&) = delete; (and either delete or define the move pair). Or apply rule-of-zero by holding std::unique_ptr<Impl> so the impl owns the stack slot.
Why: The default-generated copy ctor duplicates m_stackIndex; both copies pop on destruction → double-pop on the listener stack. The new \brief freezes RAII as the documented contract while the type silently breaks it.

xmscore/misc/Progress.h:43-69 — ProgressListener global-state API freezes "process-wide" without enforcing the invariant

/// \brief Install the process-wide ProgressListener; pass null to clear.
static void SetListener(BSHP<ProgressListener>);
static BSHP<ProgressListener> GetListener();

Change: Document (and enforce) what happens to in-flight Progress objects when SetListener is called mid-flight — either reject the swap while a stack is non-empty, or have Progress capture the listener by weak_ptr at construction so a destructor pop targets the listener that allocated the stack slot.
Why: A Progress constructed against listener A and destructed after SetListener(B) will pop the wrong stack. The new doc commits to "process-wide" semantics that the type doesn't actually maintain across listener swaps.

MINOR findings (15)

• Commit a9cad92 — no DESIGN/EVIDENCE marker; doc-restructure should carry DESIGN, and a docs PR is exactly where EVIDENCE ("rebuilt Doxygen and Sphinx; no broken refs") is most valuable.
• xmscore/misc/XmLog.h — out-of-scope deletion of commented-out MessageTypeStr block; not a \brief/\ingroup change and not mentioned in PR body.
• pydocs/source/getting_started.rst (ProgressListener example) — example builds the listener but never installs it; readers following the example will see no events.
• xmscore/dataio/daStreamIo.h:36 — \brief for ReadNamedLine says "consisting only of the given name token" but impl matches the first whitespace-separated token; trailing content is consumed, not rejected.
• xmscore/misc/StringUtil.h:73 (stMakeUnique) — brief loses the specific suffix shape (N) that callers parse back out.
• Doxygen/DoxygenMainpage.md / README.md — module list mentions xmsmesh and xmsstamper; workspace CLAUDE.md lists xmsmesher and does not mention xmsstamper. Confirm canonical product names before publishing on the front page.
• Doxygen/DoxygenMainpage.md:122-123 — xmscore_misc group documented as legacy; complete the migration of remaining xmscore/misc/*.h to \ingroup misc and drop the legacy \defgroup rather than formalizing it as a permanent fixture.
• xmscore/dataio/daStreamIo.h:171-201 — 18 free-function \brief lines each saying "Free-function form of DaStreamReader::ReadX"; replace with one section header comment.
• xmscore/misc/StringUtil.h:983-998 — STRstd<T> \brief paragraph explains overload-resolution traps; signal that the four-overload set could become one specialized template (FOR FOLLOW-UP).
• .github/workflows/XmsCore-CI.yaml — no Sphinx or Doxygen build step; this PR adds substantial doc surface with no CI gate to catch a future broken \ref, malformed RST, or automodule typo. Add a docs job running make html SPHINXOPTS='-W --keep-going' and doxygen with WARN_AS_ERROR = YES.
• xmscore/misc/Observer.h — BSHP<impl> should be std::unique_ptr<impl>; no copy/move declarations on a polymorphic base means slicing risk.
• xmscore/misc/StringUtil.h:54 — static const char* ST_WHITESPACE at namespace scope in a header gives every TU its own copy; use inline constexpr const char ST_WHITESPACE[].
• xmscore/misc/XmLog.h:112-135 — ErrCount() and LogFilename() are pure inspectors but not const; also boost::scoped_ptr<Impl> should be std::unique_ptr<Impl>.
• xmscore/testing/TestTools.h:201-216 — GetDefault, GetSkipping, DefaultValWasSet are pure inspectors but not const; cannot be called on a const ETestMessagingState&.
• xmscore/dataio/daStreamIo.h:35-119 — class \brief doesn't express that the bound std::istream& must outlive the reader; add a \note about the lifetime requirement.

Additional 2 MINOR finding(s) already covered above by higher-severity entries.

Specialist summary

  Specialist                    C   M   m   Verdict
  writer-reviewer               0   5   6   REQUEST CHANGES
  silent-failure-reviewer       0   1   2   REQUEST CHANGES
  complexity-reviewer           0   2   3   REQUEST CHANGES
  pr-coverage-reviewer          0   0   1   APPROVE
  type-design-reviewer (cpp)    0   4   5   REQUEST CHANGES
  type-design-reviewer (py)     0   0   0   APPROVE
  ─────────────────────────────────────────────────
  Aggregate (pre-dedup)         0  12  17
  Aggregate (deduped rollup)    0  11  15   REQUEST CHANGES

Skipped (project marker absent): arch-reviewer (no check_architecture.py), ddd-reviewer (no domain/)
Skipped (no matching files): cmake-structure-reviewer, image-baseline-reviewer, test-reviewer, test-smell-reviewer

Per-specialist details

writer-reviewer

REVIEW: correctness=PASS information_hiding=PASS cohesion=PASS architecture=PASS discipline=FAIL — 5 MAJOR (unittest command, ErrCount brief, ProgressStatus brief, StringUtil \ingroup split, dangling XmscoreCommonIdioms ref), 6 MINOR. Verdict: REQUEST CHANGES.

silent-failure-reviewer

REVIEW: empty_handlers=PASS broad_catches=PASS swallow_continue=PASS fallback_chains=PASS return_erasure=PASS — 1 MAJOR (filesystem.rst legitimizes silent-failure), 2 MINOR (rated MINOR by silent-failure but the writer-reviewer rated the same items MAJOR; rolled up to MAJOR per dedup rule). Verdict: REQUEST CHANGES.

complexity-reviewer

REVIEW: nesting_depth=PASS function_length=PASS parameter_list=PASS duplication=FAIL cohesion=FAIL — 2 MAJOR (README/mainpage table duplication, dangling XmscoreCommonIdioms ref), 3 MINOR. Verdict: REQUEST CHANGES.

pr-coverage-reviewer

REVIEW: critical_paths=PASS business_logic=PASS edge_cases=PASS new_branches=PASS convention_lookup=PASS — 0 MAJOR, 1 MINOR (no docs-build CI gate). Verdict: APPROVE. Confirms PR is documentation-only with no production-logic changes.

type-design-reviewer (cpp)

REVIEW: encapsulation=FAIL invariant_expression=PASS usefulness=PASS enforcement=FAIL cpp_idioms=FAIL (enc=3, inv=4, use=6, enf=3) — 4 MAJOR (Singleton, SharedSingleton, Progress RAII, ProgressListener globals), 5 MINOR. Verdict: REQUEST CHANGES. Reviewer notes: type-design issues predate this PR; the doc-only diff itself can stand on its own merits, but the new \brief lines freeze contracts that the underlying types do not enforce.

type-design-reviewer (py)

REVIEW: encapsulation=PASS invariant_expression=PASS usefulness=PASS enforcement=PASS python_idioms=PASS — 0 findings. Only Python change is pydocs/source/conf.py adding one string to MOCK_MODULES; nothing in scope. Verdict: APPROVE.

---

Aggregate Verdict: REQUEST CHANGES

Recommended sequencing if the writer wants to ship the docs first: fix the five doc-correctness MAJORs (#1, #2, #3, #4, #5), the duplicated table (#6), and the silent-failure rst paragraph (#7) in this PR. The four C++ type-design MAJORs (#8–#11) are pre-existing structural issues that this PR's \brief lines merely freeze; file them as follow-ups against the underlying types and merge the docs once the seven doc-shaped MAJORs land.