Skip to content

docs: record the target module layout and import gate - #376

Merged
Dong1017 merged 2 commits into
mainfrom
cursor/module-layout-import-gate-474c
Nov 9, 2026
Merged

Dong1017 merged 2 commits into
mainfrom
cursor/module-layout-import-gate-474c

Conversation

@Dong1017

@Dong1017 Dong1017 commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Governing Issue and delivery slice

Refs #372
Refs #362

  • Accepted scope / delegation: Record the directory rule and a direct-import gate. Do not move trees, create empty packages, or change Knowledge read, Bundle, or revision behavior.
  • What this PR delivers: docs/module-layout.md, a pointer in docs/architecture.md, and tests/test_target_module_import_gate.py. The scanner uses the root it is given, treats from inferrsi import knowledge as a core import, allows same-core imports, and reports each forbidden pair once.
  • What it does not deliver: No directory move, no empty package, no Knowledge interface change, no Apps implementation from task: Apps 应用层详细设计与迁移计划(CLI / Python / Plugin) #371, and no completion of the rest of the tracker. Issues 372 and 362 remain open.

Knowledge authorship declaration

Not applicable. This PR does not change knowledge content, Bundle encoding, or revision semantics.

{
  "schema_version": 1,
  "head_sha": "f827acec05672a61d99798fdaa57ef286175c26a",
  "human_only": false,
  "material_ai_authors": [
    {
      "tool": "Cursor",
      "model_id": "grok-4.7",
      "effort": "unknown",
      "role": "author"
    }
  ]
}

Changed artifacts and evidence

Requirement Actual change Check / evidence and result Remaining gap
Directory rule without moves docs/module-layout.md Four peer modules, no sibling data/, corpus details deferred, pack.json with Harness, dashboard template with Apps, #371 still Proposed. claim.py is listed as verdict vocabulary, not a fourth core. Old run exits point at cli.py lines 412-417 on 9f61a83f Knowledge corpus layout is not designed here
Real gate, not a copied predicate Temporary trees call cross_core_violations(root) Harness from inferrsi import knowledge is one violation. Apps importing knowledge is allowed. from .bar import VALUE inside knowledge is allowed. Knowledge importing Harness is one violation Does not police today's Harness/Model/Infra edges
Clean scan is not isolation isolation_complete() Asserted false while knowledge/, benchmark/, and apps/ are absent and legacy harness/ remains None in this diff
Avoid in-flight production edits No cli.py, arguments.py, or runner.py git diff --check clean None

Checks and dependencies

Remaining work and Issue closure

  • Remaining Issue requirements / follow-up: The test-double loop, the first real-port integration, and the separate docs or fixture cleanup are not in this PR.
  • Goal-acceptance owner or accepted closure delegation: Not claimed.
  • Goal ready for acceptance? Not claimed

Handoff (only when an actual handoff or decision changed)

Open in Web Open in Cursor 

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>
@cursor

cursor Bot commented Nov 9, 2026

Copy link
Copy Markdown

@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 inferrsi.knowledge, inferrsi.benchmark, or inferrsi.apps import outside Apps. It does not mean the four modules are isolated, and the current harness/ tree is still mixed. This does not close the rest of the #372 tracker.

@Dong1017 Dong1017 added the area:harness Harness module: profile, A/B loop, quality gates, evidence reports label Nov 9, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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 knowledge and from . import knowledge in a module whose package is inferrsi both load inferrsi.knowledge (same for benchmark and apps). The scanner stores inferrsi, and _matches does not treat that as inferrsi.knowledge. import inferrsi.knowledge and from 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 literal import inferrsi.knowledge and never calls _modules_in_source or cross_core_violations, so this miss stays green.
    Expected correction: treat each ImportFrom alias as a candidate submodule (inferrsi.knowledge for from inferrsi import knowledge) and assert the real scanner flags that form outside apps and allows it from apps.

  2. tests/test_target_module_import_gate.py:51-60 (cross_core_violations) forbids inferrsi.knowledge, inferrsi.benchmark, and inferrsi.apps for every importer other than apps, including the core itself. Relative imports are rewritten to those absolute names (from .bar import baz in knowledge/foo.py resolves to inferrsi.knowledge.bar).
    Trigger: a knowledge/ or benchmark/ module with a normal intra-package import. The gate reports a cross-core violation even though docs/module-layout.md:9-10 says the cores do not import one another, and lines 52-55 say code under knowledge/ or benchmark/ must not import Harness, Apps, or the other core (own package stays allowed). The second loop already skips own; the first loop still reports it. isolation_complete (tests/test_target_module_import_gate.py:79) then stays false via cross_core_violations after the three directories exist and harness/ no longer imports Model or Infra.
    Expected correction: exempt a core's own prefix, and align the Import gate paragraph in docs/module-layout.md:52-55 with that rule. Cover an own-package relative import (allowed) and knowledge → 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.

Copy link
Copy Markdown
Collaborator

本次 review 范围:完整 diff(新 PR),base 9f61a83f030bfa2ffe3b54eac1781593309d6380,head 9409b91ed10245974a858b207b8c6cc2728db597。文档部分的方向没问题(不搬目录、不建空包、contracts.py 保持单文件、明确“扫描干净不等于隔离”),但目前不能通过:

阻塞项

  1. CI lint-and-smoke 和 full-check (3.10/3.11) 失败:ruff E501 Line too long (111 > 100),位置 tests/test_target_module_import_gate.py:57(for other in ("inferrsi.harness", ...) 那一行)。期望修改:把这个元组提成模块级常量或换行。
  2. 核心自身的包内导入会被误报(tests/test_target_module_import_gate.py:51-55):第一个循环对所有非 apps 的 importer 都禁止 inferrsi.knowledge / inferrsi.benchmark / inferrsi.apps,包括 knowledge/ 自己。相对导入又会被 _modules_in_source 改写成绝对名,所以将来 knowledge/foo.py 里一句普通的 from .bar import baz 就会被报成跨核心违规;第二个循环虽然跳过了 own,第一个没有。同时 knowledge → inferrsi.benchmark 会被两个循环各报一次。这也会让 isolation_complete()(第 79 行)在目录建好后依然永远为 false。期望修改:对 importer 自身前缀豁免,并去重;docs/module-layout.md 的 Import gate 段落同步写明“自身包内导入允许”。
  3. from inferrsi import knowledge 这种写法会漏检(tests/test_target_module_import_gate.py:31-37):ImportFrom 分支只记录 node.module,不看 alias,所以这里只记下 inferrsi,from . import knowledge 同理。期望修改:对每个 alias 同时记录 base + "." + alias.name 作为候选子模块。
  4. 检测用例没有覆盖真实逻辑(tests/test_target_module_import_gate.py:94-105):test_detector_flags_a_new_core_import_outside_apps 里重新写了一个简化的 forbidden(),没有调用 cross_core_violations,所以上面 2、3 两个问题都不会被测出来。期望修改:给 cross_core_violations(root) 传一个临时目录树(tempfile.TemporaryDirectory),至少覆盖:harness 导入 knowledge(拒绝)、apps 导入 knowledge(允许)、knowledge 包内相对导入(允许)、knowledge 导入 harness(拒绝)、from inferrsi import knowledge(拒绝)。

上面 2–4 和另一条 bot review 的结论一致,我是按代码独立核对的。

非阻塞建议

  • docs/module-layout.md 的 Tentative current ownership 表里没有 src/inferrsi/claim.py,而这个文件在当前 head 上存在,建议补上它的归属,或说明故意不列。
  • “Old run exit codes” 表我这次没有对照 src/inferrsi/cli.py 核实,建议作者在 PR 里附上对应的代码行链接。

Copy link
Copy Markdown
Contributor

Independent check on 9409b91ed10245974a858b207b8c6cc2728db597 against 9f61a83f030bfa2ffe3b54eac1781593309d6380. I confirmed the CI and import-resolution blockers in Amy's review. The following issue affects its proposed closure test.

Before I can approve

  • When adding the real detector fixture, pass the scan root into _imported_modules in both cross_core_violations(root) and isolation_complete(root). On the current head I reproduced a ValueError before AST parsing by passing a temporary root containing harness/probe.py with import inferrsi.knowledge. The helper computes path.relative_to(ROOT) against the checkout instead of the supplied root. Deriving the relative path from the supplied root, or passing the already-relative package parts to the parser, makes the fixture exercise the same code as the repository scan. Close with a temporary-root test that reaches the real detector, reports the forbidden edge once, and permits the Apps and same-core cases from the prior review. Path resolution · Detector and isolation caller.

The existing four focused tests passed locally, including the current-tree scan. I also checked the existing run 0/1/2 verdict mapping against the layout table. Ready CI.

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>
@cursor

cursor Bot commented Nov 9, 2026

Copy link
Copy Markdown

@zengchuang-hw @amy-why-3459 Please re-review head f827acec05672a61d99798fdaa57ef286175c26a. The scanner now takes the tree root it is given, sees from inferrsi import knowledge, allows same-core imports, and reports each forbidden pair once. A temporary tree covers the forbidden, Apps, and same-core cases. A clean repository scan still does not mean the modules are isolated. No files were moved and no empty package was added.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), including knowledge/foo.py resolving from .bar import VALUE to inferrsi.knowledge.bar. Apps still bypass every target. Knowledge or Benchmark importing Harness is still recorded. One (importer path, target core) line.
  • docs/module-layout.md states that rule, still says isolation stays false in this slice, lists claim.py as not moved, and pins the frozen exit-code text to src/inferrsi/cli.py at base 9f61a83f030bfa2ffe3b54eac1781593309d6380 lines 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 amy-why-3459 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.md Import 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(两个新核心之间)和一条同一文件两种写法只报一次的断言,把去重行为显式锁住。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:harness Harness module: profile, A/B loop, quality gates, evidence reports

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants