Skip to content

docs: repo-wide review v2 (software, testing facilities, docs site) + remediation plan with environment partition - #205

Merged
pdlourenco merged 7 commits into
mainfrom
claude/charming-gauss-vglm3j
Oct 7, 2026
Merged

pdlourenco merged 7 commits into
mainfrom
claude/charming-gauss-vglm3j

Conversation

@pdlourenco

@pdlourenco pdlourenco commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Summary

Adds the 2026-10-04 repo-wide review (v2) and its remediation plan as internal engineering documents under docs/ — nothing under docsite/, so the public site is unaffected.

  • docs/analyses/2026-10-04-repo-wide-review-v2.md — point-in-time review anchored at 1785dd3 (post-0.2.0 main), per the docs/analyses convention. 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 --strict with 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-n data, absolute IO convergence test below J = 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

  • Algorithmic change (touches a spec contract — spec updated first)
  • New feature / function (non-contract)
  • Bug fix
  • Docs / tooling / CI only

Checks

  • Implementations derive behaviour independently from the spec, not ported copy-by-copy between languages — n/a (no implementation change)
  • Tests written against spec requirements, not against the other language's current output — n/a
  • Shared helpers touched? — no
  • New defaults / bounds / thresholds are covered by spec/SPEC.md — n/a
  • Fixing a known bug? — no; the analysis files findings, the plan proposes issues (§0 inventory)
  • Major / architectural decision made? — none decided here; every contract-changing proposal is marked [decision] for the maintainer (CLAUDE.md §4 recommend-don't-decide); ADR-0002, ADR-0004 and ADR-0005 revisits are proposed, not made
  • Pre-push review: three rounds. The first flagged 8 (1 blocker — the Reviewed at anchor 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 in d7e150a and 0a16c2f. 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 in 2ed7529. 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 in 2fd6fc1. 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 in 55e7f16 without a further review round.
  • Local CI: docs-only change; mkdocs build --strict green (the two files are under docs/, outside docs_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.4 runAllTests.m 374/380 (the 6 failures are the plotting files, blocked by the container's rendering defect) and validate_reference.m 32/32; no MATLAB engine in this session.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Wz15nbQvZAdxgAMJ9nkuvf

claude added 5 commits October 4, 2026 17:39
…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 pdlourenco left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

  1. 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_disc with collinear states and zero input raises a raw LinAlgError. 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 528a242 row) and the §4.5 Output-COSMIC row (line 298) say the IO vector pins a non-converged iterate; Appendix A (line 497) says reference_ltv_io converges in 14 iterations and reference_frozen_of_io is the non-converged one. The CI validator log confirms: the only notConverged warning is under reference_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:1827 cites 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".
  2. "whiteness_pass / independence_pass are 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:148 asserts WhitenessPass == manual_pass against the §14.3 rule and :155–166 does the same for IndependencePass. 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).

  3. 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**) installed and GNU Octave, version 7.1.0 in 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 at README.md:33, matlab/README.md:6, matlab/CONTRIBUTING.md:276, docs/DESIGN.md:96 and matlab/RELEASE_NOTES.md:31 have 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.

  4. §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.m 374/380 cases, 35/41 files 432/432, 41/41
    runAllExamples.m 1/12 complete 12/12
    validate_reference.m 32/32 32/32
    ft_text_renderer failures every axes call none
    Octave control/signal packages installed no packages installed

    Appendix 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)

  1. §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" fails test_ltv_disc_frozen.py:128 at 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.

  2. §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_imag alone. Line 194: the mutation run is 68 passed, not 63. Line 192: the re-stamp mechanism is broader than stated. tests.yml:43 gates only pull_request events, so every push to main, 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 three none tags, not three (the trust-region tag at SPEC.md:1833 is unrelated). Line 202: frozen_of_io carries Response_atol 1e-8, so "unfloored" applies to two vectors.

  3. §3.1 facts. Line 92: reference_internals has no C field; the af95b60/9a9da93 loosening is in reference_cosmic_internals. Line 92: 2ae1457 added Octave-side atol floors to six vectors (its own commit message lists them), not only reference_ltv_cosmic. Line 87: July C1 is the variable-length state-estimator terminal block, pinned separately by reference_ltv_state_est_varlen at 1e-6; reference_ltv_cosmic_varlen never calls the state estimator, so "the exact code area of July C1" should read "adjacent to".

  4. Coverage over-statements (§4.3–§4.5). Line 229: test_sidFreqMap.m asserts four ids by exact string (:119,:129,:210,:286), so "every sidFreqMap id (11)" should read "11 of 15"; likewise "every sidSpectrogram id" is 8 of 10. Line 266: ltv_disc is called with 3-D L=1 input at test_ltv_disc.py:146–153; what is missing is the 2-D equivalence assertion. Line 268: a_std has an ungated Monte-Carlo oracle (test_ltv_uncertainty_calibration.py:128–143), so "MATLAB-pinned only" applies to noise_cov/DoF. Line 307: MATLAB asserts CLim numerically and exercises Axes/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" is test_sidLTVStateEst.m Test 12, mirrored in Python at 1e-10; test_sidLTVdiscVarLen.m asserts only errA < 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 distinct SidError codes by AST (not 49), and the unasserted-warnings list has 11 entries, so "8 of 21" should be "10 of 21".

  5. §5.1. Line 334 (F13): u=None works for list-y in all four estimators; only u=[] fails, and it fails for any y shape. The defect is that validate_data.py:232 tests u is None and does not recognise the spec's [] idiom. Line 333 (F12): MATLAB sidFreqETFE and the Welch sidFreqMap do not crash on 2-D y with 3-D u; under Octave both silently return a single-trajectory result with NumTrajectories = 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.3 Inf sentinel, so "genuinely shared" should exclude ETFE. Line 323: the C3 fix also covers freq_btfdr, as §2.1 (line 44) correctly says.

  6. §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:1826 is 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 measured P(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: _internal docstrings are not rendered on the API reference (no _internal pages under docsite/).

  7. History window. Line 6 says the archaeology covered "all 306 commits". The checkout is a shallow clone (git rev-parse --is-shallow-repository is true); a verifier counted 597 commits on main via 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.

  8. §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.

  9. §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)

  1. Port-matching without a spec anchor. D9 (plan line 108) adds S₁ ≤ 0 guards 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-based TimeSteps "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).

  2. 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).

  3. B3 would weaken a good test. test_monotone_cost checks 49 monotone steps on the non-converging fixture, which is a real §8.12.5 check; scope the rewrite to test_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 while SPEC.md:1836 claims unit(M/Py).

  4. 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. The sid-reference-bot integration is an always-bypass actor (main.json:11–15), so A1 alone cannot meet Phase A's exit criterion; only A2 can. Line 191's fetch-depth: 0 is 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 in main.json and 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

Copy link
Copy Markdown
Owner Author

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:

  1. Internal contradictions — exec summary now says "four of the five critical findings (C3 only partially)"; §2.2 moves Fix flaky Test 10: use LTV data where perfect consistency is impossible Test 10 asserted bestFraction < 1.0, but for clean LTI data with small lambda grids, the consistency can genuinely reach 1.0. Use LTV data (from Test 9) instead, where system transitions prevent perfect consistency at threshold=0.9999. Also check the result is valid rather than asserting a specific fraction value. https://claude.ai/code/session_012GEES665avG5BVaL3uHXX4 #16/Add linting and static analysis with MISS_HIT Add MISS_HIT (Python-based MATLAB/Octave linter) for style checking, code quality, and complexity metrics. Configured with rules tuned for MATLAB/Octave signal processing conventions. New files: - miss_hit.cfg: project config suppressing rules that conflict with standard MATLAB idioms (operator spacing, uppercase parameters, etc.) - .editorconfig: formalises existing 4-space indent convention - .github/workflows/lint.yml: CI workflow running mh_lint, mh_style, and mh_metric on every push/PR Code changes: - Rename shadowed builtin eps -> eps_floor in sidUncertainty.m - Fix 27 lines exceeding 100-char limit across source and test files - Auto-fix whitespace issues (comment spacing, colon spacing) All 30 tests pass, all 10 examples pass, all three MISS_HIT checks (lint, style, metric) report zero issues. https://claude.ai/code/session_01HhcWqc6wc8KNTAyqtP5Ecs #19 to still-open and adds a "partially closed" bucket for Update README, SPEC, and roadmap to reflect current implementation status README: add Features list, How It Works section, LTV/COSMIC functions to comparison table, time-series quick start snippet, Octave installation note, and links to COSMIC theory docs. SPEC: add implementation status note distinguishing implemented sections (§1–7, §8 base) from planned sections (§8a–8e). Roadmap: add completion markers to all phases and timeline table, add Status column to function catalog, fix package structure to match actual file layout (tests/ not test/, correct test naming, add missing internal files), remove stale sid. package prefix from usage examples, update result struct with missing fields. https://claude.ai/code/session_01LNniXasxfanjoA6u8N9TT1 #10/Implement Phase 9a: multi-trajectory support for spectral functions #15; §3.1 reads "with one exception (§3.2), nothing was loosened"; §3.1's 528a242 row and the §4.5 var-len IO row both now say reference_ltv_io converges and it is frozen_of_io's IO run that caps; exec summary and §3.3 say only the MATLAB band would have passed the §8.9 bug; §3.3 says SPEC.md:1827 cites Test 15 (the exact-Hessian oracle), so MATLAB has no MC verifier in the spec.
  2. Whiteness/independence flags — §3.7, §4.5 and the exec summary now say MATLAB asserts WhitenessPass/IndependencePass against the re-derived rule (test_sidResidual.m:148,155–166), Python asserts neither, and neither port has a white-vs-coloured case; plan B3 adds that discrimination case to both ports.
  3. CI Octave is 7.1.0 — new major finding in §4.2 (job logs of the 2026-07-26/30 main runs: octave 7.1.0 under the step named "Install GNU Octave 11.x", 432/432, 12/12, 32/32 under it); the five 11+ claims (README, matlab/README, matlab/CONTRIBUTING, DESIGN, RELEASE_NOTES) are listed as unbacked; exec summary and §7's install-page item updated; plan §10 premises rewritten ("snap Octave — 7.1.0 today, despite the step name"); new Phase-A item A10 (enforce the floor in both Octave jobs, fix the install source, floor itself a [decision]) in §0, §1 and §10.3.
  4. Plan §10.1 — reworded as specific to the authoring session's container, with the fresh-container results (432/432, 12/12, figures rendered, no Octave packages) recorded as reported; the "needs PR CI / cannot render" routing is dropped from §10.3 rows B3, B4, C2, C3, C4, E2 and from §10.4; the environment recommendation is softened; Appendix A carries the same caveat plus the 464/380/432 count reconciliation.
  5. §3.5 / plan B4 — the detrend bullets are tagged noisy (residual-mean assertions exact by LS orthogonality, trend recovery noise-limited at 6e-4/1e-3); the frozen 6e-8 is named as finite-λ bias with ~1e-6 as the target; the ramp slope item says a tracking fixture is needed (over-smoothed at λ = 1e2); B4's targets are now per fixture class.
  6. §4.1 — "20 of those" → 8 (the eight fields listed); mutation outcome → "every test passes"; F1 now explains the pull_request-only gate (every push to main re-stamps) and the fix is decoupling the SHA or comparing payloads (also in plan A2); F9 scopes F3 to the two degenerate-input none tags; F6 notes frozen_of_io has Response_atol 1e-8; the opener distinguishes scripted (inventory, mutation) from read (F3, F5) claims.
  7. §3.1 — reference_cosmic_internals; 2ae1457 added atol floors to six vectors; "adjacent to July C1" with the state-estimator distinction; shallow-clone caveat (311 reachable from 2026-04-08; "born at" rows sit at the boundary and must be re-derived before A6); Method line corrected.
  8. Coverage — sidFreqMap 11 of 15 (the four asserted named), sidSpectrogram 8 of 10; ltv_disc 3-D L = 1 is called (test_ltv_disc.py:146–153) without the 2-D-equivalence assertion; a_std has the MC oracle so "MATLAB-pinned only" is noise_cov/DoF; plotting row ◐ (CLim, Axes/Channel); var-len COSMIC row ◐ (errA < 0.15, dense oracle is state-est Test 12); MATLAB cannot represent 3-D L = 1, so plan C3 scopes it to Python; 48 codes / 34 unasserted, 10 of 21 warnings, exec summary line 24 reworded; line 25 → 18 major / 22 minor / 2 nits (F12's promotion moves one row), matching the §5 tables.
  9. §5.1 — F13 → u = [] fails for any y shape (validate_data.py:232), u = None works; F12 → major (MATLAB ETFE and Welch map silently return NumTrajectories = 1, verified under Octave 8.4); "genuinely shared" excludes ETFE; F2 names freq_btfdr among the covered paths.
  10. §5.2 — ids renamed K1–K17 (plan D10 and all cross-references updated); "July C1/S1…" prefixes; K3 rewritten around the previous-block-only check (ltv_cosmic_solve.py:111, sidLTVcosmicSolve.m:70–71) with the §8.3.5 fixture caveat; iteration counts attributed to the pass's own example, your 754 / ~900× noted; K10 → typed diagnostic for negative P diagonals (−0.4 here, −1e12 yours), plan D10 no longer says "clamp"; K17 → _internal docstrings are not rendered.
  11. Method line — shallow clone, 311 reachable from 2026-04-08.
    12–13. Summary numbers and §2.2 buckets — covered under 1 and 8 above.
  12. Plan D9/D10 — D9 has a spec column and [decision] for the S₁ ≤ 0 rule; K6 time alignment gets a spec sentence first + [decision]; K5 final-μ pass classified as a contract clarification [decision].
  13. Plan B2 — §8.9 expectation sentence before re-centring; Test 13 → median statistic (per-entry reaches 1.61); campaign un-gated instead of a schedule: workflow (§9 and §10.3 updated); 1.27 vs 1.33 reconciled as different trial counts/seeds.
  14. Plan B3 — test_monotone_cost kept, rewrite scoped to test_convergence; Test 15 split plus a new MATLAB defective-stabilization test; §3.4's Test 15 row and §6's SPEC.md:1827 item corrected.
  15. Plan A1/A2 — mint-before-gate and the sid-reference-bot always-bypass are recorded (A2, not A1, meets the exit criterion); three fetch-depth: 0 checkouts.

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 2ae1457's six atol floors. To confirm: the second live ruleset "Main" (id 14923865) — recorded in plan A2 as reported, to be checked by the maintainer against the rulesets API before A1/A2 act on it.

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 | inside code spans in two table rows, "on the image" wording now contradicting the fresh-container caveat, §2.1 section cross-references); all are folded into the same commit. CI is green on 2fd6fc1. The PR description's counts and pre-push-review line are updated.


Generated by Claude Code

@pdlourenco pdlourenco left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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)

  1. docs/analyses/2026-10-04-repo-wide-review-v2.md:6 and the §3.1 caveat say "311 commits reachable in the shallow clone". That figure is the 306 commits on main plus this PR's own five commits at the time. The archaeology window is main's history, so 306, or say "including this branch's commits".
  2. Same file, §5.2 K3: the body now cites ltv_cosmic_solve.py:111 for the previous-block check, but the Where column still reads ltv_cosmic_solve.py:104,118. The raise is at :119; align the column with the body.
  3. 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.
  4. Analysis §4.1 F2 still says the frequency grid is "pinned only for three files"; Python reads Frequency for four vectors. Trivial.
  5. 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

Copy link
Copy Markdown
Owner Author

Thanks for the re-review. All five nits are taken, in 55e7f16:

  1. History window → "the 306 main commits reachable in the shallow clone" (Method line and §3.1 caveat).
  2. K3 Where column → ltv_cosmic_solve.py:104,110–111,119 (the two solves and the previous-block check), matching the body.
  3. Plan A1 → "Three of the six already start on every PR (tests.yml and docs.yml are unfiltered); the other three live in cross-validate.yml and python-tests.yml, which are path-filtered".
  4. §4.1 F2 → the frequency grid is pinned for four files.
  5. Plan B2 → the 1.27 vs 1.33 reconciliation is marked as the likely explanation, not a verified one, with your Octave-median observation recorded.

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

@pdlourenco
pdlourenco merged commit f9744bd into main Oct 7, 2026
6 checks passed
@pdlourenco
pdlourenco deleted the claude/charming-gauss-vglm3j branch October 7, 2026 20:35
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