Repository navigation
fix(ci): keep advisory Hypatia PR scans advisory - #231
Merged
Merged
Conversation
PR #224's red Hypatia result came from two new TypeScript files, not from a failure in the governance job. The advisory reusable uploads critical/high findings as error-level SARIF, so GitHub creates a separate blocking code-scanning-results check for changed files even though the scan job succeeds. Use a fork-local copy of the pinned standards reusable. Keep PR scanning, raw artifacts and the verdict grid, but skip SARIF upload on pull_request events; push and manual scans still reconcile the Security tab. Pin the standards renderer explicitly because job.workflow_sha refers to this repository for a local reusable. Add a pre-scan workflow contract with regression coverage for the local call, event boundary, renderer pin, and continued PR scan/artifact/grid behaviour. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 3
✨ Finishing Touches
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: 140 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. |
Owner
Author
|
Live governance validation on run 39921152469:
Upstream reusable enhancement tracked in hyperpolymath/standards#1327. |
Owner
Author
|
The full CI lane also ran. Its Julia failure is inherited from the current base, not introduced by this workflow-only change. The governance change itself is fully green, including the new contract and the absence of the separate Hypatia code-scanning check. I am leaving the unrelated CladeCumulus repair to its existing work in #224 rather than mixing it into this focused fix. |
|
hyperpolymath
added a commit
that referenced
this pull request
Oct 23, 2026
… cleanup (rebased onto 688d16a) (#236) 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](#224 (comment)), carried into \#232). * Before: `PLACEMENT_WEIGHT_SUM_TOL=1e-5` symmetric with `PLACEMENT_SINGLE_MIN_LWR=0.5` — shortfall of 0.96 (0.80+0.16) was `undecided`, overweight 1.04 was also `undecided` but for the same threshold, so the documented rationale (filtering may drop mass, never gains it) was not enforced. * After: `PLACEMENT_WEIGHT_SUM_TOL=0.05` lower shortfall + `PLACEMENT_WEIGHT_OVER_TOL=1e-9` tight upper. `parse_jplace` now uses for `length(cands)>1`: `weight_sum >1+OVER || weight_sum <1-TOL`. Singles keep the existing `0.5` minimum (so `_single_at(0.60)` stays `valid`). This makes the Codex example `1.04` stay `undecided` while `0.98` and `0.96` are `valid`. * Test: `_partition(0.80,0.16)` now expects `:valid` (was `:undecided` under 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 on `main` at 688d16a (`src/analysis/clade_cumulus.jl:1026` uses `cand.distal_len` etc.) and is kept. ## Other app/CI findings * **Phylogeny duplicate import** — `src/pipeline/phylogeny.jl` had two `import ..Tools` lines (one before and one after `using ..Tools: …`). Removed the first, keeping the single `import ..Tools` that lets `PH.Tools._tools` resolve (same fix PR #232 added, but de-duplicated against 688d16a's version which already had it). * **Trees route** — kept `main`'s `AbstractString` boundary for `_trees_dir`/`_study_exists`/`_tree_request_error` (PR #232 narrowed to `::String`, which would `MethodError` on `SubString` study/run). * **Validated statistics** — kept `main`'s streaming `DuckDB.Tables.partitions` + whole-body `_reject_noninteger_json_tokens` + `ijson_read` path (87092cc/fc715a0). PR #232's DataFrame+`ORDER BY feature LIMIT MAX_CELLS+1` + counts-only scanner is intentionally not taken — it reverts the overflow-safe streaming fix. * **Clade graft edge field / subtree PD** — kept `main`'s correct `u.edge_number` and subtree-PD `branch + Σ child.subtree_pd` (87092cc/d337f74). PR #232 re-introduced `u.edge_num` and the double-counting `max(branch_child)+subtree` leaf-0 form; those regressions are not taken. * **Server duplicate include** — `main` already includes cladistics once at 688d16a, so no change. * **CI validated-statistics digest** — `main` at 688d16a already publishes the grep/split digest that PR #232 added (fc715a0 + 688d16a), so not duplicated. * **SonarCloud** — local `bash scripts/lint-sonar-reliability.sh` is green on this branch (S2871/S7758 clean; `byCodeUnit` and `CHARCODE_ALLOWED` kept from 688d16a). * **Hypatia / CodeRabbit / Codecov** — Hypatia grid remains `purple` repo-wide (43 critical) but the lane is advisory per `governance.yml` (`fix(ci): keep advisory… #231`); CodeRabbit was rate-limited on #232 and SonarCloud passed there — no new findings introduced here. * **Julia tests** — the asymmetric multi tolerance now aligns with the updated community expectation while the remaining 5,450 tests stay as on `main`; local `git diff --check` clean. ## 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 tests` check while preserving #232's intent. — Co-authored-by: arena-agent Co-authored-by: arena-agent <arena-agent@users.noreply.github.com> 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.
Root cause
Investigated against #224 and check run
120302293713. The governance job itself succeeded, but its SARIF upload produced a separate Code scanning results / Hypatia failure for:frontend/src/api/ijsonJcs.tsfrontend/src/api/ijsonJcs.test.tsBoth are
cicd_rules/banned_language_filealerts. Hypatia maps critical/high findings to SARIFerror, and GitHub fails a PR code-scanning results check when a changed line has an error-level alert. This conflicts with the fork governance lane's intended advisory behaviour and with #190's decision to keep the upstream TypeScript policy finding visible rather than baseline it away.Fix
hypatia-scan-reusable.yml, sourced from standards commit01c9b0c9f2646ebea0b30a9588462642ac94aee9.pull_requestevents.job.workflow_sharefers to MetaManifold once the reusable is local.No TypeScript finding is suppressed and no broad frontend baseline is introduced. The branch ruleset was also checked: Hypatia is not a required code-scanning tool, so omitting this advisory PR upload cannot cause a missing-required-tool deadlock.
Validation
bash scripts/check-governance-contract.sh— 20/20 checks passed.yamlpackage.git diff --checkand shell syntax checks passed.Live PR checks will verify GitHub Actions accepts the local reusable and that no new Hypatia code-scanning-results check is emitted.