Repository navigation
Fix post-merge regressions: DOI module, CladeCumulus API, and route type stability - #227
Closed
hyperpolymath wants to merge 2 commits into
Closed
hyperpolymath wants to merge 2 commits into
hyperpolymath wants to merge 2 commits into
Conversation
…ype stability - src/core/doi.jl: restore the missing 'module DOI' declaration so the package loads (was 'invalid identifier' at end of file) - src/server/server.jl: drop the duplicate include of routes/cladistics.jl - src/analysis/clade_cumulus.jl: restore the #200 study-tree API that routes/cladistics.jl depends on (IndexedPhyloNode, PhylogeneticTree, parse_phylogenetic_tree, asv_tip_name, clade_counts, unifrac, faith_pd, analyse_phylogeny); fix _graft_jplace_queries to read QueryCandidate.like_weight/distal_len/pendant_len instead of the removed weight/distal_length/pendant_length; widen PLACEMENT_WEIGHT_SUM_TOL to 5e-2 for jplace writers that prune low-LWR candidates - src/server/routes/clade_cumulus.jl: AbstractString conversions at the request boundary (_load_run_table_df, table/mode strings); skip unreadable DuckDB views in the sources listing; like_weight in the diversity payload - src/server/routes/cladistics.jl: call CC.parse_phylogenetic_tree and CC.analyse_phylogeny (restored #200 API) - src/server/routes/trees.jl: AbstractString for _tree_format / _valid_tree_file - src/pipeline/phylogeny.jl: import ..Tools so Tools.tool_bin resolves - test/unit/test_clade_cumulus.jl: MetaManifold.Tools._tools instead of PH.Tools._tools (Phylogeny has no Tools binding) Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
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. |
|
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 7 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 |
![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: 756509aa1e
ℹ️ 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".
hyperpolymath
enabled auto-merge (squash)
October 23, 2026 07:53
Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
hyperpolymath
disabled auto-merge
October 23, 2026 13:50
hyperpolymath
enabled auto-merge (squash)
October 23, 2026 13:50
Owner
Author
Review findings addressed in #234All four Codex findings are fixed, but not on this branch — this branch point is
So the branch cannot be merged as-is. #234 relands this PR's genuine contribution — the indexed
#234 also adds a testset covering each of those paths (byte-buffer/multi-byte/quoted labels, the roll-up, PD rejections, both UniFrac variants and the raw-count overload, and the JSON payload the UI consumes). Closing this PR in favour of #234 keeps |
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>
|
Add Carrot credits or activate Agent usage billing to use Autofix |
Owner
Author
|
Audit follow-up: all four Codex review threads have been answered with the merged fixes and resolved. #234 explicitly supersedes this PR, with #229, #238, #239 and #240 preserving/following up the related corrections. The old branch conflicts because main already contains these changes plus newer fixes; merging it would risk regressions. Closing as superseded rather than manufacturing a conflict-resolution merge. CodeRabbit’s remaining notices are account review/Autofix billing limits, not code questions; no billing changes have been made. Continuing CI and governance/security work separately on arena/b1571410-metamanifold-webui. |
auto-merge was automatically disabled
October 23, 2026 16:05
Pull request was closed
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.
Repairs the regressions left on
mainafter the #200/#207 CladeCumulus collision and the #206 differential merge:Julia (package load + test suite)
src/core/doi.jl— restore the missingmodule DOIdeclaration;using MetaManifoldpreviously died withParseError: invalid identifierat end of filesrc/server/server.jl— drop the duplicateinclude("routes/cladistics.jl)`src/analysis/clade_cumulus.jl— restore the feat(cladistics): add CladeCumulus ASV phylogenies #200 study-tree API thatroutes/cladistics.jldepends on (IndexedPhyloNode,PhylogeneticTree,parse_phylogenetic_tree,asv_tip_name,clade_counts,unifrac,faith_pd,analyse_phylogeny); fix_graft_jplace_queriesto readQueryCandidate.like_weight/distal_len/pendant_len(the removedweight/distal_length/pendant_lengthfields causedFieldError); widenPLACEMENT_WEIGHT_SUM_TOLto 5e-2 for jplace writers that prune low-LWR candidatessrc/server/routes/clade_cumulus.jl—AbstractStringconversions at the request boundary (_load_run_table_df, table/mode strings) fixing the_load_run_table_df(::SubString)MethodError; skip unreadable DuckDB views in the sources listing;like_weightin the diversity payloadsrc/server/routes/cladistics.jl— callCC.parse_phylogenetic_treeandCC.analyse_phylogeny(restored feat(cladistics): add CladeCumulus ASV phylogenies #200 API)src/server/routes/trees.jl—AbstractStringfor_tree_format/_valid_tree_filesrc/pipeline/phylogeny.jl—import ..ToolssoTools.tool_binresolvestest/unit/test_clade_cumulus.jl—MetaManifold.Tools._toolsinstead ofPH.Tools._tools(Phylogenyhas noToolsbinding)Frontend
tsc --noEmitclean;bun test src66 pass / 0 failVerified locally
R"""..."" raw-string false positives)proofs/tests/axiom-audit.sh: clean (7 modules, AXIOM=0)CI (Julia tests, Agda, DOI, CodeQL, SonarCloud, frontend) will run on this PR; I'll follow it to green and merge.