Repository navigation
fix(cladistics): carry #230/#235 to main, close their remaining findings, harden frontend parsers - #239
Merged
Conversation
… and repair the CladeCumulus readers `main` still cannot load the package, and the CladeCumulus suite that never ran before now reports twelve defects. All of them are narrowed to: Package loading - src/core/doi.jl: #210 removed `module DOI` but left `end # module DOI`, a lowering error at include time ("end" at top level) — every test file fails before it starts. The opener is restored. - src/server/server.jl: routes/cladistics.jl was included twice, so every /cladistics route is registered twice. One include remains. CladeCumulus readers (test_clade_cumulus.jl, 3 failed / 14 errored) - routes/clade_cumulus.jl opened results.duckdb with `DuckDB.DB` and `DBInterface.connect` without importing either module, so the sources route answered 500 and every HTTP assertion downstream of it failed. Both are imported now, as the other route files already do. - `_graft_jplace_queries` and the diversity block in the route still used the pre-#207 field names (`weight`, `distal_length`, `pendant_length`) on `QueryCandidate` (whose fields are `like_weight`, `distal_len`, `pendant_len`), and `_graft_jplace_queries` used `.edge_num` on `PhyloNode` (whose field is `edge_number`). Five FieldErrors, now aligned with the structs. - parse_newick lost NHX support: the label reader consumed `[&&NHX:B=92]` before the call that records it, so `) [&&NHX:B=92] :0.1` parsed with support `nothing`. The reader now takes the capture refs. - parse_jplace rejected a single-candidate placement whose ratio is below 1.0, which is how placement tools report one site: the remainder is mass they did not place. A single candidate is now decisive when its ratio is at least the new `PLACEMENT_SINGLE_MIN_LWR = 0.5`; a multi-candidate placement must still partition the mass to within `PLACEMENT_WEIGHT_SUM_TOL`. A single candidate below half stays `undecided` with the same reason string. - test_clade_cumulus.jl reached the tool registry as `PH.Tools`, but `Phylogeny` imports `tool_bin` selectively and does not bind `Tools`; `MetaManifold.Tools` is the module that owns the registry. Guards against both merge-harm classes - scripts/check-julia-syntax.jl now rejects a bare `end` at the top level. Such a file parses cleanly and only fails at lowering, which is exactly how a missing `module` opener — the failure that stopped the whole suite — passed the parse gate. - test_routes.jl asserts that no method+path pair is registered twice and that both cladistics route files are present, which no request test could reveal (a duplicate registration silently shadows the first). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
`main` landed #222 while #223 was open, and the two repaired an overlapping set of CladeCumulus defects by different routes. This merge keeps every fix exactly once and settles the one disagreement between them. Textual conflicts (both sides fixed the same defect): * `read_label_and_quoted` NHX capture — kept main's keyword signature, which the call sites already use, with #223's clearer comment. * `PH.Tools` -> `MetaManifold.Tools` in the FastTree testset — identical on both sides; kept #223's note explaining why. Semantic conflict (git merged both halves into an incoherent rule): * #222 widened `PLACEMENT_WEIGHT_SUM_TOL` from 1e-5 to 0.05 so a lone 0.98 placement would be accepted; #223 kept 1e-5 and added an explicit single-candidate rule. Merging both left the 0.05 slack applied only to multi-candidate partitions, which is where it is least wanted. Resolved to #223's model: a multi-candidate record partitions the mass and must still sum to one within 1e-5, while a single-candidate record is judged on its own share against `PLACEMENT_SINGLE_MIN_LWR`. A truncated 0.96 partition is caught again, and a lone 0.98 still passes. Codex review finding (P1, `routes/clade_cumulus.jl`) — preserve unplaced LWR mass in diversity weights: * Accepting a single candidate down to LWR 0.5 made the diversity block's `cand.like_weight / tw` always 1.0, pushing mass the tool had explicitly left unplaced onto the reported edge and inflating that tip in weighted UniFrac and Faith's PD. Tips are now weighted by the raw like-weight ratio, and the shortfall is reported as a new `unplaced_reads` field per sample rather than renormalised away. * `_graft_jplace_queries` is deliberately unchanged: the clade tree is a count view of the observed table and still conserves every read. The difference between the two consumers is documented at the constants. Also from #223, unchanged: the `module DOI` opener, the duplicate `routes/cladistics.jl` include, the `edge_number`/`like_weight` field names, the stray-top-level-`end` gate in `scripts/check-julia-syntax.jl`, and the duplicate-route-registration test. Tests: the two-rule acceptance model is pinned at the parser, and a new route case drives a `.jplace` study tree through `/clade-cumulus` and asserts both the reported unplaced reads and that the clade tree still conserves all 110 reads.
CI on `main` (8df1041) shows `POST /clade-cumulus` answering 500 for every `study_tree` request: MethodError: no method matching _valid_tree_file(::SubString{String}) `JSON3` hands request fields back as `SubString`s and `strip` keeps them that way, but `_valid_tree_file` and `_tree_format` in `routes/trees.jl` are `String`-typed. The handler has no catch around `_resolve_tree_inputs_from_request`, so the MethodError surfaced as a 500 and took seven assertions down with it: `r_comb.status == 200` failed, and every `comb.*` access after it raised `KeyError` until the testset aborted. #222 fixed this class at `_load_run_table_df` -- "JSON3 hands request fields back as SubStrings; the DuckDB helpers are String-typed, so normalise once at this boundary" -- but `_resolve_tree_inputs_from_request` two functions below kept the unnormalised values. `cladistics.jl` already wraps its own call as `_valid_tree_file(String(published))`, so the convention is to normalise at the call site; this follows it. The existing `r_comb` assertion is the regression test: it sends `"treeFile" => "amplicon.nwk"` over JSON, which is exactly the SubString path that broke. With the `u.edge_num` -> `u.edge_number` repair already carried in from #223, this accounts for 8 of the 9 errors that run reported.
… placed mass Codex raised two findings on the diversity block, and they are the same defect seen from two sides. Retaining the raw like-weight ratio stopped a lone 0.6 placement from being inflated to 1.0 in `mat`, but `weighted_unifrac` then divided by `sum(values(ab))` -- the placed-only mass -- which restored exactly the weight that had just been withheld. The same denominator also erased ASVs the tree does not cover: a sample could be 90% unmapped and still be compared as though every read had been placed. `_sample_total` now lets a caller declare the real library size, and `weighted_unifrac`, `branch_unifrac_contributions` and `unifrac_matrix` take it (`total_a`/`total_b`/`totals`). All default to `nothing` and the mass on the tree, so every existing call is unchanged. A declared total below the on-tree mass is rejected. The route computes one denominator per sample -- the sum of the table column -- and reports `library size - on-tree mass` as `unplaced_reads`, which now covers the like-weight shortfall and uncovered ASVs together. Faith's PD and unweighted UniFrac read presence only and are untouched. Two unrelated defects the suite could not previously reach: `_reaggregate_after_move` built `subtree_pd` by summing each child's branch length plus its subtree, which drops the branch subtending the node itself -- the opposite of `clade_faith_pd`, which the builder uses. Every proposed clade therefore came back short by its own branch and was compared against a current tree measured the other way: moving ASV_4 into a clade of PD 0.60 reported 0.55. It now mirrors `_pd_accum`, and the test pins the exact values plus total-PD conservation. `test_clade_cumulus.jl` bound `const SV`; the thirteen later unit files that bind `SV = MetaManifold.Server` plainly then threw `invalid assignment to constant Main.SV`. The throw escaped the include loop in `runtests.jl`, so roughly thirty unit files and the occupancy probe never ran at all. The binding is now plain, and each file is included inside its own testset so a top-level throw is attributed to its file and does not take the rest of the suite down with it.
With the include loop no longer aborting, `test_differential.jl` and the server smoke tests ran for the first time and found three defects. `_wald` called `erfc`, which exists in no module this project loads -- `SpecialFunctions` is deliberately not a dependency, and the header of `compositional.jl` says so. Every multinomial and Dirichlet-multinomial fit therefore died with `UndefVarError: erfc` the moment it tried to report a p-value, which is to say the models could not be used at all. `compositional_erfc` now sits beside `compositional_loggamma` and `compositional_digamma` and is built the same way, from the regularised incomplete gamma function at a = 1/2: the power series for P below x^2 = 1.5 and the Legendre continued fraction for Q above it, needing only `exp` and `log`. Measured against the C library's erfc the relative error stays under 1e-14 wherever erfc(x) >= 1e-50, i.e. out to a two-sided deviate of |z| = 15, which matches the accuracy the other two already promise. The unit tests pin both sides of the crossover, the reflection, and the two-sided p-value that is its only caller. `min_prevalence` is the second positional argument of `DifferentialConfig`, not a keyword, so the "every taxon filtered out" case constructed a `MethodError` instead of reaching the engine. The same call also passed four samples that were all in group A, which the "each group needs at least one sample" guard would have answered long before the prevalence filter. Both are fixed, so the case now asserts what it says it does.
Main landed #226 while this branch was in review, and the two found three of the same defects independently. Main's resolution is taken for all three, since it is already on main: - `erfc`: main's private `_erfc` replaces the `compositional_erfc` added here. Same algorithm (incomplete gamma at a = 1/2, series below x^2 = 1.5 and a Lentz continued fraction above), so only the name and the tests differ; `compositional.jl` and `test_differential.jl` are taken wholesale from main. - `subtree_pd` in `_reaggregate_after_move`: identical fix, main's `own_pd` naming and one-line post-loop form kept. Git had merged the two variants additively, which would have counted every active internal branch twice; that is resolved to a single addition. - The `Main.SV` collision that was aborting the include loop: main renames this file's binding to `CLADE_SV` instead of dropping `const`. Main's rename is kept. - `min_prevalence` in the multinomial engine test: main's eight-sample table is better than the four-sample one here, and is kept. Kept from this branch, because main does not have them: - The raw like-weight ratio in the diversity block. Main's change to that line only renames `c.weight` to `c.like_weight` and keeps `cand.like_weight / tw`, which is the renormalisation Codex objected to on #223: it hands a lone 0.6 placement a weight of 1.0. The divisor is removed, and the mass the tool never placed is reported as `unplaced_reads`. - `_sample_total` and the `total_a`/`total_b`/`totals` arguments on `weighted_unifrac`, `branch_unifrac_contributions` and `unifrac_matrix`, with the route passing each sample's library size. Without them the metric divides by the placed-only mass and undoes the line above. - Per-file testsets in `runtests.jl`. Main fixed the `SV` collision but not the reason one bad binding could silently skip thirty-five unit files and the occupancy probe. - `_cc_find`, and the drag-and-drop assertions that pin the exact post-move PD values and total-PD conservation rather than comparing a positional index against an inequality.
`main` has moved since #230 opened (#225, #229, #231, #233), so the merge conflicted on seven files. Each conflict is resolved in favour of whichever side is now correct, and the leftovers of #230 are dropped where `main` already carries the same fix by a newer route. * `src/server/routes/clade_cumulus.jl` -- keep `main`'s `with_results_db` helper, its `_clade_cumulus_post_impl` split and its blanket 500 handler (#229) instead of #230's hand-rolled `DuckDB.DB`/`DBInterface` open and the removal of the catch. #230's `treeFile` normalisation comment is kept, since #229 landed the code it describes. * `src/analysis/clade_cumulus.jl` -- keep #230's two-rule jplace rationale and `main`'s `any_active = u.direct_count > 0` rewrite; both are additive. * `test/unit/test_clade_cumulus.jl` -- keep #230's acceptance cases and `main`'s generic `subtree_pd` expectation for the moved clade, which does not hard-code #230's branch lengths. Review findings: * Codex P2 (fully unmapped ASVs were not counted as unplaced) -- fixed by #230's own rewrite: `unplaced_reads` is now the difference between the sample's library size over the whole table and the mass the tree carries, so an ASV the tree does not cover is counted in full. No further change. * Codex P1 (unplaced mass was renormalised away by UniFrac) -- fixed by #230's `totals`/`total_a`/`total_b` denominators, which are preserved through the merge. * CodeRabbit (unplaced-reads description reversed the calculation) -- the unplaced reads are *excluded from the tree-tip abundances* but *included in the library-size denominator*; the comment in `CladeCumulusPanel.tsx` and the `unplaced_reads` doc in `api/types.ts` now say exactly that. * CodeRabbit (a move that empties a clade kept its stale activity) -- still live after the merge, in `_reaggregate_after_move`'s leaf branch: the wrapper around a moved-only child keeps the `cumulative_count` of the subtree that just left, so it stayed "active" with a nonzero `subtree_pd` and frequency. The branch now recomputes the count from `direct_count`, as the internal-node branch already did. A regression test drives `validate_drag_drop` over a tree whose `wrapper` has exactly one child and asserts the emptied clade reads zero count, zero frequency and zero PD. * CodeRabbit (route assertions used `occursin`) -- `test_routes.jl` now asserts whole method+path pairs for the four Cladistics/CladeCumulus routes, so a renamed or longer path can no longer satisfy them. Also: the single-candidate rejection reason is now `single_candidate_below_min_lwr (<lwr>)` rather than `weight_sum_out_of_tolerance`, whose sum is in tolerance by construction; the two tests that pinned the old wording are updated. Frontend: `tsc --noEmit -p .` clean. Julia is not installable here, so the Julia side is checked statically: tree-sitter parses every touched file with no ERROR/MISSING node, and every new field access resolves against the current struct definitions. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
A clade emptied by a drag-and-drop moves leaves with a zero count, zero frequency and zero `subtree_pd` after the previous commit, but it still carried the `residual_count` derived from the subtree that had just left -- so the proposal showed an empty clade inside a positive uncertainty cloud, and its parent added that stale residual a second time on top of the residual the moved subtree reports at its new location. `residual_count` measures uncertainty in the reads a clade holds. A clade left holding none has none to report, so the leaf branch derives it from the same `direct_count` as the count and PD. A clade that still holds reads keeps the residual measured for them. The regression test now pins both sides: the emptied wrapper reports `residual_count == 0`, and the moved clade's residual is unchanged. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The synchronize event for 58848d9 reached CodeQL but not CI, DOI or Governance, so the Julia suite has not run against the residual fix yet. An empty commit re-fires all of them. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com>
`main` landed its own answer to most of #230 while this branch was open: #236 adopted the asymmetric placement tolerance (shortfall tolerated to 0.05, excess rejected at 1e-9) and #237 added the unplaced-reads noise floor. So this merge gives up re-litigating the tolerance and keeps `main`'s model verbatim: `PLACEMENT_WEIGHT_SUM_TOL = 0.05`, `PLACEMENT_WEIGHT_OVER_TOL = 1e-9`, the asymmetric parse branch, and the partition expectations that follow from it (0.96 is a truncated candidate list and is `:valid`; 1.04 is overweight evidence and stays `:undecided`). What is kept from this branch is what `main` does not have: * the library-size denominators (`_sample_total`, `totals` / `total_a` / `total_b`) that stop weighted UniFrac renormalising unplaced mass back onto the tips that were placed -- the Codex P1 finding; * the route's whole-table `unplaced_reads`, which counts an ASV the tree does not cover at all -- the Codex P2 finding -- instead of summing per-row placement shortfalls. It subsumes #237's noise floor: subtracting the placed mass from the library size reports a 0.99 placement's 1% exactly, with no threshold to tune; * the emptied-clade fix (`own_count`, `own_residual`) and its regression test; * the exact method+path route assertions; * the corrected unplaced-reads wording in the panel and the API types. The single-candidate reason stays `single_candidate_below_min_lwr (<lwr>)`: under `main`'s asymmetric rule a lone candidate's sum is in tolerance by construction, so the old wording named a weight-sum error that had not happened. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…on, and explorer behavior
…end parsers Carries #230 (via #235, merged onto current main) forward on this branch and answers the review findings still open on #235: - CodeRabbit: a clade emptied by a drag-and-drop move was still counted as a taxon tip in _reaggregate_after_move (inflating purity/monophyly denominators). count_leaf_taxa! and the leaf branch now share one tip eligibility rule: childless nodes count only with direct reads or a terminal tip/ASV rank. Regression test drives the vacated-wrapper scenario with two domains so root purity (0.5 vs 0.6) discriminates. - Codex P2 on #235 (residual on vacated clades) was already fixed by 58848d9 on that branch; verified present after the merge. Security/quality (panic-attack InputBoundary, advisory): - SSE handlers in api/events.ts no longer throw on a malformed frame. - parseJplace reports invalid JSON as a jplace error instead of a raw SyntaxError; the Newick quoted-label reader no longer builds its string by concatenation in a loop (pons string-concat-in-loop). Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
This was referenced Oct 23, 2026
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Fork governance
🟣 critical · commit
Hypatia scan: 141 finding(s)
panic-attack assail: 17 finding(s)
pons asinorum: 8 finding(s)
rhodibot (RSR compliance): not_run — enrolled, but no report-back was received
panicbot (panic recovery, error handling): not_run — enrolled, but no report-back was received
echidnabot (formal verification: proofs/): not_run — enrolled, but no report-back was received
seambot (integration health: API routes): not_run — enrolled, but no report-back was received
finishingbot (release readiness): not_run — enrolled, but no report-back was received
accessibilitybot (WCAG on the React UI): not_run — enrolled, but no report-back was received
glambot (presentation quality): not_run — enrolled, but no report-back was received
cipherbot: not_applicable — no cryptography is implemented in this repository
sustainabot: not_applicable — fleet adapter slot reserved; no adapter exists yet (gitbot-fleet README)
Scope: static findings reported by each row's tool at the named commit. Purple, red, yellow and blue grade the worst finding present; orange is evidence missing, malformed or a scanner that did not run; grey is an explicit scope exclusion with its reason. No row is a security certification. report.json, badge.json and the raw findings are in the verdict-grid artifact on the linked run. |
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
`main` now holds all of it: #236/#237 the asymmetric placement tolerance and the unplaced-reads accounting, #239 the #235 work itself -- the library-size denominators (`_sample_total`, `totals`), the route's whole-table `unplaced_reads`, the emptied-clade `own_count`/`own_residual` fix with its regression test, the exact method+path route assertions, the corrected unplaced-reads wording, and the vacated-clade tip-count rule. So this merge takes `main`'s side throughout, including over the two CodeRabbit autofix commits that landed on the branch afterwards: `main`'s in-tree tip rule is the same rule as the branch's `counts_as_tip` helper, and the branch's extra 'zero-read terminal' testset asserted a target purity of 0.5 where the code computes 2/3 (two GenusA tips out of three, the unassigned tip included), so it could not have passed. Nothing here needs re-landing. The branch tree is now identical to `main`. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…green Julia tests on main) (#240) `main` is red on `Julia tests` — 5727 pass, **1 fail**, 0 error — and it was already red when #239 merged: that PR's own `Julia tests` check failed on the same line. ``` Dual-hierarchy CladeCumulus, polyphyly detection, drag-and-drop, and HTTP routes: Test Failed at test/unit/test_clade_cumulus.jl:730 Expression: ≈((jp_body.diversity.weighted_unifrac[1])[2], 0.15636363636363637, atol = 1.0e-9) Evaluated: 0.1378181818181818 ≈ 0.15636363636363637 ``` ## Which number is right The expectation predates #239's own fix, which its description states as "`totals`/`total_a`/`total_b` library-size denominators". The route now divides by each sample's 55-read library instead of the 34 and 20 reads that happened to reach the tree, which is the whole point of that change — otherwise the unplaced mass is renormalised back onto the tips. Working the fixture out from that definition, for the grafted tree of `((Ref_1:0.10{0},Ref_2:0.20{1})0.96:0.30{2},Ref_3:0.40{3}){4};` with ASV_1 placed at LWR 0.60 on edge 0 (`distal 0.05`, `pendant 0.02`) and ASV_2 at LWR 1.00 on edge 1: | edge | length | SampleA (24 / 10 placed) | SampleB (20 placed) | | --- | --- | --- | --- | | ASV_1 pendant | 0.02 | 24/55 | 0 | | Ref_1 remainder | 0.05 | 24/55 | 0 | | ASV_2 pendant | 0.02 | 10/55 | 20/55 | | Ref_2 remainder | 0.15 | 10/55 | 20/55 | | joining clade | 0.30 | 34/55 | 20/55 | | Ref_3 | 0.40 | 0 | 0 | ``` raw weighted UniFrac = (0.02*24 + 0.05*24 + 0.02*10 + 0.15*10 + 0.30*(34-20)) / 55 = 7.58 / 55 = 0.1378181818... ``` That is exactly what the route returns, so the code agrees with its documented definition and the constant is stale. Nothing else in the block changes: `mapped_tips == 2`, `unplaced_reads` 21/35 and `root.cumulative_count ≈ 110` all still hold, and no other test in the suite fails. ## Change One test assertion. It now spells out the arithmetic — `≈ 7.58 / 55` with the edge-length derivation in a comment — so a future convention change has to argue with the definition rather than with a bare decimal. No production code is touched. ## Verified The full suite on this branch is CI; locally, tree-sitter parses the edited statement and `git diff` shows only the assertion. Everything else on `main` (including #238's rooting tests) passed in the run that caught this. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…ule (#246) The last uncovered piece of the CladeCumulus move logic, plus one comment that was carried across from #232. This is what remains after #230, #232, #235 and #239 were dispositioned — all four are now closed, with every functional change they made already on `main`. ## What this adds **A test for the second half of the tip rule.** `_reaggregate_after_move` treats a childless clade as a tip when it holds reads *or* when its rank is a sequence terminal: ```julia counts_as_tip = own_count > 0.0 || u.rank in ("tip", "ASV") ``` The emptied-wrapper test on `main` pins the first half — a clade vacated by a move stops being a tip. The second half had no test, so nothing would have caught it being dropped. The new case moves a `GenusB` terminal into a clade holding a read-bearing `GenusA` tip, a **zero-read** `GenusA` tip and an unassigned zero-read tip, and runs both `"tip"` and `"ASV"` ranks. The two monophyly assertions are the regression guard, and they fail in the right direction. With the rule intact `GenusA` has two tips, so neither reads `true`. Drop the rank half and the zero-read tip leaves the global per-taxon tip count, `GenusA` looks like a singleton, the read-bearing tip flips to `true`, and the zero-read tip gets no verdict at all (`nothing`) instead of `false`. Taxon purity is asserted as well, but deliberately not as the guard: it reads `0.5` either way, because the tie-break between two equally sized genera lands on the same number by a different route. That is noted in the test so a future reader does not treat it as the thing that catches the regression. Every expected value was derived by porting `_reaggregate_after_move` to a scratch script and running the fixture under both the intact and the weakened rule — not by reasoning about it in my head. That mattered: my first draft of the comment claimed purity would move to `2/3`, which the port showed was wrong. **One comment, from #232.** Above `DuckDB.unregister_data_frame` in the `.jplace` test: the registration is a connection-local replacement scan, so leaving it in the file makes the view unreadable for every later connection. This was the only line of #232 that had no equivalent on `main`. ## Validation * tree-sitter parses the touched file with no ERROR/MISSING node. * Julia is not installable in this environment (the pinned runtime lives on `julang-s3`, unreachable here, source-only on the GitHub mirror), so CI is the real check — the `Julia tests` job exercises exactly this file. I will follow it and fix anything it turns up. ## Correction on the record On #235 I closed with a claim that the "zero-read terminal" testset from a CodeRabbit autofix commit "could not have passed". That was wrong, and I have posted a correction there. It would have passed; the reason I reached a different outcome (taking `main`'s side in the merge) was that `main` already had the equivalent rule with the emptied-wrapper assertion — not that the test was broken. --------- Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…N with 400 (#244) ## Change and reason Three things, in the order the linked review asked for them. ### 1. PR #228's merge conflicts, resolved (and shown to be a no-op) PR #228 (`arena/7805092e`) branched from `8df1041`; all six files it touched have since been rewritten on `main` by #231, #233, #234, #236, #237 and #239. Resolving the six conflicted files in favour of `main` produces a tree that is **byte-identical** to `main` (`git diff --stat origin/main` is empty), so the merge commit records the resolution and changes no content. Concretely: * `ci.yml` - main publishes a per-assertion digest of every failing validated-statistics test plus a log tail; #228's awk windows are superseded. * `src/analysis/clade_cumulus.jl` - main already reads `PhyloNode.edge_number` in `_graft_jplace_queries`, already sums `rk.subtree_pd` plus the node's own branch, and additionally recomputes an emptied clade from `direct_count`. * `src/server/routes/clade_cumulus.jl` - main already uses `with_results_db` and `String(strip(...))`, and deliberately does **not** renormalise placement like-weights by the retained total: `parse_jplace` already rejects ratios outside [0, 1], a multi-candidate placement already partitions its mass, and dividing a lone 0.6 candidate by its own total would move the 0.4 the tool left unplaced onto the reported edge (#236/#237 report it as `unplaced_reads` instead). * `src/server/routes/validated_statistics.jl` - main validates raw tokens for the whole body after `ijson_read` proves it I-JSON, and streams bounded rows instead of materialising a DataFrame. Both are strict supersets. * `test/unit/*` - main's suites are far larger; the one test #228 had that main lacked ("Nested-clade drag-and-drop counts each active branch once") is restored here. ### 2. Codex P2 on PR #228: leaf subtree PD `_reaggregate_after_move` credits an active leaf with its own incoming branch, so a parent can sum `subtree_pd` alone and count each active edge once. The ordinary builder (`build_node`) hard-coded `0.0` for leaves, so validating a drag-and-drop changed the reported PD of leaves whose topology had not changed and made the proposed tree's metrics incomparable with the original. Both builders now take a leaf's PD from `clade_faith_pd` - the function the internal-node branch already used - so the conventions cannot drift again. Internal nodes and inactive tips are unchanged. ### 3. panic-attack `InputBoundary`: malformed bodies are 400s 35 unguarded `JSON3.read(String(req.body))` call sites across eight route files raised a JSON3 parse error out of the handler, so a truncated or hostile body produced a 500. Every route now parses through `Server._request_json`, which throws the existing `BadRequest`. The helper copies `req.body`, because `String(::Vector{UInt8})` can consume the buffer. Rescuing that turned out to need one more step, which CI caught: Oxygen builds its chain as `custom(serializer(router))` and its serializer catches every handler exception and answers 500 before our middleware runs, so `_bad_request_middleware` never saw the throw. Oxygen's error mapper is the hook inside the serializer, and it is already a dispatch point (`handle_error(::ValidationError)` answers 400). Extending it for `BadRequest` keeps this server's `{"error", "message"}` shape and makes the test's `status == 400` true. ### 4. pons asinorum: tree path building is linear The six `string-concat-in-loop` hits in `TreeShapes.tsx` were real: every node appended to four shared strings inside a `useMemo` over the whole layout, which is quadratic in the node count on a view that renders every node. The fragments are collected per node and joined once; the emitted order is unchanged. ### 5. panic-attack `ProofDrift` and a guarded benchmark baseline The `@test x isa Y` assertions the rule flagged were real tautologies: they passed for any string. The occupancy pair now asserts the text the code emits (`measured 201 KiB above the predicted 101 KiB`) and the evidence round trip asserts the ISO-8601 UTC stamp `save_document` writes. The advanced zero-handling benchmark parsed its baseline outside any guard, so a truncated baseline surfaced as a JSON3 failure from inside the timing loop; it now names the file it could not read. ## Validation - `cd frontend && npx tsc --noEmit -p .` - clean. - `cd frontend && npx vite build` - clean (chunk-size warning only, pre-existing). - `cd frontend && bun test src` - **72 pass, 0 fail** (11 files). - `bash scripts/check-governance-contract.sh` - **all 20 checks passed**. - `bash scripts/lint-sonar-reliability.sh` - clean. - `bash -n` on every `scripts/*.sh` - clean. - All four workflows parse as YAML; the DEED lints clean with the pinned `deed_lint.js` from `hyperpolymath/standards@75bd5994` (see below). - **Not run locally:** the Julia suite (no Julia runtime in this environment; the two changed Julia files are covered by CI's `Julia tests` job). ## Hypatia and K9/DEED conformance * **DEED** - `MetaManifold-WebUI_chora.deed` lints clean with the exact linter the reusable lane runs (`bun 1-formats/deed/tools/deed_lint.js`). Note the stdlib Python port in the same directory is older and rejects `#u8`; the JS linter the workflow uses accepts it, so the deed is correct as written. * **K9** - this repository has no `.machine_readable/contractiles/` registry or Mustfile, and `governance.yml` does not call `k9-contractile.yml`, so the K9 contractile lane is not in scope here. What *was* wrong is that the deed declared `:report-backs ".gitbot-fleet/report-backs/"` while the directory did not exist; it now does, with the envelope contract documented, and the grid keeps rendering those members orange until a real report-back lands. * **Hypatia** - the 43 critical findings are all `cicd_rules/banned_language_file` for `frontend/src/**/*.ts`. That rule is a repo-side-unfixable policy finding: TypeScript's only carve-outs are narrow path prefixes (`bindings/typescript/`, `.d.ts`, `vite.config.ts`, ...), none of which cover an upstream-authored React frontend, and the rule cannot be suppressed from the repository. Migrating upstream's frontend to AffineScript is an estate project, not a change this fork can make. * **panic-attack / pons asinorum** - most remaining findings are stale or false positives at `main`: `api/events.ts` and `tree/model.ts` already wrap `JSON.parse` in try/catch (hardened in #239), the `DynamicCodeExecution` hits are `RCall.reval` (R source strings, not Julia `eval`), and the `unreachable-after-jump` hits point at plain arrow-function bodies. The two classes that were real - unguarded request-body parsing and quadratic path building - are fixed above. The grid's residual after this PR: Hypatia 45 critical (all `banned_language_file` on upstream TypeScript), 61 high, 35 medium; panic-attack 6 high (all `RCall.reval` false positives), 7 medium; pons asinorum 2 medium (`unreachable-after-jump` on a `useMemo` return and a `useEffect` cleanup, both false positives). ## Scope - [x] Focused on the linked review plus conformance evidence - [x] Does not alter dependencies, lockfiles, generated assets, or vendored files - [x] Secrets, local paths, and run outputs are not committed ## Contribution checks - [x] I followed CONTRIBUTING.md - [x] My commits include `Signed-off-by:` (DCO) and the licence is unchanged --------- Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #230 and #235. This branch is #235 (which already carried #230 merged onto
main) fast-forwarded onto currentmain(1f8701d, incl. #237), plus the fixes below. No merge conflicts remain: the seven-file conflict set documented on #235 was resolved there, and this branch merges cleanly.Disposition of every app finding on #230 / #235
totals/total_a/total_blibrary-size denominators); carried here, verified presentunplaced_reads= library size − tree-carried mass); carried hereapi/types.tsnow say: excluded from tip abundances, included in the library-size denominator); carried here_reaggregate_after_moverecomputes fromdirect_count); carried hereresidual_count(#235)58848d9; verified present here, no duplicate changecount_leaf_taxa!and the leaf branch of_reaggregate_after_movenow share one tip-eligibility rule (direct reads, or terminaltip/ASVrank). New regression assertion: two-domain move scenario where root purity is 0.5 with the fix vs 0.6 if the wrapper counts as a fifth tipoccursinroute assertions (#235)The unresolved review threads on #230 and #235 are being resolved with pointers to this PR; Codex is at its usage limit (per its own comments on #230), so no re-review was available from it.
Hypatia / DEED (K9-format estate governance)
.deedfile touched). Local contract check:scripts/check-governance-contract.sh— all 20 checks pass.cicd_rules/banned_language_fileagainst the upstream TypeScript frontend: this fork's deed states the frontend is upstream-owned, so rewriting it is out of scope and a.hypatia-baseline.jsonacknowledgement is an owner decision — deliberately not committed here, because committing one (withscripts/apply-baseline.sh) arms the blocking gate inhypatia-scan-reusable.yml, and that gate cannot be exercised locally (no Elixir/Julia toolchain in this environment). Arming an untested blocking gate would risk turning every PR red.InputBoundary, advisory): SSE handlers infrontend/src/api/events.tsno longer throw on malformed frames;parseJplaceinfrontend/src/tree/model.tsreports invalid JSON as a jplace error. Also fixed the ponsstring-concat-in-loopin the Newick quoted-label reader.JSON.parse(JSON.stringify(...))deep-clones incharts.ts/alphaMetrics.tsx(cannot throw on malformed input — the input is a live object), the remaining pons string-concat heuristics inTreeShapes.tsxpath building, and panic-attack's per-fileJSON3.readtry-block counts on Julia routes (requests are handled behind the routes' error boundary; changing six route files blind, without a Julia runtime, is riskier than the finding).Validation
tsc --noEmit -p .clean.git diff --checkclean; governance contract script passes (20/20).🤖 Generated with Claude Code