Repository navigation
Conversation
Refs #412 Agent-assisted-by: Claude Code (author) Signed-off-by: GUOGUO <xwdong1998@163.com>
Skill evaluationPLANNED: not executed; there are no task outcomes yet.
Author verification steps (nonpaid; not yet run)
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"}Provider/settings: Uncovered / unsupported paths (94)
|
|
@amy-why-3459 @zengchuang-hw Please review R1, tracked by Issue412. This PR retires eleven root modules, preserves the canonical Apps inspection binding, migrates active consumers, and updates the exact root10 and neutral-leaf guards. Head |
zengchuang-hw
left a comment
zengchuang-hw
left a comment
There was a problem hiding this comment.
PR review verdict — DO NOT PASS
Independent first review of R1 for #412, requested in #459 (comment).
Reviewed range: 5db1bc94042f386f0b4b35c3e6fe91598fef118f..e406ba65b61fdf722365bc94777294b9b2f86d6f (one commit). No previous review by this account on this PR; resolved prior findings: not applicable. This conclusion comes from the actual code and independent checks, not other reviewers' opinions.
Blocking regression, P1: the migrated real-port architecture guard now contradicts itself.
At tests/test_architecture_closed_loop.py:358–373, the required imports were correctly migrated to inferrsi.knowledge.knowledge_operations and inferrsi.apps.legacy.evaluation, matching tests/architecture_real_ports.py. However, the subsequent forbidden-prefix loop still rejects every inferrsi.knowledge.* and inferrsi.apps.* import, including those two mandatory imports. The test deterministically fails at line 373.
Independent reproduction on this exact head:
python -m pytest tests/test_architecture_closed_loop.py::ClosedLoopTest::test_real_ports_stay_on_three_existing_seams
# FAIL: AssertionError at line 373
The identical command against the base commit passes (1 passed). This is a regression introduced by this migration, not a pre-existing test failure.
Required before approval: migrate the guard consistently to allow only these exact intended canonical seams while continuing to reject unrelated Apps/Knowledge imports, CLI/parser and Harness runner coupling. Add negative coverage for adjacent/unintended modules; do not simply remove the broad boundary check. Rerun the architecture suite and required checks on the corrected head.
Other independently checked R1 properties:
- Eleven root files are removed, canonical host-inspection binding preserves late-bound command/snapshot ports and exceptions, and the legacy runner resolves omitted ports at call time while explicit ports win.
- The manifest archives the previous nine pair records and catalog routes unchanged; compatibility pairs are now zero. The permanent importer is the exact Infra inspection/pack-reader edge, with the separate temporary root-arguments REPO_TYPES edge still reported as an A1 blocker. The protected-path diff removes only the three deleted root entries; canonical protections remain.
- The actual-tree report has no violations but still has the temporary importer, 12 Harness→Infra hits and five runtime-loader blockers. Isolation remains false. This does not complete #412 or A1/R2/R3.
- Frozen evidence and legacy fixtures have no diff. Import/pickle incompatibility and canonical replacements are documented.
Checks actually executed in an isolated Linux/Python 3.12 checkout:
- Focused inspection, legacy-run/frozen contracts, isolation/import/retirement/root inventory, resource facade/contracts, policy, CLI and runtime suites: 484 passed, 267 subtests passed.
- Architecture closed-loop, campaign-record injection, Knowledge operations/store/CLI/tiers and resource migration: 118 passed, 2 skipped, 1 failed, 2 subtests passed (failure above).
- Optional-dependency, research, CLI/catalog, Knowledge contract, task-selection and manifest suites: 76 passed, 2 subtests passed.
- Independent base/head single-test comparison as above.
- Ruff on every surviving changed Python file and
git diff --check: passed. - Real SCM sdist → wheel build, normal installation into a fresh outside-checkout venv with PyYAML, and isolated
tests/check_installed.py: passed, 128 records and 923 resource files; inventory SHA-25698394be4528916f4de7199a9a21f2b9d00433895b8546a73ee603fbb762faf47. This local build produced version0.2.2.dev10+ge406ba65b; it is independent installed-content validation, not a claim to reproduce the author's differently versioned wheel bytes. - Exact-head GitHub checks observed: installed-package 3.10/3.11 and lint-and-smoke succeeded; full-check 3.11 failed and full-check 3.10 was still running at last observation.
Not executed: full repository suite, Windows tests, accelerator/service experiments. Initial default-Python invocation lacked pytest; after installing dependencies in a separate venv, the commands above ran. No source edits, pushes, merge or auto-merge actions were taken.
Non-blocking suggestions: none beyond keeping the already documented staged-isolation limitations explicit. Submitting COMMENT rather than REQUEST_CHANGES under the review action scope; approval is withheld until the regression is fixed and revalidated.
|
本次看的是
R1 主线（删除 11 个根模块、Apps inspection 绑定与按次读取端口、 |
|
@amy-why-3459 @zengchuang-hw Both reported regressions are fixed in |
zengchuang-hw
left a comment
zengchuang-hw
left a comment
There was a problem hiding this comment.
PR review verdict — DO NOT PASS (required-check evidence pending; previous code blocker resolved)
Independent incremental re-review of R1 / #412. Previous review: #459 (review) . Reviewed increment: e406ba65b61fdf722365bc94777294b9b2f86d6f..225d578ef623324329a46db04eb368a3644f603f (one commit); current base 5db1bc94042f386f0b4b35c3e6fe91598fef118f. I read the actual two-file diff and affected assembler/test code independently, without using other reviewers' opinions.
Resolved: the real-port architecture test no longer simultaneously requires and bans its two canonical seams. The six banned families remain intact; exemptions are the two exact module names, not prefix allowances. Added controls reject adjacent/descendant modules, package imports, aliases, nested imports, CLI/parser/runner/Benchmark imports, and mutations of the real assembler. The import collector retains all module names checked previously and additionally inspects absolute from-import candidates. No product/runtime source changed in this increment. The regenerated skill-coverage digest matches its sources.
Checks actually executed on this exact head in an isolated Linux/Python 3.12 worktree:
PYTHONPATH=src:tests python -m pytest -o addopts='' -q tests/test_architecture_closed_loop.py tests/test_first_wave_isolation_policy.py tests/test_target_module_import_gate.py: 82 passed, 293 subtests passed. This includes the previously failing test and the new negative/mutation controls.- Ruff on the changed Python file: passed.
python tools/build_skill_coverage.py --check: passed.git diff --check e406ba65 HEAD: passed.
Approval prerequisite still outstanding: GitHub's exact-head full-check (3.10) and full-check (3.11) are both still in progress at this review. I have not independently rerun the full repository suite, so I cannot treat these aggregate checks as passed. Complete them successfully, or investigate any failures, before approval. Exact-head installed-package 3.10/3.11, lint-and-smoke, dashboard-browser and pr-references succeeded. A later declarations run succeeded after an earlier failed run; that does not replace the unfinished full-check matrix.
Not rerun this round: full suite, independent sdist/wheel installation, Windows or accelerator/service checks. The prior review's extensive unchanged-code and installed-content evidence remains historical evidence at its stated old head, not a claim of re-execution here. This narrow re-review resolves the reported R1 test regression; it does not declare overall #412 isolation or A1/R2/R3 complete.
Non-blocking suggestions: none. No new code blocker found in the increment. Publishing this exact-head COMMENT now rather than withholding the review while CI runs; no REQUEST_CHANGES, approval, push, merge or auto-merge action.
zengchuang-hw
left a comment
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Independent R1 re-review for #412, current base 5db1bc94042f386f0b4b35c3e6fe91598fef118f, exact head 225d578ef623324329a46db04eb368a3644f603f.
Increment reviewed: e406ba65b61fdf722365bc94777294b9b2f86d6f..225d578ef623324329a46db04eb368a3644f603f.
Previous blocking review: #459 (review) . Detailed incremental code/test review: #459 (review) . This approval completes that review after the outstanding aggregate CI evidence arrived; it is not based on other reviewers' opinions.
Resolved: the architecture guard's required-versus-banned canonical-import contradiction. Only the two exact canonical seams are exempted; the six forbidden families and negative/mutation coverage remain enforced. The coverage-document input digest is regenerated correctly. No runtime/product code changes in this increment, and no new blocking code issue found.
Independent checks actually executed on this head: architecture closed-loop, first-wave isolation policy and target-module import-gate suites: 82 passed, 293 subtests passed; changed-file Ruff, skill-coverage --check and incremental git diff --check: passed.
The remaining approval prerequisite is now satisfied: exact-head GitHub full-check (3.10) and full-check (3.11) both completed successfully, alongside installed-package 3.10/3.11, lint-and-smoke, dashboard-browser and pr-references. I also verified the apparent declarations conflict: run 43758975445 failed, then run 43759111145 succeeded for the same head, same workflow ID 391541746 and .github/workflows/knowledge-model-policy.yml, same pull_request_target event type. The separately published current knowledge-model-policy commit status is success. Thus the earlier historical failure is superseded by the later successful declaration reconciliation, not an uninvestigated failure being ignored. The execute job is skipped, not a passing execution claim.
Not independently rerun this round: complete repository suite, local sdist-to-wheel installation, Windows or accelerator/service experiments. Complete-suite and installed-package evidence this round is the exact-head CI above; the prior review's local installed-content verification remains evidence only for its stated old head. This approval is scoped to R1 and does not assert overall #412 completion, final isolation, A1/R2/R3 delivery or campaign performance PASS.
Approval-before-do items: none outstanding for this reviewed R1 change. Non-blocking suggestions: none. PR author, open state, base/head and this account's absence of an existing same-head approval were rechecked. No source edits, push, merge or auto-merge actions.
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed range e406ba65b61fdf722365bc94777294b9b2f86d6f..225d578ef623324329a46db04eb368a3644f603f (one fix commit) against unchanged main 5db1bc94042f386f0b4b35c3e6fe91598fef118f. This is a re-review of the two blockers I reported on the previous head.
Verified:
- The seam guard now exempts only the two exact canonical seams (
inferrsi.knowledge.knowledge_operations,inferrsi.apps.legacy.evaluation) and still bans the six original families, including sibling/parent/child modules, CLI/parser,harness.runner, and Benchmark. The three new negative tests cover those cases. Independently ran the fulltests.test_architecture_closed_loopsuite: 18 passed, including the previously failingtest_real_ports_stay_on_three_existing_seams. docs/skill-coverage.mdmatches an independenttools/build_skill_coverage.py --checkandtests/test_skill_coverage_view.py(18 passed). The regenerated input digest issha256:fa0002a1fd489bb9d70a4462ed2e941b98f5641e2cc7ef72a682ed0c5a0d6bb9. My previous comment swapped the committed vs regenerated digests; reproducing--checkone406ba65confirms the committed file still hadfbc4d29eand the generator producedfa0002a1. This head has the correct generated digest, and only that line of the view changed.- GitHub exact-head checks observed green: full-check 3.10/3.11, installed-package 3.10/3.11, lint-and-smoke, dashboard-browser, pr-references, and the later knowledge-model-policy declarations run. The earlier declarations failure on this same head was superseded by that later success after the PR body declaration was updated to this SHA. The skill-eval execute job is skipped, not a passing execution claim.
This approval is scoped to the R1 fix on this head. It does not claim #412 completion, final isolation, or A1/R2/R3.
Summary
Refs #412. R1 of the agreed C1 → C2 → R1 → A1 → A2 → M1 → R2 → R3 queue; Issue412 remains open for R2/R3.
evaluation,execution,reporting, andinspection, with no forwarding shells or module aliases. Migrate current CLI/arguments, tools, tests, and active documentation to canonical owners. CHANGELOG maps every retired import and explains external import/pickle impact; historical frozen evidence remains unchanged.inferrsi.apps.inspectionover the unchanged Infra facts API, retaining command/snapshot/repository-identity injection and failure behavior.apps.legacy.run.run_experimentresolves omittedexecute/snapshotports from its module at call time (defaults nowNone); this preserves the retired root entry's patchable binding and is explicitly documented.compatibility_pairsfrom nine to zero, archiving the original pair/routes records unchanged. The sole permanent neutral-leaf rule is exactinfra.inspection → resources.repository_packs, binding onlyrepository_packthrough a plain top-level import, with negative controls. Rootarguments.pyhas a separate temporary exactREPO_TYPESimporter/blocker, due for removal in A1; it is not a compatibility pair or a permanent Apps allowance.Independent validation
REPO_TYPESidentity and all seven pack bytes match. Wheel version0.1.11.dev19+ge406ba65b; SHA-25697f6dd0c6743f8c4fa0de74da0aec55b8c33c444125e8463b3e6932fe16f9d88.Review-fix follow-up (2026-11-16, Asia/Shanghai)
Follow-up commit
225d578ef623324329a46db04eb368a3644f603fchanges onlytests/test_architecture_closed_loop.pyand the generateddocs/skill-coverage.md. Both reported failures were real R1 regressions, not baseline failures: Linux full-check 3.10 and 3.11 each reported 2 failed / 4650 passed at the originale406ba65head.tools/build_skill_coverage.py, then its--checkand the two affected committed-view/byte-identical-regeneration tests: all exit 0. The tool and local metadata-reader imports/calls were audited: no AI inference or network/model call is used. There was no manual edit to generated content and no claim that the model executed the generator.fbc4d29e...; the unchanged generator correctly emits input digestfa0002a1.... This corrects the earlier relayed digest direction; these are input digests, not whole-document hashes. The generated document's actual SHA-256 is645f404a8127251b20518fcd4bac198416d05bc9340eec5cca0b89cb9293220d.git diff --checkpassed. Ordinarygit addinitially refused this generated document outside the existing sparse definition; the retained log records that environment/staging error. Supportedgit add --sparse --staged exactly the two accepted paths, without changing sparse/global configuration. Normal commit and SSH push succeeded; no hooks were skipped, no history was rewritten, and the retained untracked log was not staged.5db1bc94042f386f0b4b35c3e6fe91598fef118f. No runtime source, policy, manifest, frozen input or generator changed in this follow-up. The earlier installed-wheel evidence remains tied to originale406ba65; no new-head wheel/full-suite/Linux-pass claim is made and no unchanged wheel/broad suite was rerun. New-head CI and independent reviewer rechecks remain pending.Integration and remaining scope
Normal checkpoint
e406ba65b61fdf722365bc94777294b9b2f86d6fis based on main5db1bc94042f386f0b4b35c3e6fe91598fef118f(merged #458). No rebase/amend/force/reset/clean; unrelated old #408 work, evidence branches and retained worktrees are untouched. Main was independently re-read through GitHub at 2026-11-16T03:03:05+08:00 (Asia/Shanghai), still the same SHA; exact-head-branch PR lookup was empty. The publisher must re-check main and branch immediately before push/creation.The real gate has zero violations and zero compatibility pairs, but three remaining blockers: the temporary root-arguments leaf importer (A1), twelve Harness→Infra package/artifact path hits across six modules (R2), and five runtime resource loaders (R3). Both isolation flags remain false. This PR does not implement A1/A2/M1/R2/R3 or claim final isolation.
Validation is Windows/local and targeted, not a full-suite or Linux-pass claim. Four pre-existing Ruff F811 findings in unchanged
harness/run_store.pyremain outside this diff. An older root-data diagram predating R1 was not expanded into this change. A1 read-only planning produced no final plan at its finite bound and has not been represented as completed or implemented.Review and authorship
Formal architecture review is required for the manifest revision, permanent exact leaf rule, temporary importer and protected-path maintenance; passing tests are not that approval.
All substantive hand-authored source/tests/docs edits were produced by the same admitted Claude Code Fable 5.2/max implementation session, including the final migration and active-consumer docs and this review-fix test change. The coordinator performed scope control, independent verification, mechanical execution of the unchanged deterministic generator, Git operations and PR metadata, not substantive product authorship. The generated coverage view was not model-executed or manually authored. Model/effort values are client-requested/reported provenance, not independent backend attestation.