Repository navigation
Fix CladeCumulus graft edge field and tree-file validation type - #229
Merged
Merged
Conversation
…rance, route table loading The CladeCumulus testset (the last red testset in the Julia suite: 817 pass / 3 fail / 14 error, all inside it) shipped in #207 with defects that the long-broken build had masked: - _graft_jplace_queries read QueryCandidate.weight/.distal_length/ .pendant_length; the struct fields are like_weight/distal_len/ pendant_len (FieldError on any valid jplace graft). - A single retained placement at LWR 0.98 was marked undecided: PLACEMENT_WEIGHT_SUM_TOL=1e-5 rejects truncated candidate lists whose retained ratios legitimately sum to slightly less than 1. Tolerance is now 0.05; a 0.40 sum stays undecided. - The Newick label reader consumed an NHX comment after ')' without capturing it, dropping [&&NHX:B=92] support; capture refs are now threaded through read_label_and_quoted. - The sources route listed views via SHOW TABLES and then failed to bind a session-registered DataFrame view on a fresh connection (500); it now enumerates real tables via duckdb_tables(). - _load_run_table_df rejected the SubString table names JSON3 returns; arguments are normalised to String at the boundary. - Test used PH.Tools, but Phylogeny imports only selected names from Tools; the mock-tool override now addresses MetaManifold.Tools. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
src/core/doi.jl ended with 'end # module DOI' but never opened the module, so including it into MetaManifold failed to parse (ParseError at doi.jl:681) and no Julia test could even load the package. Routes and tests address MetaManifold.DOI, so the file is a submodule whose opener was lost in an earlier repair. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
…_tree_file - The jplace graft walk read u.edge_num/cur.edge_num; PhyloNode's field is edge_number (QueryCandidate.edge_num is a different, legitimate field). Grafting any valid placement threw FieldError. - _valid_tree_file only accepted ::String, but strip() of a request field yields a SubString, so POST clade-cumulus with treeSource study_tree returned 500. It now accepts AbstractString. 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 24 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: 133 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: 238598e8e0
ℹ️ 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".
| for q in jd.queries | ||
| q.status === :valid && !isempty(q.candidates) || continue | ||
| total_w = sum((c.weight for c in q.candidates); init = 0.0) | ||
| total_w = sum((c.like_weight for c in q.candidates); init = 0.0) |
There was a problem hiding this comment.
When a CladeCumulus POST uses a valid .jplace file and the selected table has sample columns, execution now gets past this corrected grafting code but reparses the candidates in src/server/routes/clade_cumulus.jl:279-285 using c.weight and cand.weight. QueryCandidate exposes only like_weight, so this raises a FieldError outside the route's analysis try/catch and the request still returns 500 instead of the diversity result; update those remaining accesses as part of this field-name fix.
Useful? React with 👍 / 👎.
| _tree_format(file::String) = endswith(lowercase(file), ".jplace") ? "jplace" : "newick" | ||
|
|
||
| function _valid_tree_file(file::String)::Bool | ||
| function _valid_tree_file(file::AbstractString)::Bool |
There was a problem hiding this comment.
_tree_format
When treeFile comes from the JSON request, _resolve_tree_inputs_from_request obtains a SubString from strip; widening this validator lets that value pass, but the same path then calls the still-String-only _tree_format(tree_file) at src/server/routes/clade_cumulus.jl:113. Thus every treeSource: "study_tree" POST reaches a MethodError before the analysis try/catch and returns 500; convert tree_file to String at the request boundary or widen _tree_format too.
Useful? React with 👍 / 👎.
| # like-weight ratios can legitimately sum to slightly less than 1 (a single | ||
| # retained placement at 0.98 is complete evidence). Sums far from 1 indicate | ||
| # a malformed or partial record and stay undecided. | ||
| const PLACEMENT_WEIGHT_SUM_TOL = 0.05 |
There was a problem hiding this comment.
When malformed .jplace candidates total between 1 and 1.05, such as 0.60 + 0.44, this symmetric tolerance marks the query valid even though candidate truncation can only explain a total below one. The grafting path then normalizes those invalid weights into abundance shares and reports derived clade and diversity results rather than leaving the query undecided; keep only a small rounding epsilon above 1 while applying the wider allowance on the lower side.
Useful? React with 👍 / 👎.
…ing like_weight fields The POST handler let unhandled exceptions escape as bare Oxygen 500s with no body, leaving CI annotations with nothing to diagnose. It is now wrapped: failures are logged with a backtrace and returned as a structured clade_cumulus_failed error naming the exception. Also fixes the last QueryCandidate .weight reads in the diversity tip-weight computation (field is like_weight), and the route test surfaces the error body when the status assertion fails. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
_tree_format only accepted ::String, but the clade-cumulus route passes strip() output (a SubString) for the requested tree file, so study_tree requests failed with MethodError (surfaced by the new structured clade_cumulus_failed body). The tree helpers now accept AbstractString throughout. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…SV in validated tests - _reaggregate_after_move's recompute stored subtree_pd excluding the node's own stem branch, while the build path stores clade_faith_pd (stem-inclusive except at the root). A proposed move into a clade therefore appeared to *reduce* its PD. The recompute now threads stem-inclusive PD up the recursion and stores it with build-path semantics. - test_validated_statistics.jl assigned SV without const; with test_clade_cumulus.jl's const SV in the same Main session the full suite died with 'invalid assignment to constant Main.SV' before the file's tests could run. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Every server test file assigns SV = MetaManifold.Server at top level; test_clade_cumulus.jl (and briefly test_validated_statistics.jl) declared it const, so whichever file runtests.jl included next died with 'invalid assignment to constant Main.SV' in the full-suite run. Plain assignment everywhere matches the established convention. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…mial test config - _wald called bare erfc, but CompositionalModels deliberately carries no SpecialFunctions dependency (local loggamma/digamma), so every multinomial/Dirichlet-multinomial fit died with UndefVarError. compositional_erfc computes Q(1/2, x^2) by series / Lentz continued fraction (machine-precision against libm erfc). - The 'no composition to model' test passed min_prevalence as a keyword; it is the second positional of DifferentialConfig. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Main landed #226, which contains equivalents of this branch's _erfc implementation, min_prevalence test fix, edge_number graft fix and stem-inclusive subtree PD, plus #217's validated-statistics implementation and the CLADE_SV rename that removes the Main.SV const collision. Conflicts resolved in favour of main for those; this branch keeps its unique additions: AbstractString tree helpers and the structured clade_cumulus_failed catch-all with the like_weight diversity fixes. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
…ings (#234) This is #227's contribution, re-landed on current `main` with its review findings fixed. ## Why not merge #227 directly #227's branch point is `8b76b71`, which predates #222, #229 and #233 on `main`, so its diff *also* reverted: - NHX `[&&NHX:B=…]` capture threading in `parse_newick` (#222) - `PLACEMENT_SINGLE_MIN_LWR` handling for single-candidate placements (#222/#233) - the `edge_number` graft walk fix (#229) - `unplaced_reads` in the clade-cumulus diversity payload and its test (#233) - Canonical I-JSON writers (`jcs_json`/`ijson_read`) and the `AbstractString` request boundaries in `routes/trees.jl`, `routes/clade_cumulus.jl`, `src/server/server.jl` Taking the union naively would have regressed all of that, so this branch takes **only** what `main` is genuinely missing. ## What `main` is actually missing `src/server/routes/cladistics.jl` calls `CC.asv_tip_name` (line 123) and `CC.analyse_phylogeny` (line 211), and `bench/cladistics.jl` calls the latter, but neither was defined after the #207/#206 merges — every cladistics build and every compute job died with `UndefVarError`/`MethodError`. Restored here as an indexed study-tree layer alongside the existing `PhyloNode` API: | Symbol | Purpose | | --- | --- | | `IndexedPhyloNode`, `PhylogeneticTree` | parents-precede-children node vector + `tips` index | | `parse_phylogenetic_tree` | strict Newick → `PhylogeneticTree` (quoted/Unicode labels, comments, support labels, non-binary nodes) | | `asv_tip_name` | content-addressed, tool-safe ASV tip names | | `clade_counts` | nodes × samples cumulative roll-up | | `unifrac` | raw samples × ASVs **or** an existing roll-up, unweighted/weighted | | `analyse_phylogeny` | the single call whose NamedTuple the cladistics route writes to `results.json` | The payload matches `CladisticsResult` in `frontend/src/api/types.ts` exactly. ## Codex findings from #227 1. **P1 — `SubString` from the byte buffer.** `_ip_label!` and `_ip_length!` called `SubString(p.bytes, first:last)` on a `Vector{UInt8}`, which has no such method, so every ordinary unquoted label and branch length raised `MethodError`. Labels/lengths are now sliced straight into `String`s; the label return is an explicit `String` instead of a `strip` SubString. 2. **P1 — graft walk read `u.edge_num`.** Already fixed on `main` by #229 and not reintroduced: `edge_num` now only ever names `QueryCandidate`'s field, and the new code touches no `PhyloNode` field. 3. **P2 — Faith PD accepted invalid cumulative counts.** `_faith_pd_indexed` treated negative/`NaN` entries as absent (`> 0` is false) and returned a plausible but underestimated PD. It now validates finite, non-negative counts like `clade_counts` and `unifrac`. 4. **P2 — raw-count UniFrac overload missing.** `unifrac(tree, counts, tip_names; weighted=…)` now exists and delegates through `clade_counts`, matching the adjacent `faith_pd(tree, counts, tip_names)`. ## Also - `bench/cladistics.jl` builds its tree with `parse_phylogenetic_tree` (it calls `analyse_phylogeny`, which needs the indexed type). - Dropped the duplicated `import ..Tools` in `src/pipeline/phylogeny.jl`. - New testset covers the parser (byte-buffer labels, multi-byte/quoted labels, trailing comments, duplicate tips, negative lengths, missing/extra `;`), the roll-up, Faith's PD including the new rejections, both UniFrac variants and the raw-count overload, and the JSON payload with the same row-major conversion the route performs. - `docs/cladistics/clade-cumulus.md` documents the new API. ## Verification - tree-sitter-julia parse of every touched file: clean (the two files over the JS binding's 32 KiB limit were parsed in top-level chunks; the inserted block parses standalone). - No Julia runtime is reachable from the authoring sandbox (julialang-s3 and GitHub release assets are blocked), so the Julia suite is CI on this PR. #227 can be closed once this lands. --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Oct 23, 2026
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
## Scope Security/governance follow-up to #227. All four Codex findings on that old PR already landed in #229/#234 and follow-ups; I answered/resolved its threads and closed it as superseded, rather than merging a conflicting branch that could undo newer fixes. CodeRabbit's remaining notices are account limits/billing, not code questions. ## Changes - Add pinned, least-privileged full-history/working-tree secret scanning and default-branch Scorecard publication; no inherited secrets or new alert suppressions. - Add Dependabot, SECURITY.md, editor settings and a repository-specific AI manifest without changing existing licensing/architecture. - Add independent pinned K9 conformance and its artifact to the PR verdict. The current empty corpus is explicitly grey, not a semantic pass. Missing semantic evidence fails closed. Keep DEED syntax and advisory Hypatia as separate evidence families. - Bound Julia CI at 90 minutes and preserve producer **and** log-writer failures. Add 8 negative/mutation tests and 4 exit-combination tests. - Refresh the bun lockfile within current major ranges for real security advisories, notably DOMPurify, Vite and transitive build dependencies; raise minimum Vite to 6.4.5 and Router DOM to 6.30.6. This is why the lockfile changes. No generated assets are included. - Audit high/critical frontend dependencies in CI. ## Verification Bun **1.3.10**: frozen install, TypeScript checking, **72 frontend tests / 200 assertions**, production build, and high/critical audit pass. Full audit reduced from **31 advisories (1 critical, 13 high, 14 moderate, 3 low) to 2 moderate Router advisories**. This is not a zero-vulnerability claim. Canonical pinned DEED lint, all 39 governance contract checks, 8 weakening mutations, 4 pipeline-exit combinations, unique-key YAML parsing, all embedded shell syntax, Sonar reliability lint, proof axiom audit (7 modules), and git diff checks pass locally. Julia/R/Agda/Nickel are not installed in this sandbox; hosted CI must supply those results. No local runtime/semantic pass is claimed. ## Hosted wrapper fixes The first run found two issues in the pinned standards wrappers. Fork-local copies retain the same immutable standards validator/renderer/config sources and all gates, but fix the terminal optional-log copy for an empty K9 corpus and upgrade Gitleaks from 8.18.4 to 8.30.1 (upstream release-asset digest). The old rules falsely matched historical DNA/FASTQ and schema labels; no history exclusions or local rule suppressions were added. A synthetic AWS credential must still fail in CI. Three K9 copy cases and a credential-sensitivity test also pass locally. Hosted checks are rerunning. ## Remaining / intentionally not done See `docs/release-notes/2026-10-23-governance-security.md` for the source Hypatia run and complete triage. Two moderate React Router advisories need a v7 migration review. Estate language bans conflict with the settled React/TS stack and independent Python reference oracles; W3C SVG namespace URIs are not HTTP requests. Do not fix those by rewriting the architecture or breaking namespaces. No configured mirror target exists. Existing upstream licensing headers remain protected. Fleet bots without report-backs remain not_run. No fake conformance reports have been added. The integration returns 403 for code-scanning/Dependabot alert listing and the sandbox blocks Actions artifact/log storage hosts; no alerts have been dismissed. Published grids/check APIs are still readable. Scorecard's default-branch publication can only be verified after landing; K9 has no committed contracts and therefore cannot claim substantive contract coverage yet. ## PR #228 security follow-up (`1be406d`) Independent verification found that the recorded conflict-resolution tree for #228 is identical to main; #228 is being closed as superseded, not conflict-merged. Its valid leaf-PD finding is already implemented in **unmerged** #242/#244 and is not duplicated here. #242's integration run 39986326202 passed all six jobs; #244's Julia validation remains pending at review time. The R-evaluation warning was not dismissed as a language-detector false positive. This follow-up validates metadata column identifiers before they enter PERMANOVA's parsed R formula and validates package names before provenance interpolates them into R source. Expressions, quotes/escapes, controls and reserved formula terms are covered by regression cases. Plain identifiers preserve the existing formula and scientific tests; non-identifier metadata columns must be renamed, as documented in the function contract. Local verification for the new commit: 39 governance assertions, 8 weakening mutations, 3 K9 copy cases, 4 CI logging cases, scanner-boundary tests, complete working-tree/history shell scanning, git diff checks and syntax parsing of all four changed Julia files pass. **Syntax parsing is not execution**: the new Julia/R regressions await hosted CI. This does not certify all remaining R call sites or dismiss outstanding advisory findings. ### Main advanced during review #242 has now merged as `b656d2a99e53267108d51ad87d7eaee13222d368` (16:46:42 UTC), after its full integration suite passed. The leaf-PD fix is therefore on main. #228 is confirmed closed/unmerged. This PR (#243) now has conflicts with the newly updated main and is **not merge-ready**; no main-merge commit or unrequested history rewrite has been made. The new R-boundary/scanner tests are running at `1be406d`; earlier passing checks must not be mistaken for validation of a future rebased head. --------- Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
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.
Follow-up to #222 (merged before this commit arrived). The Julia suite on #222 reached 825 pass / 1 fail / 9 error, all remaining in CladeCumulus, from two root causes:
u.edge_num/cur.edge_num;PhyloNode's field isedge_number(QueryCandidate.edge_numis a different, legitimate field). Grafting any valid placement threw FieldError._valid_tree_fileonly accepted::String, butstrip()of a request field yields aSubString, so POST clade-cumulus withtreeSource: study_treereturned 500. It now acceptsAbstractString.Julia evidence is CI on this PR.