Skip to content

Introduce Spectral Sentinel — an online subspace anomaly detector - #827

Merged
da2ce7 merged 92 commits into
torrust:developfrom
da2ce7:20260219_sentinel
Sep 20, 2026
Merged

da2ce7 merged 92 commits into
torrust:developfrom
da2ce7:20260219_sentinel

Conversation

@da2ce7

@da2ce7 da2ce7 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

A sentinel is not a judge. It stands watch, keeps its bearings, and reports what has changed.

That is the idea behind Spectral Sentinel. It does not decide whether a pattern is dangerous, important, or actionable. It measures structure in a stream, learns what has been ordinary so far, and returns statistical readouts when new observations depart from the learned geometry.

The "spectral" part is literal: each selected region is modelled with low-rank subspace trackers, and a second tier models the spectrum of those scores across related cells. The "sentinel" part is restraint: the crate observes, scores, and reports, but policy stays with the host.


Summary

This PR adds torrust-sentinel to the workspace: a library crate for hierarchical online subspace anomaly detection over positionally structured observation streams. It is rebuilt as a single commit on the current develop (after #884), so it sits on the MSRV-1.90 floor of ADR-T-011 (raised on develop after this branch was first cut, and now published by the package README) and follows the per-crate versioning of ADR-T-012.

It is built on top of Mudlark. Mudlark provides the adaptive spatial substrate — regions that receive more observation volume earn finer resolution, quiet regions remain coarse. Spectral Sentinel uses that structure to decide where statistical trackers are worth maintaining. It selects significant V-Tree entries, closes them under G-tree ancestry so every selected cell has a complete ancestor chain back to the root, and scores incoming batches against learned subspace models at every selected scale.

The simplest way to think about it: Mudlark decides where the stream has shape; Sentinel measures whether the recent shape still looks like what that region has learned to expect.

The crate is deliberately policy-free. Reports carry raw measurements — four scoring axes (novelty, displacement, surprise, coherence), maturity, baselines, CUSUM drift accumulators, geometry, contour summaries, and health snapshots. They do not encode threat levels, recommended actions, or decisions. The host reads the measurements and decides what they mean.

The core invariant is feed-forward: every input value updates Mudlark with exactly one unit of observation volume. Anomaly scores never flow back into the spatial index. That keeps spatial adaptation driven by traffic structure, not by the detector's own conclusions. Temporal policy is host-controlled: Sentinel never applies decay automatically.

A second analysis tier — coordination trackers — runs at internal G-tree nodes whose subtrees both contribute competitive cells. It scores cross-cell patterns of the four axes, so a coordinated shift that no single cell would flag still surfaces in the report.

Contents

The addition is substantial, but almost entirely self-contained within packages/sentinel.

  • A new torrust-sentinel crate (1.0.0) with a narrow public surface exposed through flat crate-root re-exports
  • SpectralSentinel<C, V, N> as the generic engine, with Sentinel128 and Sentinel64 aliases for the common domain widths
  • SentinelConfig, NoiseSchedule, and SvdStrategy for host-controlled measurement parameters, with structured ConfigError / ConfigErrors / ConfigWarning validation rather than panics
  • Public readout types for batch, cell, coordination, contour, health, maturity, geometry, baseline, score, and analysis-set summaries — ordered deterministically by GNodeId
  • Analysis-set selection over Mudlark's G-V Graph: top-K competitive V-entries plus ancestor closure into the investment set
  • Per-cell subspace trackers scoring novelty, displacement, surprise, and coherence; EWMA baselines with upper-tail clipping; CUSUM drift accumulators with a separate slow EWMA
  • Hierarchical coordination trackers scoring cross-cell score patterns at G-tree internal nodes
  • Automatic synthetic noise warm-up for every newly created tracker, plus a deferred staging area and optional background warming thread so cell creation does not block the ingest hot path
  • Brand incremental SVD for subspace evolution, with a naive thin-SVD path used both as a fallback for small/numerically sensitive cases and as a debug-mode oracle
  • Optional serde support (off by default; pulls in torrust-mudlark/serde)
  • A Criterion benchmark suite (15 bench functions across 8 groups: encoding, ingest, auxiliary, convergence, scaling, temporal, analysis)
  • 21 architecture decision records covering measurement-not-opinions, the feed-forward invariant, Mudlark integration, deterministic ordering, analysis-set recomputation, automatic noise injection, scoring geometry, decay semantics, routing, degenerate-dimension guards, test budgets, warm-up convergence, visibility, cell-creation performance, Brand SVD, deferred warm-up, generic domain parameters, investment-set terminology, and the clip-pressure / mean-centred-variance EWMA refinements
  • README, public API reference, algorithm document, and implementation notes (~4.7k lines of crate-level documentation total)
  • 642 tests with all features enabled (631 in the default configuration) across crate-level tests (src/tests/), integration tests (tests/) and doc-tests, with the README compiled as a doc-test via #[cfg(doctest)] include_str!
  • Two pedagogy integration tests (pedagogy.rs, pedagogy_advanced.rs) written to be read end-to-end as a walkthrough of the public surface

Changes outside sentinel

  • Cargo.tomlpackages/sentinel added as a workspace member
  • packages/mudlark — unchanged: the series rides on the released Mudlark 1.1.0 that develop carries, whose structural mutation counters and semi_internal_count() accessor the report reads; the branch's own earlier cut of that feature is dropped, and no commit in the series touches the Mudlark package
  • Cargo.lock — 76 entries added for the dependency closure (faer, rand_distr, plus dev-only criterion and tracing-subscriber) against the lockfile develop refreshed under the 1.90 floor; no existing entry moves, and fifteen bare dependency lines gain a version qualifier because the closure introduces a second compatible release of thiserror, thiserror-impl, rand_chacha and r-efi
  • AGENTS.md — adds Sentinel's S- cross-reference prefix to the package table and ADR examples

Manifest under ADR-T-012

The dependency on the sibling torrust-mudlark pins version = "1.1.0" beside its path, because cargo publish writes that requirement into the published manifest and the report reads the structural mutation counters that arrive with that minor. faer, tracing and criterion name the 0.x line the sources are written against (0.24, 0.1, 0.8) instead of a bare 0, for the reason #884 gave for the root's requirements. cargo publish --dry-run -p torrust-sentinel stops at resolution because torrust-mudlark is not on crates.io yet; that is the publication order ADR-T-012 documents, and torrust-mudlark itself dry-runs cleanly.

Reviewing this

The best starting point is the public surface:

packages/sentinel/src/lib.rspackages/sentinel/docs/api.mdpackages/sentinel/README.md

From there, the main implementation path is src/sentinel/mod.rs for the orchestrator, src/analysis_set.rs for competitive selection and ancestor closure, src/sentinel/tracker.rs for per-cell scoring, src/sentinel/{cusum,staging,warming_thread}.rs for drift and warm-up, and src/maths/ for the SVD plumbing.

For a focused review, I would look at:

  • the public API shape and the flat crate-root re-exports
  • configuration validation, defaults, and the structured error/warning types
  • the feed-forward Mudlark integration (Δ = 1 per observation, scores never feed back)
  • report semantics, deterministic ordering, and what is and isn't part of the public surface
  • tracker warm-up, the deferred staging area, and the optional background warming thread
  • the Brand SVD fallback boundary (small d, narrow rank gaps) and the debug-mode oracle path
  • integration tests that assert invariants through the public API only

The pedagogy tests are intended to be readable end-to-end; running cargo test -p torrust-sentinel --test pedagogy -- --nocapture produces a narrated walk through the public surface.

Verification

On the rebuilt commit: cargo fmt --check clean; cargo clippy --workspace --all-targets --all-features -- -D warnings clean under the workspace lint table; the crate's 565 tests and 15 doc-tests pass; the whole workspace passes (2,370 tests, none failed); cargo audit keeps the vulnerability count of develop (the one rsa advisory with no fixed release) and adds a single allowed unmaintained-crate warning, RUSTSEC-2024-0436 (paste, a proc-macro pulled by faer through gemm).

Notes

  • Ships at 1.0.0: the public surface documented in docs/api.md is covered by semver guarantees from this release onwards. A sibling crate consumes it through a version-beside-path pin, the same discipline this manifest applies to torrust-mudlark, so the version a consumer pins is the one the manifest declares.
  • MSRV 1.90, inherited from the workspace (ADR-T-011). Paragraphs below that report verification on 1.89.0 record runs made while the workspace floor was 1.89; the floor was raised to 1.90 on develop afterwards and this branch now sits on it.
  • No unsafe code; #![forbid(unsafe_code)] at the crate root.
  • AGPL-3.0-only, inherited from the workspace. Unlike Mudlark, no linking exception is shipped with this crate.
  • Default features: none. serde is opt-in.
  • The crate measures only. Interpretation and response remain external host policy.
  • Temporal policy is host-controlled: Sentinel never applies decay automatically.
  • Configuration prefers structured errors over panics.
  • Sentinel docs and ADRs use the S- cross-reference prefix added in this PR.

Review fixes

Since the previous head, nine commits on top of the three original ones fix every finding an automated review of the package produced and a code-level verification confirmed: the configuration validation refuses non-numbers, an unrepresentable depth-buffer headroom and a coordinate width below the tracker minimum (two additive error variants); the full-width cell owns the domain maximum and the root leaves the analysis candidates before the capacity cut; staged cells warm by their real volume and a pass with no competitive scores retires every coordination context; health and batch reports count the online sets, populate the semi-internal count from the graph (one additive mudlark accessor, and the headroom arithmetic in mudlark saturates instead of wrapping), count the whole contour and order coordination reports by depth then identifier; the geometric schedule reports its true maximum, a failed corrective factorisation reports failure so the dispatcher falls back, and the unread round scores are gone. Prose follows the code (the z-score denominator, the live-tracker figure, the open bit-source trait, the implemented dimension guard), and the one exact float equality in the invariants suite compares bit patterns. Nothing on the public surface is removed or reshaped.

Second round of review fixes

Five further commits fix every finding of a second automated review at the previous head. The warming thread's shutdown transition is made under the staging lock its wait is paired with, so a shutdown can no longer be lost between the worker reading its predicate and sleeping on it, which left the join — reached from Drop — waiting forever; the u128 centred-bit conversion caps the requested width at the type's own instead of indexing past its backing array; the centred bit vector gains a validated constructor and a length accessor so an implementation of the open bridge trait outside the crate can return the value its impl must produce; the online summary reports the investment count over the whole selection, warming cells included, as its contract states. The test support's four-bit generator refuses a nibble at sixteen or above (which shifted every set bit out of the coordinate and aliased the range sixteen below), a six-bit generator carries the sprays that claim sixty-four distinct ranges, and the ordering, budget and concentration witnesses assert the documented order, the structure's own budget and a report below the root. The upper bounds of analysis entries and coordination contexts document the top-of-domain exception, the analysis set's full field is named as the investment set it is, the thread-safety plan states the Send + Sync the crate asserts statically, and section-mark references with no referent leave the record and the test banners. Public surface: three additive constant functions on the centred bit vector; nothing is removed or reshaped. Verified on stable 1.98 (the toolchain this repository lints and tests with), 1.89.0 and nightly.

Third round of review fixes

Four further commits fix every finding of a third automated review at the previous head. The headroom a depth pair demands was computed with one checked step and three unchecked ones around it, so a creation depth of zero beside an eviction depth at the top of the range overflowed inside the very method that promises to hand back its faults as values; the computation now lives in a helper whose every step is checked, and any overflow reports the existing structured error for a buffer too large to honour, with the widest pair a budget can clear pinned as accepted. The centred-bit vector holds at most 128 values, but the coordinate trait it is fed from is open to wider types and the only width guard compared the tracker's dimension with the coordinate's declared bits, so a 200-wide tracker over a 256-bit coordinate was admitted and fed from a 128-slot vector; the sentinel now refuses a width above the vector's ceiling with a configuration error naming the width and the maximum, and the ceiling is documented on the bit source, on the bit vector and in the crate docs. The prefix generators in the test support guarded their ranges with assertions that release builds compile out; all three sites assert unconditionally. The dimension guard's doc said widths at or below the minimum are refused where the predicate refuses only widths below it, and now names the side of the boundary that is kept; a bare section ordinal in the exponential-average module is replaced by the sense it carried. Public surface: one additive configuration-error variant; nothing is removed or reshaped. Verified on stable 1.98 (the toolchain this repository lints and tests with), 1.89.0 and nightly.

Fourth round of review fixes

Three further commits correct every finding of a fourth automated review at the previous head; all eight are documentation, and no Rust moves. The two warming modes draw from two different generators: synchronous warming drains from the sentinel's own generator, while background warming draws from a second one seeded on the worker and promotes whatever the worker has finished at each ingest, against a map the main thread is concurrently writing. Four sites promised bit-for-bit reproducibility across runs without saying which mode delivers it; each now scopes the claim to synchronous warming on a fixed build, in one identical clause, and names the background-mode interleaving as the second source of randomness that reaches the scores. Three lifecycle records described mechanisms that no longer run where they said: the cell-width rejection lives in the suffix-width filter applied at runtime rather than in configuration validation, and its effective range is stated; creation schedules noise injection rather than performing it, now that the injection itself is deferred; and the bounded per-ingest work of the deferred warm-up record is stated for the background mode it holds in, with the default mode's in-line drain named beside it. The dimension guard's record said cells at or below the minimum are excluded where the filter keeps a cell at it, and now carries the same words as the constant's own documentation. Verified on stable 1.98 and nightly, with the crate's rustdoc and doc tests, since the README is the crate's front-page documentation.

Fifth round of review fixes

Three further commits correct every finding of a fifth automated review at the previous head. Construction asked the operating system for the background warming thread and aborted the host when the request was refused, over a resource limit that has nothing to do with the configuration's correctness; the request now returns the environment's own account as a configuration error naming the setting, and it arrives alone because the thread is asked for only once validation has passed. Reset, which has no error channel, keeps the sentinel running and warms cells synchronously instead, recording the refusal as a warning: the warm-up dispatch keys on whether a thread is present rather than on the flag, so the fallback is complete and every report is produced as before. Seeding one baseline from another copied the numbers but only ever raised warmth, so a receiver seeded from a cold source stayed warm over placeholder statistics and the cold path that replaces them never ran again; warmth is now part of what is handed over, in both directions, with a witness that fails at the previous head. Three tests and their prose claimed more, or other, than the engine guarantees: the determinism test compared three lengths and a few means where it now compares whole reports figure by figure with equal bit patterns; the coordination-report ordering test asserted ascending handle where the producer sorts by depth and then handle, and both prose statements of the handle-only order are corrected with it; and the reproducibility claim at the top of the determinism suite is scoped to synchronous warming on a fixed build, which is the configuration those tests share. Public surface: one additive configuration-error variant, and the configuration-error enumeration is marked non-exhaustive ahead of first publication so a later refusal is additive too; the warming-thread handle whose signature changed is crate-internal. Verified on stable 1.98 (the whole workspace lints clean; the crate's tests pass), 1.89.0 and nightly, with the crate's rustdoc and doc tests.

Rebased onto the released Mudlark

The series is rebased onto the develop that merged Mudlark 1.1.0. The rebase drops the branch's own cut of the structural mutation counters and the two Mudlark hunks two Sentinel commits carried, keeps every other commit byte-identical in patch, author and order, and re-resolves the lockfile against the refreshed one: the resolver accepts the result unchanged under --locked, and the full bar — nightly tests with and without features, nightly and stable clippy with warnings denied, stable tests, the 1.90 check, and rustdoc with warnings denied — is green at the tip.

Later rounds of review fixes

The remaining rounds of automated review, each verified at code level before a change was made, are answered by the commits after the fifth round. The CUSUM allowance now follows the algorithm text, κσ·√v_slow, with the denominator-protection constant kept out of it; the geometric noise schedule saturates an unrepresentable exponent instead of wrapping it, so a public caller with an arbitrarily deep argument still lands on the floor; coordination contexts are retained by online competitive membership rather than by which cells happened to score in the batch, so a quiet batch no longer destroys a context that the next joint batch would have to re-warm. Every section reference in source and tests uses the qualified §ALGO S-N form and points at the section that carries the cited content; the clip-pressure implementation plan moved to docs/plans/ so the ADR identifier it borrowed resolves to one record; the implementation guide describes the lazy coordination warm-up the code performs; a failed warming worker is recorded rather than allowed to take reset() down; the report's geometry record describes the model that produced the scores beside it; the compile-time width bound is named where a reader would look for it in the error list; and a test comment that cited a document the package never contained now derives its tolerance in place. Finally, the structural mutation counts in the contour snapshot are read from the spatial layer's own counters instead of being inferred from node and terminal deltas, an inference that a last-child eviction falsified; that is what the Mudlark minor bump carries.

The round that followed the rebase is answered by two further commits. The first repoints every stale algorithm citation in the Sentinel records, plans and source comments at the section that now specifies the behaviour each one describes, including the three that named a chapter the document no longer has and the one requirement the algorithm never specified at all. The second orders the warm-up queue by depth before identifier at both promotion sites, so an ancestor that ties with a descendant on volume is warmed first even when arena slot reuse has handed that descendant the smaller identifier, with a witness on each path that fails without the depth comparison.

The notes that round left on unchanged code are answered by four further commits. The first fixes a tracker report that mixed two models, publishing an energy share and a leading singular value read after the subspace had already been replaced, so that every reported model figure now describes the model that actually scored the batch. The second gives the API reference the two re-exported observation types it had never documented, states the real ordering of each report vector against the code that produces it, and extends the configuration-error table from a third of the enum to all of it. The third repoints four records at the specification sections that carry the material they cite, two of which named a section about something else and two a chapter the document no longer has. The fourth brings the batch warmer's equal-volume ordering into line with the two live drains, so an ancestor is warmed before the cells beneath it on whichever path serves the queue.

The round after that left four notes on unchanged records, answered by five further commits. The first states the warm-up ordering the staging area actually keeps, volume then depth then the identifier with the reason each layer exists, in both the deferred-warm-up record and the investment-set record's synchronous-drain decision, so the two describe one ordering rather than two. The second makes the contour snapshot's type-level contract count what its producer counts, terminal nodes together with semi-internal ones, and replaces the configuration record's overflow-prone headroom example with the checked helper and caller the validator actually runs. The third repoints all seventeen citations of the retired chapter 18 at the sections that replaced it. The fourth gives the specification the tie-breaks its ancestor-first guarantee depends on: the priority key becomes the lexicographic triple of volume, depth and node identifier, with the reason each layer is load-bearing. The fifth repoints thirty-four further citations left pointing at sections two renumberings removed, each target verified by reading the section rather than by arithmetic on the number.

The following round is answered by three further commits. The first recovers a sentinel whose warming worker died: a panicked background thread left its handle standing, so every later reconciliation notified a dead thread and every cell staged from then on stayed off the producing set while reports kept arriving; the worker is now reaped where it would otherwise be handed more work, the cells it was holding are rebuilt, and warming continues synchronously for the sentinel's remaining life, with a witness that fails without the detection. The second repoints fifty-six stale algorithm cross-references at the sections that actually specify what each citing line describes, after auditing all four hundred and seventeen citation sites in the package against the document's headings and bodies. The third marks the noise-schedule default's remaining-work row done, so the record stops contradicting the 450 root rounds the configuration ships.

One further commit repairs the recovery's witness, which could not reach its own claim on a small machine: its setup now warms synchronously so every scheduled cell is online before the sentinel crosses into background warming for the strand itself, and every assertion after the strand stands exactly as it did.

The round after that left three notes on unchanged code, answered by four further commits. A checked-out cell now returns with the volume the graph has now, so an ingest landing mid-warm-up can no longer leave the busiest cell queued at its pre-ingest importance. The analysis-set summary type says what each of its producers counts, instead of writing one producer's online scope into fields three producers fill. The three texts justifying the volume refresh now name the tie-breaks that would otherwise decide a newly queued cell, rather than a tie-break the queue no longer has. The specification states that the root is a member of the investment set by construction and never a competitive target, and the sites asserting it cite the section that now carries it.

The next round left four notes, answered by three further commits. A coordinate's domain membership is now decided once at the ingestion boundary, so a value outside [0, 2^N) is counted in no total, moves no partition and reaches no tracker; before, the spatial layer and the lifetime count accumulated it while every tracker was left unaware of it, and the encoder would have presented it as the in-domain value it is congruent to. Every reference to the algorithm specification in the package now uses the qualified §ALGO S- form and names the section that specifies what the citing line describes, nine of them re-targeted from sections whose content is not what the text describes; two bare numbers stay as they are because they name no section that exists. The API reference's projection-energy note points at the section that proves the redundancy instead of the glossary appendix it had named since it was written.

The round after that left one note on the new boundary and one on unchanged code, answered by two further commits. Domain membership is now decided by comparison against the domain's own bounds rather than inferred from the coordinate width: the trait that admits a coordinate type into the sentinel is public, and a signed or NaN-capable type a host writes was admitted wholesale at full width and below the origin at every width, so the predicate is now the root cell's containment test itself, and whatever ingestion admits the partition delivers for any coordinate type, witnessed over a signed type the crate does not ship. The API reference documents SentinelConfig::warnings() and the ConfigWarning variants, placed beside the errors where the contrast a reader needs, a refusal against an accepted configuration whose results may not be worth reading, is visible.

One further commit answers the round after that. The full-width exception had asked the coordinate width whether the domain's top is a value of the domain, which is true for the integer coordinates, where the exclusive bound is unrepresentable and domain_max names the maximum instead, and false for every continuous or host-written type, where domain_max at full width is the representable exclusive bound; the boundary, the interval scan and the scoring-bound conversion now put that one question to the trait through the discriminator the conversion already carried, witnessed by ingesting exactly 2^64 on the signed test type.

Two documentation commits follow, correcting record claims their own sources do not support. The deferred warm-up record and the specification's work-variance section had said cell creation and noise injection contribute zero cost to an observation call; under background warming the engine still recomputes the selection on every call and still allocates a tracker in line for every cell entering the investment set, so the bound now carries the ranking and closure terms and the per-entering-cell allocation term, and the zero-cost claim is narrowed to the noise injection that deferral actually removes. The mean-centred variance record's confidence-convergence bullet had credited a fourfold surprise inflation at batch size one; the record's own bias table derives an unbounded collapse there, with the bounded factors belonging to the larger batches, and the bullet now says so.

Two commits follow. The first states the Rust floor the workspace manifest gives, so the crate's published minimum supported version and the rust-version it inherits stop giving a consumer two answers to a question that has one; the floor is the one the workspace's own version job checks and tests. The second stamps the batch arrival at the call boundary, ahead of the domain decision, so the reported observation age spans the filtering pass it previously left out and is the interval the algorithm and the report field both promise; every age assertion in the test suite is an ordering, positivity or absence claim that the earlier stamp preserves.

Two commits follow. The first withdraws the three hidden crate-root re-exports, so the surface the stability statement covers is the surface the crate can support: a hidden attribute removes a name from the generated documentation without making it private, and each of these names could be written by a consumer who could never obtain a value of it, since its only producer is the crate-private tracker. The witness that asserted their reachability goes with them, as do the two EWMA test affordances the exports had kept public and the per-tracker report's unread depth echo; the API document now states that the root re-exports nothing hidden. The second states the condition of the warm-start guarantee in the noise-injection record: a cell whose schedule requests at least one round at its depth reaches real traffic warm, while a schedule that requests none, whether an empty explicit vector or a geometric taper whose zero floor has decayed past half a round, stages the cell straight to ready and it starts cold by configuration.

One commit follows. It corrects the API plan's crate-root section, which still said a few support types are re-exported behind a hidden attribute: the crate root now exports nothing beyond the surface the plan's excerpt and inventory cover, and the internal types the crate-private modules contain are reachable by no path from outside the crate.

Two documentation commits follow. The first restates the top-of-domain exception at every public interval contract the way the code decides it: the upper bound belongs to the cell only when it is the domain's last value, which happens for a coordinate type that cannot represent 2^N at its full width and names its domain maximum as the bound, while a type that can represent 2^N keeps the bound exclusive at every width; the earlier wording had credited the exception to the width alone. The second corrects the deferred warm-up record, which promised promotion at the top of each ingest call: a batch that is empty on arrival or emptied by the domain decision returns the empty report before promotion is reached, so promotion runs at the top of each call that carries observations.

@codecov

codecov Bot commented Feb 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.76115% with 218 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.37%. Comparing base (843aaff) to head (bab66fa).

Files with missing lines Patch % Lines
packages/sentinel/src/sentinel/mod.rs 91.34% 53 Missing and 21 partials ⚠️
packages/sentinel/src/config.rs 78.02% 49 Missing ⚠️
packages/sentinel/src/maths/bench_tracing.rs 0.00% 45 Missing ⚠️
packages/sentinel/src/sentinel/tracker.rs 96.13% 13 Missing and 4 partials ⚠️
packages/sentinel/src/maths/mod.rs 91.25% 13 Missing and 1 partial ⚠️
packages/sentinel/src/maths/brand_svd.rs 91.46% 6 Missing and 1 partial ⚠️
packages/sentinel/src/sentinel/staging.rs 98.56% 3 Missing and 3 partials ⚠️
packages/sentinel/src/sentinel/warming_thread.rs 96.05% 3 Missing ⚠️
packages/sentinel/src/analysis_set.rs 98.33% 2 Missing ⚠️
packages/sentinel/src/maths/naive_svd.rs 97.61% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #827      +/-   ##
===========================================
+ Coverage    68.52%   72.37%   +3.85%     
===========================================
  Files          161      175      +14     
  Lines        13111    15757    +2646     
  Branches     13111    15757    +2646     
===========================================
+ Hits          8984    11404    +2420     
- Misses        3853     4048     +195     
- Partials       274      305      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 98b82af to 0d4bf03 Compare March 25, 2026 05:05
@da2ce7 da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Apr 23, 2026
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 0d4bf03 to 84defab Compare April 23, 2026 10:36
@da2ce7 da2ce7 added Needs Rebase Base Branch has Incompatibilities and removed Needs Rebase Base Branch has Incompatibilities labels Apr 23, 2026
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 84defab to 7d2fd0c Compare May 1, 2026 12:50
@da2ce7 da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label May 1, 2026
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 7d2fd0c to 57ac0f5 Compare May 12, 2026 17:37
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 57ac0f5 to 3972d50 Compare May 12, 2026 22:39
@da2ce7 da2ce7 changed the title 20260219 sentinel Introduce Sentinel — an online subspace anomaly detector May 12, 2026
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from 3972d50 to afde2c7 Compare May 12, 2026 23:01
@da2ce7
da2ce7 force-pushed the 20260219_sentinel branch from afde2c7 to 022a672 Compare May 12, 2026 23:17
@da2ce7
da2ce7 marked this pull request as ready for review May 12, 2026 23:26
Copilot AI review requested due to automatic review settings May 12, 2026 23:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

…ead of racing for it

The witness needs a producing cell below the root before it can strand one on a dying worker, and it reached that state by ingesting eight batches with background warming on and expecting the worker to have promoted something by the time the helper looked. Nothing in the engine makes that true. The reconciliation notifies the worker and promotes whatever is already ready in the same breath, so whether any cell is online at that instant is the scheduler's decision, and on a machine with few cores the answer is no: the helper finds nothing to strand and the witness fails on its own precondition instead of on its claim. That is the failure a witness can least afford, because it says nothing about the recovery it was written to measure.

The starting state is built now. The setup ingests run with no worker, which sends the reconciliation down the synchronous drain, so every cell the schedule asks for is online by the time each call returns, on any machine and at any speed. Nothing is lost by moving them: the analysis set is a function of the value stream alone, so the same eight batches name the same cells they always did, and only the moment those cells come online has stopped being a bet. A worker is started after them, so the strand still runs against a live one, the failure it induces is still the worker's own poisoned-staging path, and the recovery measured afterwards is still the one that follows a worker that died holding a cell.

Every assertion after the strand is unchanged — the stranded cell must come back, every cell the selector pays for must be online, and the staging area must be empty — and the witness still detects what it was written for. Forcing the failed-worker detection to answer not-finished fails it at the first of those assertions rather than at the precondition, which is the whole difference between a witness that cannot reach its claim and one that makes it.

This is the same reading the recovery's own witness already applies one step further in: the stranded state is built rather than waited for, because whether a panic lands in the few instructions that produce it is the scheduler's decision. The state the strand starts from was still being waited for, and it is now built too.

The affordance that crosses a sentinel into background warming is confined to test builds and moves the configuration flag with the handle, because that flag is what reset reads to decide whether to spawn again; a flag left disagreeing with the field would make reset the one call that silently changed the mode.
…s now

The staging area caches each warming cell's volume so the queue can spend the next warm-up round on the busiest cell. `update_volumes` refreshes that cache from the graph for every cell in the warming map, but a cell the background worker has checked out is not in that map: it lives in `in_flight` as a marker while the worker holds the cell itself, and the refresh cannot reach it. The worker then hands the cell back with the volume it carried out, so an ingest landing during the noise injection — the expensive part of a pass, and therefore the part most likely to overlap a checkout — leaves the cell queued at its pre-ingest importance. The next checkout, which is the one decision the cached volume exists to make, can then go to a rival the traffic has already passed, against the volume-first rule the queue serves.

The in-flight record already carries the competitive flag for exactly this reason: reconciliation keeps learning about a cell while the worker owns it, and both return paths copy what it learned back. Volume is the same kind of fact, so it is recorded beside the flag rather than in a structure of its own, and it is seeded at checkout with the volume carried out — a return then always applies the record and never has to ask whether a refresh happened in between. Eviction while in flight still discards the cell, because the record goes with it and there is nothing left to apply, and a completed cell still takes only the flag, because the ready queue is served in the order it was filled and has no priority to honour.

Both refreshes read the graph through one function, so a waiting cell and a checked-out cell cannot come to hold volumes derived differently — including for a node the graph no longer has, which reads as nought to both.
…unts

The summary type documented its fields as online readings — the producing competitive set, the producing full set, the allocated trackers — but three producers fill it, and only one of them reads that way. `AnalysisSet::summary_online()` is the online reading the batch report is built from; `AnalysisSet::summary()`, public and reachable through the sentinel's analysis-set accessor, takes every figure over the whole selection whether or not a cell has a tracker yet; and the batch report then replaces two fields with figures the sentinel can see directly and a selection snapshot cannot. A caller of the whole-selection reading was therefore handed counts whose documented meaning they do not have.

Both readings are deliberate — their own documentation says the difference between them is exactly what a caller has to choose — so the honest repair is to stop writing one producer's scope into the type. The fields now say what they count, the type says which producer supplies which scope, and the figure that is never narrowed by online status says so once, in the place a reader of any of the three would look.

Documenting rather than narrowing keeps the whole-selection reading available. Removing it, or renaming it out of the way, would take the investment-set view from every caller in order to repair a sentence, and would break a public method whose own documentation was already accurate about what it returns.
Three texts justify refreshing the cached volumes after the enqueue loop rather than before it, and two of them argued from a tie-break the queue no longer has: they said a field of zero volumes would be decided in favour of the newest and deepest cell. The queue now resolves equal volumes toward the shallower cell and then the smaller identifier, so the stated consequence is not merely out of date, it is the opposite of what would happen.

The reason survives the correction intact, and is worth stating accurately because it is what makes the ordering of the two steps matter: a cell enters the queue at zero volume, and with the whole field at zero the deterministic tie-breaks decide the order in full. Which cell is warmed first would then be settled by where the cells sit in the tree rather than by the traffic behind them, which is the one thing the cached volume exists to prevent. The third text said only that the queue could not prioritise by traffic, which is true but silent about what takes over instead; it now names the tie-breaks as well, so all three give the same reason.
The implementation drops the root before the top-K cut, a test asserts that it never appears in the competitive set, and the API document states it as a rule — but the specification never said it. §8.1 defined the eligible set by V-Tree depth and suffix width, both of which the root satisfies, so the document as written admitted the root into the competition it is in fact barred from, and the code that bars it had no clause to cite.

The exclusion belongs where the competitive targets are defined, because that is the set it constrains: a union that adds the root to the investment set is silent about whether the root could also have arrived through competition. It is stated in §8.1 as a predicate on the eligible set, with the reason beside it — the root is the whole domain, so it has nothing to be ranked against; it answers the population-level question rather than a regional one and is permanent, which leaves nothing for a competitive slot to decide; and it belongs to the investment set by construction in any case. The order of the operations is part of the rule rather than an implementation detail: the root's importance freezes at its first split while its children start from zero, so an exclusion applied after the cut would hold a slot until a child overtook a frozen total, and at a capacity of one the competitive set would never fill at all.

The citations follow the clause. Sites that cited the ancestor-closure section for the root never being competitive now cite the section that says so, while the halves of those sentences about the root's presence in the investment set, and about its permanence, keep the sections that carry them.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Out-of-domain narrow-width coordinates can update Mudlark totals while being omitted from every Sentinel tracker, violating the feed-forward reporting invariant.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)

Comment thread packages/sentinel/src/sentinel/mod.rs Outdated
Comment thread packages/sentinel/src/report.rs Outdated
Comment thread packages/sentinel/src/report.rs Outdated
Comment thread packages/sentinel/src/report.rs Outdated
…sees it

The domain is [0, 2^N), and where N is narrower than the coordinate type a
value at or above 2^N is representable. Each of the three layers below ingest
read such a value differently. The spatial layer routes it rightward at every
level and accumulates it in the topmost cell, whose interval does not contain
it. The encoder reads the low N bits, so it returns the vector of the in-domain
value the arrival is congruent to — a genuine observation's suffix, produced
from something that is not that observation. The interval scan matches no cell
at all, not even the root, because the root's interval ends at the exclusive
bound. So the lifetime count rose for an arrival that no tracker was ever
shown, which is the divergence the mandatory multi-scale delivery exists to
rule out, and the justification for scanning intervals instead of walking
ancestors — every ancestor's interval contains the observation — quietly
stopped holding.

Membership of the domain is decidable from the width alone, unlike the
positional structure the host must guarantee, so ingest decides it once, before
the spatial layer, the encoding and the routing alike. A value outside the
domain is then counted nowhere: it raises no total, moves no partition and
reaches no tracker. A batch that loses values emits one warning naming how many
went, and a batch left with nothing is the same non-event as a batch that
arrived empty. The count stays out of the report, which describes the
observations the sentinel made, and these were not observations of its domain.

At a width that fills the coordinate type every value the type can hold is
inside the domain, so that configuration is untouched: the topmost interval
still owns its upper bound, and a batch with nothing to drop is not copied.
References to the algorithm specification were written in three forms its own
convention does not admit: a chapter named in prose, an appendix named in
prose, and bare section numbers standing alone or under a table column that
carried the qualifier for them. A bare number is unambiguous only inside the
document that establishes it, and none of these sites are in that document, so
each one asked a reader to work out which document was meant before they could
follow it at all.

Several had drifted onto the wrong target as well, which a qualified form makes
checkable. The scoring polarity invariant was cited to the empirical warm-up
appendix and the projection-energy redundancy proof to the glossary; the table
of scoring axes pointed at the baseline-tracking chapter rather than at the
four sections that define the axes; and a question about the coordination tier
pointed at the observation algorithm. Each citation was rewritten against the
section whose content the citing text describes, at the granularity that text
means — chapter-level where it means the chapter, subsection where it means a
subsection.

Two bare numbers stay as they are, because both name a section that exists in
no document: a benchmark banner and a clip-bias tolerance in a test comment.
Picking the nearest plausible heading for either would manufacture a reference
rather than repair one, and would hide the fact that the target is missing.
… carries it

The API reference sends a reader who asks why there is no fifth scoring axis to
the specification's Appendix B, which is the glossary. The argument it promises
— that under the centred binary encoding every observation has the same norm,
so projection energy is a perfect affine function of residual energy and
carries no independent information — is proved in §ALGO S-15.2, and the
reference has pointed past it for as long as it has existed.

The form was already qualified here, which is why a sweep that looked for
unqualified references did not stop on it: a citation can name its document
correctly and still name the wrong part of it. This site was that sweep's own
precedent for the appendix form, so the form travelled and the target did not.
The two remaining appendix citations were checked against their targets rather
than against this one: the README sends production callers to Appendix A for
warm-up recommendations, which its noise-schedule section carries, and the
mean-centred variance record cites Appendix A for convergence methodology,
which its methodology section is. Both stand.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Generic full-width coordinates can bypass domain validation, allowing invalid observations into the graph and trackers.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Low severity Document SentinelConfig::warnings() in API reference

packages/​sentinel/​src/​config.rs:915

The canonical API reference omits this public method: docs/api.md §5.1 currently lists only validate() and Default for SentinelConfig. Since the README promises that every crate-root API method is documented there, add warnings() to that method list.

Comment thread packages/sentinel/src/sentinel/mod.rs Outdated
…inate type

`in_domain` inferred membership from the coordinate width: at `N == C::BITS` it short-circuited to true, admitting every representable value without looking at it, and below that width it compared against the upper bound alone. Both readings are sound only for the unsigned integers.

`Coordinate::BITS` is documented as the width `N` is validated against and promises nothing about which values the type can hold; `CentredBitSource` is public and nothing closes the set of its implementations; and `Coordinate` is implemented for the floats as well as for the unsigned integers, and carries `is_nan`. A coordinate type a host writes may therefore be signed and NaN-capable, and for such a type the old predicate admitted a value below the origin, a NaN and an infinity at a width that fills the type, and a value below the origin at every narrower width. The three layers below the boundary each read such a value differently — the spatial layer accumulates it in a terminal whose interval does not contain it, the encoder presents it as some in-domain value, and the interval scan matches no cell at all — so the lifetime total would record an arrival no tracker was shown, which is the divergence deciding membership once exists to rule out.

Membership is now decided by comparison against the domain's own bounds: at or above the origin, and below `domain_max(N)` or equal to it at a width that fills the type. That is the root cell's own containment test, and the root is permanent, so whatever the boundary admits the partition delivers — the mandatory delivery of the observation algorithm holds for any coordinate type a host may bring, not only for the two the crate ships. A NaN needs no case of its own, since every comparison with it is false.

The witness is a coordinate type the crate does not ship, a newtype over `f64` delegating every coordinate method to `f64`'s own so that it claims no behaviour the trait does not already permit. The unsigned witnesses are unchanged and hold the unsigned reading exactly as before.
`SentinelConfig::warnings()` is public and `#[must_use]`, and `ConfigWarning` is re-exported from the crate root, but the reference described neither: §5.1 listed `validate()` and `Default` alone, and the enum appeared only in the re-export list. The reference is where the crate's public surface is defined and where the semver guarantee is anchored, so a public method missing from it is a method a caller has no documented contract for.

The method now appears in §5.1's method list with what it is for, and §7.1 gains a variant table for `ConfigWarning` in the form the reference already uses for `ConfigError`. Advisory diagnostics belong beside the errors rather than among the configuration fields, because what a reader needs in order to act on one is the contrast with a refusal: an error refuses the configuration and construction returns it, while a warning accepts the configuration and says the results may not be worth reading. The two are also read at one boundary and in one order — validation first, then the advisories on success — so a caller writing that code finds both in one place.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Full-width continuous coordinates incorrectly admit the exclusive domain bound, and the lost-wakeup regression test remains nondeterministic.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Comment thread packages/sentinel/src/sentinel/mod.rs Outdated
… domain

The domain boundary read the full-width exception off the coordinate width alone: `value == domain_max(N)` was admitted whenever `N == C::BITS`, and the interval scan handed that value to the topmost cell on the same reading. The exception is narrower than the width. `Coordinate::domain_max(n)` is documented as the exclusive upper bound `2^n`, and only the integer implementations substitute the type's maximum at `n == BITS`, because `2^BITS` is not representable for them; there the value it names is the last value of the domain and must be admitted, routed and delivered. Every other implementor keeps the bound exclusive at every width — the floats return `2^n` for every `n`, and `CentredBitSource` is public, so nothing closes the set of types that do the same — and for those `2^BITS` is a representable value outside `[0, 2^N)`. Admitting it raised the graph total and the lifetime count for a value the domain does not contain, and then delivered it to a cell whose interval does not contain it either. Two layers agreeing to admit such a value is not the domain being respected: it is both of them reading a width where the coordinate type is what decides.

The conversion from a spatial bound to a scoring bound already carried the discriminator that tells the two cases apart, and it asks the trait rather than naming types. `is_final` reports the unit interval `[0, 1)` indivisible at depth zero for exactly the coordinates that subdivide down to single values, which are the ones whose `domain_max` carries the substitution; for a continuous coordinate, which terminates by depth instead, it is false. Hoisting that decision into one helper puts the single question to all three layers that meet the top of the domain — the boundary that admits a value, the scan that must then deliver it, and the conversion — so none of them can part from the others over it.

Where the helper answers no, the predicate is at or above the origin and strictly below `domain_max(N)`, which is the root cell's own half-open containment test, so every admitted value is still matched by the permanent root and the mandatory delivery of the observation algorithm holds; the topmost cell no longer claims a bound that lies outside the domain. Where it answers yes, nothing moves: the unsigned coordinates keep the inclusive maximum they had, and the scoring-bound conversion keeps its behaviour exactly, since its condition is the expression it was already computing.

The witness ingests `2^64` at a full-width continuous coordinate — the exact literal, asserted against `domain_max(64)` so that it is the bound itself rather than a neighbour of it — and holds the graph total, the lifetime count and the root's sample count to the in-domain value alone, with a batch of nothing but that bound a non-event. The unsigned witnesses are unchanged and hold the integer exception as before: the maximum of a full-width domain still reaches a tracker, and the first value above a narrower one still reaches none.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The shutdown race test is probabilistic, and two architecture records contain materially inaccurate claims.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Low severity Include synchronous ingest costs in the bound

packages/​sentinel/​adr/​017-deferred-cell-warm-up.md:62

This bound omits synchronous work that remains on ingest(): analysis-set recomputation and SubspaceTracker allocation/enqueueing for every newly selected cell. Background warming removes the noise-injection rounds, but cell creation does not contribute zero cost, so the stated formula and zero-cost claim should include or explicitly scope out those terms.

Low severity Align the claimed inflation with the variance analysis

packages/​sentinel/​adr/​021-ewma-mean-centred-variance.md:176

The claimed inflation contradicts this ADR's own derivation and adjacent consequence, which say the old b = 1 variance collapses to ε and surprise reaches O(10^5). Keep the consequence consistent with that analysis.

The work variance bound recorded the per-call cost of `ingest()` as the spatial pass plus per-tracker scoring, and said cell creation and noise injection contribute zero cost. Under background warming the engine still recomputes the selection on every call — the eligible entries within the depth cutoff are ranked for the top-K cut and closed under ancestry — and still allocates a tracker in line for every cell entering the investment set; only the noise rounds go to the background worker. A bound that omits work paid on every call is not a bound of that call, and the zero-cost sentence is false of the half of cell creation that never left it.

The bound now carries the ranking and closure terms and the per-entering-cell allocation term, each in the specification's own symbols, and distinguishes the unconditional terms from the one that bursts at a selection change. The claim that survives is the one deferral actually earns: the tens to hundreds of noise rounds a new cell commits to leave the call, and what stays is bounded by the selector's parameters rather than by traffic or by the warm-up schedule.

The specification's work variance section carried the same sentence and the same omission, so it is corrected with it and gains the entering-cell count as a fourth source of call-to-call variance — the one source that does not change slowly.
The host-impact section claimed a fourfold surprise inflation at b = 1 under the within-batch variance estimator. The record's own bias table derives the opposite shape at that batch size: the bias is the full -100%, the variance erodes to the stability constant, and surprise reaches O(10^5) — the adjacent bullet says exactly that. No batch size yields a fourfold factor either, since the inflation the table derives is b/(b-1): it is 2x at b = 2 and 33% at b = 4, and falls toward 1 as b grows.

The bullet now states the collapse the derivation gives at b = 1 and attributes the bounded factors to the batch sizes that actually produce them, so the host-facing consequence and the analysis it summarises say the same thing. The reason confidence settles faster is unchanged; what changes is the magnitude it is credited to.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The reported batch age omits domain-filtering work, and the published MSRV documentation disagrees with the inherited workspace floor.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Correct published MSRV to inherited Rust 1.90 floor

packages/​sentinel/​README.md:25

This advertises Rust 1.89, but the crate inherits the workspace's current rust-version = "1.90" (Cargo.toml:42). Downstream users on 1.89 will therefore be told the crate is supported even though Cargo rejects that toolchain; update the published MSRV statement to the inherited floor.

The crate sets `rust-version.workspace = true`, so its floor is whatever
`[workspace.package] rust-version` holds; that field reads 1.90, while the
README's stability section still published 1.89. A crate whose own README
contradicts the manifest it inherits from gives a consumer two answers to a
compatibility question that has one, and the wrong one of the two is the
answer a reader reaches first.

ADR-T-011 settles which source governs: because a raise leaves no new ADR
behind, the current pin is read from the manifest's `rust-version` field, and
stale MSRV prose is corrected as part of the raise. The figure here therefore
comes from the manifest rather than from any prose record of an earlier raise.

The sentence takes the sibling package's form. The "tested in CI" clause is
claimed on evidence rather than by symmetry: the MSRV job reads `rust-version`
from the root manifest, installs exactly that toolchain, and runs its build
check and test run across the whole workspace, of which this package is a
member — so the floor is enforced here, not merely asserted. The ADR-T-011
reference is kept, since it is what tells a reader why the number is not
negotiated per crate.
Two documents promise that the reported observation age spans the call. The
algorithm's observation-age section says a batch arrives whole, so its
observations share a single arrival; the report field's own documentation says
the same in the same words, "at the call boundary"; and the comment at the
emission site says the age is read last so that it covers every part of the
work this call did on the batch.

The stamp did not sit at that boundary. `ingest` ran the domain decision and
its early return first and only then read the clock, so the interval began
after the filtering pass rather than at entry. Filtering is unambiguously work
this call does on the batch — it walks the input and may allocate a retained
copy — so every reported age was short by the filter's own duration, and short
by more as batches grow. The figure was a lower bound presented as the
measurement, which is the one thing an age a host uses to judge staleness must
not be.

Taking the stamp as the first statement of the function makes the boundary the
code's boundary and the documents' boundary the same instant, and leaves the
arrival comment true where it now sits. It costs one clock read on the path
where a batch is emptied by the domain decision: that path returns
`empty_report()`, which states no age, so the reading is taken and discarded.
Paying an unconditional clock read is the cheaper side of the trade against
stamping late and reporting a number that is not the interval it claims.

No assertion changes. The tests that bound the age do so against an instant
taken outside the call, which still precedes the new stamp, so the bound holds
with a narrower margin; the tests that require an emptied or empty batch to
report no age read the constant in `empty_report` and never the stamp.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The large numerical and concurrent API addition still has unresolved public-API stability and documentation-contract issues.

Review effort: Balanced
Findings: None

Previously missed (3)

In code that hasn't changed since last review

Medium severity Hidden types remain public and outside the stated stable API

packages/​sentinel/​src/​lib.rs:155

#[doc(hidden)] only removes these items from generated documentation; it does not make them private. EwmaStats, SubspaceUpdate, and TrackerReport remain externally nameable public API, which conflicts with the crate's 1.0 stability statement and docs/api.md saying hidden affordances are outside the stable surface. Remove the public re-exports (and move the integration witness that uses TrackerReport behind the crate boundary), or explicitly include these types in the supported semver contract.

Low severity Correct the MSRV claim from Rust 1.89 to 1.90

packages/​sentinel/​Cargo.toml:16

The PR description says this crate sits on an MSRV 1.89 floor and reports verification on 1.89, but this inherited value currently resolves to workspace rust-version = "1.90" (also stated in the crate README). Cargo will reject 1.89 before compilation, so update the PR's MSRV and verification claims to 1.90.

Low severity Qualify the no-cold-tracker guarantee for zero-round schedules

packages/​sentinel/​adr/​007-automatic-noise-injection.md:15

This claim is false when NoiseSchedule::Explicit(vec![]) (or any zero-round schedule) is configured: enqueue promotes the new tracker directly to ready without injecting noise, and cold_config() relies on that behavior. Qualify the guarantee so it applies only when the schedule requests at least one round; otherwise the ADR promises callers that cold trackers cannot occur when they can.

This issue also appears on line 35 of the same file.

The crate root re-exported three support types behind a doc-hidden attribute. Hiding an item from generated documentation does not make it private, so those names stayed part of the crate's external surface while the stability statement said that surface is exactly what semver covers and the API document said hidden affordances sit outside it. Two statements about the same three names cannot both hold, and the one that describes what a consumer can actually write is the stability statement.

Nothing outside the crate can obtain a value of any of the three. The per-tracker report is produced only by the subspace tracker, which stays crate-private under the visibility decision recorded in ADR-S-014, and the cell and coordination reports carry their own score fields rather than embedding it. The exports therefore offered names with no reachable values behind them, which is a promise to maintain something no caller could use.

The integration witness asserted precisely the property being withdrawn — that the per-tracker report is reachable through the flat public surface — so it goes with the export rather than being replaced: a witness for a retracted guarantee would only re-state the guarantee.

Withdrawing the exports left three items reachable from nothing but the crate's own tests. The two EWMA methods are confined to the configuration that uses them, which is what they were already for. The per-tracker report's depth field was a pure echo of the argument its caller had just passed, read by no code in the crate; the depth a host needs travels on the cell report, assembled from the cell, so the echo carried nothing and is removed with the test half that asserted it.

The package is introduced on this branch with no released version behind it, so narrowing the crate root breaks no published contract.
The record claimed in two places that no cell reaches real traffic cold and that every tracker starts warm. Both are unconditional, and both are false under a schedule that requests no rounds: the staging area sends a cell with a zero round target straight to the ready queue, and the crate's own unit and integration tests depend on exactly that path — one proves the direct promotion, another configures an empty explicit schedule precisely to obtain cold trackers.

An empty explicit vector is not the only way to request none. A geometric schedule with a zero floor tapers as root times decay raised to the depth, and once that product falls below half a round it rounds to zero, so the same cold start arrives at sufficient depth from a schedule that looks like it warms everything. The condition therefore has to be stated in terms of what the schedule requests at the cell's depth, not in terms of one constructor.

Stating the condition is what makes the guarantee usable: a reader can now tell which configurations it covers, and a host that deliberately disables warming is no longer reading a record that says its own configuration cannot happen.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The shutdown regression test remains scheduler-dependent, and the API plan contradicts the implemented public surface.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread packages/sentinel/docs/plans/api-md-plan.md Outdated
The plan's crate-root section still told the reader that a few support types are re-exported behind a doc-hidden attribute for tests, diagnostics and benchmarks. The crate root carries no such re-export: every module in it is crate-private, its whole public surface is the seven re-export lines and the two type aliases the section's own excerpt shows, and the attribute appears nowhere in the package source. The canonical API reference already states as much, so the plan was the last record describing a surface that does not exist.

Deleting the sentence would have left the paragraph owing the reader something it no longer said. The first clause exists to explain an abridgement — the excerpt lists three report types where the root re-exports twenty-one, and the report-types section carries the full inventory — and a reader just told that the excerpt is incomplete needs to be told where completeness ends. The withdrawn sentence answered that question wrongly; the replacement answers it: the excerpt and the inventory together cover the whole public surface, and the internal types the crate-private modules contain are reachable by no path from outside the crate.

The internal-machinery module table is left as it stands. It inventories what each crate-private module contains, which states where a type lives rather than claiming the type is exported, and every row remains true of the crate as it now is.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Empty ingests skip ready-cell promotion, and several public interval contracts misdescribe custom-coordinate bounds.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Low severity API reference incorrectly promises inclusive maxima by width alone

packages/​sentinel/​docs/​api.md:188

The canonical API reference promises the inclusive maximum whenever the coordinate width fills its type, but the implementation correctly keeps domain_max(N) exclusive for continuous/custom full-width coordinates. Since this document defines the supported 1.0 API contract, describe the exception in terms of whether domain_max(N) is the last in-domain value, not width alone.

Low severity AnalysisEntry misstates full-width coordinate bound semantics

packages/​sentinel/​src/​analysis_set.rs:48

AnalysisEntry is generic over an open Coordinate, so filling C::BITS does not necessarily make domain_max(N) inclusive. For continuous/custom coordinates it can still be the representable exclusive bound; document the same coordinate-specific exception that ingestion and scoring use.

Low severity Overbroad contract for full-width coordinate upper bounds

packages/​sentinel/​src/​report.rs:112

This contract is too broad for the crate’s open coordinate API. Full-width continuous/custom coordinates can represent 2^N, so domain_max(N) remains exclusive; only coordinate implementations for which it denotes the last in-domain value (such as full-width unsigned integers) use the inclusive exception. Please match the domain_top_is_in_domain() behavior.

This issue also appears in the following locations of the same file:

  • line 176
  • line 668
  • line 859

The seven public statements of the top-of-domain exception justified it by coordinate width: a width filling the coordinate type was said to leave no value above the bound to be excluded. The crate does not decide it that way. The exception belongs to the coordinate type rather than to the width, and width alone does not distinguish the two cases — a type that represents `2^N` at its full width holds a value above every cell bound and keeps the bound exclusive there, so a width-only reading hands the topmost cell a value the domain does not contain. What makes that cell own its bound is the absence of a representation for `2^N`, which leaves the type's domain maximum standing in for a bound that cannot be spelled.

Each site now states that condition as a contract a host can rely on — which value belongs to which cell — rather than naming the discriminator the crate computes it with, which is private and promises nothing. The interface reference carries the same statement as the field documentation, because it is the contract of record for the release and a reader must not have to reconcile two accounts of the same boundary.
The record described the interaction surface as a staging map with atomic promotion at the top of each `ingest()` call. Promotion does not run on each call. A batch that is empty on arrival, or left empty by the domain decision, returns the empty report before the promotion step is reached, so a call carrying no observations promotes nothing.

The specification is explicit that an empty batch returns the empty report without modifying any state, and its procedure places that return ahead of the promotion step. The record is the artifact that promised otherwise, so the record is what changes: the promise now names the calls that carry observations and says what the empty call does instead. Moving promotion ahead of the empty return would contradict the specification and buy nothing — warm-up advances independently of observation cadence, and a newly online cell exists to take part in the routing of the call that promotes it, which an empty call has nothing to offer it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The large numerical and concurrent subsystem warrants final human validation despite no confirmed blocking defect in the reviewed paths.

Review effort: Balanced
Findings: None

@da2ce7

da2ce7 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

ACK 4d37309

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants