Repository navigation
Package shared technical contracts and retire root modules - #453
Conversation
Agent-assisted-by: Claude Code (author) Agent-assisted-by: Grok Bot (author) Signed-off-by: GUOGUO <xwdong1998@163.com>
Agent-assisted-by: Grok Bot (author) Signed-off-by: GUOGUO <xwdong1998@163.com>
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base a92c77c and head 7b4f142.
Verified #451 C1 delivery: json_v1/runspec/claim moved into inferrsi.contracts at 100% similarity; contracts/__init__.py re-exports only RunSpec; the four root modules are deleted without forwarding shells; consumers, isolation gates, changelog and layout docs were updated; the exact 23-file root inventory guard is in place and shrinks in later removal PRs. Leaves import only the stdlib, and the cold-import / no-I/O / identity / retirement tests match the accepted acceptance criteria. Remote CI is green (lint-and-smoke, declarations, pr-references, installed-package 3.10/3.11, full-check 3.10/3.11, dashboard-browser). Knowledge-model-policy: Not applicable.
|
@amy-why-3459 @zengchuang-hw Please review C1, tracked by Issue451. This PR packages shared contracts, retires four root modules, migrates consumers, and adds root-inventory and RunSpec identity/no-IO checks. It includes the latest main changes; validation results and baseline limitations are documented in the PR body. |
zengchuang-hw
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS (C1 only)
Initial independent review requested in #issuecomment-6894982574.
Reviewed base a92c77c → head 7b4f142. Previous review: none from this account for this PR; resolved prior findings: not applicable.
I read the actual diff, #451 C1 requirements, AGENTS and the repository review guidance, without using other reviewers' opinions. The three moved implementation files are byte-for-byte identical to their base versions. Active consumers use the new canonical modules; the initializer has only the RunSpec re-export. The exact 23-file root guard, bounded import-closure changes and negative mutation tests fit this slice, and the changelog explicitly discloses retired import/pickle paths. I found no blocking defect in C1.
Independently executed in an isolated Linux/Python 3.12 checkout:
- Eight focused suites (contracts package, root inventory, first-wave isolation, target-module import gate, claim, Apps assess, Harness run store and execution): 208 passed, 1 skipped (Windows Job Object), 86 subtests passed.
- Changed existing/new Python files: Ruff passed; git diff --check passed.
- Built real SCM sdist and wheel, installed wheel into a fresh venv with the skills extra, ran check_installed.py outside checkout with Python -I and no PYTHONPATH: 128 source-grounded records and 923 resource files verified against checkout, including retirement/identity/root checks.
- Verified implementation byte equality and absence of active source imports of the retired modules.
- Observed eight successful hosted checks on this exact head, including ready full-check and installed-package on Python 3.10/3.11 and dashboard-browser. These supplement, not replace, the source review.
The first local test invocation used a relative PYTHONPATH and failed one subprocess import after changing its working directory; the complete eight-suite rerun with an absolute source path passed as reported. The first wheel checker lacked the documented optional PyYAML dependency; installing the skills dependency and rerunning the isolated checker passed.
Not independently executed locally: whole-repository pytest, Windows tests, browser suite, hardware/performance experiments. No hardware or performance claim is made. Local SCM version reflects the tags present in this clone and is not a reproduction of the author's wheel hash.
Before approval: no remaining C1 blocker or required evidence gap found. Optional suggestions: none. C2 and the other architecture slices remain separate; this approval neither closes #451 nor establishes final target-tree acceptance. No merge or auto-merge performed.
Governing Issue and delivery slice
Refs #451
inferrsi.contracts(json_v1,runspec,claim); retire the four corresponding root files in this PR without forwarding shells; migrate active consumers; retain exactly one RunSpec initialization path and same-object facade/consumer identities; enforce the exact 23-file root inventory and bounded technical-contract import rules.Knowledge authorship declaration
The Claude model value is the requested/reported client alias, not independent backend attestation. The coordinator directly made mechanical updates to
AGENTS.md,CHANGELOG.md,docs/contracts/current-design.md, anddocs/module-layout.md; these are not attributed to Claude. Both narrow same-session documentation continuations ended with API request timeouts before editing. Across all 52 changed paths, the existing protected-path and policy-path matchers each matched zero paths; expected knowledge-policy result is Not applicable, not an exemption or admission of unknown model settings.Changed artifacts and evidence
_json_v1,runspec,claimimports/pickle paths fail as intended;inferrsi.contractsremains the canonical package, not a retired import name; exact root guard and mutation negatives passChecks and dependencies
f05118ea1f74005ef869f0b41c64d1f26af2f102, then an ordinary clean merge of main/Connect report Dashboard and trace to fixed attempt records #450, producing head7b4f142ee5f224f25731c7bfc3a5dff762df1641. Latest pre-push main read wasa92c77ccaed68426c83154cff7619745f34d0058; no rebasing, history rewriting or force push. Main's eight-file Connect report Dashboard and trace to fixed attempt records #450 change and all C1 work were retained. Preserved candidate-file hashes were compared across integration, with three genuine Connect report Dashboard and trace to fixed attempt records #450 overlap files checked separately.tests/test_apps_assess.pyalone: 28 passed on the exact merged head. Already-passed suites were not rerun in the final fixture-only segment.inferrsi-0.1.11.dev18+g7b4f142ee-py3-none-any.whl, SHA256c759cadafa1b6bb9b2d97a3b172bccfc074170372a7d86d746afcd00f63c8bca; 1102 entries, no mandatory runtime dependencies.request_digestshadow diagnostics inharness/run_store.py; exact diagnostic signatures reproduce on current main. No suppression or public-keyword rename was introduced. Four broader-regression Windows skips remain (symlink privilege/POSIX process semantics). These are not presented as passing checks. Remote CI had not yet been observed when this body was created; no all-green claim.Remaining work and Issue closure
Handoff
C2's retained implementation plan is explicitly coordinator-verified planning, not a final Opus output or approval: the original read-only planner ended without a final response. It preserves resource identity, splits pure evidence paths from app-side directory creation, retires the old two roots in its own slice, and adds only the exact
contracts.evidenceboundary exception rather than a broad package allowance. No next implementation is running.