feat(architecture): observed dependency ratchet(Phase 2) - #447
Conversation
`introduce-executable-architecture-contracts` Phase 2(tasks 2.1–2.5)。把 desired contract 從「宣告」變成「可被觀測反駁」:靜態掃描 committed source 產生 observed service dependency graph 與各 service 內部 module cycle,對照 contract `may_call`、 `architecture/deltas/*.json` 與 approved baseline 做 ratchet。 新 edge 必須同時被 contract 與某個 delta 宣告;新 cycle signature 或超出 cycle budget 一律 fail closed。既有狀態記入 `architecture/observed-baseline.json`,每筆 debt 附 owner / reason / target phase。 宣告偏離:tasks 2.1 原文寫「from GitNexus」。GitNexus CLI 在本 repo 有 transport 失敗與 stale index 紀錄(NOW.md S4-B closeout),不能當 fail-closed gate 的輸入, 改用純標準函式庫靜態掃描,GitNexus 降為 advisory 並記於 config 的 `advisory_only_sources`。static scan 看不到 runtime 才解析的位址,所以 observed graph 是 lower bound,只擋新增、不宣稱窮舉。 `ARCH-GRAPH-001` 由 `planned` 改 `active`;同一 PR 修改本 change 自己的「Honest phased enforcement」spec requirement,定義何時允許 activation(canonical gate、 approved baseline、記載 scope 限制),避免與自家 normative 文字牴觸。 三層交叉對抗 review 共修掉 9 個會讓 gate 靜默放行的缺陷,全部有回歸測試: 非-object JSON 讓 status 變 passed;scan root 無 __init__.py 時相對 import 全丟 (kit-manager-api module graph 為空);suppression 比對含註解的原始行,一句 `// cors` 就刪掉真 edge;baseline 把 forbidden edge 標成 `declared` 即可繞過 contract 檢查;scan root 打錯字靜默失效;`</h1>` 被當 regex、`/api/kit/*` 被當 block comment 而吃掉整段檔案;delta 用全域 added-removed 讓歷史移除永久否決後續 宣告;schema 檔本身損毀會關閉所有 required/enum 檢查;清空 `inbound_edge_ports` 讓該 service 的呼叫從「被擋」變成「看不見」。 Verification: - python -m pytest tests -q → 283 passed(Phase 1 後為 237) - python scripts/dev/export_observed_architecture.py --strict → PASSED 205 files / 7 edges / 3 cycles / 0 errors / 0 warnings - python scripts/dev/validate_architecture_contract.py --strict → PASSED - openspec validate --all --strict → 71 passed, 0 failed - scripts/verify-all.ps1 -PlanOnly → dispatch root-contracts + agent-governance + secret scan - scan-secret-patterns.ps1 → passed - Linux 產生的 baseline 在 Windows checkout 通過,cross-platform byte parity 成立 - GitNexus detect-changes --repo <path> → Risk low / 0 affected processes(index 尚未涵蓋新增檔案,advisory 非 gate pass) Boundary: 未碰 governance-service/app.py、governanceProxy 契約、conversion_authority.py 或任何 product runtime;未做 Phase 3–5。 Full completion claimed: no(僅 Phase 2)
📝 WalkthroughWalkthroughIntroduces a deterministic static observed-architecture scanner, approved baseline and schemas, dependency-cycle ratchet enforcement, CLI reporting, verification wiring, documentation, and comprehensive fail-closed tests. ChangesObserved architecture enforcement
Estimated code review effort: 5 (Critical) | ~90+ minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
scripts/lib/observed_architecture.py (4)
727-729: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused unpacked variable.
parent_positionis never read; Ruff RUF059 flags it.🧹 Rename to a throwaway
- if work: - parent, parent_position = work[-1] - low[parent] = min(low[parent], low[node]) + if work: + parent = work[-1][0] + low[parent] = min(low[parent], low[node])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/observed_architecture.py` around lines 727 - 729, Update the unpacking in the work-processing block to use a throwaway variable for the unused parent position, while preserving the existing parent and low-link update logic.Source: Linters/SAST tools
532-538: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_python_module_namehardcodes the.pysuffix length.If
source_suffixes.pythonever includes another suffix (e.g..pyi),parts[-1][: -len(".py")]leaves a trailing fragment (foo.pyi→foo.), which silently fails to resolve and drops module edges. Strip the actual suffix instead.♻️ Suffix-agnostic stem
- if parts[-1] == "__init__.py": + if path.stem == "__init__": parts.pop() else: - parts[-1] = parts[-1][: -len(".py")] + parts[-1] = path.stem🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/observed_architecture.py` around lines 532 - 538, Update _python_module_name to remove the actual configured Python source suffix from the final path component instead of hardcoding the length of ".py". Preserve the existing __init__.py handling, and ensure suffixes such as ".pyi" produce the correct module stem without a trailing fragment.
1182-1195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
inbound_edge_portsbypasses the hand-written check.The guard only fires when the key exists and is an empty sequence. An entry that omits
inbound_edge_portsentirely has the same effect (the service leaves the port-ownership map, so calls to it become invisible) but is caught only by the schema — which this module deliberately does not want to be a single point of failure (see the comment at Lines 1065-1067).🛡️ Treat missing as empty
- ports = entry.get("inbound_edge_ports") - if _is_sequence(ports) and not ports and not entry.get("browser_client"): + ports = entry.get("inbound_edge_ports") + if not (_is_sequence(ports) and ports) and not entry.get("browser_client"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/observed_architecture.py` around lines 1182 - 1195, Update the inbound_edge_ports validation around _is_sequence so a missing key is treated the same as an explicitly empty sequence. Continue exempting entries marked browser_client, and preserve the existing no_inbound_ports issue for all other services.
839-876: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompose parsing is silently indentation- and style-sensitive.
_COMPOSE_SERVICE_HEADER/_COMPOSE_DEPENDS_ON/_COMPOSE_DEPENDS_ITEMrequire exactly 2/4/6-space indentation and block sequences. A reformatted compose file, aservices:-nested key at different depth, or an inlinedepends_on: [a, b]yields zero observed compose edges with no diagnostic — the same silent-gap failure mode_validate_config_rootswas added to prevent. Consider recording a diagnostic when a configured compose file parses into no mapped services, so a formatting change fails loudly instead of quietly shrinking the observed graph.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/observed_architecture.py` around lines 839 - 876, Add a diagnostic in the compose-file processing flow when a configured file yields no mapped services or compose dependency edges, using the existing diagnostics mechanism and identifying the file in the message. Track whether the parsing loop around _COMPOSE_SERVICE_HEADER and dependency matching observed any mapped service, then report the silent parse gap after processing the file while preserving existing edge collection behavior.scripts/dev/export_observed_architecture.py (1)
66-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--report-onlysilently ignores--formatand--strict.In this branch the output is always report JSON and the exit code is always 0, so
--format humanand--strictare accepted but have no effect. Either reject the combinations or document the precedence in the flag help text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/dev/export_observed_architecture.py` around lines 66 - 75, Update the --report-only handling around build_observed_report so --format and --strict are not silently ignored: either reject incompatible combinations with a clear error and nonzero exit, or explicitly document their precedence in the corresponding argument help text and implement that behavior consistently. Ensure the report-only output format and exit status match the chosen contract.tests/test_observed_architecture.py (1)
262-281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional baseline cycle indices make this test brittle.
load_baseline()["cycles"][0]/[2]couple the test to the ordering and exact length of the baseline'scyclesarray; a reordering or an added entry turns this into a confusing failure (or silently weakens the swap scenario). Select the entries byscopeinstead, and reuse the singlebaselinealready loaded on Line 265.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_observed_architecture.py` around lines 262 - 281, Update test_cycle_swap_that_keeps_the_count_is_still_rejected to select baseline cycle entries by their scope values instead of positional indices, and reuse the already loaded baseline variable when obtaining their members. Preserve the existing cycle-swap scenario and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@architecture/observed-baseline.json`:
- Line 6: Update the descriptive metadata near the baseline-generation statement
to remove the claim that deleting an entry is always allowed, and accurately
state that an observed edge is permitted only when retained in the baseline or
declared by both the architecture contract and a delta. Preserve the existing
grandfathering and fail-closed behavior.
In `@openspec/changes/introduce-executable-architecture-contracts/tasks.md`:
- Line 25: Update the closeout entry in the task list to remove the future-dated
July 31, 2026 completion claim: use the actual completion date if the follow-up
occurred, otherwise rewrite it as planned or pending work while preserving the
existing advisory result and unchecked task status.
- Around line 25-143: 將此 tasks.md 中新增的 Phase 2
標題、交付說明、驗證結果及所有敘述性內容翻譯為繁體中文;保留程式碼、檔名、命令、識別符、規範要求及解析器必要的標題不變,並維持 Markdown
結構與勾選狀態。
- Around line 139-143: Update the Verification record to use the mandated
isolated interpreter command ./.venv/Scripts/python.exe -m pytest tests -p
no:cacheprovider, replacing the system-interpreter pytest command while
preserving the reported test results and other verification commands.
---
Nitpick comments:
In `@scripts/dev/export_observed_architecture.py`:
- Around line 66-75: Update the --report-only handling around
build_observed_report so --format and --strict are not silently ignored: either
reject incompatible combinations with a clear error and nonzero exit, or
explicitly document their precedence in the corresponding argument help text and
implement that behavior consistently. Ensure the report-only output format and
exit status match the chosen contract.
In `@scripts/lib/observed_architecture.py`:
- Around line 727-729: Update the unpacking in the work-processing block to use
a throwaway variable for the unused parent position, while preserving the
existing parent and low-link update logic.
- Around line 532-538: Update _python_module_name to remove the actual
configured Python source suffix from the final path component instead of
hardcoding the length of ".py". Preserve the existing __init__.py handling, and
ensure suffixes such as ".pyi" produce the correct module stem without a
trailing fragment.
- Around line 1182-1195: Update the inbound_edge_ports validation around
_is_sequence so a missing key is treated the same as an explicitly empty
sequence. Continue exempting entries marked browser_client, and preserve the
existing no_inbound_ports issue for all other services.
- Around line 839-876: Add a diagnostic in the compose-file processing flow when
a configured file yields no mapped services or compose dependency edges, using
the existing diagnostics mechanism and identifying the file in the message.
Track whether the parsing loop around _COMPOSE_SERVICE_HEADER and dependency
matching observed any mapped service, then report the silent parse gap after
processing the file while preserving existing edge collection behavior.
In `@tests/test_observed_architecture.py`:
- Around line 262-281: Update
test_cycle_swap_that_keeps_the_count_is_still_rejected to select baseline cycle
entries by their scope values instead of positional indices, and reuse the
already loaded baseline variable when obtaining their members. Preserve the
existing cycle-swap scenario and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3120772b-6f1e-4577-910e-b53c73c5f30c
📒 Files selected for processing (15)
.gitignorearchitecture/README.mdarchitecture/architecture-contract.jsonarchitecture/deltas/introduce-executable-architecture-contracts.jsonarchitecture/observed-baseline.jsonarchitecture/observed-baseline.schema.jsonarchitecture/observed-graph.config.jsonarchitecture/observed-graph.config.schema.jsonopenspec/changes/introduce-executable-architecture-contracts/design.mdopenspec/changes/introduce-executable-architecture-contracts/specs/executable-architecture-contracts/spec.mdopenspec/changes/introduce-executable-architecture-contracts/tasks.mdscripts/dev/export_observed_architecture.pyscripts/lib/observed_architecture.pyscripts/verification-manifest.jsontests/test_observed_architecture.py
| "schema_version": "ai-bim-observed-baseline/v1", | ||
| "approved_on": "2026-07-30", | ||
| "approved_by": "introduce-executable-architecture-contracts Phase 2", | ||
| "method": "Generated by scripts/dev/export_observed_architecture.py from the committed source tree at the time of approval. Entries recorded here are grandfathered: the ratchet permits them and fails closed on anything new. Removing an entry is always allowed and is the intended direction of travel.", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the baseline-removal claim.
An observed edge is permitted only when it remains in the baseline or is declared by both the architecture contract and a delta. Removing a baseline entry while the source edge remains undeclared will fail the ratchet, so removal is not “always allowed.”
Suggested wording
- "method": "Generated by scripts/dev/export_observed_architecture.py from the committed source tree at the time of approval. Entries recorded here are grandfathered: the ratchet permits them and fails closed on anything new. Removing an entry is always allowed and is the intended direction of travel.",
+ "method": "Generated by scripts/dev/export_observed_architecture.py from the committed source tree at the time of approval. Entries recorded here are grandfathered: the ratchet permits them and fails closed on anything new. Removing an entry is allowed when the edge is removed or declared by both the contract and a delta.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "method": "Generated by scripts/dev/export_observed_architecture.py from the committed source tree at the time of approval. Entries recorded here are grandfathered: the ratchet permits them and fails closed on anything new. Removing an entry is always allowed and is the intended direction of travel.", | |
| "method": "Generated by scripts/dev/export_observed_architecture.py from the committed source tree at the time of approval. Entries recorded here are grandfathered: the ratchet permits them and fails closed on anything new. Removing an entry is allowed when the edge is removed or declared by both the contract and a delta.", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@architecture/observed-baseline.json` at line 6, Update the descriptive
metadata near the baseline-generation statement to remove the claim that
deleting an entry is always allowed, and accurately state that an observed edge
is permitted only when retained in the baseline or declared by both the
architecture contract and a delta. Preserve the existing grandfathering and
fail-closed behavior.
| - 1.8 passed change-specific strict validation; `openspec validate --all --strict` also passed 71 items with 0 failures. | ||
| - 1.9 remains open: `verify-all -PlanOnly` selected root contracts, agent governance, and secret/security gates; root contracts passed 181 tests and both secret/security checks passed, but the canonical run failed on pre-existing agent-skill integrity drift outside this change. | ||
| - 1.10 remains open: the command ran with the current checkout selected explicitly, but the index was stale and untracked payload files were not mapped; `No changes detected` is advisory, not an accepted gate pass. | ||
| - 1.10 invocation follow-up (2026-07-31): the earlier failure mode is now understood. `detect-changes` aborts with `Multiple repositories indexed` unless the checkout is disambiguated, because several worktrees of this repo are indexed under the same label. With `--repo "C:\Repos\active\iot\AI-BIM-governance"` the command completes and reports `Risk level: low`, `Affected processes: 0`. The task stays unchecked: the index still predates the new files, so only Markdown symbols were mapped and the Python modules added by Phase 2 are absent. The result is advisory, not a gate pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the future-dated closeout evidence.
Line 25 claims a completed follow-up on July 31, 2026, but the current date is July 30, 2026. Use the actual completion date, or mark this as planned work instead of completed evidence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/introduce-executable-architecture-contracts/tasks.md` at
line 25, Update the closeout entry in the task list to remove the future-dated
July 31, 2026 completion claim: use the actual completion date if the follow-up
occurred, otherwise rewrite it as planned or pending work while preserving the
existing advisory result and unchecked task status.
| - 1.10 invocation follow-up (2026-07-31): the earlier failure mode is now understood. `detect-changes` aborts with `Multiple repositories indexed` unless the checkout is disambiguated, because several worktrees of this repo are indexed under the same label. With `--repo "C:\Repos\active\iot\AI-BIM-governance"` the command completes and reports `Risk level: low`, `Affected processes: 0`. The task stays unchecked: the index still predates the new files, so only Markdown symbols were mapped and the Python modules added by Phase 2 are absent. The result is advisory, not a gate pass. | ||
| - 1.11 passed independent review after schema-instance enforcement was added and its missing-required/additional-property counterexamples were proven fail-closed. | ||
|
|
||
| ## Phase 2 — Observed architecture ratchet | ||
|
|
||
| - [ ] 2.1 Export service/module dependency observations from GitNexus into a deterministic report. | ||
| - [ ] 2.2 Compare desired, intended, and observed dependency edges. | ||
| - [ ] 2.3 Establish an approved baseline for existing cycles and forbidden edges. | ||
| - [ ] 2.4 Fail on any new dependency edge not declared by contract + delta. | ||
| - [ ] 2.5 Fail on any increase in cycle count or baseline violations. | ||
| - [x] 2.1 Export service/module dependency observations ~~from GitNexus~~ into a deterministic report. **Deviation: GitNexus replaced by a standard-library static scan — see delivery note below.** | ||
| - [x] 2.2 Compare desired, intended, and observed dependency edges. | ||
| - [x] 2.3 Establish an approved baseline for existing cycles and forbidden edges. | ||
| - [x] 2.4 Fail on any new dependency edge not declared by contract + delta. | ||
| - [x] 2.5 Fail on any increase in cycle count or baseline violations. | ||
|
|
||
| ### Phase 2 delivery notes — 2026-07-30 | ||
|
|
||
| **Declared deviation (2.1).** The task text named GitNexus as the observation | ||
| source. GitNexus is not used as a gate input: its CLI has been repeatedly | ||
| observed to fail with transport errors and to serve a stale index (recorded in | ||
| `docs/plans/NOW.md`, S4-B closeout, "GitNexus: detect_changes 三次 Transport | ||
| closed, index stale"), which cannot back a fail-closed CI gate. The observation | ||
| is produced instead by a standard-library static scan | ||
| (`scripts/lib/observed_architecture.py`), matching the existing architecture | ||
| validator's no-production-dependency rule. GitNexus is recorded as | ||
| `advisory_only_sources` in `architecture/observed-graph.config.json`. The task | ||
| outcome — a deterministic observed dependency report — is unchanged; the source | ||
| of the observation is not. | ||
|
|
||
| **What the gate does and does not claim.** | ||
|
|
||
| - Service-level edges come from two low-false-positive signals only: schemed URL | ||
| literals (`http/https/ws/wss://host:port`) resolved through the contract's own | ||
| port ownership, and compose `depends_on` / env URLs. Runtime-resolved addresses | ||
| are invisible to a static scan, so `web-viewer-sample → bim-streaming-server` | ||
| is permitted by the contract but absent from the observed graph. The ratchet | ||
| therefore blocks *new statically detectable* edges; it does not claim to have | ||
| enumerated every real call. | ||
| - CORS / allowed-origin / CSP lines are suppressed, because an inbound | ||
| allowlist is not an outbound dependency. Without this, `KIT_MANAGER_CORS_ORIGINS` | ||
| produced a false `kit-manager-api → bim-review-coordinator` edge and a false | ||
| service-level cycle. | ||
| - Cycle detection runs on the service graph plus each service's internal module | ||
| graph. Cross-service module graphs are out of scope. | ||
| - `apps/kit-manager-web` reaches coordinator `:8004` but is not a declared | ||
| contract service. It is recorded as `undeclared-node` debt in | ||
| `architecture/observed-baseline.json`, owned by this change and targeted at | ||
| Phase 3. Declaring the node is a desired-architecture change and needs its own | ||
| delta; it must not be resolved by re-baselining. | ||
|
|
||
| **Baseline approved on 2026-07-30**: 7 service edges (5 contract-declared, | ||
| 2 undeclared-node debt) and 3 module-level cycles (streaming Kit extension | ||
| entry point, governance `diff_engine`, viewer `App`/`Window`/`StreamOnlyWindow`), | ||
| each with an owner, reason, and target phase. | ||
|
|
||
| **`ARCH-GRAPH-001` moved from `planned` to `active`.** The "Honest phased | ||
| enforcement" requirement in this change's spec delta previously said the invariant | ||
| SHALL remain planned; it is modified in the same PR to define when activation is | ||
| permitted (an executable gate in canonical verification, an approved baseline with | ||
| attributed debt, and documented scope limits). Activating it without that spec | ||
| change would have contradicted this change's own normative text. | ||
|
|
||
| **Three-layer adversarial review findings fixed before merge.** Independent | ||
| reviewers found five ways the gate could report `passed` without actually | ||
| enforcing anything. All are fixed and covered by regression tests: | ||
|
|
||
| 1. A file containing valid-but-non-object JSON (`null`, `[]`, `123`) made the | ||
| loader return zero issues, so `echo null > observed-baseline.json` produced a | ||
| green run. Non-object documents now raise explicit errors, and `RatchetResult` | ||
| carries a `compared` flag so a run that never reached the comparison cannot | ||
| report `passed`. | ||
| 2. Relative imports in a package without `__init__.py` resolved to `.sibling` and | ||
| were dropped, leaving `kit-manager-api` with an empty module graph and its | ||
| cycle budget unenforceable. Fixed; that service now has 9 module edges. | ||
| 3. Suppression patterns were matched against the raw line including comments, so | ||
| a trailing `// not a CORS thing` silently deleted a real edge. Suppression now | ||
| runs on comment-stripped code (`tokenize` for Python). | ||
| 4. A baseline entry short-circuits the contract check, and `status` was never | ||
| verified — mislabelling a forbidden edge as `declared` bypassed everything. | ||
| Declared entries are now checked against the contract. | ||
| 5. A mistyped or renamed scan root silently scanned nothing. Missing roots now | ||
| fail closed. | ||
|
|
||
| Also fixed: the TypeScript scanner treated `</h1>` as a regex literal and | ||
| `<code>/api/kit/*</code>` as a block-comment opener, either of which silently | ||
| erased every dependency in the rest of a file; delta declarations used a global | ||
| `added - removed` difference, so one historical removal permanently vetoed any | ||
| later legitimate re-declaration; and unreadable or unparseable files were skipped | ||
| without a trace. | ||
|
|
||
| A third independent verification round re-tested every fix and found four more: | ||
|
|
||
| 6. The two `*.schema.json` files were themselves unvalidated, so replacing one | ||
| with `null` disabled all `required` / `enum` / `additionalProperties` checks | ||
| while still reporting `passed`. Corrupt schema files now fail, and the | ||
| baseline's `status` enum, debt attribution, and duplicate-pair checks are | ||
| additionally enforced in Python so a single corrupt file cannot disable them. | ||
| 7. Emptying a service's `inbound_edge_ports` removed it from the port ownership | ||
| map, making every future call to it invisible instead of flagged — with no | ||
| error or warning. Services must now either declare inbound ports or be marked | ||
| `browser_client`. | ||
| 8. A `.py` file containing a NUL byte crashed the scan with an uncaught | ||
| `ValueError` (`read_text` accepts NUL, `ast.parse` rejects it) instead of | ||
| producing a structured finding. | ||
| 9. In a region the scanner declined to strip, the `//` inside `http://` was read | ||
| as a line-comment opener and erased the rest of the line, including any real | ||
| dependency after it. `://` is now never a comment opener. | ||
|
|
||
| The same round confirmed by differential testing against two independently | ||
| written implementations (2000 random graphs) that the iterative Tarjan SCC | ||
| implementation is correct, and confirmed byte-identical repeat runs. | ||
|
|
||
| **Known limits, not claimed as solved**: the TypeScript scanner is a heuristic | ||
| state machine, not a parser. Its `clean` flag catches structural failures | ||
| (unterminated block comment or template) but cannot detect every misread. The | ||
| canonical-repository test is a snapshot of today's tree and is not a | ||
| mutation-detection net; that role belongs to the constructed negative tests. | ||
|
|
||
| **Verification**: `python -m pytest tests -q` — 283 passed on the real Windows | ||
| checkout (was 237 before this change's tests, 181 before Phase 1); the canonical | ||
| ratchet passes against a baseline generated on Linux, confirming cross-platform | ||
| byte parity. `python scripts/dev/export_observed_architecture.py --strict` | ||
| PASSED (205 files, 7 edges, 3 cycles, 0 errors, 0 warnings). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Translate the changed OpenSpec content to Traditional Chinese.
This file is under openspec/changes/**, whose proposal, design, tasks, and spec content must use Traditional Chinese. The added headings and delivery notes remain in English; only parser-required headers may remain unchanged.
As per coding guidelines, openspec/{changes,specs}/**/*.md content must use Traditional Chinese while preserving parser-required headers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/introduce-executable-architecture-contracts/tasks.md` around
lines 25 - 143, 將此 tasks.md 中新增的 Phase 2
標題、交付說明、驗證結果及所有敘述性內容翻譯為繁體中文;保留程式碼、檔名、命令、識別符、規範要求及解析器必要的標題不變,並維持 Markdown
結構與勾選狀態。
Source: Coding guidelines
| **Verification**: `python -m pytest tests -q` — 283 passed on the real Windows | ||
| checkout (was 237 before this change's tests, 181 before Phase 1); the canonical | ||
| ratchet passes against a baseline generated on Linux, confirming cross-platform | ||
| byte parity. `python scripts/dev/export_observed_architecture.py --strict` | ||
| PASSED (205 files, 7 edges, 3 cycles, 0 errors, 0 warnings). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Record pytest verification with the mandated interpreter and cache isolation.
python -m pytest tests -q uses the system interpreter and omits -p no:cacheprovider, so the reported result does not satisfy the repository verification rule.
As per coding guidelines, use ./.venv/Scripts/python.exe -m pytest tests -p no:cacheprovider for test verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/introduce-executable-architecture-contracts/tasks.md` around
lines 139 - 143, Update the Verification record to use the mandated isolated
interpreter command ./.venv/Scripts/python.exe -m pytest tests -p
no:cacheprovider, replacing the system-interpreter pytest command while
preserving the reported test results and other verification commands.
Source: Coding guidelines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13f38e8ef6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "scripts/lib/observed_architecture.py", | ||
| "scripts/dev/export_observed_architecture.py" |
There was a problem hiding this comment.
Include Compose inputs in observed-ratchet dispatch
When a PR changes only compose.runtime-manager.yml or compose.host-kit.yml, the changed-path classifier does not schedule root-contracts: this path class omits both files even though build_observed_report() scans them, and the affected-path fixture: compose config test explicitly expects no root-contract gate. Consequently, a new Compose depends_on or URL-derived service edge can merge without running the active observed-architecture ratchet; add both Compose inputs to this class and the matching target metadata.
Useful? React with 👍 / 👎.
| if language == "python": | ||
| # Only string constants carry call targets, so comments and | ||
| # docstring prose can never manufacture an edge. | ||
| candidates = _python_string_literals(text) |
There was a problem hiding this comment.
Exclude Python docstrings from edge candidates
When a module, class, or function docstring mentions another local service URL such as http://127.0.0.1:49102, _python_string_literals() returns that docstring's ast.Constant, so this branch records a dependency that executable code never makes and can fail the ratchet for a documentation-only source edit. Filter recognized docstring nodes while retaining assigned and otherwise executable string literals.
Useful? React with 👍 / 👎.
| if _COMPOSE_DEPENDS_ON.match(stripped): | ||
| in_depends = True | ||
| continue |
There was a problem hiding this comment.
Parse inline Compose dependency declarations
When a valid Compose file uses an inline dependency form such as depends_on: [governance-service] or an inline mapping, this matcher recognizes only a standalone depends_on: line and _service_edges_from_compose() returns no edge. After Compose files are routed through the ratchet, a dependency introduced with either inline form will therefore still bypass ARCH-GRAPH-001; parse the YAML structure or explicitly support the inline list and mapping forms.
Useful? React with 👍 / 👎.
| for path in base.rglob("*"): | ||
| if not path.is_file() or path.suffix not in suffixes: | ||
| continue |
There was a problem hiding this comment.
Restrict architecture scans to tracked source files
When a checkout contains an untracked .py, .ts, or .tsx scratch/generated file beneath a configured service root, rglob() includes it even though the report claims to scan committed source. Such a file can manufacture an edge or cycle and make the same commit pass in CI but fail locally, while also breaking the promised byte-identical report across working copies; enumerate tracked files or otherwise exclude untracked paths before scanning.
Useful? React with 👍 / 👎.
| if language == "python": | ||
| edges = _python_module_edges(root_path, repo_root, files, diagnostics) | ||
| module_nodes.update(_python_module_name(root_path, path) for path in files) |
There was a problem hiding this comment.
Combine all service roots before resolving module imports
When modules in the two configured bim-streaming-server extension roots import each other, _python_module_edges() receives only the files from the current root, so neither absolute import can resolve against the other root and a cross-extension cycle is reported as zero edges. Because the configuration groups both roots into one service-level module graph, collect a service-wide module index before resolving imports so cycles spanning the messaging and setup extensions cannot bypass the ratchet.
Useful? React with 👍 / 👎.
Change Classification
Behavior contract changed: yesrefers to the repository governance contract, notto product behaviour: this PR adds two new machine-readable JSON Schemas
(
architecture/observed-graph.config.schema.json,architecture/observed-baseline.schema.json)and activates
ARCH-GRAPH-001. No product API, route, event, or runtime behaviourchanges — every service
src/tree and both compose files are byte-identical tomain.AI Coding Governance
introduce-executable-architecture-contractsPhase 2, adopted by the user on 2026-07-30 as the 6th active change (docs/plans/NOW.md例外揭露)openspec/changes/introduce-executable-architecture-contracts/tasks.mdPhase 2 (2.1–2.5)detect-changes --scope compare --base-ref main --repo "C:\Repos\active\iot\AI-BIM-governance"→ 8 files, 21 symbols, Affected processes 0, Risk level low. Advisory only, not a gate pass: the index predates this branch, so only Markdown symbols were mapped and the new Python modules are absent. The--repoargument is required because several worktrees of this repo are indexed under the same label, which is what caused the previously recorded transport/ambiguity failures.architecture/,scripts/lib,scripts/dev,tests/)architecture/README.md§5–§7 documents the agent workflow and the declared deviationWhat this does
Phase 2 turns the desired architecture contract from a declaration into something the
observed code can contradict.
scripts/lib/observed_architecture.pystatically scans committed source into adeterministic observed dependency report, then applies a ratchet:
architecture/observed-baseline.jsonmay_callobserved.edge.not_allowed(error)observed.edge.undeclared(error)observed.cycle.new/observed.cycle.count_increase(error)*.baseline_stale(warning — tighten the baseline)passedBaseline identity compares
(from, to)pairs and cycle member sets only, neverfile:line, so line drift cannot break the ratchet. Cycles compare both signatureand count, so swapping one cycle for another at a constant total still fails.
Approved baseline
7 service edges (5 contract-declared, 2
undeclared-nodedebt) and 3 module cycles,each with an owner, reason, and target phase.
apps/kit-manager-webreaches coordinator:8004but is not a declared contractservice. Its two edges are recorded as debt, not laundered into the contract. The
edge direction respects
ARCH-HTTP-001(it targets the coordinator entrypoint); thegap is the missing node declaration, which needs its own delta in Phase 3 and must not
be resolved by re-baselining. Independent review confirmed
kit-manager-web → kit-manager-apiis a composedepends_onstartup ordering only —there is no browser-to-internal HTTP call in
apps/kit-manager-web/src.Declared deviation
Task 2.1 originally read "Export … from GitNexus …". GitNexus is not used as a
gate input: its CLI has been observed to fail with transport errors and serve a stale
index (
docs/plans/NOW.md, S4-B closeout), which cannot back a fail-closed CI gate.The observation is produced by a standard-library static scan instead, matching the
existing architecture validator's no-production-dependency rule. GitNexus is recorded
as
advisory_only_sourcesinarchitecture/observed-graph.config.json. The originaltask text is preserved with strikethrough in
tasks.md.This deviation is an AI adjudication and can be overridden.
Scope limits — stated, not hidden
resolved through the contract's own port ownership, and compose
depends_on/ envURLs. Runtime-resolved addresses are invisible to a static scan, so
web-viewer-sample → bim-streaming-serveris permitted by the contract but absentfrom the observed graph. The ratchet blocks new statically detectable edges; it
does not claim to have enumerated every real call.
Cross-service module graphs are out of scope.
outbound dependency. Without this,
KIT_MANAGER_CORS_ORIGINSproduced a falsekit-manager-api → bim-review-coordinatoredge and a false service-level cycle.cleanflagcatches structural failures but cannot detect every misread.
ARCH-GRAPH-001: planned → activeThis change's own spec delta previously said the invariant SHALL remain
planned.The same PR modifies that requirement to define when activation is permitted — an
executable gate in canonical verification, an approved baseline with attributed debt,
and documented scope limits. Flipping the status without that spec change would have
contradicted this change's own normative text.
Three-layer adversarial review
Nine ways the gate could report
passedwithout enforcing anything were found andfixed, each with a regression test:
null,[],123) produced zero issues, soecho null > observed-baseline.jsongave a green run.RatchetResultnow carriesa
comparedflag: a run that never reached the comparison cannot reportpassed.__init__.pyresolved to.siblingand weredropped, leaving
kit-manager-apiwith an empty module graph and its cycle budgetunenforceable. Now 9 module edges.
// not a CORS thingsilently deleted a real edge. Now runs on comment-strippedcode (
tokenizefor Python).statuswas never verified,so mislabelling a forbidden edge as
declaredbypassed everything.</h1>was read as a regex literal and<code>/api/kit/*</code>as ablock-comment opener; either silently erased every dependency in the rest of a file.
added - removeddifference, so one historicalremoval permanently vetoed any later legitimate re-declaration.
*.schema.jsonfiles were themselves unvalidated — replacing one withnulldisabled allrequired/enumchecks while still reportingpassed.inbound_edge_portsmade calls to a service invisible rather than flagged.Differential testing against two independently written SCC implementations over 2000
random graphs confirmed the iterative Tarjan implementation.
Verification
python -m pytest tests -qpython scripts/dev/export_observed_architecture.py --strictpython scripts/dev/validate_architecture_contract.py --strictopenspec validate introduce-executable-architecture-contracts --strictopenspec validate --all --strictscripts/verify-all.ps1 -PlanOnlyscripts/tests/scan-secret-patterns.ps1Boundary
No product runtime touched:
governance-service/app.py, thegovernanceProxycontract,
conversion_authority.py, all servicesrc/trees and both compose filesare byte-identical to
main. No new OpenSpec change; active WIP stays 6/6. Phases 3–5are not started.
Full completion claimed: no (Phase 2 only)