Skip to content

docs: classify architecture invariants by enforcement mechanism (#9152) #9689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yiliang114 merged 5 commits into from
Aug 22, 2026
Merged

docs: classify architecture invariants by enforcement mechanism (#9152) #9689

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
06be3c5
docs: classify architecture invariants by enforcement mechanism (#9152)
yiliang114 Aug 21, 2026
951b4b9
docs: correct guard references and complete the invariant inventory (…
yiliang114 Aug 21, 2026
a5670f3
docs: fix #9145 attribution and drift-guard line counts (#9152)
yiliang114 Aug 22, 2026
82869c0
docs: classify Web Shell UI conventions and reclassify Node engines (…
yiliang114 Aug 22, 2026
c6c3f59
docs: cover the full Web Shell convention set without a count claim (…
yiliang114 Aug 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
266 changes: 266 additions & 0 deletions docs/design/9152-architecture-invariant-classification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,266 @@
# Architecture invariant classification

Issue #9152 asks for a policy decision: which architectural invariants in this
repository are enforced mechanically, which are left to review, and which are
not worth enforcing — written down where a new invariant will encounter it.

This document is that record. It is the canonical answer to "should this
invariant have a guard?" for every invariant asserted in AGENTS.md or an open
architecture issue.

## Classification scheme

| Category | Meaning | Failure mode if unguarded |
| --- | --- | --- |
| **Mechanically enforced** | A guard fails the build, lint, or test suite. No human judgment needed. | Drift reintroduced silently by a merged PR. |
| **Review-only** | Requires judgment no lint rule can approximate. Enforced by the two-tier gate and reviewer checklist. | Missed in review; no mechanical backstop. |
| **Not worth enforcing** | The cost of a guard exceeds the cost of occasional violations. | Acceptable — the invariant is a preference, not a load-bearing constraint. |

A guard is **gap** if the invariant is classified as mechanically enforced but
no guard exists yet, or the existing guard is partial.

---

## AGENTS.md conventions

### 1. ESM throughout (`"type": "module"`)

**Mechanically enforced.** `package.json` declares `"type": "module"` in every
package. Node and the TypeScript compiler reject `require()` at runtime and
`import =` syntax at compile time under ESM mode. No separate guard needed.

### 2. TypeScript strict mode

**Mechanically enforced.** `tsconfig.json` sets `"strict": true`,
`"noImplicitAny": true`, `"strictNullChecks": true`, `"noUnusedLocals": true`,
`"verbatimModuleSyntax": true`. `npm run typecheck` fails on any violation.

### 3. Prettier formatting

**Mechanically enforced.** `lint-staged` runs Prettier via the `pre-commit`
hook (`scripts/pre-commit.js`). CI runs `npm run format` as part of preflight.

### 4. No `any` types

**Mechanically enforced.** `@typescript-eslint/no-explicit-any` is configured
as an error in `eslint.config.js`.

### 5. Consistent type imports

**Mechanically enforced.** `@typescript-eslint/consistent-type-imports` in
`eslint.config.js`.

### 6. No relative imports between packages

**Mechanically enforced.** Two guards:
- `eslint-rules/no-relative-cross-package-imports.js` (custom ESLint rule)
- `import/no-relative-packages` (built-in)

### 7. Tests collocated with source (`file.test.ts` next to `file.ts`)

**Review-only.** The collocation convention cannot be mechanically enforced
without false positives — a test file may legitimately live in a shared
`__tests__/` directory for cross-cutting integration tests, and AGENTS.md
itself allowlists this. A lint rule would either miss violations or flag
legitimate exceptions. The reviewer checklist covers this.

### 8. File naming: `PascalCase.tsx` for React components, `kebab-case.ts` for `.ts` in `packages/core` and `packages/cli`

**Mechanically enforced.** `check-file/filename-naming-convention` in
`eslint.config.js` enforces `KEBAB_CASE` for `*.ts`. Legacy camelCase files
are allowlisted in `eslint.config.js`. `PascalCase.tsx` is the existing
convention but is not separately enforced — a `.tsx` file in kebab-case would
pass the rule. **Gap:** add `PASCAL_CASE` for `*.tsx` or accept the convention
as socially enforced. Low priority — React tooling and imports naturally
converge on PascalCase for components.

### 9. Comments: default to none

**Review-only.** The absence of comments is a style preference, not a
structural invariant. A lint rule cannot distinguish a necessary "why" comment
from noise. The reviewer checklist in AGENTS.md covers this.

### 10. Conventional Commits

**Not worth enforcing.** There is no `commitlint` config or `commit-msg` hook.
The convention is socially enforced through PR titles and squash-merge. A
commit-msg hook would block worktree commits and CI automation without
meaningfully improving the commit history (squash-merge rewrites messages
anyway). The cost exceeds the benefit.

### 11. Node.js `>=22`

**Mechanically enforced.** `package.json` declares
`"engines": { "node": ">=22.0.0" }`. `npm` warns on version mismatch. CI runs
on Node 22. No separate guard needed.

### 12. Core modules are maintainer-only (two-tier gate)

**Review-only.** The two-tier gate in AGENTS.md is a policy, not a lint rule.
It requires judgment about scope, confidence, and downstream consumers —
exactly the kind of decision that cannot be approximated mechanically. The
gate is enforced through the triage skill and reviewer judgment.

---

## Architecture issues

### #8084: acp-integration must not import serve/ internals

**Mechanically enforced — gap.** PR #9144 is still **open** and contains:
- A `no-restricted-imports` entry blocking `**/serve/*` and `**/serve/**` from
`packages/cli/src/acp-integration/**`
- A `scripts/tests/acp-serve-boundary-guard.test.js` source-level boundary test

Until #9144 merges, the invariant is **unguarded on `main`**. The existing
`no-restricted-imports` for `utils/ → serve/` (from merged #9147) covers one
direction but not the acp-integration direction. **Action: merge #9144.**

### #9145: approval-mode value domain must agree across SDKs

**Mechanically enforced — partial.** `packages/sdk-typescript/test/unit/approval-mode-drift.test.ts`
asserts that `DAEMON_APPROVAL_MODES` (SDK) mirrors `APPROVAL_MODES` (core)
exactly, including order. This covers the SDK ↔ core drift.

**Gap:** the issue also identifies drift in Python and Java SDKs, and in
desktop's `cyclablePermissionModes`. PR #9003 (in progress) addresses Python
and Java. Desktop's `cyclablePermissionModes` is an intentionally different
domain (allow-all/safe/ask/auto-edit) — the drift test explicitly excludes it
to avoid false positives. This is the correct decision. **No further guard
needed for desktop.** The Python/Java gap closes when #9003 merges.

### #9146: utils/ must be a leaf layer

**Mechanically enforced — partial.** Merged PR #9147 added a
`no-restricted-imports` entry blocking `packages/cli/src/utils/**` from
importing `**/serve/*` and `**/serve/**`. This covers the `serve/` direction
only. The issue identifies 7 upward-importing directories in CLI (`config`,
`ui`, `i18n`, `nonInteractive`, `serve`, `commands`, root) and 10 in core
(`config`, `tools`, `services`, `core`, `agents`, `telemetry`, `memory`,
`hooks`, `models`, root barrel).

**Gap:** the remaining directories are not yet blocked. This is intentional
— the issue's own plan describes steps 5-7 as needing decisions or being
large enough to warrant separate PRs. The guard should be extended directory
by directory as each move lands. **No new guard until the moves are done.**
Prematurely blocking imports into `config/` or `ui/` would break the build
before the code is moved.

### #9151: cross-package constants and contracts must agree

**Mechanically enforced.** Issue is **closed**. Merged PR #9497 added:
- `scripts/tests/cross-package-contracts.test.js` — a table-driven source test
that pins the single owner file and import path for `LIVE_TASK_TOOL_NAMES`,
`LiveTaskToolName`, and `MAX_SUB_SESSION_PROMPT_CHARS`.
- `docs/design/9151-cross-package-contracts.md` — ownership decisions.

This is the reusable drift-guard pattern (see below).

---

## #4063 structural problems

The 14 structural problems in #4063 are a problem register, not a set of
invariants. Most describe "this module is too large" or "this type is
over-coupled" — they are refactoring targets, not ongoing constraints to
guard. Two exceptions:

### #4063 item 5: Barrel export self-references (core modules import from `'../index.js'`)

**Mechanically enforced — gap (on feature branch, not main).** An
`eslint-rules/no-core-root-barrel-import.js` rule exists on the
`lane4-rebase` branch but has **not merged to `main`**. The rule blocks
`packages/core/src/**` from importing the root barrel `../index.js`. **Action:
merge the rule or record why it was deferred.**

### #4063 item 2: Config god-object

**Review-only.** The two-tier gate in AGENTS.md already covers this — core
infrastructure changes require maintainer awareness. A line-count guard was
considered and rejected (the issue itself argues against it; #4063's
commentary and the Qwen Code review of #9152 both reject line-count lint).
Size is a symptom; the invariant is "don't add new responsibilities to
Config," which is a judgment call.

The remaining #4063 items (AppContainer size, useGeminiStream size, naming
like `core/coreToolScheduler.ts`, non-interactive code scattered across 4
locations) are refactoring targets, not invariants. They are tracked in
#4063 itself.

---

## Drift-guard mechanism

Issue #9152 asks whether a reusable drift-guard mechanism should be extracted
from `check-voice-guard-sync.js`, or whether #9145 and #9151 demonstrate that
they do not need one.

**Decision: do not extract a reusable mechanism.** The two drift guards that
now exist — `cross-package-contracts.test.js` and
`approval-mode-drift.test.ts` — are each 40-60 lines of table-driven test
code, purpose-built for their contracts. They share a *pattern* (assert
single owner, assert import path, assert value equality) but not enough
structure to justify a shared abstraction:

1. **The contracts differ in shape.** Cross-package constants use source-grep
(symbol → file). Approval-mode uses runtime value equality (import both
lists, compare). Voice-guard uses TypeScript AST parsing (block/function
extraction). A shared framework would need to support all three extraction
strategies, which is more complexity than the three independent tests.

2. **Each guard is stable.** Once a contract is pinned, the test rarely
changes. The maintenance cost of three independent 50-line tests is lower
than one 200-line framework plus three 30-line config files.

3. **New drift guards are rare.** The issue itself notes that #9145 and #9151
are the only cross-package drift cases found. A framework for two future
users is speculative abstraction — exactly what AGENTS.md's Simplicity
First principle prohibits.

The *pattern* is documented here. When a new cross-package drift case arises,
copy the `cross-package-contracts.test.js` structure: declare a `definitions`
array mapping symbol → owner file, assert single owner via `git grep`, assert
import paths. If a third case needs runtime value comparison, copy
`approval-mode-drift.test.ts`. Extract a shared utility only if a fourth case
demonstrates that the copy-paste cost exceeds the abstraction cost.

`check-voice-guard-sync.js` remains as-is. Its AST-based mirror-set
extraction is specific to the voice-code CLI ↔ desktop mirroring problem and
does not generalize to contract drift.

---

## Summary table

| Invariant | Source | Classification | Guard | Status |
| --- | --- | --- | --- | --- |
| ESM only | AGENTS.md | Mechanical | `package.json` `"type": "module"` | ✅ |
| TS strict mode | AGENTS.md | Mechanical | `tsconfig.json` | ✅ |
| Prettier formatting | AGENTS.md | Mechanical | `lint-staged` / pre-commit | ✅ |
| No `any` types | AGENTS.md | Mechanical | `@typescript-eslint/no-explicit-any` | ✅ |
| Consistent type imports | AGENTS.md | Mechanical | `@typescript-eslint/consistent-type-imports` | ✅ |
| No relative cross-package imports | AGENTS.md | Mechanical | `no-relative-cross-package-imports.js` + `import/no-relative-packages` | ✅ |
| Tests collocated | AGENTS.md | Review-only | — | ✅ (by design) |
| kebab-case `.ts` filenames | AGENTS.md | Mechanical | `check-file/filename-naming-convention` | ✅ |
| PascalCase `.tsx` filenames | AGENTS.md | Mechanical | — | ⚠️ Gap (low priority) |
| No comments by default | AGENTS.md | Review-only | — | ✅ (by design) |
| Conventional Commits | AGENTS.md | Not worth enforcing | — | ✅ (by design) |
| Node ≥22 | AGENTS.md | Mechanical | `package.json` `"engines"` | ✅ |
| Core modules maintainer-only | AGENTS.md | Review-only | Two-tier gate | ✅ (by design) |
| acp-integration off serve/ | #8084 | Mechanical | `no-restricted-imports` + boundary test | ❌ PR #9144 open |
| Approval-mode SDK ↔ core drift | #9145 | Mechanical | `approval-mode-drift.test.ts` | ⚠️ Partial (Python/Java in #9003) |
| utils/ is a leaf layer | #9146 | Mechanical (partial) | `no-restricted-imports` (serve/ only) | ⚠️ Intentionally incremental |
| Cross-package constants agree | #9151 | Mechanical | `cross-package-contracts.test.js` | ✅ Closed |
| No core root barrel self-import | #4063 | Mechanical | `no-core-root-barrel-import.js` | ❌ On feature branch, not merged |
| Config god-object | #4063 | Review-only | Two-tier gate | ✅ (by design) |

## Open actions

1. **Merge PR #9144** — closes the #8084 guard gap (acp-integration → serve/).
2. **Merge `no-core-root-barrel-import.js`** from `lane4-rebase` or record
why it was deferred — closes the #4063 item 5 guard gap.
3. **Track #9003** — when it merges, the #9145 Python/Java drift gap closes.
4. **Extend `utils/` leaf guard** directory by directory as #9146 steps 5-7
land — no premature blocking.
5. **No drift-guard framework** — the pattern is documented here; copy it
when a new case arises.
Loading