feat(ci): enforce coding standards and architecture boundaries - #59
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds Prettier and ESLint configuration, package verification scripts, a custom architecture validator, rule fixtures and tests, a Node 22 CI standards job, coding-standard documentation, and scoped annotations for existing exceptions. ChangesStandards validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Scripts as package.json scripts
participant Validator as architecture-check.mjs
participant Tests as architecture-check.test.mjs
CI->>Scripts: run standards verification
Scripts->>Validator: execute architecture check
Scripts->>Tests: execute architecture tests
Validator-->>CI: return diagnostics and exit status
Tests-->>CI: return test status
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
yohnark
force-pushed
the
feat/25-coding-standards
branch
from
August 7, 2026 13:15
3d8cb0b to
4d519a3
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
|
This was referenced Aug 7, 2026
yohnark
added a commit
that referenced
this pull request
Aug 7, 2026
startTask now rejects task starts where the local base branch trails origin/<branch> when policy.worktree.staleBaseBranch=enforce, preventing CI from seeing files the local branch point never saw (root cause behind PR #59's post-merge boundary gaps). Also adds governance:pr:local, a CLI wrapper around the existing validatePullRequest that lets PR title/body/files be checked locally before gh pr create, instead of only after CI runs validate-pr.mjs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
scripts/architecture-check.mjs (4)
279-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
processAccessmisses indirectprocessaccess.The function only matches a direct
process.<name>access.const { env } = process,const p = process; p.env, andglobalThis.process.envbypass the boundary rules. The rule is still useful, but the gap should be documented or covered, because the boundary allowlist is presented as executable enforcement.🤖 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/architecture-check.mjs` around lines 279 - 289, Update processAccess to recognize indirect process references such as destructuring, aliases, and globalThis.process accesses, or explicitly document and test these as unsupported cases. Ensure the boundary allowlist’s executable enforcement behavior matches the supported access patterns.
723-742: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTool detection depends on hardcoded declaration names.
checkLocalToolDefinitionsonly inspectslocalTools,worktreeNewTool, andissueViewTool. A new tool constant with a different name passes the check silently, so theOUTPUT_SCHEMAand annotation contract is not enforced for it. Detect tool objects structurally instead, for example by the presence ofnameplusinputSchema, or by aTooltype annotation.Run the following script to see how many tool definitions the current list misses:
#!/bin/bash # List top-level const declarations in src/local-tools.ts and their type annotations. fd -t f 'local-tools.ts' src | while IFS= read -r file; do rg -n -C 2 '^(export )?const \w+' "$file" done🤖 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/architecture-check.mjs` around lines 723 - 742, Update checkLocalToolDefinitions to identify tool declarations structurally rather than by the hardcoded localTools, worktreeNewTool, and issueViewTool names. Inspect top-level object declarations that expose the tool shape, such as name plus inputSchema, or use a Tool type annotation, and pass each matching object to checkToolObject while preserving the existing source-file restriction.
122-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStart the file walk at
src/.
collectFileswalks the whole repository and then drops every path that does not start withsrc/. Passingpath.join(root, "src")as the initialcurrentkeeps the same result and avoids scanning unrelated directories.🤖 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/architecture-check.mjs` around lines 122 - 137, Update collectFiles so its initial traversal starts at path.join(root, "src") instead of the repository root, while preserving the existing recursive collection and filtering behavior.
835-835: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
continuestatement.Line 835 is the last statement of the loop body, so the
continuehas no effect. The special-edge case is already handled byisDependencyAllowedon line 824.♻️ Proposed change
- if (allowedSpecialEdges.has(special)) continue;Then remove the now-unused
specialbinding on line 823.🤖 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/architecture-check.mjs` at line 835, Remove the dead continue statement guarded by allowedSpecialEdges.has(special) in the loop, then remove the now-unused special binding from the surrounding dependency-check logic. Leave isDependencyAllowed and the remaining edge-handling behavior unchanged.package.json (2)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse double quotes for glob arguments in scripts.
Single quotes are not removed by
cmd.exe. On Windows,eslint 'src/**/*.ts'receives the quotes as part of the pattern and matches nothing. Theprettierentries pass literal file paths, so they only need quoting removed or converted. Use double quotes for the glob patterns to keep the scripts portable.♻️ Proposed change
- "format": "prettier --write 'scripts/architecture-check.mjs' 'scripts/architecture-check.test.mjs' 'eslint.config.mjs' 'prettier.config.mjs' 'package.json'", - "format:check": "prettier --check 'scripts/architecture-check.mjs' 'scripts/architecture-check.test.mjs' 'eslint.config.mjs' 'prettier.config.mjs' 'package.json'", - "lint": "eslint 'src/**/*.ts' 'src/**/*.mjs' 'scripts/**/*.mjs' 'eslint.config.mjs'", + "format": "prettier --write scripts/architecture-check.mjs scripts/architecture-check.test.mjs eslint.config.mjs prettier.config.mjs package.json", + "format:check": "prettier --check scripts/architecture-check.mjs scripts/architecture-check.test.mjs eslint.config.mjs prettier.config.mjs package.json", + "lint": "eslint \"src/**/*.ts\" \"src/**/*.mjs\" \"scripts/**/*.mjs\" eslint.config.mjs",🤖 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 `@package.json` around lines 27 - 32, Update the package scripts’ glob arguments in format:check and lint to use double quotes instead of single quotes so Windows cmd.exe passes valid patterns; remove or convert quoting for the literal Prettier file paths in format and format:check as appropriate.
47-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
typescript-eslintto8.66.0and update the documented version.
typescript-eslint@8.66.0supports the project’s TypeScript range. Replace^8.66.0with8.66.0, and changedocs/coding-standards.mdfrom8.35.0to8.66.0.🤖 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 `@package.json` around lines 47 - 51, Pin the typescript-eslint dependency in package.json by changing its version from ^8.66.0 to 8.66.0, and update the documented typescript-eslint version in docs/coding-standards.md from 8.35.0 to 8.66.0.
🤖 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 `@AGENTS.md`:
- Around line 220-227: Use one normative documentation source for executable
standards: update AGENTS.md lines 220-227 to either retain the command-and-rule
list as the canonical documentation or replace it with a concise pointer to the
canonical configuration/source; update CONTRIBUTING.md lines 78-80 to preserve
the no-duplication rule and link to that same source. Ensure both documents are
consistent and do not maintain duplicate normative lists.
In `@package.json`:
- Around line 47-51: Pin the typescript-eslint dependency consistently: in
package.json lines 47-51, change the caret range to exact version 8.66.0; in
docs/coding-standards.md lines 23-30, replace the documented 8.35.0 with 8.66.0.
In `@scripts/architecture-check.mjs`:
- Around line 219-258: Update layerForFile in scripts/architecture-check.mjs so
the listed server-related files return the documented server layer instead of
upstream, and update docs/coding-standards.md lines 38-54 to describe the
dependency chain produced by the corrected layer assignments. Keep layerRules
and the documented architecture consistent across both sites.
- Around line 263-277: Update hasRuleMarker to clamp the computed searchToLine
so it never falls below searchFromLine before calling
getPositionOfLineAndCharacter. Preserve the existing boundaryLine and
trailing-line limits while ensuring same-line top-level statements, including
line 1, produce a valid search range instead of throwing.
- Around line 929-931: Update the entry-point check around runArchitectureCheck
to import and use node:url’s fileURLToPath(import.meta.url) instead of new
URL(import.meta.url).pathname, while preserving the existing path.resolve
comparison and process arguments.
In `@scripts/fixtures/architecture/accepted-double-assertion.ts`:
- Around line 1-2: Update the marker comment above value to keep the
“architecture-check allow: double-assertion --” prefix unchanged while
translating only the existing reason into Japanese. Preserve the explanation
that this fixture models a validated native interop boundary, and do not modify
the assertion.
In `@scripts/fixtures/architecture/accepted-local-tool.ts`:
- Around line 1-14: Update scripts/fixtures/architecture/accepted-local-tool.ts,
specifically OUTPUT_SCHEMA, to declare all required named fields from the shared
output envelope in src/envelope.ts. Update scripts/architecture-check.test.mjs
at lines 17-20 to add a local-tool fixture or case using outputSchema: {} and
assert RULE_IDS.localToolSchema is reported.
In `@src/adaptive/trace.ts`:
- Around line 258-263: Translate only the rationale after the machine-readable
“architecture-check allow” marker into Japanese why-comments, without changing
the markers or adding what-comments. Update src/adaptive/trace.ts lines 258-263
for request records, 276-283 for execution records, and 292-312 for review and
dispatch normalization; update src/compress/code.ts lines 31-41 for parser and
native-handle exceptions. Ensure each rationale explains only why the TypeScript
architecture exception is permitted.
---
Nitpick comments:
In `@package.json`:
- Around line 27-32: Update the package scripts’ glob arguments in format:check
and lint to use double quotes instead of single quotes so Windows cmd.exe passes
valid patterns; remove or convert quoting for the literal Prettier file paths in
format and format:check as appropriate.
- Around line 47-51: Pin the typescript-eslint dependency in package.json by
changing its version from ^8.66.0 to 8.66.0, and update the documented
typescript-eslint version in docs/coding-standards.md from 8.35.0 to 8.66.0.
In `@scripts/architecture-check.mjs`:
- Around line 279-289: Update processAccess to recognize indirect process
references such as destructuring, aliases, and globalThis.process accesses, or
explicitly document and test these as unsupported cases. Ensure the boundary
allowlist’s executable enforcement behavior matches the supported access
patterns.
- Around line 723-742: Update checkLocalToolDefinitions to identify tool
declarations structurally rather than by the hardcoded localTools,
worktreeNewTool, and issueViewTool names. Inspect top-level object declarations
that expose the tool shape, such as name plus inputSchema, or use a Tool type
annotation, and pass each matching object to checkToolObject while preserving
the existing source-file restriction.
- Around line 122-137: Update collectFiles so its initial traversal starts at
path.join(root, "src") instead of the repository root, while preserving the
existing recursive collection and filtering behavior.
- Line 835: Remove the dead continue statement guarded by
allowedSpecialEdges.has(special) in the loop, then remove the now-unused special
binding from the surrounding dependency-check logic. Leave isDependencyAllowed
and the remaining edge-handling behavior unchanged.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc175781-e865-42d8-b26c-077070399af6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.github/workflows/ci.ymlAGENTS.mdCONTRIBUTING.mddocs/coding-standards.mdeslint.config.mjspackage.jsonprettier.config.mjsscripts/architecture-check.mjsscripts/architecture-check.test.mjsscripts/fixtures/architecture/accepted-boundary.tsscripts/fixtures/architecture/accepted-double-assertion.tsscripts/fixtures/architecture/accepted-local-tool.tsscripts/fixtures/architecture/rejected-boundary.tsscripts/fixtures/architecture/rejected-double-assertion.tsscripts/fixtures/architecture/rejected-local-tool.tssrc/adaptive/trace.tssrc/compress/code.test.tssrc/compress/code.ts
Comment on lines
+220
to
+227
| ### 実行可能規則（Issue #25） | ||
|
|
||
| - `pnpm run format:check` は新規standard tooling（`scripts/architecture-check*`、ESLint/Prettier設定、`package.json`）をPrettier検証。既存production codeの一括整形は別変更。 | ||
| - `pnpm run lint` はESLintでproduction/test TypeScript、`.mjs`、standard configを検証。broad `any` と理由なしTypeScript suppressionを拒否。 | ||
| - `pnpm run architecture:test` はvalidatorのaccepted/rejected fixtureを実行。 | ||
| - `pnpm run architecture:check` はTypeScript ASTとNodeNext module resolutionで、相対runtime import拡張子、未解決import、runtime dependency direction、import-time execution、MCP stdout、process/global boundary、unsafe type escape、local-tool `OUTPUT_SCHEMA`/annotationsを検証。 | ||
| - `pnpm run verify:standards` は上記4コマンドのcombined check。CIは原因を分離したstepで実行。 | ||
| - architecture layer map、boundary allowlist、suppression markerの正本は `scripts/architecture-check.mjs`。allow markerには局所理由必須。 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one normative source for executable standards.
AGENTS.md lists executable rules normatively, while CONTRIBUTING.md forbids a second normative list. Keep the configuration files as the implementation source and make the documentation a single consistent pointer.
AGENTS.md#L220-L227: Replace the normative command-and-rule list with a concise pointer, or define it as the canonical documentation.CONTRIBUTING.md#L78-L80: Keep the no-duplication rule and link to the selected canonical source.
📍 Affects 2 files
AGENTS.md#L220-L227(this comment)CONTRIBUTING.md#L78-L80
🤖 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 `@AGENTS.md` around lines 220 - 227, Use one normative documentation source for
executable standards: update AGENTS.md lines 220-227 to either retain the
command-and-rule list as the canonical documentation or replace it with a
concise pointer to the canonical configuration/source; update CONTRIBUTING.md
lines 78-80 to preserve the no-duplication rule and link to that same source.
Ensure both documents are consistent and do not maintain duplicate normative
lists.
Comment on lines
+219
to
+258
| function layerForFile(relative) { | ||
| if ( | ||
| relative === "src/index.ts" || | ||
| relative === "src/cli.ts" || | ||
| relative === "src/init.ts" || | ||
| relative.startsWith("src/commands/") | ||
| ) | ||
| return "entry"; | ||
| if ( | ||
| relative === "src/server.ts" || | ||
| relative === "src/proxy.ts" || | ||
| relative === "src/local-tools.ts" || | ||
| relative === "src/broker.ts" || | ||
| relative === "src/catalog.ts" || | ||
| relative === "src/code-search.ts" || | ||
| relative === "src/execution.ts" | ||
| ) | ||
| return "upstream"; | ||
| if (relative === "src/upstream.ts" || relative === "src/upstream-call.ts" || relative === "src/auth.ts") | ||
| return "upstream"; | ||
| if (relative.startsWith("src/adaptive/") || relative.startsWith("src/read-governor/")) return "adaptive"; | ||
| if (relative.startsWith("src/compress/")) return "compression"; | ||
| if (relative.startsWith("src/state/") || relative.startsWith("src/workflow/") || relative === "src/retrieve.ts") | ||
| return "persistence"; | ||
| if ( | ||
| relative === "src/config.ts" || | ||
| relative === "src/envelope.ts" || | ||
| relative === "src/logging.ts" || | ||
| relative === "src/telemetry.ts" | ||
| ) | ||
| return "shared"; | ||
| if (relative === "src/subprocess.ts") return "utility"; | ||
| return "shared"; | ||
| } | ||
|
|
||
| export function isDependencyAllowed(sourceLayer, targetLayer, targetPath = "") { | ||
| if (sourceLayer === targetLayer) return true; | ||
| if (allowedSpecialEdges.has(`${sourceLayer}->${targetLayer}:${targetPath}`)) return true; | ||
| return layerRules[sourceLayer]?.has(targetLayer) ?? true; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The documented server layer is unreachable. layerForFile never returns server, so layerRules.server is dead and the documented dependency chain does not describe the enforced behavior.
scripts/architecture-check.mjs#L219-L258: returnserverforsrc/server.ts,src/proxy.ts,src/local-tools.ts,src/broker.ts,src/catalog.ts,src/code-search.ts, andsrc/execution.ts, or delete theserverentry fromlayerRules.docs/coding-standards.md#L38-L54: update the chain on line 41 to match the layers thatlayerForFileactually assigns.
📍 Affects 2 files
scripts/architecture-check.mjs#L219-L258(this comment)docs/coding-standards.md#L38-L54
🤖 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/architecture-check.mjs` around lines 219 - 258, Update layerForFile
in scripts/architecture-check.mjs so the listed server-related files return the
documented server layer instead of upstream, and update docs/coding-standards.md
lines 38-54 to describe the dependency chain produced by the corrected layer
assignments. Keep layerRules and the documented architecture consistent across
both sites.
typescript-eslint@8.35.0 declares a peer range of typescript <5.9.0, but the lockfile resolves typescript to 5.9.3 — the lint stack was running outside its declared support range. 8.66.0 raises the peer ceiling to <6.1.0.
hasRuleMarker() matched an allow-marker against the whole file's text, so a single marker for double-assertion or import-time-side-effect silently suppressed the rule for every other violation anywhere else in that file — not just the annotated line, contrary to the "local marker" documentation. hasRuleMarker() now searches a narrow window around the flagged node (one leading line, one trailing line bounded by the next sibling statement) instead of the full source text. This surfaced 9 previously-hidden double-assertion violations in src/adaptive/trace.ts that were masked by one marker on an unrelated function; each now carries its own local marker. src/compress/code.ts markers were moved next to the statements they actually cover. Added regression tests proving a marker no longer suppresses unrelated violations elsewhere in the same file.
CI failed because the branch predated main's Repository Semantic IR addition (PR #58); the new architecture checker flagged its top-level snapshot()/create*Id() helper calls and TextEncoder as import-time side effects. Rebased onto main and extended the pure-call/constructor allowlists to cover these declarative factory patterns. Also addresses CodeRabbit findings: - hasRuleMarker no longer throws when two top-level statements share one line (clamp searchToLine to searchFromLine) - entry-point check uses fileURLToPath instead of URL().pathname - removed the unreachable "server" layer; upstream-facing files keep the "upstream" layer that matches their actual dependency direction - pinned typescript-eslint to an exact version, matching eslint/prettier - AGENTS.md points to docs/coding-standards.md instead of duplicating the executable-rules list (per CONTRIBUTING.md's no-duplication rule) - translated remaining English marker/rationale comments to Japanese - strengthened the accepted-local-tool fixture's OUTPUT_SCHEMA to the real shared envelope shape and added a rejected-fixture regression test for inline (non-shared) output schemas
yohnark
force-pushed
the
feat/25-coding-standards
branch
from
August 7, 2026 14:13
96e6a16 to
1ad1c8b
Compare
This was referenced Aug 8, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduce repository-wide coding standards and automated architecture-boundary checks for TypeScript source, local MCP tools, process-facing code, and workflow workers.
Linked issue
Closes #25
Scope
Establish deterministic formatting, linting, and architecture validation in local development and CI without changing runtime request behavior.
Included
Excluded
Implementation
The checker uses the TypeScript compiler API with NodeNext resolution and reports deterministic rule IDs, file paths, line numbers, detected constructs, and fixes. Existing intentional boundaries are represented by explicit, reasoned suppression markers or narrowly scoped allowlists. The task worker remains a process entry point because it must consume argv and emit one JSON result on stdout for the concurrency test.
Behavioral changes
CI now rejects formatting drift, lint violations, invalid architecture imports, unsafe process/output boundaries, and malformed local-tool definitions. Runtime request routing and upstream behavior remain unchanged.
Validation
pnpm run typecheck)pnpm test: 651/651;pnpm run architecture:test: 8/8)pnpm run build)npm pack --dry-run)pnpm run verify:standards)pnpm run governance:test: 18/18)git diff --check)src/compress/code.test.ts.Risks
Breaking changes
No. Runtime API and request behavior remain unchanged.
Review focus