Repository navigation
Qualify RSI trials with explicit completeness and lifecycle checks - #341
Conversation
Skill evaluationPLANNED: not executed; there are no task outcomes yet.
Author verification steps (nonpaid; not yet run)
python tools/build_skill_index.py --source-revision 1e87be46b8d080d439bb516ed9fa9092f8306f62 --checkThe declared source pin matches this head's canonical library bytes. The command has not run: retain its exit code and full drift/success output. It checks the generated projection, not claim truth or skill efficacy. Evidence and logs · Retention: 30 days. Plan bindings, budget, frozen exam and advisory detailsAdvisory: obtain missing evidenceRetained evidence availability: planned / unexecuted. No approval or merge authorization is issued.
Model/settings:
Automatic execution / cumulative PR budget: {"reason":"Trusted configuration or pricing missing, invalid or expired","remaining_usd":"unknown","reserved_or_unresolved_usd":"unknown","spent_or_legacy_usd":"unknown","state":"blocked"}Projection check selection (not an executed result): {"declared_in":"src/inferrsi/data/repo_packs/inferrsi/knowledge/README.md","source_revision":"1e87be46b8d080d439bb516ed9fa9092f8306f62","status":"ready"}Provider/settings: Uncovered / unsupported paths (124)
|
Dong1017
added
the
area:workbench
ChatGPT Codex Connector
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Scoped static review of draft #341 at head 8d01acd0 against Issue #340: inspected CLI/RunSpec freezing, per-trial result collection, evaluation precedence, the synthetic tests, and the existing HTML/SVG report consumer. One P2 evidence-presentation risk is attached inline: unqualified trials correctly suppress improvement_pct, but the report still plots an apparent gain ratio from their raw medians. Please make the unqualified status unmistakable in that consumer. This does not challenge retention of raw measurements or the declared adapter-attestation boundary.
I did not run tests, model workloads, or hardware experiments. The separate current-head knowledge-authorship admission failure remains an author/maintainer matter; this COMMENT is neither approval nor integration/goal acceptance.
| t.get("status") == "completed" and t.get("metric") is not None for t in baseline + candidate | ||
| ) | ||
| if all_metrics: | ||
| if all_metrics and trials_qualified: |
There was a problem hiding this comment.
P2 — unqualified trial checks still produce a comparative gain chart. With baseline=100 ms, candidate=80 ms and evidence.workers_closed=false, this branch correctly leaves improvement_pct=None and yields INCONCLUSIVE. However harness/reporting.py::_summary_figure() calls summary_chart.py::spec_from_evaluation(), which derives a candidate/baseline ratio directly from the retained medians and renders it in report.html even when checks failed. The point note says INCONCLUSIVE, but the chart still visually communicates an improvement that this new gate is meant to withhold. Consider suppressing the gain figure for an unqualified comparison, or explicitly marking the plotted ratio as raw/unqualified — not an accepted gain. Please add a regression that reads the HTML/SVG report after a false/missing required check; the existing tests only assert evaluation.json.
There was a problem hiding this comment.
Addressed in 8970059. evaluate() now records trials_qualified, and spec_from_evaluation() refuses to derive a candidate/baseline ratio when it is false. For an unqualified comparison, report.html labels the retained medians as raw measurements (not an accepted gain) and no summary-chart.svg is written; qualified comparisons still plot the measured ratio. Added regressions that read the HTML report for both a false and a missing evidence.workers_closed check (baseline 100 ms / candidate 80 ms), asserting no SVG and no 0.80×. ruff and pytest tests/test_trial_checks.py tests/test_summary_chart.py tests/test_runtime.py pass locally; CI pending.
There was a problem hiding this comment.
Follow-up on the 8970059 repair (static review only): the single-run path now guards trials_qualified=false, but harness/reporting.py:69–75 prioritizes a pre-existing summary-chart.json and never calls the guarded spec_from_evaluation() in that branch. With a valid campaign spec alongside an unqualified current evaluation, _summary_figure() still writes and inlines an SVG, so “no gain figure on unqualified checks” is not guaranteed for this path. Please clarify whether the campaign figure is intentionally independent historical evidence; if so, label that boundary explicitly and ensure it cannot represent the unqualified current step as an accepted gain. Otherwise apply the qualification guard before either chart path. A regression using summary-chart.json plus trials_qualified=false would distinguish the intended behavior. No tests or model/device experiments run here.
There was a problem hiding this comment.
Fixed in 1cdbb70: when the current evaluation has trials_qualified=false, the report now stops before both chart paths, so no gain chart is written or inlined, even if a summary-chart.json exists. The spec file stays on disk untouched and the report notes it as separate historical evidence that was not drawn. Regression test: a summary-chart.json that would render 0.80× plus trials_qualified=false produces no chart and leaves the spec bytes unchanged.
There was a problem hiding this comment.
Scoped, non-voting recheck at head 1cdbb70d11b4d335ba907113791c4a096beb75bd (API-reported PR base 663f3b327998526a13aa31e72ad729025c18983c): the campaign-chart bypass identified in my follow-up is addressed.
The qualification guard returns before either chart path. An evaluation with trials_qualified=false therefore cannot cause the report to render or write a gain SVG from an existing summary-chart.json. The JSON remains unchanged, and the report separates that historical specification from the unqualified step while labeling retained medians as raw measurements. The report-level regression supplies a valid campaign specification that otherwise renders 0.80×.
I inspected the four-file incremental diff and relevant evaluation, caller and test context. The Ready CI passed all six jobs; both full suites report 3,571 passed, 13 skipped and 284 subtests. They checked out merge 38cf7fba1ca3bf9778622aeb122df28cc165b204, whose Git tree matches this head.
This clears this specific presentation finding only, not whole-PR admission or acceptance. The current-head knowledge-authorship check remains noncompliant under current main 9dded7ad. No local tests, model/device work, formal approval or merge action were performed.
A false or missing required check already leaves improvement_pct unset and the verdict INCONCLUSIVE, but the HTML report still plotted a candidate/baseline ratio from the retained medians. Refuse that derived figure and say the medians are raw measurements, not an accepted gain. Addresses #341 (comment) Agent-assisted-by: Cursor Grok 4.7 (implementation) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
summary-chart.json is historical campaign evidence and still wins for a qualified run, but an unqualified evaluation was inlining that figure as this step's gain summary. Guard both chart paths before rendering, leave the committed specification unchanged, and say it was not drawn for this step. Addresses #341 (comment) Agent-assisted-by: Cursor Grok 4.7 (implementation) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: amy-why-3459 <amy-why-3459@users.noreply.github.com>
|
@zengchuang-hw @Dong1017 PTAL |
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
PR review verdict: DO NOT PASS pending authorship reconciliation.
@amy-why-3459 Rechecked base 6fefa0108232fcf210aa7c0f2ba3fa936439bd02 and head 5e573b2318926543cc974883ba6b2cf81b1f7e48 in response to PTAL.
Blocker: reconcile the Codex-only declaration with the recorded Cursor contribution. The current PR block lists only Codex, but 8cc3649 and 5e573b23 explicitly attribute conflict resolution to Cursor / grok-4.7 with unknown effort, including changes under protected generated knowledge paths. Policy requires every material AI author and gives generated changes no automatic exemption. Please restore truthful material-author coverage, retaining unknown settings and using the documented exception route where necessary, or provide an evidence-backed explanation reconciling that recorded contribution with the narrower declaration. Do not guess historical settings or rewrite historical commits. The new green declaration job validates the submitted metadata; it does not resolve this discrepancy.
Technical recheck: no new runtime blocker found in CLI/RunSpec freezing, boolean collection, evaluation and report consumers. The guard still precedes both chart paths, so the earlier presentation finding remains resolved. All six Ready jobs passed. Both Python suites report 3,605 passed, 13 skipped and 284 subtests; tested merge 32de60a0407554853fa537f20fbdbd22a61a5b78 has the same Git tree as this head.
Review performed by dot under Dong1017's delegated technical-review authority. Static review plus inspection of existing CI, with no local test rerun or model/device experiment. This is non-voting COMMENT feedback, not formal approval, a policy exception or campaign acceptance.
|
PR review verdict — DO NOT PASS Reviewed base 本轮已解决
批准前必做（Before I can recommend approval）
非阻塞问题或建议 The PR's validation text still says 79 generated records. Update it to the verified current count of 80. Actual checks and remaining limits On Windows / Python 3.12.1 / pytest 9.1.1 / Ruff 0.15.15, with imports bound to this checkout, the focused trial/runtime/chart selection reported 102 passed, 1 failed, 15 subtests passed. The sole failure was Codex performed this independent, non-voting review through |
|
Addressed the author-side corrections in Dong1017's review and the follow-up by updating this PR's description at unchanged head
@Dong1017: please consider the exact-head exception request in the PR body's single |
Add optional per-trial qualification without changing default command behavior. Document Omni reference, streaming, graph and lifecycle evidence contracts. Agent-assisted-by: Codex (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
|
PR review verdict — DO NOT PASS pending policy admission Metadata follow-up at unchanged head 本轮已解决 Material-author coverage now preserves Codex, the older Cursor 批准前必做（Before I can recommend approval）
非阻塞问题或建议 No additional finding in this metadata correction. I compared the declaration with the recorded contributions and the target policy's protected-path set, and inspected the current check evidence. Code is unchanged, so the prior software review and its validation limits remain applicable. Codex performed this non-voting follow-up through |
Use the evaluator's existing selected checks and comparison result to decide whether either report chart input may be rendered. Preserve raw evidence and remove a previous generated SVG when rebuilding a report without a qualified comparison. Cover both arms, missing/false/non-boolean checks, campaign input, direct conversion and report regeneration. Refresh the coverage view. Agent-assisted-by: Codex / gpt-6-astra [effort=xhigh] (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
Refresh all 80 catalog records from the committed GPT-authored source and retain the canonical source hashes and metadata. Agent-assisted-by: Codex / gpt-6-astra [effort=xhigh] (implementation) Signed-off-by: amy-why-3459 <wuhaiyan17@huawei.com>
5e573b2 to
4cbe29b
Compare
|
Updated the branch at the contributor's explicit request to remove the Cursor/Grok changes and rebuild the contribution with GPT. New head: This addresses the authorship review through a source/history replacement, not a relabeling of the previous tree:
Validation: 118 functional/runtime/chart tests and 15 subtests passed, plus 49 index/coverage/library/knowledge tests passed. The new regressions reproduced 15 failures before the replacement fix. The full pinned index CLI (80 records), coverage generation check, Ruff and whitespace checks passed. The local declaration assessment using trusted Please recheck the rebuilt head against the authorship finding and the original chart feedback. This supersedes my previous exception request for the old head; it does not claim approval, merge authorization or campaign acceptance. |
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
Head / conclusion
Reviewed rebuilt head 4cbe29b5374b9cf1282b1855e1631ec9e7b7e22e against target e843b9d31cb86e1fd12d00890c89798b0bedd1fe and the prior reviews. LGTM for the bounded #340 software delivery. No unresolved blocking finding in this review. This is non-voting COMMENT feedback, not a formal approval.
Resolved
- Authorship reconciliation and superseded exception: the branch now contains three commits from
6fefa010. All 11 changed-file blobs in replayed30c51match the original Codex commit9e6a51c; the four identified Cursor/Grok commits are not ancestors of the rebuilt head. The replacement chart implementation/tests are in1e87be4, and all 80 generated records now pin that source. Together with the contributor's current declaration and withdrawal, this resolves the declaration/history discrepancy in my previous review for this head. The latest trusted-main declaration assessment is compliant without an exception. This verifies the recorded reconciliation, not private execution settings. - Original gain-chart finding and campaign-spec bypass: the shared eligibility check respects the evaluator's selected checks and withheld comparison. Reporting checks it before either chart input; direct conversion also checks it. Rebuilding an unqualified report removes the old SVG before enumerating links, while preserving campaign JSON and raw measurements. Both-arm false/missing/non-boolean cases are covered. Legacy unconfigured behavior and qualified zero/negative gains remain supported.
- Catalog receipt: the description now consistently reports 80 records and retains the exact-source index-check command, output and verification limits. Generated changes do not substitute for claim or efficacy evidence.
Verification / limits
- Independently ran
pytest -q tests/test_trial_check_reports.py tests/test_summary_chart.py: 57 passed, using exact-head files whose 15 local Git blob hashes matched the fetched revisions. Additional synthetic checks covered 140 invalid/incomplete evaluator cases across 280 default/campaign report routes, plus legacy and qualified zero/negative-gain controls. - All six current Ready jobs passed. Both Python 3.10/3.11 logs report 3,772 passed, 13 skipped, 284 subtests passed, testing merge
83fab8aof this head intoe843b9d3. Both installed-wheel jobs and KV qualification passed. The latest declaration success supersedes the initial failed declaration run at this head. - Local work was focused source/hash and synthetic testing, not a local full suite or complete index CLI rerun. Hosted software checks, adapter attestations and declaration compliance do not establish actual worker/device health, model quality, performance or skill efficacy. No model/device experiment or spending occurred; merge and Issue acceptance remain separate.
|
@zengchuang-hw @Dong1017 PTAL |
Dong1017
left a comment
Dong1017
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict: PASS
Head / conclusion
Approve the bounded #340 software delivery at head 4cbe29b5374b9cf1282b1855e1631ec9e7b7e22e against target e843b9d31cb86e1fd12d00890c89798b0bedd1fe. This formal approval follows the account holder's explicit authorization for this PR and the completed current-head review. No unresolved blocking finding remains in that reviewed scope.
Resolved
The rebuilt contribution resolves the recorded authorship discrepancy; the current declaration is compliant without an exception. The shared chart eligibility check handles both chart sources, direct conversion and stale SVG removal. The catalog receipt correctly covers 80 generated records. See the linked review for the source/history verification and regression evidence.
Verification / limits
Immediately before approval, head/base and the discussion were unchanged. All six Ready jobs still pass, and the latest knowledge-model-policy assessment is successful. No new blocking review was present. Reused the recorded 57-test independent review and hosted full-suite/wheel evidence; no additional tests or experiments were run for this approval.
Acting reviewer: dot through Dong1017 under explicit per-PR approval delegation. This approves the software change, not hardware/model efficacy or campaign acceptance. Merge and whole-Issue acceptance remain separate.
|
Incremental review: PR review verdict: PASS for the bounded #340 software delivery. No unresolved approval-blocking finding remains in this reviewed scope. This exact-head review has now been followed by formal Approve through the connected account. 本轮已解决 / Resolved
批准前必做（Before I can approve）None remaining within this technical review's bounded software scope. The previous policy exception is no longer a prerequisite. There is no additional code fix, CI rerun or evidence request from this review. The account holder directly authorized formal approval; the recorded Approve is bound to this reviewed head. 非阻塞问题或建议 / Non-blocking limitsThe selected booleans remain adapter attestations backed by receipts. They do not independently establish worker/device health, model quality, performance or skill efficacy. Broader Issue acceptance and any model/device qualification remain with the requester; no such result is inferred from software CI. Actual checks and unverified scope
|
zengchuang-hw
left a comment
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Approve head 4cbe29b5374b9cf1282b1855e1631ec9e7b7e22e against main at e843b9d31cb86e1fd12d00890c89798b0bedd1fe, following the completed exact-head incremental review.
本轮已解决
- Selected-check failures withhold a qualified comparison across both report chart sources and direct conversion. Rebuilding an unqualified report removes a stale generated SVG while preserving raw measurements and campaign JSON.
- The rebuilt source/history and current author declaration are reconciled. All 80 generated records cite the replacement source; the current knowledge-policy assessment is successful.
- The exact-head independent Windows review recorded 57 passing report/chart tests, clean whitespace and an unchanged working tree. All six Ready jobs and the current knowledge policy remain successful.
批准前必做（Before I can approve）
None. No unresolved blocking finding remains in the reviewed PR content.
非阻塞问题或建议
No additional finding. Adapter attestations and software checks do not establish model/device efficacy or campaign acceptance.
Actual checks and limits
Rechecked the open PR, unchanged target/head, current discussion, review history and checks immediately before approval. Reused the recorded exact-head source/history and 57-test review; no additional tests or model/device workloads were run for this vote.
Delegation and execution: directly authorized by account holder zengchuang-hw to approve reviewed PR content with no unresolved blocker, explicitly including this PR. Executor: Codex as independent reviewer through the connected GitHub account zengchuang-hw. Authorized action exercised: formal Approve, bound by commit_id to the reviewed head. Merge and Issue closure are separate actions.
Problem and resulting behavior
Refs #340
An external benchmark can finish and emit a faster metric without proving that every request completed, workers exited cleanly, or the intended graph controls were effective. Repeatable
--require-trial-check DOTTED_KEYoptions freeze adapter-produced boolean checks in RunSpec. Every selected check must be literaltrueon every measured trial in both arms; missing, malformed or false evidence yieldsINCONCLUSIVEand withholdsimprovement_pct.The HTML report and direct evaluation-to-chart conversion honor that comparison decision. When selected checks leave no qualified comparison, neither raw medians nor a pre-existing campaign specification produces a gain chart. Raw measurements and campaign JSON remain available. Rebuilding a report also removes an obsolete generated SVG and its report link. Qualified comparisons and commands without selected checks retain their chart behavior.
The packaged RSI evidence workflow documents reference initialization/RNG contracts, feedback-aware stages, matching full/stream reference decoding, effective graph controls, streaming preflight, device qualification, and complete paired reruns after harness repair. These checks are adapter attestations backed by receipts, not independent inspection of worker/device state or a substitute for quality and source-integrity gates. No campaign thresholds, accelerator dependencies or CI lanes are added.
Implementation
required_trial_checksandimprovement_pctfields, before choosing either chart input.1e87be46b8d080d439bb516ed9fa9092f8306f62; the coverage view is refreshed from the current registry and matrices.Validation and limits
Checked content: head
4cbe29b5374b9cf1282b1855e1631ec9e7b7e22e, Python 3.12. The functional tests ran on the final code before the generated-index-only commit; index/knowledge checks cover the regenerated artifacts. The working tree is clean.pytest tests/test_trial_check_reports.py tests/test_trial_checks.py tests/test_summary_chart.py tests/test_runtime.py: 118 passed, 15 subtests passed. The new report regressions first reproduced 15 failures on the original GPT implementation before the replacement fix.pytest tests/test_skill_index.py tests/test_skill_coverage_view.py tests/test_skill_library.py tests/test_knowledge_contract.py: 49 passed.python tools/build_skill_index.py --source-revision 1e87be46b8d080d439bb516ed9fa9092f8306f62 --check: passed, 80 records, using an isolated Git 2.49.0 installation.python tools/build_skill_coverage.py --check,ruff check src tests tools, andgit diff --check: passed.Knowledge authorship
The current contribution was rebuilt on
mainat6fefa0108232fcf210aa7c0f2ba3fa936439bd02from the original Codex functionality, followed by a newly authored chart fix and regenerated projections. Its material AI author is Codex using the contributor-confirmedgpt-6-astra/xhighsettings. This declaration covers the current head; previous heads retain their historical attribution in the PR discussion. Maintainer review, merge and Issue acceptance remain separate.