Repository navigation
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.
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.