Repository navigation
chore: retire the synthetic CPU KV slice tooling - #357
Conversation
Remove the bounded synthetic KV experiment's tooling, tests, task fixtures, operator pages and manual pilot workflow, plus its ready-lane qualification job. The slice had its own consumers only: nothing in the general harness, the SkillEval modules, the runner or the evaluator imported it, so the dependency ran one way onto shared parts. Retained evidence is untouched. docs/kv-reclamation-qualification.md stays at its path, rewritten as a retirement record, because a hash-pinned evidence README links to it and must not be edited. Agent-assisted-by: Claude Code / claude-opus-5 (author) Signed-off-by: Dong1017 <55723162+Dong1017@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. |
|
Review at head
Non-blocking: the claims in lines 37-42 of the rewritten page (both 2026-10-31 attempts No approve or request-changes vote at this head. |
Dong1017
left a comment
There was a problem hiding this comment.
Incremental read-only review at head 029878bbb000d82e6e6d38d895c0865a4adbf43b against e843b9d31cb86e1fd12d00890c89798b0bedd1fe. Scope: the required-check question in the existing review, not a fresh approval or full re-review of the removed KV tooling.
- Verified for the active ruleset: main ruleset
protect-maincurrently declares onlyknowledge-model-policyunderrequired_status_checks;kv-task-qualificationis not required by that ruleset. I also read the actual.github/workflows/ci-ready.ymlremoval. Therefore the proposed deletion does not remove a status check required by this active ruleset. - Remaining verification boundary: reading the separate classic
branches/main/protectionendpoint returned HTTP 403 (Resource not accessible by integration), so this does not prove that no classic branch-protection setting exists. A maintainer with settings access can confirm that last point if necessary. - The existing request to preserve a discoverable historical revision for removed paths in retained evidence is not cleared by this check. No tests, model calls, or device experiments were run for this review.
This is scoped COMMENT feedback, not a merge/acceptance verdict.
|
Dear committer @amy-why-3459 @zengchuang-hw, can you review this pr when you have a chance? |
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Initial independent review of 9dded7ad..029878bb (37 changed files), at head 029878bbb000d82e6e6d38d895c0865a4adbf43b. Assessed integration with current main 01c73521ced6ca760cdfa42c9b937fe985a20d64; the PR metadata still reports base e843b9d3. The merge-tree check against current main is clean. I read the earlier review and the required-check follow-up. Conclusion: no outstanding content blocker for this retirement change.
本轮已解决
- The removed modules, commands, fixtures and qualification job form the retired bounded KV slice. Searches of remaining source, tools, tests and workflows found no active consumer of the deleted modules. The general editing adapter remains, and the four operator rows are removed without unrelated changes.
- All 64 files in the four retained KV evidence directories, and the evidence inventory, have unchanged Git blobs. The retirement page preserves the pinned README's documentation target. Historical source identity is already retained in the frozen plan and actor receipt, so the deletion does not erase their source provenance.
- The dropped
kv-task-qualificationjob is not required: the active main ruleset requires onlyknowledge-model-policy. A successful main branch metadata read also reports classic protectionenabled=false, enforcementoffand empty required-check lists. This supplies evidence beyond the earlier separate protection-endpoint 403.
批准前必做(Before I can approve)
None for this reviewed change. Issue #248 remains unfinished; this cleanup does not accept its experiment, change its frozen verdicts or authorize another attempt.
非阻塞问题或建议
Adding a direct last-available source tree link to the retirement page would make old paths easier to find. Historical analysis should still use the original revision recorded in each receipt. Links to the particular live-attempt receipts would also improve navigation. These are documentation discoverability improvements, rather than missing source provenance or a broken active operator.
Checks and limits
Independent local verification at the exact head, on Windows / CPython 3.12.1 / pytest 9.1.1:
- 25 tests passed across
test_evidence_inventory.py,test_frozen_bytes.pyandtest_module_boundaries.py. - 3,183 tests collected with no collection errors.
python -m tools.validate_evidence: exit 0, 430 hashes / 21 bundles checked, no errors; pre-existing external/unavailable evidence retains its stated limits.- Ruff 0.15.15 (
src tests tools) and diff whitespace checks passed.
Current-head Ready CI passed both Python 3.10/3.11 full-check and installed-package jobs, plus PR references. The required policy result explicitly evaluates this head as successful / Not applicable. Main's intervening label/report changes do not touch the deleted KV slice or knowledge-policy scope.
I did not locally rerun the full suite or installed-wheel matrix, or execute hardware, provider or model experiments. The clean current-main merge tree was inspected, not separately run through a full test matrix. This is a PR content review, not a campaign PASS.
Direct delegation: zengchuang-hw, the account holder, authorized Codex in this chat to independently review other authors' InferMatrixLab/InferRSI PRs and submit formal Approve when content blockers are absent and applicable verification passes. Executor: Codex, with separate technical and scope/evidence cross-checks; acting GitHub account: zengchuang-hw (repository write access or higher). This account and reviewing task did not author, materially contribute to, or parent this delivery slice. Authorized action here: revision-bound APPROVE; scope: this PR's retirement diff and its verification.
|
@amy-why-3459 can you give the suggestion, if there is anything not fix in the pr? |
Record where the removed paths remain readable, and bind each claim on the page to the receipt that supports it. The retained evidence is hash-pinned, so it still names eleven files and modules that the removal deletes. Name them explicitly and point at the last revision shared with main, so every reference stays resolvable through git history rather than appearing as a broken bundle. Correct the sourcing of the outcome claims. The live-pilot bundle records "not executed" and holds no arm outcome; the INCONCLUSIVE results and the HTTP 403 come from the workflow run records, not from the bundle. Separate the two and link each. Also record that A3 never started on the second attempt. Signed-off-by: Dong1017 <55723162+Dong1017@users.noreply.github.com> Agent-assisted-by: Claude Code / claude-opus-5 (author)
|
Both points resolved at head 2.
|
| Bundle | Files | Reference |
|---|---|---|
kv-live-pilot-20261031 |
patch/README.md |
tests/fixtures/kv_reclamation/candidate.py |
kv-live-pilot-20261031 |
plan/plan.json, plan/A2.json, plan/A3.json, plan/second-attempt/{plan,A2,A3}.json, plan/second-attempt-1200/plan.json |
tools.qualify_kv_reclamation, tools.kv_live_job |
kv-actor-qualification-20261030 |
results/hosted-41154288295/qualification.json, results/hosted-41182807900/qualification.json |
kv_edit_workbench |
kv-reclamation-20261030 |
patch/README.md |
tests/fixtures/kv_reclamation/candidate.py |
The page now states that each resolves at e843b9d3 and none resolves in the working tree, which is the expected result of the removal rather than a broken bundle.
Non-blocking: you were right, and the claims were worse than unverified
Checking the receipts for lines 37-42 found that the page attributed its outcome claims to the wrong source.
evidence/kv-live-pilot-20261031/results/README.md says:
Prospective pair: not executed … No paid model output, timing result or efficacy verdict exists in this bundle.
So the retained bundles hold frozen inputs and a not-executed marker. They contain no arm outcome at all. The INCONCLUSIVE results and the HTTP 403 come from the workflow run records (41159550621, 41194057284), assessed in issue 248 — not from the evidence the page points at. Reading the page as written, those claims looked bundle-backed, which they are not.
The section is now a per-claim table:
| Claim | Receipt |
|---|---|
| No completed arm, per-arm outcome or A/A variance | results/README.md, quoted above |
| Dollar figures are reserved upper bounds, not observed spend | plan/second-attempt-budget.json: worst_case 162,720,000 microUSD against the PR and pilot lifetime caps |
Both attempts INCONCLUSIVE, the second stopping on HTTP 403 |
The two run records and the #248 assessment |
The page now says outright that the third row is not recorded inside the bundles, and that a workflow conclusion of success means orchestration and retention finished rather than a request being delivered or charged. One factual gap also closed: A3 never started on the second attempt, which the page did not say.
Checks at this head
Local, on the pushed tree:
| Check | Result |
|---|---|
ruff check src tests tools |
All checks passed |
pytest --collect-only -q |
3,183 collected, 0 collection errors |
python -m tools.validate_evidence |
exit 0, 21 bundles, 430 hashes, 0 errors |
The retirement page is not itself hash-pinned — grep finds it in no manifest, only in three retained evidence files that link to it — so editing it changes no pinned digest, and the validator result above confirms that.
No local full-suite pass is claimed; the hosted lanes are the authority for that.
@zengchuang-hw — your approval was on 029878bb and this push dismisses it under dismiss_stale_reviews_on_push. Sorry for the re-request; the change is documentation only.
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Incremental review 029878bbb000d82e6e6d38d895c0865a4adbf43b..10f70e09c3874629c38a56b308fbba32ab7e93b4. Reviewed head 10f70e09c3874629c38a56b308fbba32ab7e93b4 targets main (PR base metadata e843b9d31cb86e1fd12d00890c89798b0bedd1fe, current main 5c86eb57030c4aea50592b66451929c8cee705d6, actual merge base 9dded7adac0ac835f6a73493309399e328a43ab2). The retirement change has no remaining approval blocker. Previous review · Re-review request
本轮已解决
- The retirement page now points to a readable pre-retirement snapshot on main. I checked the 11 retained references named by the inventory and resolved the four historical source files. Their blobs match the actual merge base. All 64 files across the four retained bundles and the evidence inventory are unchanged in the PR. Earlier finding · Historical source tree
- The page separates the bundle’s frozen inputs and not-executed marker from the live attempt records. The budget file records a 162,720,000 microUSD reserved bound. The run records and Issue assessment identify both attempts as INCONCLUSIVE, with HTTP 403 on the second attempt and A3 unstarted. Budget record · Run and outcome assessment
- The main ruleset requires
knowledge-model-policy, and classic status enforcement is off. The retiredkv-task-qualificationjob is outside the required checks. Main rules · Branch metadata
批准前必做(Before I can approve)
None for the retirement PR. The broader task and its acceptance criteria remain open under #248. Governing Issue
非阻塞问题或建议
Call e843b9d3 a pre-retirement snapshot on main. The actual common ancestor is 9dded7ad. The inventory could also distinguish the four A2/A3 configuration files that name kv_edit_workbench.py from the plan files that name the two tools modules. The historical paths already resolve. Retirement page
Checks and limits
Reviewed the single-file delta, checked Git diff whitespace and evidence paths, and cross-checked the evidence and ancestry with an independent review agent. All five current-head Ready jobs passed, including Python 3.10 and 3.11 full-check and installed-package jobs. The policy status passed. This incremental review used hosted CI for runtime tests and the existing records for the historical attempts. It did not re-run a live experiment or verify actual billing. Ready CI · Policy
Direct delegation: zengchuang-hw, the account holder, authorized this review in the Codex chat. Codex executed the independent review using the connected GitHub account zengchuang-hw, verified with repository admin permission. The account and reviewing task are independent of the delivery slice. The delegation covers the PR content and applicable checks, responses to review requests, and a revision-bound formal Approve when no blocker remains. This submission uses that Approve authority.
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed incremental range 029878bbb000d82e6e6d38d895c0865a4adbf43b..10f70e09c3874629c38a56b308fbba32ab7e93b4 at head 10f70e09c3874629c38a56b308fbba32ab7e93b4.
Checked:
- Diff is docs-only (
docs/kv-reclamation-qualification.md, +38/−5). - History pointer to
e843b9d3is present; that commit is an ancestor of currentmain; spot-checked removed paths still readable there (tools/qualify_kv_reclamation.py,tests/fixtures/kv_reclamation/candidate.py,src/inferrsi/harness/kv_edit_workbench.py) and absent at this head. - Reference inventory names the retained files that still cite deleted paths; claim/receipt table matches
evidence/kv-live-pilot-20261031/results/README.md(not-executed) andplan/second-attempt-budget.json(prior_binding.worst_case162720000). - Required-check claim holds: classic
mainprotection has empty required contexts; combined status for this head reports onlyknowledge-model-policy(success); nokv-task-qualificationcheck run at this head. Ready jobs (full-check / installed-package 3.10+3.11, pr-references, declarations) are green.
Prior amy points from #357 (comment) are resolved. No remaining content blocker for this retirement docs fix.
Governing Issue and delivery slice
Refs #248
Removes the tooling of the bounded synthetic CPU KV experiment. All retained evidence stays in place and unmodified. This is a cleanup of code that no longer has a path to run; it decides nothing about #248's acceptance, which remains open with neither completion criterion met.
Why this is removable
The slice had its own consumers only. Nothing in the general harness, the SkillEval modules, the runner or the evaluator imported it:
git grep -E '\bkv_|kv\.'acrosssrc/inferrsi/harness/skill_eval*.py,evaluation.py,runner.py,improvement.py,reporting.pyandtools/skill_eval_*.pyreturns nothing. The dependency ran one way only — from this slice onto shared parts (skill_eval_engineering.sandbox_command,skill_eval_budget,harness.runner.run_experiment,infra.artifacts,model.anthropic_agent,runspec.RunSpec). Removing it cannot reduce the general SkillEval capability.Its only non-self consumers were the
kv-task-qualificationjob in the ready lane — self-described there as "not a branch-protection or efficacy gate" — and four rows indocs/operations.md.What changed
tools/kv_*.py,tools/qualify_kv_*.py,tools/kv_live_authorization.jsonsrc/inferrsi/harness/kv_reclamation.py,kv_edit_workbench.pytests/test_kv_*.py,tests/test_qualify_kv_*.pytests/fixtures/kv_reclamation/docs/kv-edit-integration.md,kv-live-launcher.md,kv-pilot-admission.md.github/workflows/kv-edit-pilot.ymlAlso: the
kv-task-qualificationjob drops out of.github/workflows/ci-ready.yml(52 lines), and the four operator rows drop out ofdocs/operations.md.What is kept, and one constraint that forced it
evidence/kv-reclamation-20261030/,kv-actor-qualification-20261030/,kv-actor-qualification-20261031/andkv-live-pilot-20261031/are untouched — 64 files, 2.3 MiB — along with their four entries intools/evidence_inventory.json.docs/kv-reclamation-qualification.mdstays at its path, rewritten as a retirement record rather than deleted.evidence/kv-reclamation-20261030/README.md:59links to it, and that README's SHA-256 is pinned in its ownplan/publication-integrity.json(7c6d65e4…), so the link cannot be edited out without breaking the manifest. Deleting the page would leave a dangling link inside hash-pinned evidence. The rewritten page records what was removed, where the evidence lives, and what that evidence does and does not show.src/inferrsi/model/anthropic_edit_agent.pyis also kept. Its only current consumers are the removed code and its own test, but it is a general edit-capable model adapter rather than an experiment-specific module. Removing it would widen this change beyond cleanup; flagging it here so a maintainer can decide separately.Checks
Local, on Python 3.13 at this head:
ruff check src tests tools: passed.python -c "import inferrsi.harness, …skill_evaluation, …skill_eval_engineering, …runner, …evaluation": passed.python -m tools.validate_evidence: exit 0, 430 hashes checked, all 21 bundles including the 4 retained KV bundles verified.git diff --cached --check: clean.docs/operations.mdshows exactly 4 removed lines, with no line-ending noise.Not run locally: the full suite. Several of this repository's tests probe bubblewrap isolation and time out on Windows, so a full local run is not meaningful here. The ready lane runs it on Ubuntu; that is the full-suite evidence for this PR, and I am not claiming a local pass.
Limitations
This changes no behavior of any remaining feature. It is not an acceptance, rejection or rescoping of #248: both of that Issue's completion criteria are still unmet, its two 2026-10-31 attempts remain
INCONCLUSIVE, and the retained evidence that records those failures is deliberately preserved here. Removing the tooling does not make the experiment repeatable or unrepeatable — the Issue's own freeze already forbids a third attempt.Merge and any scope decision on #248 remain with the maintainers.