Repository navigation
Validated statistics layer: catalogue, cancellation, Julia UI and evidence (issue #1) - #257
Conversation
- Publish the approved method catalogue as KYAML and serve it from GET /api/v1/statistics/catalogue as canonical I-JSON/JCS; validate every limit against the constants the code enforces. - Emit JCS for validated-statistics responses and add an opt-in time_limit_seconds deadline: 408 statistics_cancelled, no partial result, deadline never persisted or fingerprinted. - Add a Julia-rendered, script-free /statistics page (no new application TypeScript) with catalogue block, provenance, accessibility affordances and explicit failure states; exempt it from the SPA catch-all. - Add boundary, adversarial, property and UI tests plus seeded simulation and mutation scripts; register them in the test runner and focused CI step. - Add a CI standards job: pinned yq + ijson-jcs, kyaml-format --check, I-JSON/JCS gate, simulation study and mutation campaign with retained artifacts. - Document numeric/cross-language/storage contracts, the catalogue approval record and the acceptance evidence map. 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 41 minutes. View limit details
✨ Finishing Touches: 💡 1
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 |
Fork governance
🟣 critical · commit
Hypatia scan: 140 finding(s)
panic-attack assail: 10 finding(s)
pons asinorum: 4 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. 🟢 conformant · commit
Scope: K9 components L0–L2 against contract v1.0.0 at the named standards commit. L3 signature verification and routed coordination-dialect files are not certified. Green is limited to this scope. Orange is missing evidence; yellow is grandfathered debt or warnings; grey is an explicit scope exclusion. report.json, badge.json and the validator logs are in the k9-grid artifact on the linked run. |
… a script - The mutation campaign is now scripts/statistics/mutation_check.jl, matching the repository's Julia-owned tooling and removing a Python entry point whose CLI paths SonarCloud rated as high-severity taint findings. - Tool installation (HTTPS-only curl with a pinned SHA-256, exact-commit fetches, cargo build) moves from inline workflow steps into scripts/ci/fetch-statistics-tools.sh. - The I-JSON/JCS gate enforces the canonical config document only; the oracle vector file is deliberately non-canonical input and is not a document to enforce. The repository-wide pass stays report-only and retained. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
![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: 0f277cbfe9
ℹ️ 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".
| - name: Set up Julia | ||
| run: bash scripts/install-ci-julia.sh |
There was a problem hiding this comment.
On a fresh ubuntu-24.04 runner, this step only installs the Julia binary—scripts/install-ci-julia.sh downloads the runtime and adds it to PATH, but does not instantiate the project. Consequently, the later simulation and mutation steps invoke using MetaManifold without the dependencies from Manifest.toml being installed and the new standards job fails for every relevant change. Add the same Pkg.instantiate() step used by the main test job before running this evidence.
Useful? React with 👍 / 👎.
| function _html_escape(value) | ||
| replace(String(value), | ||
| '&' => "&", '<' => "<", '>' => ">", | ||
| '"' => """, '\'' => "'") |
There was a problem hiding this comment.
A successful form result contains numeric values such as totals, richness, policy limits, and diagnostics, but String(value) has no method for Julia integers. Rendering the first such value therefore raises a MethodError, which the POST handler converts into a 500 error page instead of displaying the result. Use string(value) before applying the HTML replacements so both textual and numeric result fields can be rendered.
Useful? React with 👍 / 👎.
| String("<code>", _html(print_value), "</code><br><span class=\"display\">", | ||
| _html(kind), " · ", _html(string(bits)), "-bit · display: ", | ||
| _html(display), "</span>") |
There was a problem hiding this comment.
When a high-precision metric is defined, this branch calls String with multiple fragments, but Julia's String constructor is not a variadic concatenation function. After the general numeric escaping failure is fixed, every nonzero high-precision result still fails with a MethodError while rendering Shannon or Gini–Simpson. Use string(...), interpolation, or an IOBuffer to assemble the markup.
Useful? React with 👍 / 👎.
| id: "exact-descriptive-summaries", | ||
| status: "supported", | ||
| summary: "Per-sample and per-group integer totals, observed richness, and exact rational proportions.", | ||
| implementation: "src/analysis/exact_summaries.jl", |
There was a problem hiding this comment.
The approved catalogue publishes observed richness as an output of exact-descriptive-summaries, but SampleSummary, GroupSummary, and summary_to_storage provide only counts, totals, and proportions; the new UI even displays “not reported in exact_counts.” Clients using the catalogue to select a supported method therefore cannot obtain one of its advertised results. Either compute and serialize richness for samples and groups or remove it from this method's approved summary.
Useful? React with 👍 / 👎.
| killed = code != 0 | ||
| survived.append(mutant.name) if not killed else None |
There was a problem hiding this comment.
The campaign treats any nonzero test exit as proof that the planted mutant was killed. If the selected test already fails because of an unrelated regression, setup error, or malformed environment, every affected mutant is reported as killed and the report can claim complete: true without showing that the mutation changed the outcome. Run each selected baseline test set on the restored tree first, or otherwise compare mutant results against a known passing baseline.
Useful? React with 👍 / 👎.
| table = CSV.read(input, DataFrame; copycols=false) | ||
| ncol(table) >= 2 || _usage("the CSV needs an identifier column and at least one sample") | ||
| feature_names = String.(string.(table[!, 1])) |
There was a problem hiding this comment.
A header-only CSV with an identifier column and at least one sample passes this check, producing a 0 × N matrix. Both summaries then classify every sample as a zero-total undefined case, the comparison loop records no failure, and the script exits 0 with complete: true, so an empty dataset can be presented as successful biological validation. Require at least one data row before running the summaries.
Useful? React with 👍 / 👎.
| metrics, achieved = NumericPolicy.with_precision(; bits=policy.precision_bits) do | ||
| [_metric(column, policy) for column in columns] | ||
| results = Vector{Any}(undef, length(columns)) | ||
| for j in eachindex(columns) | ||
| check_cancelled!(cancellation) |
There was a problem hiding this comment.
For concurrent high-precision requests, with_precision can block on the global _PRECISION_LOCK, but the first cancellation check in this block runs only after that lock has been acquired. A request with a short time_limit_seconds can therefore remain pinned behind a long million-bit calculation well past its deadline before finally returning 408. Use a cancellable/periodically polled lock acquisition or check the token while waiting so the advertised per-request deadline also bounds queueing on this lock.
Useful? React with 👍 / 👎.
| value = if raw isa Integer && !(raw isa Bool) | ||
| Float64(raw) | ||
| elseif raw isa AbstractFloat | ||
| Float64(raw) | ||
| else |
There was a problem hiding this comment.
The direct endpoint's raw-token pass rejects decimal deadlines, but the run/DuckDB endpoint calls _statistics_body without that pass and this branch explicitly accepts any finite AbstractFloat. Thus POST /api/v1/studies/{study}/runs/{run}/statistics/summaries accepts values such as 1.5, despite the documented shared contract requiring whole seconds and rejecting decimal tokens. Restrict this field to non-Boolean integers, or apply a field-aware lexical check on the run endpoint.
Useful? React with 👍 / 👎.
…lures - first(with_precision(...) do ... end) binds the do-block to first(), not to with_precision(); bind the tuple first and then unwrap it. This affected the simulation study and the property-test reference helpers. - Emit a checks-API annotation with the log tail when the simulation study or the mutation campaign fails, so the reason is visible without the raw log. - Run the mutation campaign even when the simulation step failed. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf91f9caa
ℹ️ 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".
| rng = MersenneTwister(seed) | ||
| exact_failures = String[] | ||
| metric_failures = String[] | ||
| maxima = Dict(Int, Dict{String,BigFloat})( |
There was a problem hiding this comment.
Every simulation run reaches Dict(Int, Dict{String,BigFloat}), which calls the Dict constructor with two type objects instead of constructing a dictionary type. This raises a MethodError before any replicate runs, so the seeded simulation step cannot produce its evidence report even after project dependencies are installed. Use Dict{Int,Dict{String,BigFloat}}(...) to construct the maxima dictionary.
Useful? React with 👍 / 👎.
| open(log, "w") do io | ||
| println(io, "\$ ", command) | ||
| flush(io) | ||
| return success(pipeline(command; stdout=io, stderr=io, dir=ROOT)) |
There was a problem hiding this comment.
On the first mutant, _run_tests passes dir=ROOT to pipeline, but Julia's pipeline does not accept a dir keyword; working directories are configured on Cmd instead. The resulting MethodError propagates out of the campaign after the target is restored, without executing the tests or writing a mutation report. Construct Cmd(command; dir=ROOT) before passing it to pipeline.
Useful? React with 👍 / 👎.
| @test rejected(doc -> doc["methods"][1]["evidence"] = Any[]) | ||
| # Flipping an unsupported entry to supported without evidence is refused. | ||
| @test rejected(doc -> doc["methods"][3]["status"] = "supported") | ||
| @test rejected(doc -> doc["methods"][3]["evidence"] = Any[]) |
There was a problem hiding this comment.
With the shipped catalogue, the third method is parametric-model-fits, which is unsupported and already has evidence: []. This mutation therefore leaves the document valid: _validate_method requires nonempty evidence only for supported methods, so rejected(...) returns false and this assertion fails on every baseline run. Test this requirement against a supported entry, or assert acceptance for the unsupported entry, rather than rejecting the catalogue's intentional representation of an unimplemented method.
Useful? React with 👍 / 👎.
| _adv_body(overrides::Dict) = merge(_ADV_BASE, overrides) | ||
| _adv_body(key::String, value) = merge(_ADV_BASE, Dict(key => value)) |
There was a problem hiding this comment.
Most callers pass one or more Pair arguments, starting with _adv_body("time_limit_seconds" => bad) in the deadline test and including three-pair calls in the resource-limit tests. Neither helper method accepts those arguments: one requires a Dict, and the other requires a string followed by a value. These calls raise MethodError before reaching the endpoints, aborting the affected testsets and failing the expanded CI suite. Add a variadic Pair overload that builds the override dictionary, or convert the callers to the supported signatures.
Useful? React with 👍 / 👎.
The evidence scripts load JSON3 and the statistics modules, so the job cannot run them against an uninstantiated environment. The Julia tests job already performs this step; the standards job now does the same. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
- Cmd(...; dir=ROOT) is the supported way to set a working directory; pipeline has no dir keyword. - MetaManifold loads RCall, whose build and init need an R installation even when no R code runs. The standards job installs the pinned R runtime from tool_versions.yml (read with yq) before instantiating the project. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| Threads.atomic_xchg!(token.cancelled, false) && | ||
| throw(OperationCancelledError("cancelled by caller")) |
There was a problem hiding this comment.
For a token cancelled through cancel! without a deadline, the first check_cancelled! clears the cancellation flag before throwing, so every subsequent check succeeds. A caller that catches the exception and passes the same token to exact_summary or high_precision_summary can therefore obtain a successful result despite having cancelled that token. Read the atomic flag without resetting it so cancellation remains observable, and cover repeated checks of an explicitly cancelled token as well as an expired deadline.
Useful? React with 👍 / 👎.
| @test permuted.samples[j].total == summary.samples[j].total | ||
| @test permuted.samples[j].richness == summary.samples[j].richness | ||
| if !isnothing(summary.samples[j].shannon) | ||
| @test permuted.samples[j].shannon == summary.samples[j].shannon |
There was a problem hiding this comment.
When a permutation changes the order of entropy terms, _metric accumulates them in a different order using finite-precision MPFR subtraction, so mathematically equivalent inputs need not produce bit-identical Shannon values. I reproduced this with 256-bit MPFR for counts [123456, 654321, 981234, 432198] and the permutation [981234, 432198, 654321, 123456]. This exact-equality assertion can therefore reject correct results in the seeded property suite and fail CI. Use the existing _close tolerance for Shannon here; Gini–Simpson can retain exact equality because its accumulation uses integer arithmetic.
Useful? React with 👍 / 👎.
- The _statistics_cancellation docstring ended with a stray ')' instead of the closing triple quote, which made the whole server module unparseable. - join(gen, delim) needs its generator parenthesised; the large-matrix UI test lacked them. Both were caught by CI; a tree-sitter parse pass over every .jl file in the repository now shows no errors in changed files. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
The governance contract allows exactly two exit-preserving pipelines in ci.yml; the two evidence steps added a second pair and tripped test-ci-logging.sh. Rather than relax the contract, the logic moves into scripts/ci/run-with-log.sh, which the same test now exercises directly: producer status preserved, failure annotated with an escaped log tail, log retained. scripts/ci/install-r-runtime.sh does the pinned R install for both jobs (core for the standards job, full for the Julia suite), which also removes the duplicated wget block SonarCloud flagged. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
shell:S6506 is a vulnerability, so it moves the security rating on its own. --https-only constrains the first request only; curl with both --proto and --proto-redir is the spelling this repository already uses for every pinned download, and the key is now written from a private temporary file so a truncated download is never left in /usr/share/keyrings. Also: [[ instead of [ throughout the new scripts, the failure annotation moves to stderr (the runner parses workflow commands on both streams), and docs/sonar-quality-gate.md records the rule and the reasoning, as that page requires. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cbda4101f
ℹ️ 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 bad in (9007199254740992, "-1", "1.5", "1e3") | ||
| body = _adv_body("counts" => [[bad, 2], [3, 4]]) | ||
| @test _adv_post("/api/v1/statistics/summaries", body).status == 422 |
There was a problem hiding this comment.
Once the request helper accepts Pair arguments, the 9007199254740992 case still fails this assertion: _statistics_body calls ijson_read, whose unsafe-integer check in src/canonical.jl throws ArgumentError, and _statistics_guard maps that exception to HTTP 400, not 422. The new boundary suite already expects 400 for the same token. Separate this numeric-token case from the invalid count strings and expect 400 so the adversarial suite does not reject correct endpoint behavior.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c22ba72ba
ℹ️ 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".
| "complete" => isempty(survived), | ||
| ) | ||
| open(output, "w") do io | ||
| println(io, JSON3.pretty(report)) |
There was a problem hiding this comment.
When the mutation campaign finishes, JSON3.pretty(report) prints the JSON to stdout and returns nothing, so this writes only nothing\n into the report file. Consequently, the standards job uploads an invalid JSON artifact instead of the mutation evidence, even if the campaign succeeds. The same mistake affects the --output branches in simulation_study.jl and biological_validation.jl. Pass io directly to JSON3.pretty(io, report) in all three scripts; the pinned version's implementation explicitly defaults the one-argument overload to stdout.
Useful? React with 👍 / 👎.
…259) Follow-up to #257, which merged with its statistics jobs still red: the two committed bugs below were only reachable once the job got past precompilation, so the evidence gate never ran to completion on the merged commit. ## What was broken - **`_html_escape` called `String(value)`.** Storage dictionaries carry integers as JSON numbers (`precision_bits`, `round_digits`, `richness`, diagnostics), and `String(::Int)` is a `MethodError`. Every result page therefore fell into its 500 branch, and the UI tests saw the failure block instead of a result. `string(value)` is the contract: any printable value is rendered. - **`Dict(Int, Dict{String,BigFloat})(...)`** in the simulation study called the two-argument `Dict` constructor instead of parametrising `Dict`, so the study threw a `MethodError` before running a single replicate. ## Diagnostics The failure digest in the Julia job cut nothing, so a single evaluated HTML page filled every annotation and hid the assertions that failed. Each line is now cut to 320 characters and the chunks are larger, so the digest names the failures instead of reproducing a page. ## Evidence The `Statistics standards and evidence` and `Julia tests` jobs run on this PR; the remaining statistics failures (property tolerances, storage round-trip) are being diagnosed from those annotations, not assumed fixed. Refs #1. --------- Co-authored-by: Arena Agent <agent@arena.ai> Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
Closes the implementation scope of #1 (validated Julia statistics layer) and leaves the two owner/reviewer gates explicitly open rather than self-certified.
What this adds
Bounded method catalogue
config/defaults/validated_statistics.kyaml(KYAML, standards YAML-POLICY Y-3 / D307) is the source of truth: 6 methods withsupported/unsupported/deferred/out_of_scope, plus the enforced limits.GET /api/v1/statistics/cataloguepublishes it as canonical I-JSON/JCS withcatalogue_sha256,supported_method_ids,method_countandfile.src/analysis/method_catalogue.jlrefuses an unapproved catalogue, a supported method without evidence, and any limit that differs from the constant the code enforces.Canonical responses and cancellation
jcs_json(they were JSON3-encoded while the trust contract already claimed JCS).time_limit_seconds(whole seconds, 1–3600) threads aCancellationTokenthrough the summary loops and the DuckDB row stream. Exhaustion returns 408statistics_cancelledwith no partial result. The deadline is operational only: it is not fingerprinted and not stored in a derived record.Julia-owned UI
GET/POST /statisticsis server-rendered with no script andContent-Security-Policy: default-src 'none': form, catalogue block, per-sample/group tables, provenance (input_sha256, policy fingerprint), explicit failure states, optional immutable record. Exempted from the SPA catch-all. No new application TypeScript.Evidence
test_method_catalogue,test_statistics_boundaries,test_statistics_adversarial,test_statistics_properties,test_statistics_ui(registered intest/runtests.jland the focused CI step).scripts/statistics/simulation_study.jl— seeded exact replicates (tolerance 0) and 128/256/512-bit error study against an independent 4096-bit computation, with the tolerance declared before the run.scripts/statistics/mutation_check.py— six planted defects, each required to turn a named test red; refuses a dirty target and verifies restoration.scripts/statistics/biological_validation.jl— compares the layer with the existing Float64DiversityMetricson a real matrix; refuses to run without a provenance string.Statistics standards and evidence: pinned yq v4.53.3 (SHA-256 verified) and pinnedhyperpolymath/standards+hyperpolymath/ijson-jcscommits,kyaml-format.sh --check,check-ijson-jcs.sh --enforce, simulation and mutation, with retained artifacts.Docs:
docs/statistics/numeric-contracts.md,docs/statistics/method-catalogue-v1.md,docs/statistics/validation-evidence.md, plus updatedVALIDATED_STATISTICS.md.Deliberately not self-certified
Everything else in the acceptance list is mapped to a checkable artifact in
docs/statistics/validation-evidence.md. Exact statistical tests remain deferred; symbolic mathematics is out of scope and #2 stays blocked.Claim language
Notes for review
numeric-contracts.mdas the one file-level exception to J-1, whose gate covers repository-tracked JSON.time_limit_secondsaccepts whole seconds over JSON; decimal tokens are refused by the same raw-number rule as counts.