Repository navigation
docs: repo-wide review v2 (software, testing facilities, docs site) + remediation plan with environment partition - #205
Conversation
…ocs site — with remediation plan Point-in-time review anchored at 1785dd3 (post-0.2.0 main HEAD), following the docs/analyses convention: dated, commit-anchored, immutable. The analysis re-checks what the July remediation (#134-#145) actually closed, then covers: tests wrongfully relaxed / weakened / never re-tightened (the review's focus), the cross-vector infrastructure rule-by-rule with mutation tests of both consumers, CI gates, both unit suites, a consolidated functionality-coverage matrix, spec-to-implementation conformance of all three halves (frequency-domain, COSMIC, utilities/examples/packaging), spec defects, the documentation site (built --strict with notebook execution), and the publishable parts. The companion plan sequences the fixes: gates first, then the tests that lean on them, coverage, conformance (spec-first), spec and site hygiene, release mechanics, publication track. Method: eight independent review passes seeded with REVIEW_CONTEXT.md plus first-hand numerical probes; Python executed (415 passed / 4 skipped at HEAD), MATLAB read statically (no engine available), site built clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
…MATLAB vs maintainer) §10 classifies every remediation item by where it can be done: a cloud session (Python + apt Octave 8.4), the maintainer's local MATLAB (toolboxes, engine tolerances), the PR's own CI jobs (MATLAB R2025a + snap Octave 11), or maintainer-only actions (decisions, secrets, canonical regeneration). Records what was established empirically under cloud Octave 8.4 at this commit: runAllTests 374/380 (the 6 failures are the plotting files, blocked by an Octave-8.4 text-renderer defect on this image), validate_reference 32/32, MISS_HIT clean, examples not executable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
…oncatenated dump) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
- analysis: correct the 'Reviewed at' anchor (1785dd3); state that Octave 8.4 was installed after the review passes and point at the executed results; stop claiming the S10(e') 8.4 failure would recur (it does not, verified); caveat the MATLAB-examples execution claim; count detrend's 3-D crash as major, not critical; fix the regeneration-commit count - plan: mark [decision] on A2, A6, A9, D10, D11, E1 in the inventory; fill the D10 row; rephrase D10-C3 as spec-conformance rather than port-matching; qualify A7/E1 in the §10.4 summary; D1 severity split (F1/F2 critical, U5 major) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
…into the review and plan Analysis: correct the §2.2 closed/open lists (#1, #3 are open; #10, #15 only partially closed); C3 is partially fixed; static-reading caveat on the MATLAB claim in the summary; §9 phase letters aligned with the plan; ADR-0014 is the seed's ADR; test-file count; a wrong citation removed; add Appendix A — the execution record under apt Octave 8.4 (374/380, 32/32, generator RNG stream differs from MATLAB's) plus the facts it surfaced: min-deps run 414/1 on a 1-ULP atol-less tolerance, reference_ltv_io does converge, test_reviewFixes.m calls freqz unguarded, Octave's singular backslash returns finite values where MATLAB returns Inf/NaN, local-ci prefers Octave, the PyPI name `sid` is taken, branch protection is live. Plan: [decision] markers and spec-first notes on D2, D10 C7-C16, E1; §9 graph note for B2's schedule trigger; A1/A2/A3/A6/A7/A9 refined with the verified CI facts (drift check goes red on the PR, cross-validate needs only its workflow-level paths filter removed, Octave-regenerated vectors differ at the input level, PR MATLAB job can run the validator, A6's premise corrected); §10 corrected on the session's GitHub identity (owner with admin — maintainer actions are governance-gated, not capability-gated), figure-free plot error identifiers, onCleanup lifetime under run(), the installed Octave control and signal packages, and the frozen_of_io generator case being the non-converged one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
pdlourenco
left a comment
There was a problem hiding this comment.
Review of PR #205 — docs: repo-wide review v2 + remediation plan
Posted as a comment review because this session's GitHub identity is the PR author, so GitHub does not allow a "request changes" state. The verdict is: request changes (see §4).
Reviewed at PR head 2ed7529 against the anchor tree 1785dd3 (= main). Both files were read in full. Mechanical checks ran in the reviewing session: the full Python suite, the MATLAB suite and examples under apt Octave 8.4.0, testdata/validate_reference.m under Octave, the CI job logs, and Python/Octave reproductions of the headline findings. Ten of the analysis's sections were additionally checked claim by claim by independent verifier agents (335 claims confirmed); the remaining sections and every discrepancy reported below were re-checked first-hand.
1. Summary
The PR adds two internal engineering documents: docs/analyses/2026-10-04-repo-wide-review-v2.md (the analysis) and docs/plans/2026-10-04-review-v2-remediation-plan.md (the plan). Direction is right and most of the content is accurate: every file:line citation in both documents resolves to an existing file and in-range lines, and the headline findings reproduce (the three criticals F1/F2/U1, the model-order gap-rule failure, the mutation holes in the Python cross-validation consumer, the 21/32-file regeneration churn, the never-reached Welch sentinel, the widened model-order bound, the 1.07 calibration ratio, the P1 packaging failure). It is not ready to merge as is. The analysis contradicts itself in six places, one coverage claim that drives a plan item is false for MATLAB, and the plan's execution-environment partition rests on two premises that do not hold: CI's Octave is 7.1.0, not 11, and the Octave-8.4 figure defect is specific to the authoring container. Because docs/analyses/README.md rule 3 freezes an analysis once merged, these must be fixed inside this PR.
What works well. The method is sound and mostly honest about what was executed versus read. The cross-vector mutation testing (§4.1) and the "both ports wrong" oracle proposal (A7) are the most valuable parts of the review. The plan recommends rather than decides for the big items (A2, A3, D3–D7, D10 C1/C2/C3, E1) and marks them [decision], which is exactly CLAUDE.md §4. The §4.5 coverage matrix and the Verified-by over-claim list (§3.7, §6) are the kind of right-side-of-the-V audit the project asked for.
2. Issues to address before merge
Severity: blocker = must change before this analysis is frozen; non-blocker = should change, cheap.
Blockers
-
Internal contradictions in the analysis (principle 5: the document is the evidence a reader will act on).
- Line 18 says July "closed every critical finding"; line 44 marks July C3 "Partially fixed" and §5.1 F2 (line 323) files its residual as critical. Fix: "closed four of five criticals (C3 only partially, see F2)".
- Line 73 lists July #16 and #19 under "Closed minor findings (verified)"; §5.2 C3 (line 353) and C8 (line 358) report both still open. Reproduced #16:
ltv_discwith collinear states and zero input raises a rawLinAlgError. Fix: move both to the still-open sentence. - Line 79 says nothing in PRs #134–#198 was loosened; §3.2 (lines 94–104) documents #157 widening a bound from 6 to 12. Fix: "with one exception (§3.2)".
- Line 88 (the
528a242row) and the §4.5 Output-COSMIC row (line 298) say the IO vector pins a non-converged iterate; Appendix A (line 497) saysreference_ltv_ioconverges in 14 iterations andreference_frozen_of_iois the non-converged one. The CI validator log confirms: the onlynotConvergedwarning is underreference_frozen_of_io.json. Fix both to match the appendix. - Line 23 says both Monte-Carlo bands "would have passed the very bug §8.9 was corrected for". Only the MATLAB band does. The Python test's own docstring (
test_ltv_uncertainty_calibration.py:134–137) says the old code gives ~1.64, which the 1.2 bound rejects, and a verifier ran the pre-fix code: median 1.70, fails. §3.3 does not make the "both" claim; §1 overstates it. - Line 108 says Test 13 "is cited in SPEC §8 Verified by: as the MATLAB verifier of §8.9".
spec/SPEC.md:1827cites Test 15, the exact-Hessian oracle, which does reject the old formula. Line 171 (§3.7) agrees with the spec, so §3.3 contradicts §3.7. Fix: narrow the finding to "MATLAB has no MC verifier in the spec; Test 13 would not have caught S1".
-
"
whiteness_pass/independence_passare asserted nowhere in either port" is false for MATLAB (analysis line 135, the §4.5 row at line 305, and plan B3 at line 81).matlab/tests/test_sidResidual.m:148assertsWhitenessPass == manual_passagainst the §14.3 rule and:155–166does the same forIndependencePass. The true gap is Python-only, plus the absence of a white-vs-coloured discrimination case in either port. Rewrite the cell and plan B3 accordingly (principle 5). -
CI's Octave is 7.1.0, not 11.x; the plan partitions on a false premise (plan lines 157 and 160; analysis line 218). Both the tests.yml "Install GNU Octave 11.x" step and the cross-validate.yml step print
octave 7.1.0 from GNU Octave (octave-snap**) installedandGNU Octave, version 7.1.0in every run pulled (main runs 30523302952 on 2026-07-30 and 30214149818 on 2026-07-26), with the suites green at 432/432 and 32/32. Consequences: analysis §4.2's "a runner shipping Octave 8.4 would run below the floor unnoticed" has already happened, two major versions lower, and belongs in §4.2 as an observed major finding (a gate that cannot do its job, by the document's own scale); every §10.3 row that says "PR CI confirms" for Octave gives no Octave-11 evidence; and the "GNU Octave 11+" claims atREADME.md:33,matlab/README.md:6,matlab/CONTRIBUTING.md:276,docs/DESIGN.md:96andmatlab/RELEASE_NOTES.md:31have no CI backing. Add a Phase-A item: enforce the floor in both Octave jobs and fix the install source, with the floor itself a[decision]given that 7.1 and 8.4 are evidently green. -
§10.1's environment facts are specific to the authoring container (plan lines 163–176, and the recommendation at line 176). On a fresh cloud container with the same apt Octave 8.4.0:
Check Document Fresh container runAllTests.m374/380 cases, 35/41 files 432/432, 41/41 runAllExamples.m1/12 complete 12/12 validate_reference.m32/32 32/32 ft_text_rendererfailuresevery axescallnone Octave control/signalpackagesinstalled no packages installedAppendix A may stand as one session's record (that is what an analysis is), but the plan should not classify every figure-producing item as "PR CI or LOCAL" or recommend an environment change on a defect that does not reproduce in the next container. Reword §10.1 as "observed in the authoring session; re-check in a fresh container" and drop the "cloud cannot render" classification from §10.3/§10.4.
Non-blockers (factual corrections)
-
§3.5 and plan B4 would break tests if executed. Analysis line 151 calls the detrend assertions "noiseless polynomial fits"; all five cited Python cases add noise (
test_detrend.py:35,64,102,160,190, σ from 0.001 to 1). The residual-mean assertions are zero by LS orthogonality whatever the noise, but the trend-recovery ones (:67,:163) measure 6e-4 and 1e-3, so plan line 82's "detrend absolute tolerances → 1e-10" fails them. Line 82's "frozen → 1e-8" failstest_ltv_disc_frozen.py:128at the analysis's own measured 6e-8 (finite-λ bias, not round-off). The ramp test's proposed slope assertion fails on the current fixture at λ=1e2. Tag each §3.5 bullet noiseless / noisy / finite-λ and tighten per class. -
§4.1 counts and mechanism. Line 184: of the 78 unfloored fields, 8 (not 20) contain exact zeros or entries ≤ 1e-15; the rule-1 verdict survives on
freqmap_bt.Response_imagalone. Line 194: the mutation run is 68 passed, not 63. Line 192: the re-stamp mechanism is broader than stated.tests.yml:43gates onlypull_requestevents, so every push tomain, including docs-only merges (#197, #198, #110), regenerates and re-stamps; path-gating regeneration (plan A2) would not stop it, the provenance SHA must be decoupled from HEAD or the commit step must compare payloads. Line 196: F3 is the real reason for two of the threenonetags, not three (the trust-region tag atSPEC.md:1833is unrelated). Line 202:frozen_of_iocarriesResponse_atol 1e-8, so "unfloored" applies to two vectors. -
§3.1 facts. Line 92:
reference_internalshas noCfield; theaf95b60/9a9da93loosening is inreference_cosmic_internals. Line 92:2ae1457added Octave-side atol floors to six vectors (its own commit message lists them), not onlyreference_ltv_cosmic. Line 87: July C1 is the variable-length state-estimator terminal block, pinned separately byreference_ltv_state_est_varlenat 1e-6;reference_ltv_cosmic_varlennever calls the state estimator, so "the exact code area of July C1" should read "adjacent to". -
Coverage over-statements (§4.3–§4.5). Line 229:
test_sidFreqMap.masserts four ids by exact string (:119,:129,:210,:286), so "everysidFreqMapid (11)" should read "11 of 15"; likewise "everysidSpectrogramid" is 8 of 10. Line 266:ltv_discis called with 3-D L=1 input attest_ltv_disc.py:146–153; what is missing is the 2-D equivalence assertion. Line 268:a_stdhas an ungated Monte-Carlo oracle (test_ltv_uncertainty_calibration.py:128–143), so "MATLAB-pinned only" applies tonoise_cov/DoF. Line 307: MATLAB assertsCLimnumerically and exercisesAxes/Channel(test_sidSpectrogramPlot.m:68–71,:77,test_sidMapPlot.m:79–83), so the plotting row is not ✗/✗ for those options. Line 295: the "dense oracle" istest_sidLTVStateEst.mTest 12, mirrored in Python at 1e-10;test_sidLTVdiscVarLen.masserts onlyerrA < 0.15. Line 235: MATLAB cannot represent a 3-D array with a trailing singleton, so plan C3's MATLAB "3-D L=1" test cannot be written. Lines 260–262: 48 distinctSidErrorcodes by AST (not 49), and the unasserted-warnings list has 11 entries, so "8 of 21" should be "10 of 21". -
§5.1. Line 334 (F13):
u=Noneworks for list-yin all four estimators; onlyu=[]fails, and it fails for anyyshape. The defect is thatvalidate_data.py:232testsu is Noneand does not recognise the spec's[]idiom. Line 333 (F12): MATLABsidFreqETFEand the WelchsidFreqMapdo not crash on 2-Dywith 3-Du; under Octave both silently return a single-trajectory result withNumTrajectories = 1, which is "silent numerical error in realistic use" and major by the document's scale. Line 318: ETFE does not go through the shared helper in either port (freq_etfe.py:60–61,sidFreqETFE.m:397–398) and does not apply the §3.3Infsentinel, so "genuinely shared" should exclude ETFE. Line 323: the C3 fix also coversfreq_btfdr, as §2.1 (line 44) correctly says. -
§5.2. Line 369 reuses July's IDs (C1, S1, …) in a section whose own table defines C1–C17; write "July C1". Line 353: the input that reproduces C3 violates §8.3.5's existence condition, so "
SPEC.md:1826is false" is overstated; the stronger finding is that the §8.3.4 check inspects only the previous block and so can never catch the singular last block in either port. Lines 347 and 355 quote iteration counts and J reductions for "the hard case" without naming it; on the repo's own Test-11 data a verifier measured 754 iterations and ~900×, not 1100 and 9×. Line 360: a verifier measuredP(k)diagonals of order −1e12 on rank-deficient data, so "tiny negative" understates it and plan D10's "clamp before sqrt" would mask a non-PSD posterior; a typed diagnostic is the principle-8 fix. Line 367:_internaldocstrings are not rendered on the API reference (no_internalpages underdocsite/). -
History window. Line 6 says the archaeology covered "all 306 commits". The checkout is a shallow clone (
git rev-parse --is-shallow-repositoryis true); a verifier counted 597 commits onmainvia the API, and the §3.1 "born at" commits dated 2026-04-08 sit exactly at the shallow boundary. State the window and re-derive the first-appearance claims on an unshallowed clone before A6 acts on them. -
§1 summary numbers (lines 24–25): "16 of 21 MATLAB warning identifiers" are sites (14 identifiers); "35 of 49 Python error codes" is 34 of 48; "some forty minor" is 23 labelled minor plus 2 nits, reaching ~35 only with the unlabelled E1–E4/P1–P5 items.
-
§2.2 bucket labels (line 73): #10 and #15 are listed as "closed (verified)" with caveats that say they are only partly closed, and commit
2ed7529's message says "partially closed". Give them their own bucket.
Non-blockers (plan vs the project's principles)
-
Port-matching without a spec anchor. D9 (plan line 108) adds
S₁ ≤ 0guards to MATLAB that only Python has and no spec section defines, with no spec column or[decision]marker in the inventory (line 45). D10 C6 (line 109) adopts Python's 0-basedTimeSteps"per §6.7", but §6.7 fixes no indexing and MATLAB documents a 1-based contract; this needs a spec sentence first and a[decision]. D10 C5 states "the final μ = 0 pass is the base alternation" as settled; §8.12.4 does not classify it, so it is a contract clarification (principles 1–3). -
B2 pins a test to the implementation. Re-centring the Python band on the measured 1.07 (lines 80 and 200) encodes current behaviour, not a stated expectation; neither §8.9 nor the derivation says how far above 1 the ratio may sit at mid-λ, and the spec line should come first (principle 3). Also: Test 13's per-entry statistic cannot be tightened to fail 1.64 while passing the corrected code (a verifier measured corrected-code ratios up to 1.61 under Octave); the gated campaign passes in about 24 s when run, so un-gating it is a cheaper alternative to the repo's first
schedule:workflow; and line 200's "1.27 at λ = 1e0" conflicts with the analysis's 1.33 (line 113). -
B3 would weaken a good test.
test_monotone_costchecks 49 monotone steps on the non-converging fixture, which is a real §8.12.5 check; scope the rewrite totest_convergence. Test 15 never claimed defective-matrix coverage, so the proposed split rests on a conflation; the real gap is that MATLAB has no defective-stabilization test whileSPEC.md:1836claimsunit(M/Py). -
A1/A2 mechanics. The app-token mint step precedes the relevance gate (
tests.yml:104–109), so requiring the MATLAB context blocks every fork PR. Thesid-reference-botintegration is an always-bypass actor (main.json:11–15), so A1 alone cannot meet Phase A's exit criterion; only A2 can. Line 191'sfetch-depth: 0is needed in three checkouts (both cross-validate jobs and python-tests). A verifier also reported, via the rulesets API, a second live ruleset named "Main" (id 14923865) that is not inmain.jsonand not drift-checked; this was not re-queried by the lead reviewer, so treat it as something to confirm.
3. Follow-up suggestions
- Add a
(static)tag or column to §2.1's MATLAB verdicts; the blanket header note is easy to miss. - Reconcile the two MATLAB case counts (464 static increments in §4.3, 380 executed in Appendix A) in one sentence.
- Label the §4.1 opener honestly: F3's JSON semantics and F5's CI mechanics were read, not scripted.
- Rename plan rows C1–C5 or the §5.2 items to avoid two different C1–C5 in the same pair of documents.
- The arXiv references in §8 could not be checked from the reviewing container (egress blocked); confirm them.
4. Verdict
Request changes. The work is substantial and largely correct, and nothing here changes its direction. But an analysis is immutable once merged, so the six internal contradictions, the false MATLAB coverage claim, and the two false environment premises in the plan need to be fixed in this PR, not in a follow-up. Mechanical checks did run in this review (Python suite 415 passed / 4 skipped / 9 warnings; Octave 8.4 suite 432/432 and examples 12/12; validator 32/32; CI job logs; Python and Octave reproductions), so this is a verification-and-validation review. Not verified here: MATLAB-proper behaviour, the mkdocs build figures (CI's strict build passed on the PR head), the minimum-dependency run, and the second ruleset.
Generated by Claude Code
Analysis: fix the six internal contradictions (critical-findings count, July #16/#19 still open with a partially-closed bucket, the §3.2 exception, which IO run caps at MaxIter, which MC band passes the §8.9 bug, SPEC:1827 cites Test 15); correct the MATLAB residual-flag claim (test_sidResidual.m does assert both flags); add the CI-Octave-is-7.1.0 major finding to §4.2 and the unbacked 11+ claims; reclassify F12 as major (MATLAB ETFE/Welch silently return NumTrajectories = 1); fix F13 to u = [] for any y shape; rename §5.2 ids C1–C17 to K1–K17; rewrite K3 around the previous-block-only conditioning check; correct the counts (48 codes / 34 unasserted, 10 of 21 warnings, 8 of 10 and 11 of 15 ids, 8 near-zero fields, 18 major / 22 minor / 2 nits); tag the detrend tolerances as noise-limited; add the shallow-clone caveat and the Appendix A container/case-count caveats. Plan: new Phase-A item A10 (enforce and install the Octave floor, floor itself a decision); §10 premises and classifications reworded as session-specific (a fresh container rendered and ran 432/432); B2 un-gates the campaign and uses a median statistic for Test 13; B3 keeps test_monotone_cost and adds a MATLAB defective-stabilization test; B4 per fixture class; A1/A2 record the mint-before-gate and always-bypass facts and the reported second ruleset; D9/D10 K5/K6 gain spec columns and decision markers; F12 re-homed under D6. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
|
Thanks for the review — every verifiable claim in it checked out against the sources, and all 17 items plus the follow-ups are applied in-PR in 2fd6fc1. Per item:
Follow-ups: "(static)" note on §2.1; case-count reconciliation in Appendix A; §4.1 opener labelled; C→K rename; arXiv identifiers marked as confirmed by web search. Accepted as reported rather than re-verified here: the fresh-container run (432/432, 12/12, rendering, no packages), the 754-iteration / ~900× Test-11 measurement, and A fresh-context pre-push reviewer ran on the response diff and found 19 more items (two stale C→K ids colliding with plan C3/C4, F12 still in D11's minor bucket, a stale "49 codes", the §9 graph missing A10, unescaped Generated by Claude Code |
pdlourenco
left a comment
There was a problem hiding this comment.
Re-review of PR #205 at 2fd6fc1
Posted as a comment review because this session's GitHub identity is the PR author, so GitHub does not allow an approving review state. The verdict is: approve (see below).
Scope: the one new commit since the first review (2fd6fc1, 126 insertions / 119 deletions across the two documents), read as a word-level diff against 2ed7529; the citation resolver, the plan/analysis consistency checks and the link check re-run on the new versions; the new citations the response introduced opened and read; CI on the new head. Base is unchanged at 1785dd3.
Summary
Every item of the first review is addressed in the document text, not only in the reply. The six internal contradictions are gone and the corrected statements agree with each other and with the tree (July C3 partial; #16/#19 moved to still-open with a partially-closed bucket for #10/#15; the §3.2 exception; reference_ltv_io converges and frozen_of_io is the capped run, in §3.1 and §4.5; only the MATLAB Monte-Carlo band passes the old bug; SPEC.md:1827 cites Test 15). The MATLAB whiteness-flag claim is corrected in §3.4, §4.5 and §1. The CI-Octave finding is in §4.2 as a major with the five unbacked floor claims plus the two "Tested in CI (11.x via snap)" sentences (matlab/README.md:158, matlab/RELEASE_NOTES.md:243, both verified), and the plan carries it as A10 in the inventory, Phase A, the §9 graph and §10. §10.1 and Appendix A now say the rendering defect and the Octave packages were specific to the authoring container, with the fresh-container results attributed, and §10.3/§10.4 no longer route figure-producing items to PR CI. The non-blocking corrections landed too: the §3.5 noisy/finite-λ tagging and per-class B4, the §4.1 counts and push-event mechanism, the §3.1 facts, the coverage over-statements, F12 major with the Octave-verified silent acceptance, F13 as the u = [] idiom, the §5.2 rename to K1–K17 with no stale references anywhere, K3 rewritten around the previous-block-only check, K10 as a non-PSD posterior with a diagnostic instead of a clamp, D9/K5/K6 given spec columns and [decision] markers, B2 spec-first and un-gated, B3 keeping test_monotone_cost, and A1/A2 recording the mint-before-gate, the bot bypass, three checkouts and the reported second ruleset.
Mechanical state: all 306 file:line citations in the two documents resolve to existing files and in-range lines; every finding id the plan uses exists in the analysis; the §5 severity tallies are 3 critical / 18 major / 22 minor / 2 nit, matching §1 and the PR body; [decision] markers agree between the inventory, the phase text and §10.4; relative links resolve; CI is green on the head (lint and strict site build; the engine jobs are correctly gated off for a docs-only change).
Remaining nits (none blocking)
docs/analyses/2026-10-04-repo-wide-review-v2.md:6and the §3.1 caveat say "311 commits reachable in the shallow clone". That figure is the 306 commits onmainplus this PR's own five commits at the time. The archaeology window ismain's history, so 306, or say "including this branch's commits".- Same file, §5.2 K3: the body now cites
ltv_cosmic_solve.py:111for the previous-block check, but the Where column still readsltv_cosmic_solve.py:104,118. The raise is at:119; align the column with the body. docs/plans/2026-10-04-review-v2-remediation-plan.md:66, A1: "All six now start on every PR" is contradicted by the rest of the same sentence. Three start on every PR; the other three are path-filtered.- Analysis §4.1 F2 still says the frequency grid is "pinned only for three files"; Python reads
Frequencyfor four vectors. Trivial. - Plan line 82 explains the 1.27 versus 1.33 ratios at λ = 1e0 as "different trial counts and seeds". That is a plausible reconciliation, not a verified one; a verifier in the first round found 1.27 matches the Octave median of MATLAB Test 13 at HEAD. Either wording is fine as long as it is marked as unverified.
Verdict
Approve. The two documents are now consistent with each other and with the tree at the anchor, the plan's premises match what CI actually runs, and nothing false would be frozen by merging. The five nits above are one-line edits; take them or leave them.
Generated by Claude Code
History window stated as the 306 main commits reachable in the shallow clone; K3 Where column aligned with the body (previous-block check at :110–111, solves at :104/:119); plan A1 says three of the six checks already start on every PR; F2 counts four Frequency-pinned vectors; the 1.27-vs-1.33 reconciliation in B2 marked as likely, not verified. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf
|
Thanks for the re-review. All five nits are taken, in
Nothing else changed. The PR now waits on the maintainer to merge (I don't merge my own PRs, per CLAUDE.md §5). Generated by Claude Code |
Summary
Adds the 2026-10-04 repo-wide review (v2) and its remediation plan as internal engineering documents under
docs/— nothing underdocsite/, so the public site is unaffected.docs/analyses/2026-10-04-repo-wide-review-v2.md— point-in-time review anchored at1785dd3(post-0.2.0main), per thedocs/analysesconvention. It re-checks what the July remediation (Variable-length ltv_state_est/sidLTVStateEst uses wrong terminal block — silently wrong states #134–Cross-validation infrastructure gaps: mis-generated reference P, four orphaned reference JSONs, divergent tolerance handling #145) actually closed, then covers: tests wrongfully relaxed / weakened / never re-tightened; the cross-vector infrastructure rule-by-rule (ADR-0002) with mutation tests of both consumers; CI gates; both unit suites; a consolidated functionality-coverage matrix; spec-to-implementation conformance of the frequency-domain, COSMIC and utilities halves; spec defects; the documentation site (built--strictwith notebook execution); the publishable parts; and, as Appendix A, the execution record of the MATLAB suite / validator / examples under apt Octave 8.4.docs/plans/2026-10-04-review-v2-remediation-plan.md— sequenced plan (gates first, then the tests that lean on them, coverage, conformance spec-first, spec/site hygiene, release mechanics, publication track), with every contract-changing item marked[decision]for the maintainer, plus an execution-environment partition (§10) of every item: what a cloud session with Python + Octave can do, what needs local MATLAB, what the PR's own CI jobs confirm, and what is maintainer-only.Method: eight independent review passes seeded with
docs/REVIEW_CONTEXT.md, plus first-hand numerical probes. Python was executed (415 passed / 4 skipped at HEAD); MATLAB findings are static readings (no MATLAB engine); after the review passes, GNU Octave 8.4 was installed in the session and the MATLAB suite / validator / examples were executed under it — 374/380 cases and 32/32 vectors pass; the six failing test files and 11/12 examples were blocked by a figure-rendering defect specific to that session's container (Appendix A; a fresh container ran 432/432 and 12/12). The site built clean with--strict.Headline findings (details in the analysis): the numerical cores match independent oracles to machine precision in both ports; the cross-vector gate has holes confirmed by mutation (Python consumer ignores stored fields, Octave validator cannot fail on NaN, canonical generator not reproducible with regeneration committed under
[skip ci], three 1 % vectors with no rationale, one pinning a non-converged iterate); tests and cross-validation are not required checks, and the CI Octave job runs 7.1.0 against a documented 11+ floor; 3 critical / 18 major / 22 minor / 2 nit conformance defects, including three spec-level numbers both ports agree on that the spec does not sanction (ADR-0004 gap rule on exact rank-ndata, absolute IO convergence test belowJ = 1, validation loss ÷N + 1).Spec sections touched
none — no algorithmic change. (The analysis reports spec defects in §2.6–2.7, §3.3–3.5, §4, §5.4, §6.5–6.6, §7.2, §8.2, §8.3.4, §8.4.2–8.4.3, §8.9.2–8.9.3, §8.12.2–8.12.4, §8.12.9, §8.12.12, §9, §14, §15, and
spec/cosmic/output.md/uncertainty_derivation.md; the plan proposes edits, each gated on a maintainer decision. No spec file is modified by this PR.)Type of change
Checks
spec/SPEC.md— n/a[decision]for the maintainer (CLAUDE.md §4 recommend-don't-decide); ADR-0002, ADR-0004 and ADR-0005 revisits are proposed, not madeReviewed atanchor SHA was mistyped; 7 non-blockers — CI-workflow line citations taken from a concatenated dump, an Octave-8.4 claim contradicted by the measured result, missing[decision]markers in the plan inventory, port-matching wording on one COSMIC item, two count/wording nits), fixed ind7e150aand0a16c2f. The second (the workflow that adversarially verified the §10 partition, 9 agents) flagged 9 (1 blocker — §2.2 listed July Implement Phase 7: sidFreqETFE and sidFreqBTFDR #1/Fix DFT source bug and test failures #3 as closed while §5.1 reports them open; 8 non-blockers) and corrected the partition on ~20 points, all fixed in2ed7529. The third ran on the response to the maintainer's review and flagged 19 (2 blockers — stale §5.2 ids after the C→K rename colliding with plan C3/C4; 17 non-blockers — F12 still in D11's minor bucket, a stale code count, the §9 graph missing A10, unescaped|in two table rows, "on the image" wording contradicting the fresh-container caveat, §2.1 cross-references), all fixed in2fd6fc1. The re-review's five nits (history window 306, K3 Where column, A1 "three of six", F2 four files, B2 reconciliation marked unverified) are one-line edits applied in55e7f16without a further review round.mkdocs build --strictgreen (the two files are underdocs/, outsidedocs_dir); Python suite green at HEAD (415 passed, 4 skipped); MISS_HIT, ruff and both header checkers clean; under this session's apt Octave 8.4runAllTests.m374/380 (the 6 failures are the plotting files, blocked by the container's rendering defect) andvalidate_reference.m32/32; no MATLAB engine in this session.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf