Skip to content

Commit bb40ab0

Browse files
hyperpolymathArena Agentarena-agentcoderabbitai[bot]
authored
fix(cladistics): carry #230/#235 to main, close their remaining findings, harden frontend parsers (#239)
Supersedes #230 and #235. This branch is #235 (which already carried #230 merged onto `main`) fast-forwarded onto current `main` (`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 | Finding | Status | |---|---| | Codex P1 — unplaced mass renormalised away by UniFrac (#230) | Fixed on #230/#235 (`totals`/`total_a`/`total_b` library-size denominators); carried here, verified present | | Codex P2 — fully unmapped ASVs not counted as unplaced (#230) | Fixed on #230/#235 (`unplaced_reads` = library size − tree-carried mass); carried here | | CodeRabbit — unplaced-reads copy reversed the calculation (#230) | Fixed on #235 (panel + `api/types.ts` now say: excluded from tip abundances, included in the library-size denominator); carried here | | CodeRabbit — emptied clade kept stale activity (#230) | Fixed on #235 (`_reaggregate_after_move` recomputes from `direct_count`); carried here | | Codex P2 — vacated clade kept stale `residual_count` (#235) | Already fixed on #235 by `58848d9`; verified present here, no duplicate change | | CodeRabbit — emptied internal clade still counted as a taxon tip (#235) | **Fixed in this PR**: `count_leaf_taxa!` and the leaf branch of `_reaggregate_after_move` now share one tip-eligibility rule (direct reads, or terminal `tip`/`ASV` rank). 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 tip | | CodeRabbit `occursin` route assertions (#235) | Addressed on #235 (whole method+path pairs); carried here | 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) - **DEED lane: 🟢 conformant** on #235's head and unchanged here (no `.deed` file touched). Local contract check: `scripts/check-governance-contract.sh` — all 20 checks pass. - **Hypatia grid: 🟣 critical (45C/61H/35M)** — unchanged in kind. The criticals are `cicd_rules/banned_language_file` against 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.json` acknowledgement is an owner decision — deliberately **not** committed here, because committing one (with `scripts/apply-baseline.sh`) arms the blocking gate in `hypatia-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. - Real findings fixed here anyway (panic-attack `InputBoundary`, advisory): SSE handlers in `frontend/src/api/events.ts` no longer throw on malformed frames; `parseJplace` in `frontend/src/tree/model.ts` reports invalid JSON as a jplace error. Also fixed the pons `string-concat-in-loop` in the Newick quoted-label reader. - Left as documented false positives / follow-ups: `JSON.parse(JSON.stringify(...))` deep-clones in `charts.ts`/`alphaMetrics.tsx` (cannot throw on malformed input — the input is a live object), the remaining pons string-concat heuristics in `TreeShapes.tsx` path building, and panic-attack's per-file `JSON3.read` try-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 - Frontend: `tsc --noEmit -p .` clean. - `git diff --check` clean; governance contract script passes (20/20). - Julia is not installable in this environment (as on #230/#235), so the Julia changes are static-checked and rely on this PR's Julia CI job as the real check — watching it before merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: Arena Agent <agent@arena.ai> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 1f8701d commit bb40ab0

10 files changed

Lines changed: 562 additions & 66 deletions

File tree

‎frontend/src/api/events.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,25 @@ export type SSEHandler = {
1313
export function openEventStream(handlers: SSEHandler & { onConnectedChange?: (connected: boolean) => void }): () => void {
1414
const es = new EventSource(apiUrl('/api/v1/events'))
1515

16+
// A malformed frame must not throw inside the listener: EventSource would
17+
// keep the stream open but the exception surfaces as an uncaught error and
18+
// the update is lost silently either way. Drop the bad frame instead.
19+
const parseFrame = (data: string): unknown => {
20+
try {
21+
return JSON.parse(data)
22+
} catch {
23+
return undefined
24+
}
25+
}
26+
1627
es.addEventListener('job_update', (e: MessageEvent) => {
17-
handlers.onJobUpdate?.(JSON.parse(e.data) as Job)
28+
const job = parseFrame(e.data)
29+
if (job !== undefined) handlers.onJobUpdate?.(job as Job)
1830
})
1931

2032
es.addEventListener('stage_update', (e: MessageEvent) => {
21-
handlers.onStageUpdate?.(JSON.parse(e.data))
33+
const ev = parseFrame(e.data)
34+
if (ev !== undefined) handlers.onStageUpdate?.(ev as Parameters<NonNullable<SSEHandler['onStageUpdate']>>[0])
2235
})
2336

2437
es.onopen = () => handlers.onConnectedChange?.(true)

‎frontend/src/api/types.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -968,6 +968,17 @@ export interface CladeCumulusRequest {
968968
export interface CladeCumulusDiversity {
969969
samples: string[]
970970
mapped_tips: number
971+
/**
972+
* Reads per sample that no tip on the tree carries: a query whose kept
973+
* `.jplace` like-weight ratios do not reach 1 contributes only that share,
974+
* and an ASV the tree does not cover contributes nothing.
975+
*
976+
* These reads are excluded from the tree-tip abundances, but they are still
977+
* counted in each sample's library size, which is the denominator the
978+
* weighted UniFrac metrics divide by. That is what keeps the shortfall off
979+
* the tree instead of renormalising a partially placed sample back up to
980+
* abundance 1. Absent for responses produced before this field existed.
981+
*/
971982
unplaced_reads?: Record<string, number>
972983
faith_pd: Record<string, number>
973984
unweighted_unifrac: number[][]

‎frontend/src/components/CladeCumulusPanel.tsx‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ export interface CladeCumulusPanelProps {
3333
runs: CladeCumulusRunOption[]
3434
}
3535

36+
/**
37+
* Explore a study run's clade hierarchy, diversity and unplaced reads. Loads
38+
* available sources and computes results on request; move checks can preview
39+
* counterfactual trees or save sidecar proposals without changing canonical tables.
40+
*/
3641
export function CladeCumulusPanel({ study, runs }: CladeCumulusPanelProps) {
3742
const toast = useToast()
3843
const reqCounter = useRef(0)
@@ -249,6 +254,9 @@ export function CladeCumulusPanel({ study, runs }: CladeCumulusPanelProps) {
249254
return div.normalized_weighted_unifrac
250255
}, [result, unifracKind])
251256

257+
// Only samples that actually lost mass are worth naming; a run with a
258+
// complete placement file (or one produced before the field existed) reports
259+
// nothing here.
252260
const unplacedSamples = useMemo<[string, number][]>(() => {
253261
const div = result?.diversity
254262
if (!div?.unplaced_reads) return []
@@ -708,10 +716,25 @@ export function CladeCumulusPanel({ study, runs }: CladeCumulusPanelProps) {
708716
))}
709717
</div>
710718

719+
{/*
720+
Reads no tip carries: like-weight mass a placement left on no
721+
edge, plus ASVs the tree does not cover. They are absent from
722+
the tree-tip abundances but they are *not* dropped from the
723+
sample: each one still counts in its sample's library size,
724+
which is the denominator the weighted UniFrac metrics divide
725+
by. That is what keeps them off the tree instead of letting a
726+
partially placed sample renormalise back up to abundance 1, so
727+
the shortfall is stated here as well.
728+
*/}
711729
{unplacedSamples.length > 0 && (
712730
<p style={{ fontSize: '.75rem', margin: '0 0 12px', color: 'var(--color-fg-muted, #57606a)' }}>
713-
Unplaced reads (like-weight mass left off reference edges, excluded from weighted abundances above):{' '}
714-
{unplacedSamples.map(([s, n]) => `${s} ${n.toFixed(2)}`).join(', ')}
731+
Unplaced reads (carried by no tip — like-weight mass left on no edge, plus
732+
ASVs the tree does not cover — excluded from the tree-tip abundances but
733+
counted in each sample’s library size, so they lower its relative
734+
abundances rather than being renormalised onto the placed tips):{' '}
735+
{unplacedSamples
736+
.map(([s, n]) => `${s} ${n.toFixed(2)}`)
737+
.join(', ')}
715738
</p>
716739
)}
717740

‎frontend/src/tree/model.ts‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,14 +112,14 @@ export function parseNewick(text: string): SourceTree {
112112
skipSpace()
113113
if (s[i] === "'" || s[i] === '"') {
114114
const q = s[i++]
115-
let out = ''
115+
const parts: string[] = []
116116
while (i < s.length) {
117117
if (s[i] === q) {
118-
if (s[i + 1] === q) { out += q; i += 2; continue }
118+
if (s[i + 1] === q) { parts.push(q); i += 2; continue }
119119
i++
120-
return out
120+
return parts.join('')
121121
}
122-
out += s[i++]
122+
parts.push(s[i++])
123123
}
124124
fail('unclosed quote')
125125
}
@@ -209,7 +209,18 @@ export function parseNewick(text: string): SourceTree {
209209
}
210210

211211
export function parseJplace(text: string): SourceTree {
212-
const doc = JSON.parse(text)
212+
let doc: {
213+
tree?: unknown
214+
fields?: string[]
215+
placements?: Record<string, unknown>[]
216+
metadata?: Record<string, unknown>
217+
version?: unknown
218+
}
219+
try {
220+
doc = JSON.parse(text)
221+
} catch {
222+
throw new Error('jplace file is not valid JSON')
223+
}
213224
const tree = parseNewick(String(doc.tree))
214225
const fields: string[] = doc.fields ?? []
215226
const col = (f: string) => fields.indexOf(f)

‎scripts/check-julia-syntax.jl‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,15 @@ for file in files
2020
issue = ex
2121
break
2222
end
23+
# A bare `end` at the top level parses without error and then fails
24+
# at lowering (`syntax: "end" at top level`), which is how a file
25+
# that lost its `module Foo` opener reaches CI looking syntactically
26+
# fine. An `end` balances exactly one opener, so top level cannot
27+
# hold one: this is a missing or duplicated opener, and the file
28+
# would not load.
2329
if ex === :end
24-
issue = "stray `end` at top level (missing or duplicated opener in this file)"
30+
issue = "stray `end` at top level (a `module`/`function`/... opener " *
31+
"is missing or was duplicated in this file)"
2532
break
2633
end
2734
next_position > position ||

0 commit comments

Comments
 (0)