Repository navigation
docs: record the target module layout and import gate - #376
Conversation
State the four-module directory rule without moving files, and fail the build if a new Knowledge, Benchmark, or Apps import appears outside Apps. A clean scan does not mean those modules are isolated. Agent-assisted-by: Cursor Grok 4.7 (author) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Dong1017 <Dong1017@users.noreply.github.com>
|
@zengchuang-hw @amy-why-3459 Please review this directory-rule and import-gate slice for #372. It does not move files, does not create empty packages, and does not change Knowledge interfaces. A clean import scan means no new |
There was a problem hiding this comment.
PR review verdict — DO NOT PASS
Reviewed base 9f61a83f030bfa2ffe3b54eac1781593309d6380 and head 9409b91ed10245974a858b207b8c6cc2728db597 (full diff, opened PR #376).
docs/architecture.md correctly points at docs/module-layout.md and states that this page does not move code. The layout page keeps contracts.py as a single re-export, leaves src/ as the import root, and refuses empty knowledge/, benchmark/, and apps/ directories in this slice. The new import gate does not implement the rule that page states.
Blockers:
-
tests/test_target_module_import_gate.py:31-37(_modules_in_source, ImportFrom branch) records only the module base and drops imported names.
Trigger:from inferrsi import knowledgeandfrom . import knowledgein a module whose package isinferrsiboth loadinferrsi.knowledge(same forbenchmarkandapps). The scanner storesinferrsi, and_matchesdoes not treat that asinferrsi.knowledge.import inferrsi.knowledgeandfrom inferrsi.knowledge import …are recorded.
test_detector_flags_a_new_core_import_outside_apps(tests/test_target_module_import_gate.py:94-105) rebuilds a smaller check on the literalimport inferrsi.knowledgeand never calls_modules_in_sourceorcross_core_violations, so this miss stays green.
Expected correction: treat each ImportFrom alias as a candidate submodule (inferrsi.knowledgeforfrom inferrsi import knowledge) and assert the real scanner flags that form outsideappsand allows it fromapps. -
tests/test_target_module_import_gate.py:51-60(cross_core_violations) forbidsinferrsi.knowledge,inferrsi.benchmark, andinferrsi.appsfor every importer other thanapps, including the core itself. Relative imports are rewritten to those absolute names (from .bar import bazinknowledge/foo.pyresolves toinferrsi.knowledge.bar).
Trigger: aknowledge/orbenchmark/module with a normal intra-package import. The gate reports a cross-core violation even thoughdocs/module-layout.md:9-10says the cores do not import one another, and lines 52-55 say code underknowledge/orbenchmark/must not import Harness, Apps, or the other core (own package stays allowed). The second loop already skipsown; the first loop still reports it.isolation_complete(tests/test_target_module_import_gate.py:79) then stays false viacross_core_violationsafter the three directories exist andharness/no longer imports Model or Infra.
Expected correction: exempt a core's own prefix, and align the Import gate paragraph indocs/module-layout.md:52-55with that rule. Cover an own-package relative import (allowed) andknowledge→inferrsi.harness(forbidden) on the real scanner.
Limits: I did not run tests/test_target_module_import_gate.py. I did not compare the "Old run exit codes" table in docs/module-layout.md to src/inferrsi/cli.py; the review seed is the diff. importlib imports sit outside the AST walk (name that bound in the gate paragraph if it is intentional).
This comment does not approve the PR or change its scope. I will re-read after the author pushes.
|
本次 review 范围:完整 diff(新 PR),base 阻塞项
上面 2–4 和另一条 bot review 的结论一致,我是按代码独立核对的。 非阻塞建议
|
|
Independent check on Before I can approve
The existing four focused tests passed locally, including the current-tree scan. I also checked the existing |
Resolve from-import aliases, allow same-core imports, and pass the scan root into the parser. A temporary package tree now checks the forbidden, Apps, and same-core cases without claiming isolation. Agent-assisted-by: Cursor Grok 4.7 (author) Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Dong1017 <Dong1017@users.noreply.github.com>
|
@zengchuang-hw @amy-why-3459 Please re-review head |
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base 9f61a83f030bfa2ffe3b54eac1781593309d6380 and head f827acec05672a61d99798fdaa57ef286175c26a.
Incremental range 9409b91ed10245974a858b207b8c6cc2728db597..f827acec05672a61d99798fdaa57ef286175c26a.
Earlier finding — resolved:
_modules_in_source now records each ImportFrom alias (base.alias), so from inferrsi import knowledge and from . import knowledge in package inferrsi both yield inferrsi.knowledge (same for benchmark and apps). cross_core_violations classifies those names through _core_of. test_real_tree_allows_apps_and_same_core_only calls that helper on a fixture: harness/probe.py (from inferrsi import knowledge) and knowledge/out.py (import inferrsi.harness) are the only hits; apps imports and same-core imports stay off the list.
Verified on this diff:
- Same-core skip (
importer == target), includingknowledge/foo.pyresolvingfrom .bar import VALUEtoinferrsi.knowledge.bar. Apps still bypass every target. Knowledge or Benchmark importing Harness is still recorded. One(importer path, target core)line. docs/module-layout.mdstates that rule, still says isolation stays false in this slice, listsclaim.pyas not moved, and pins the frozen exit-code text tosrc/inferrsi/cli.pyat base9f61a83f030bfa2ffe3b54eac1781593309d6380lines 412-417.
Not verified here:
- Tests were not executed. CI was not checked.
- The repository tree was not scanned in this session. The new detector test builds a temporary tree.
- The cli.py blob lines 412-417 were not opened.
Optional: rename test_real_tree_allows_apps_and_same_core_only so the name matches the temporary tree. from inferrsi import * stays unresolved, because * has no alias names.
No outstanding blockers.
amy-why-3459
left a comment
There was a problem hiding this comment.
LGTM
PR review verdict — PASS
Reviewed base 9f61a83f030bfa2ffe3b54eac1781593309d6380 and head f827acec05672a61d99798fdaa57ef286175c26a; incremental re-review of 9409b91ed10245974a858b207b8c6cc2728db597..f827acec05672a61d99798fdaa57ef286175c26a (1 commit).
核对内容:
- 上次的 4 个阻塞项都已修复:E501 已消除(本文件无超过 100 字符的行,
lint-and-smoke通过);同核心导入(含相对导入)豁免、按 (importer, target) 去重;from inferrsi import knowledge/from . import knowledge通过 alias 候选名被检出;测试改为在临时目录树上调用真实的cross_core_violations(root)。 - zengchuang-hw 指出的 root 问题已修复:
_imported_modules(path, root)在cross_core_violations和isolation_complete里都传入给定 root。 - 我另外用临时树独立跑了这个 head 上的检测逻辑:knowledge→benchmark、benchmark→
from inferrsi import apps、benchmark→harness 子模块、顶层模块from . import knowledge各报一次;apps→harness/knowledge、harness 内from inferrsi.harness import knowledge均不误报,结果符合docs/module-layout.mdImport gate 段的描述。 - 非阻塞项也已处理:归属表补了
claim.py;cli.py链接的 L412–L417(@9f61a83f)对应return (0 … 1 … 2)和except (OSError, ValueError, json.JSONDecodeError, sqlite3.Error)→argument_parser.error,与退出码表一致。 - 当前 head 上所有 check 均为 success(lint-and-smoke、full-check 3.10/3.11、installed-package 3.10/3.11、pr-references、declarations)。PR 描述里的
head_sha与当前 head 一致。
可选后续(不阻塞):临时树用例可以再加一条 knowledge→benchmark(两个新核心之间)和一条同一文件两种写法只报一次的断言,把去重行为显式锁住。
Governing Issue and delivery slice
Refs #372
Refs #362
docs/module-layout.md, a pointer indocs/architecture.md, andtests/test_target_module_import_gate.py. The scanner uses the root it is given, treatsfrom inferrsi import knowledgeas a core import, allows same-core imports, and reports each forbidden pair once.Knowledge authorship declaration
Not applicable. This PR does not change knowledge content, Bundle encoding, or revision semantics.
Changed artifacts and evidence
docs/module-layout.mddata/, corpus details deferred,pack.jsonwith Harness, dashboard template with Apps, #371 still Proposed.claim.pyis listed as verdict vocabulary, not a fourth core. Oldrunexits point atcli.pylines 412-417 on9f61a83fcross_core_violations(root)from inferrsi import knowledgeis one violation. Apps importing knowledge is allowed.from .bar import VALUEinside knowledge is allowed. Knowledge importing Harness is one violationisolation_complete()knowledge/,benchmark/, andapps/are absent and legacyharness/remainscli.py,arguments.py, orrunner.pygit diff --checkcleanChecks and dependencies
PYTHONPATH=src python3 -m unittest tests.test_target_module_import_gate: 4 passed.python3 -m ruff check tests/test_target_module_import_gate.py: passed (ruff 0.17.4).cli.py,arguments.py, andrunner.py. This branch does not. feat: share RSI optimization management across project and benchmark tasks #338, task: Apps 应用层详细设计与迁移计划(CLI / Python / Plugin) #371, and task: 支持 MindIE Agent 对接(知识、执行与评测交接) #374 are untouched.Remaining work and Issue closure
Handoff (only when an actual handoff or decision changed)
f827acec05672a61d99798fdaa57ef286175c26a.