Organize src/core into real modules - #795
Conversation
…interiors Phase 0 of giving src/core an interior. Deletes core/review/address.ts (a speculative primitive whose only importer was its own test) and adds three boundary rules: no-dead-modules (any src/ module unreachable from a production entry point fails the check, with a shrink-only test-only allowlist), core-leaves-never-reimport-types (freezes the cycle fix so the extracted leaves can never import core/types.ts back), and the first per-module interior rule making core/review/reducer.ts importable only from within the review model. Also fixes core/watch/signature.ts to import CliInput from its leaf home, caught by the new freeze rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
…eset/ Phase 1 of giving src/core an interior. Twelve root files — the model, its patch-text builder, the loaders, and the file-level helpers — become one module with a declared surface: model, loaders, diffFile, fileSource, fileLanguage, binary, diffPaths, hunkHeader, and hunkSummary stay public, while fromPatch, sidecar, and fileLanguageLookup are enforced module-internal. Pure move: no exported symbol renamed, and core/types.ts keeps re-exporting the model for existing import sites. Also repoints the leaf-freeze rule at the moved paths so it keeps protecting the cycle fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
Phase 2 of giving src/core an interior. The nine files describing how a run is asked for — command inputs, config, the command catalog, invocation errors, experimental flags, paths, tab width, reload capability, and version — become core/invocation/. The CLI-input type definitions (session/markup/extension command inputs and ParsedCliInput) move out of core/types.ts into invocation/commandInputs.ts, with core/types re-exporting the names still consumed elsewhere so no import site changes. The leaf-freeze rule follows commandInputs to its new path; all nine files have outside consumers, so this module declares no interior yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
Phase 3 of giving src/core an interior. terminal, pager, jobControl, shutdown, projectRoot, appStateFile, updateNotice, and startupNotice move from core root into core/runtime/. Pure move with no symbol renames; all eight have outside consumers, so the module declares no interior. core root now holds only types.ts, reviewDigest.ts, and liveComments.ts beside the module directories. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
Phase 4, the last of giving src/core an interior. core/types.ts had 147 importing files bound to it for names that phases 1-3 had already moved; every import site now names the declaring module directly (changeset model, invocation inputs, extension-api contract, runtime notices), the re-exports are deleted, and the file becomes core/bootstrap.ts holding only the AppBootstrap/ReloadContext contract - 28 importers instead of 147, so editing it no longer invalidates half the typecheck graph. Stragglers rehomed by role: TerminalThemeMode to theme/detection, ExtensionsConfig/UserKeyBinding/PersistedViewPreferences to invocation/config, UserNoteLineTarget to liveComments. The leaf-freeze rule becomes core-leaves-stay-below-bootstrap across all eight core module dirs, with one named exception (changeset/loaders.ts assembles the bootstrap value) recorded for a future move to src/app. Internal import paths only; the published extension and opentui entry points are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
The pair now tells one story in the repo's own vocabulary: core/run/ is what a Hunk run was asked to be (inputs, config, catalog, invocation errors), and core/process/ is the OS process and terminal it executes in (terminal, pager, job control, shutdown, notices). "invocation" was formal, and "runtime" one letter from colliding with "run"; renaming both keeps the distinction memorable. Directory rename only — no file or symbol renamed, boundary rules repointed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Merges main, whose new cache-layer benchmark (#791) landed importing the core/types shell that this branch melts; DiffFile now comes from its declaring module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
|
@greptile-apps review |
Greptile SummaryThis PR reorganizes the loose
Confidence Score: 4/5The PR appears safe to merge, with only non-blocking stale module names in the architecture documentation. The declaration split, import rewrites, published entrypoints, and type-only bootstrap dependency remain coherent; the accepted issue is limited to documentation that still references superseded directory names. Files Needing Attention: docs/module-boundaries.md Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
App[src/app] --> Bootstrap[src/core/bootstrap.ts]
App --> Changeset[src/core/changeset]
App --> Run[src/core/run]
App --> Process[src/core/process]
Bootstrap --> Changeset
Bootstrap --> Run
Bootstrap --> Process
UI[src/ui] --> Changeset
UI --> Run
UI --> Process
Rules[dependency-cruiser rules] -. enforce interiors .-> Changeset
Rules -. enforce reachability .-> App
Prompt To Fix All With AI### Issue 1
docs/module-boundaries.md:168-169
**Stale core module names**
The final bootstrap dependency list still names `invocation` and `runtime`, although this PR establishes `run` and `process`; these stale paths direct maintainers toward nonexistent module names, and the same terminology remains in the final module inventory below.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(bench): repoint highlight-cache-laye..." | Re-trigger Greptile |
| `changeset/model`, `invocation/commandInputs`, `invocation/config`, `runtime/startupNotice`, | ||
| `theme/detection`, and `vcs/types`, and `core-leaves-stay-below-bootstrap` forbids the reverse |
There was a problem hiding this comment.
The final bootstrap dependency list still names invocation and runtime, although this PR establishes run and process; these stale paths direct maintainers toward nonexistent module names, and the same terminology remains in the final module inventory below.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/module-boundaries.md
Line: 168-169
Comment:
**Stale core module names**
The final bootstrap dependency list still names `invocation` and `runtime`, although this PR establishes `run` and `process`; these stale paths direct maintainers toward nonexistent module names, and the same terminology remains in the final module inventory below.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Greptile caught six references in docs/module-boundaries.md still naming invocation/ and runtime/ — the rename sweep matched path-like forms and skipped these prose mentions. Genuine English uses of "invocation" and "runtime" stay. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1
Problem
src/corehad become a pile of ~30 loose files that any code anywhere could reach into. The worst symptom wascore/types.ts: 147 files imported it, it mixed four unrelated audiences (the changeset model, CLI inputs, app bootstrap, and re-exported extension contract types), and touching it invalidated a big chunk of the typecheck graph. There was no way to tell which files were "API" and which were internals — everything was public because nothing said otherwise.What this does
Core is now a set of directories named for what they hold, each with a declared public surface, and
core/types.tsis gone — every file imports the module that actually declares the thing it wants, so the dependency graph shows real relationships instead of "everything depends on types.ts".flowchart TD ui["src/ui"] --> changeset ui --> run ui --> process app["src/app"] --> bootstrap app --> changeset app --> run session["src/session"] --> changeset session --> run subgraph core["src/core"] bootstrap["bootstrap.ts<br/>AppBootstrap + ReloadContext<br/>28 importers, down from 147"] changeset["changeset/<br/>the model + how it loads<br/><i>internal: fromPatch, sidecar,<br/>fileLanguageLookup</i>"] run["run/<br/>what a run was asked to be:<br/>inputs, config, command catalog"] process["process/<br/>where it executes: terminal,<br/>pager, job control, shutdown"] existing["review/ vcs/ theme/<br/>watch/ patch/<br/><i>already coherent, unchanged</i>"] end bootstrap --> changeset bootstrap --> runThe module boundaries aren't just convention: a dependency-cruiser rule fails CI if anything outside a module imports its internals, and a reachability rule fails on any
src/file no entry point can reach — which is howcore/review/address.ts(zero consumers) got deleted.Reviewing this
It's a big diff but a shallow one: everything is
git mvplus import-path updates. No exported symbol was renamed and no behavior changed — the publishedhunkdiff/extensionandhunkdiff/opentuientry points are untouched. The commits are reviewable independently if the whole thing is too much at once.Three judgment calls worth an opinion:
core/review/annotations.tsturned out to genuinely useAgentAnnotationfromextension-api/types— the old shell was hiding that edge. The seam test now allows that single file as a containment target rather than pretending the dependency doesn't exist.changeset/loaders.tsimports the bootstrap type it assembles (loadAppBootstrap), which is backwards for the layering. It's a named exception in the rules; moving that function up tosrc/appwould retire it, left as a follow-up.AgentCard.tsx+agentPopover.tsappear to be dead (nothing renders them since notes moved into the diff flow) but only their tests keep them alive. They're quarantined in the dead-module allowlist instead of deleted — happy to delete them here if you agree they're gone.🤖 Generated with Claude Code
https://claude.ai/code/session_01XcuayyxJrwQd9EHpuxNef1