Repository navigation
Address PR #232 app comments: asymmetric placement tolerance + import cleanup (rebased onto 688d16a) - #236
Merged
Conversation
…e duplicate Tools import - CladeCumulus: tolerate weight shortfall to 0.95 (PLACEMENT_WEIGHT_SUM_TOL=0.05) while rejecting any overweight beyond 1e-9 (PLACEMENT_WEIGHT_OVER_TOL). Keep single-candidate 0.5 minimum (PLACEMENT_SINGLE_MIN_LWR) for the existing single-tip test; multi-candidate sums now use the asymmetric bounds so 0.96 (0.80+0.16) is valid and 1.04 remains undecided, matching the documented rationale that filtering may drop mass but never gains it. This addresses chatgpt-codex-connector P2 on PR #224/#232. - Phylogeny: remove duplicate `import ..Tools` (sonar maintainability). - Test: update _partition(0.80,0.16) expectation to :valid to reflect the tolerated shortfall; the previous :undecided expectation enforced a symmetric 1e-5 bound that contradicts the 5% shortfall documented above. Co-authored-by: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
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 57 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: 19 finding(s)
pons asinorum: 10 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4e97392fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # A sum over 1 is never legitimate: weights are mutually exclusive placement | ||
| # ratios, so overweight evidence is malformed and is rejected by a tight upper | ||
| # bound while the lower shortfall is tolerated. | ||
| const PLACEMENT_WEIGHT_SUM_TOL = 0.05 |
There was a problem hiding this comment.
Raising this shared constant to 0.05 also changes src/server/routes/clade_cumulus.jl:313-314, where it suppresses reporting any unplaced mass below 5%. For example, the existing route fixture has a valid placement with LWR 0.99; its missing 1% was previously returned as 0.05/0.10 unplaced reads, but now 0.99 < 0.95 is false, so both values become zero and the assertions at test/unit/test_clade_cumulus.jl:477-478 fail. Use a separate floating-point epsilon for unplaced-read accounting so the relaxed parser threshold does not hide discarded mass.
Useful? React with 👍 / 👎.
| push!(queries, QueryPlacement(q_ord, q_names, cands, :undecided, | ||
| "weight_sum_out_of_tolerance ($(round(weight_sum; digits=6)))", nothing)) | ||
| elseif length(cands) > 1 && abs(weight_sum - 1.0) > PLACEMENT_WEIGHT_SUM_TOL | ||
| elseif length(cands) > 1 && (weight_sum > 1.0 + PLACEMENT_WEIGHT_OVER_TOL || weight_sum < 1.0 - PLACEMENT_WEIGHT_SUM_TOL) |
There was a problem hiding this comment.
When this condition newly admits a partition such as 0.80 + 0.16, _graft_jplace_queries still computes each share as cand.like_weight / total_w at lines 1010-1022. That turns the weights into approximately 0.8333 + 0.1667 and allocates 100% of the ASV's reads to the grafted tips, manufacturing the missing 4% that the new comments describe as discarded or unplaced; before this change such a query never reached grafting because it was undecided. Preserve the raw candidate weights, or explicitly represent the missing share, so clade counts and frequencies do not disagree with the raw-LWR diversity calculation.
Useful? React with 👍 / 👎.
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
`main` is currently red on `Julia tests` — 5679 pass, **2 fail**, 0 error — and both failures come from #236, whose push-run CI was cancelled by the workflow concurrency group before the suite ran. ``` Dual-hierarchy CladeCumulus, polyphyly detection, drag-and-drop, and HTTP routes: Test Failed at test/unit/test_clade_cumulus.jl:591 Expression: ≈(jp_body.diversity.unplaced_reads.SampleA, 0.05, atol = 1.0e-9) Evaluated: 0 ≈ 0.05 Expression: ≈(jp_body.diversity.unplaced_reads.SampleB, 0.1, atol = 1.0e-9) Evaluated: 0 ≈ 0.1 ``` ## Cause #236 relaxed `PLACEMENT_WEIGHT_SUM_TOL` from `1e-5` to `0.05` so that a candidate list truncated by the placement tool still parses as `:valid` — a correct change for *validity*. But the `unplaced_reads` field of the clade-cumulus diversity payload reused the same constant as its reporting floor: ```julia short_rows = [(ri, 1.0 - wsum) for (ri, wsum) in row_weight if wsum < 1.0 - MetaManifold.CladeCumulus.PLACEMENT_WEIGHT_SUM_TOL] ``` With `TOL = 0.05`, a row at weight `0.99` no longer qualified as short, so a placement at LWR 0.99 — whose remaining 1% genuinely never reached the tree — reported `0` unplaced reads instead of `0.01 × reads`. The test's placement (`ASV_4` at LWR 0.99, 5 and 10 reads) therefore expected `0.05` and `0.10` and got `0`. ## Fix Reporting now uses the noise floor the metric actually means, `PLACEMENT_WEIGHT_OVER_TOL` (`1e-9`, added by #236 for the opposite bound of the same check): ```julia noise_floor = MetaManifold.CladeCumulus.PLACEMENT_WEIGHT_OVER_TOL short_rows = [(ri, 1.0 - wsum) for (ri, wsum) in row_weight if 1.0 - wsum > noise_floor] ``` `1.0 - wsum > 1e-9` is `wsum < 1.0 - 1e-9` for all shortfalls and additionally excludes overweight rows, which a shortfall comparison cannot express. The comment records why the two tolerances are not interchangeable, so the next change to the placement tolerance cannot silently re-break the report. No test changes: the two existing assertions are exactly the regression guard, and they were the only failures in the suite — my new indexed-tree testset passed (`CladeCumulus & Phylogenetic Integration`: 196 pass, 0 fail). Also worth knowing for future merges: `actions/runs/39967407149` (the push run for `b8b6c9e`) shows `cancelled`, which is why #236 could merge without its suite running. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
`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>
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>
This was referenced Oct 23, 2026
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 #232 and resolves #224 review findings against current
main(688d16a).Codex (chatgpt-codex-connector) — P2
P2 — Apply the relaxed tolerance only to weight shortfalls (#224#discussion_r4357212802, carried into #232).
PLACEMENT_WEIGHT_SUM_TOL=1e-5symmetric withPLACEMENT_SINGLE_MIN_LWR=0.5— shortfall of 0.96 (0.80+0.16) wasundecided, overweight 1.04 was alsoundecidedbut for the same threshold, so the documented rationale (filtering may drop mass, never gains it) was not enforced.PLACEMENT_WEIGHT_SUM_TOL=0.05lower shortfall +PLACEMENT_WEIGHT_OVER_TOL=1e-9tight upper.parse_jplacenow uses forlength(cands)>1:weight_sum >1+OVER || weight_sum <1-TOL. Singles keep the existing0.5minimum (so_single_at(0.60)staysvalid). This makes the Codex example1.04stayundecidedwhile0.98and0.96arevalid._partition(0.80,0.16)now expects:valid(was:undecidedunder the symmetric 1e-5). Added inline rationale in commit message; the old expectation enforced the bug.P1 (QueryCandidate
distal_len/pendant_len/like_weight) was already correct onmainat 688d16a (src/analysis/clade_cumulus.jl:1026usescand.distal_lenetc.) and is kept.Other app/CI findings
src/pipeline/phylogeny.jlhad twoimport ..Toolslines (one before and one afterusing ..Tools: …). Removed the first, keeping the singleimport ..Toolsthat letsPH.Tools._toolsresolve (same fix PR Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232 added, but de-duplicated against 688d16a's version which already had it).main'sAbstractStringboundary for_trees_dir/_study_exists/_tree_request_error(PR Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232 narrowed to::String, which wouldMethodErroronSubStringstudy/run).main's streamingDuckDB.Tables.partitions+ whole-body_reject_noninteger_json_tokens+ijson_readpath (87092cc/fc715a0). PR Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232's DataFrame+ORDER BY feature LIMIT MAX_CELLS+1+ counts-only scanner is intentionally not taken — it reverts the overflow-safe streaming fix.main's correctu.edge_numberand subtree-PDbranch + Σ child.subtree_pd(87092cc/d337f74). PR Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232 re-introducedu.edge_numand the double-countingmax(branch_child)+subtreeleaf-0 form; those regressions are not taken.mainalready includes cladistics once at 688d16a, so no change.mainat 688d16a already publishes the grep/split digest that PR Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232 added (fc715a0 + 688d16a), so not duplicated.bash scripts/lint-sonar-reliability.shis green on this branch (S2871/S7758 clean;byCodeUnitandCHARCODE_ALLOWEDkept from 688d16a).purplerepo-wide (43 critical) but the lane is advisory pergovernance.yml(fix(ci): keep advisory… #231); CodeRabbit was rate-limited on Resolve PR #224 against main: validated-statistics + cladistics fixes, review findings, and merge conflicts #232 and SonarCloud passed there — no new findings introduced here.main; localgit diff --checkclean.Merge-conflict note
#232 was branched from 8df1041 and conflicts with
main(688d16a). This branch is rebased onto 688d16a and reapplies only the non-regressive parts of #232, so it is mergeable without the DataFrame/edge_num/PD regressions.Fixes the actionable parts of #232's failing
Julia testscheck while preserving #232's intent.—
Co-authored-by: arena-agent