History

Add version control foundation plans

- Add phase 1 VCS driver plan - Add phase 2 source control provider plan - Index both plans in `.plans/README.md`

Julius Marminge committed 6 months ago

Commit 0fa4e4c1 parent 460d9c33 files · +486 −0Browse files

Unified · Split

3 files
  1. .plans/19-version-control-phase-1-vcs-driver-foundation.md +216 −0
  2. .plans/20-version-control-phase-2-source-control-provider-foundation.md +268 −0
  3. .plans/README.md +2 −0
.plans/19-version-control-phase-1-vcs-driver-foundation.md+216−0
@@ -0,0 +1,216 @@
1+# Version Control Phase 1: VCS Driver Foundation
2+
3+## Goal
4+
5+Introduce a provider-neutral VCS layer and rewrite the local Git implementation as an Effect-native driver. This phase should preserve user-visible behavior while replacing the Git-first service boundary with an abstraction that can support Git, Jujutsu, and later Sapling or another viable VCS.
6+
7+The existing `GitCore` implementation is a behavior reference and source of regression tests, not the target architecture. New code should follow the newer package style used by `effect-acp` and `effect-codex-app-server`: typed service tags, schema-backed tagged errors, scoped process usage, explicit decode boundaries, and no Promise-based process helper as the core execution primitive.
8+
9+## Scope
10+
11+- Add VCS-domain contracts in `packages/contracts/src/vcs.ts`.
12+- Add shared runtime parsing helpers in `packages/shared/src/vcs/*` only when they are useful to both server and web.
13+- Add server services under `apps/server/src/vcs`:
14+ - `Services/VcsDriver.ts`
15+ - `Services/VcsRepositoryResolver.ts`
16+ - `Services/VcsProcess.ts`
17+ - `Layers/GitVcsDriver.ts`
18+ - `errors.ts`
19+- Migrate server callers from Git-specific terms where the operation is actually VCS-generic.
20+- Update active consumers to the new VCS APIs in the same phase; do not add backwards-compatible export shims.
21+- Leave source-control hosting providers out of this phase except for remote metadata needed to describe repository status.
22+
23+## Non-Goals
24+
25+- No GitLab, Azure DevOps, or GitHub provider rewrite yet.
26+- No Jujutsu driver yet, but every interface must be designed so a Jujutsu driver does not have to pretend to be Git.
27+- No T3 Review implementation yet.
28+- No broad UI redesign.
29+
30+## Driver Model
31+
32+Use provider-neutral nouns in new APIs:
33+
34+- `VcsDriver`: local repository mechanics.
35+- `RepositoryIdentity`: detected VCS kind, root path, common metadata path when available, remotes.
36+- `WorkingCopyStatus`: dirty state, changed files, aggregate insertions/deletions, current branch/bookmark/change name.
37+- `ChangeSet`: a committed or pending unit of change, not necessarily a Git commit.
38+- `RefName`: branch, bookmark, tag, or provider-specific ref.
39+
40+The initial driver capabilities should be explicit:
41+
42+```ts
43+export interface VcsDriverCapabilities {
44+ readonly kind: "git" | "jj" | "sapling" | "unknown";
45+ readonly supportsWorktrees: boolean;
46+ readonly supportsBookmarks: boolean;
47+ readonly supportsAtomicSnapshot: boolean;
48+ readonly supportsPushDefaultRemote: boolean;
49+}
50+```
51+
52+Do not model Jujutsu as `GitCoreShape extends ...`. The Git driver can expose Git-specific implementation details internally, but the public VCS layer should describe operations by intent:
53+
54+- `detectRepository(cwd)`
55+- `status(cwd, options)`
56+- `listRefs(cwd, query/pagination)`
57+- `checkoutRef(cwd, ref)`
58+- `createRef(cwd, ref, from?)`
59+- `createWorkspace(cwd, ref, path?)`
60+- `removeWorkspace(path)`
61+- `prepareChangeContext(cwd, filePaths?)`
62+- `createChange(cwd, message, options)`
63+- `push(cwd, target?)`
64+- `rangeContext(cwd, base, head)`
65+- `listWorkspaceFiles(cwd, options)`
66+
67+## Effect Process Layer
68+
69+Create a small reusable `VcsProcess` service instead of using `runProcess`.
70+
71+Requirements:
72+
73+- Implement with `ChildProcess` and `ChildProcessSpawner` from `effect/unstable/process`.
74+- Support scoped acquisition/release for long-running commands and interruption.
75+- Support bounded stdout/stderr collection with truncation markers.
76+ - DO not eagerly consume full stdout/stderr, return stream apis and expose helpers for consumers so we don't consume streams to memory unnecessarily...
77+- Support stdin.
78+- Support timeout through Effect scheduling/interruption, not ad-hoc timers.
79+- Stream output lines to progress callbacks as Effects.
80+- Return a typed `ProcessOutput` value for successful execution.
81+- Fail with typed errors, not generic thrown exceptions.
82+
83+Errors should be schema-backed tagged classes, for example:
84+
85+- `VcsProcessSpawnError`
86+- `VcsProcessExitError`
87+- `VcsProcessTimeoutError`
88+- `VcsOutputDecodeError`
89+- `VcsRepositoryDetectionError`
90+- `VcsUnsupportedOperationError`
91+
92+Every error should carry operation name, command display string, cwd when applicable, exit code when applicable, stderr/stdout tails when useful, and original cause where available. Override `message` for user readable messages that provides meaning and hints where appropriate. Errors are schema backed so the full error details will be persisted and serialized properly when stored to DB/Logfiles.
93+
94+## Git Driver Rewrite
95+
96+Rewrite Git support against `VcsProcess`.
97+
98+Carry forward current behavior from:
99+
100+- `apps/server/src/git/Layers/GitCore.ts`
101+- `apps/server/src/git/Layers/GitCore.test.ts`
102+- current Git status/branch/worktree contracts
103+
104+But split the implementation into smaller modules:
105+
106+- command execution and hardening config
107+- repository detection
108+- status parsing
109+- branch/ref parsing
110+- worktree operations
111+- commit/range context generation
112+- push/pull operations
113+
114+Keep parsing deterministic. Prefer Git porcelain formats, null-separated output, and schema decoding for JSON-like command output. Avoid regex parsing where Git gives a structured format.
115+
116+## Freshness and Local Caching
117+
118+Define freshness rules in the VCS layer before adding more providers. Local VCS status is cheap enough to refresh often; network-backed status is not.
119+
120+Treat these as live/local:
121+
122+- repository detection for the active cwd
123+- working copy dirty state
124+- staged/unstaged/untracked file summaries
125+- current branch/bookmark/change name
126+- local branch/bookmark lists
127+- local worktree/workspace lists
128+
129+These may run on user-visible polling, but should still be debounced and coalesced per repository root. Prefer filesystem-triggered invalidation where available, with a short fallback poll interval. Concurrent requests for the same repository/status shape should share one in-flight Effect.
130+
131+Treat these as cached or explicit-refresh only:
132+
133+- remote tracking branch refreshes
134+- ahead/behind counts that require network fetches
135+- default branch discovery from a remote provider
136+- remote branch lists beyond locally known refs
137+
138+The VCS driver should expose freshness metadata with status results:
139+
140+```ts
141+export interface VcsFreshness {
142+ readonly source: "live-local" | "cached-local" | "cached-remote" | "explicit-remote";
143+ readonly observedAt: string;
144+ readonly expiresAt?: string;
145+}
146+```
147+
148+Remote refreshes should be opt-in per operation, for example `refresh: "local-only" | "allow-cached-remote" | "force-remote"`. The default for background status should be `local-only`.
149+
150+Use Effect `Cache` for repository identity and expensive local metadata:
151+
152+- key by resolved repository root plus VCS kind
153+- invalidate on cwd/root changes and workspace mutation operations
154+- use short TTLs for local status caches when filesystem events are unavailable
155+- never hide command failures behind stale values unless the caller explicitly accepts stale data
156+
157+## Cutover Policy
158+
159+Prefer direct migration and deletion over compatibility wrappers.
160+
161+Rules:
162+
163+- Update consumers to call `VcsDriver`/`VcsRepositoryResolver` directly as soon as the new API exists.
164+- Delete migrated `GitCore` service methods and tests in the same PR that moves their consumers.
165+- Do not keep backwards-compatible export shims, barrel aliases, or old service names for convenience.
166+- Transitional modules are allowed only when a caller group is too complex or risky to migrate in the same PR.
167+- Every transitional module must have a narrow owner, a removal checklist, and a test proving it delegates to the new implementation.
168+- No new feature work may depend on transitional modules.
169+
170+Expected transitional candidates:
171+
172+- The highest-level `GitManager` orchestration can be migrated in slices if doing the full Commit + PR flow in one PR is too risky.
173+- WebSocket payload compatibility can remain only where changing it would require a coordinated UI/server protocol migration. Internal server code should still use the new VCS contracts.
174+
175+## Tests
176+
177+Add integration-style tests with real temporary Git repositories for the new Git driver:
178+
179+- non-repository detection
180+- status for clean/dirty/untracked/staged states
181+- branch/ref list with pagination
182+- checkout/create branch
183+- worktree create/remove
184+- commit context generation with file filters
185+- commit creation with hook progress events
186+- push behavior against a local bare remote
187+- status polling does not perform remote network refresh by default
188+- concurrent duplicate status requests are coalesced
189+- bounded output/truncation
190+- timeout/interruption
191+- typed error shape for command failure and missing executable
192+
193+Move or duplicate only the tests needed to prove behavior, then delete the old service tests in the same migration slice.
194+
195+## Migration Steps
196+
197+1. Add `vcs` contracts and tagged errors.
198+2. Add `VcsProcess` and unit tests around process execution semantics.
199+3. Add `VcsDriver` and `VcsRepositoryResolver` service contracts.
200+4. Implement `GitVcsDriver` with real Git command integration tests.
201+5. Move `GitStatusBroadcaster` and branch/worktree flows to the VCS service directly.
202+6. Move commit/range/push callers to the VCS service directly.
203+7. Delete migrated `GitCore` internals and tests as each caller group moves.
204+8. Add a transitional adapter only for any remaining `GitManager` path that is explicitly too complex to cut over safely in one PR.
205+9. Remove every transitional adapter before starting Phase 2 unless the adapter is documented as blocking on the provider cutover.
206+
207+## Acceptance Criteria
208+
209+- Current Git branch/status/worktree/commit behavior remains intact.
210+- New Git implementation does not depend on `processRunner.ts`.
211+- New errors are typed and inspectable by tests.
212+- VCS interfaces contain no GitHub/GitLab/Azure concepts.
213+- Active consumers use the new VCS APIs directly; any remaining transitional module has a written removal checklist and no compatibility export shim.
214+- Background status refresh is local-only by default and cannot hit provider rate limits.
215+- Jujutsu can be added by implementing a real driver instead of conforming to Git command semantics.
216+- `bun fmt`, `bun lint`, and `bun typecheck` pass.
.plans/20-version-control-phase-2-source-control-provider-foundation.md+268−0
@@ -0,0 +1,268 @@
1+# Version Control Phase 2: Source Control Provider Foundation
2+
3+## Goal
4+
5+Introduce a pluggable source-control provider layer and rewrite GitHub support as an Effect-native provider. This phase should preserve the existing GitHub Commit + PR flow while making GitLab and Azure DevOps additive drivers rather than branches inside GitHub-oriented code.
6+
7+The existing `GitHubCli` service and GitHub-specific `GitManager` paths are behavior references. The new provider layer should use detailed tagged errors, schema decode boundaries, `effect/unstable/process`, capability flags, and provider-neutral change-request types.
8+
9+## Scope
10+
11+- Add provider-domain contracts in `packages/contracts/src/sourceControl.ts`.
12+- Add provider URL/reference parsing helpers in `packages/shared/src/sourceControl/*`.
13+- Add server services under `apps/server/src/sourceControl`:
14+ - `Services/SourceControlProvider.ts`
15+ - `Services/SourceControlProviderRegistry.ts`
16+ - `Services/SourceControlProcess.ts`
17+ - `Layers/GitHubSourceControlProvider.ts`
18+ - `errors.ts`
19+- Migrate PR creation, PR lookup, default-branch lookup, clone URL lookup, and PR checkout through the provider layer.
20+- Update active consumers to the provider APIs directly; do not add backwards-compatible `GitHubCli` export shims.
21+- Keep GitHub as the only production provider at the end of this phase, but make GitLab and Azure implementation paths obvious and bounded.
22+
23+## Non-Goals
24+
25+- No GitLab implementation in this phase, except fixtures/contracts that prove the abstraction can represent merge requests.
26+- No Azure DevOps implementation in this phase, except URL/reference parser test cases if cheap.
27+- No in-app review UI yet.
28+- No hard dependency on one CLI forever. The first GitHub driver may use `gh`, but the interface should support REST/GraphQL implementations later.
29+
30+## Provider Model
31+
32+Use provider-neutral names:
33+
34+- `SourceControlProvider`: hosted repository and change-request mechanics.
35+- `ChangeRequest`: GitHub pull request, GitLab merge request, Azure pull request.
36+- `ChangeRequestThread`: review or discussion thread.
37+- `ChangeRequestComment`: top-level or inline comment.
38+- `ProviderRepository`: owner/project/repo identity plus clone URLs.
39+
40+Core provider operations:
41+
42+- `detectRemote(remoteUrl)`
43+- `checkAuth(cwd)`
44+- `getRepository(cwd | remoteUrl)`
45+- `getDefaultTargetRef(repository)`
46+- `listChangeRequests(repository, filters)`
47+- `getChangeRequest(repository, reference)`
48+- `createChangeRequest(repository, input)`
49+- `checkoutChangeRequest(cwd, changeRequest, options)`
50+- `getCloneUrls(repository)`
51+
52+Review-facing operations should be designed now, even if unimplemented:
53+
54+- `listReviewThreads(changeRequest)`
55+- `createReviewComment(changeRequest, input)`
56+- `replyToReviewThread(thread, input)`
57+- `resolveReviewThread(thread)`
58+- `submitReview(changeRequest, input)`
59+
60+Each operation should be guarded by capabilities:
61+
62+```ts
63+export interface SourceControlProviderCapabilities {
64+ readonly kind: "github" | "gitlab" | "azure-devops" | "unknown";
65+ readonly supportsCreateChangeRequest: boolean;
66+ readonly supportsCheckoutChangeRequest: boolean;
67+ readonly supportsReviewThreads: boolean;
68+ readonly supportsInlineComments: boolean;
69+ readonly supportsDraftChangeRequests: boolean;
70+}
71+```
72+
73+## Provider Registry
74+
75+Add a registry that resolves a provider from repository remotes and explicit user input.
76+
77+Rules:
78+
79+- Detection should be pure where possible and testable without spawning CLIs.
80+- Remote URL parsing belongs in `packages/shared`, not server-only provider layers.
81+- Unknown providers should return explicit unsupported-operation errors, not silently fall back to GitHub.
82+- Provider selection should be stable per operation and logged with enough context to debug bad remote detection.
83+
84+The registry should support multiple provider implementations at runtime, not a single dispatcher file with inline provider branches.
85+
86+## Rate Limits and Provider Caching
87+
88+Design the provider layer around a strict freshness budget. Provider API and CLI calls must not be part of frequent background polling unless the operation is explicitly marked safe and cached.
89+
90+Default behavior:
91+
92+- Pure URL/remote parsing is always live because it is local.
93+- Provider detection from local remotes is live-local.
94+- Authentication checks are cached.
95+- Repository metadata is cached.
96+- Default branch metadata is cached.
97+- Change-request lists are cached and refreshed on explicit user actions or coarse intervals.
98+- Full review threads, comments, file diffs, and timeline data are fetched only when the user opens the relevant review surface or explicitly refreshes it.
99+- Create/update operations invalidate affected cache keys immediately after success.
100+
101+The provider API should make freshness explicit:
102+
103+```ts
104+export interface SourceControlFreshness {
105+ readonly source: "live-local" | "cached-provider" | "live-provider";
106+ readonly observedAt: string;
107+ readonly expiresAt?: string;
108+ readonly stale?: boolean;
109+}
110+
111+export type ProviderRefreshPolicy =
112+ | "cache-first"
113+ | "stale-while-revalidate"
114+ | "force-refresh"
115+ | "local-only";
116+```
117+
118+Every read operation that can touch a provider should accept a refresh policy. Background UI reads should default to `cache-first` or `stale-while-revalidate`; direct user actions like pressing refresh can use `force-refresh`.
119+
120+Use Effect `Cache` for provider data:
121+
122+- auth status: key by provider kind, hostname, workspace identity, and account if known; TTL around minutes, not seconds
123+- repository metadata/default branch: key by provider repository stable ID or normalized remote URL; TTL around tens of minutes
124+- change-request summary lists: key by provider repository, state/filter, source ref, target ref; short TTL with stale-while-revalidate
125+- individual change-request summaries: key by provider repository and provider CR ID; short TTL, invalidated after create/update/comment operations
126+- review threads/comments/diffs: key by provider CR ID and head SHA/version when available; fetch on demand for T3 Review
127+
128+Provider drivers should surface rate-limit signals when available:
129+
130+- remaining quota
131+- reset time
132+- retry-after duration
133+- whether the limit is primary, secondary/abuse, or unknown
134+
135+Rate-limit errors should be typed, retryable when the provider gives a reset/retry time, and visible enough for the UI to avoid repeatedly retrying a blocked operation.
136+
137+Avoid rate-limit footguns:
138+
139+- no provider calls from render loops or fast status polling
140+- no listing all PRs/MRs across all repos to infer one branch state
141+- no silent GitHub fallback for unknown providers
142+- no unbounded cache cardinality for branch names or free-form search queries
143+- no per-thread duplicate provider refresh when multiple views observe the same repository
144+
145+## GitHub Provider Rewrite
146+
147+Rewrite GitHub support as `GitHubSourceControlProvider`.
148+
149+Carry forward behavior from:
150+
151+- `apps/server/src/git/Layers/GitHubCli.ts`
152+- `apps/server/src/git/Layers/GitHubCli.test.ts`
153+- `apps/server/src/git/githubPullRequests.ts`
154+- GitHub-specific `GitManager` PR paths
155+
156+Implementation requirements:
157+
158+- Use `SourceControlProcess` built on `effect/unstable/process`, not `runProcess`.
159+- Decode `gh api` and `gh pr --json` responses with Effect Schema.
160+- Use typed errors for auth failure, missing CLI, command failure, output decode failure, unsupported reference, and provider mismatch.
161+- Keep stdout/stderr bounded.
162+- Avoid global mutable auth caches unless they are Effect `Cache` values with explicit keys, TTLs, and invalidation behavior.
163+- Parse provider rate-limit headers or CLI/API error payloads when available and map them to typed rate-limit errors.
164+- Keep GitHub nouns inside the GitHub driver; convert to `ChangeRequest` at the provider boundary.
165+
166+## GitManager Cutover
167+
168+Refactor `GitManager` so it coordinates three independent services:
169+
170+- `VcsDriver` for local repository mechanics.
171+- `SourceControlProviderRegistry` for hosted provider selection.
172+- `TextGeneration` for message/body generation.
173+
174+`GitManager` should stop depending directly on GitHub services. User-visible step labels should be provider-neutral unless the selected provider is known and the label is intentionally provider-specific.
175+
176+The Commit + PR flow should become:
177+
178+1. Resolve VCS repository and local status.
179+2. Resolve source-control provider from remotes.
180+3. Generate commit content through the existing text generation service.
181+4. Create local change through `VcsDriver`.
182+5. Push through `VcsDriver` or a narrow provider push helper only if the VCS requires provider-specific target syntax.
183+6. Generate change-request title/body.
184+7. Create the change request through `SourceControlProvider`.
185+
186+## Cutover Policy
187+
188+This phase should aggressively remove old GitHub-specific internals.
189+
190+Rules:
191+
192+- Move each active consumer directly to `SourceControlProviderRegistry` or a concrete provider test layer.
193+- Delete migrated `GitHubCli` methods, tests, and GitHub-specific helper exports in the same PR that moves their final consumer.
194+- Do not add compatibility export shims from `apps/server/src/git` to `apps/server/src/sourceControl`.
195+- Transitional modules are allowed only for a bounded `GitManager` slice that cannot move safely with the rest of the provider cutover.
196+- Every transitional module must have an owner comment, a removal checklist, and no public exports consumed by new code.
197+- Provider-neutral web parsing should replace GitHub-only parsing directly; do not keep parallel parser stacks unless a route still requires both during a single PR.
198+
199+## GitLab and Azure Readiness
200+
201+Use the triaged references as implementation inputs, not merge targets:
202+
203+- GitLab PR #592 is useful for `glab mr` command mapping and JSON normalization.
204+- Azure issue #1138 defines a good first Azure slice: remote/URL detection and change-request thread setup for same-repo URLs.
205+
206+The abstraction should let Phase 3 add:
207+
208+- `GitLabSourceControlProvider` using `glab`.
209+- `AzureDevOpsSourceControlProvider` using `az repos pr` or REST APIs.
210+
211+No provider should need to edit GitHub code to join the registry.
212+
213+## T3 Review Design Constraint
214+
215+Do not optimize only for creation/checkout. The provider layer must be able to support a future in-app review surface.
216+
217+That means contracts should include stable IDs and enough metadata for:
218+
219+- file-level diffs
220+- inline review threads
221+- resolved/unresolved state
222+- top-level discussion comments
223+- pending review submission
224+- provider URL back-links
225+
226+Provider-specific fields can live in a metadata bag, but core review behavior should not require the UI to know whether the backing service is GitHub, GitLab, or Azure DevOps.
227+
228+## Tests
229+
230+Add tests at three levels:
231+
232+- Pure parser tests for GitHub, GitLab, and Azure remote URLs and change-request references.
233+- Provider unit tests with fake `SourceControlProcess` output and schema decode failures.
234+- Integration-style GitHub CLI tests only where they can run hermetically or be skipped without hiding unit coverage.
235+
236+Required cases:
237+
238+- GitHub PR URL, number, and branch-ish references.
239+- GitLab MR URL/reference parsing.
240+- Azure DevOps PR URL parsing for same-repo URLs.
241+- unknown provider returns unsupported-operation errors.
242+- missing CLI and auth failures produce distinct typed errors.
243+- invalid CLI JSON fails at decode boundary with useful context.
244+
245+## Migration Steps
246+
247+1. Add `sourceControl` contracts and provider-neutral schemas.
248+2. Add shared remote/reference parser helpers and tests.
249+3. Add `SourceControlProcess` and provider errors.
250+4. Add provider registry with GitHub-only registration.
251+5. Implement `GitHubSourceControlProvider` from scratch against the new process layer.
252+6. Cut GitHub PR operations in `GitManager` over to the provider registry.
253+7. Replace web PR-reference parsing with provider-neutral parser output while keeping current GitHub UX.
254+8. Add provider cache metrics and tests for cache hit, stale refresh, invalidation, and rate-limit error mapping.
255+9. Delete the migrated `GitHubCli` implementation, tests, and GitHub-specific helper exports unless an explicit transitional checklist remains.
256+
257+## Acceptance Criteria
258+
259+- Existing GitHub Commit + PR and PR checkout flows still work.
260+- `GitManager` no longer imports or depends on `GitHubCli`.
261+- Active consumers use source-control provider APIs directly; any remaining transitional module has a written removal checklist and no compatibility export shim.
262+- Source-control contracts can represent GitHub PRs, GitLab MRs, and Azure DevOps PRs.
263+- Unknown/unsupported providers fail explicitly and visibly.
264+- GitHub command execution does not depend on `processRunner.ts`.
265+- Background provider reads are cached/coalesced and do not consume provider API quota on every status refresh.
266+- Rate-limit responses become typed errors with retry/reset metadata where available.
267+- The provider API includes the review operations needed by future T3 Review work, even if they are capability-gated.
268+- `bun fmt`, `bun lint`, and `bun typecheck` pass.
.plans/README.md+2−0
@@ -10,3 +10,5 @@ · 9 unmodified lines
1010 8. `08-precommit-format-and-lint.md`
1111 9. `09-event-state-test-expansion.md`
1212 10. `10-unify-process-session-abstraction.md`
13+19. `19-version-control-phase-1-vcs-driver-foundation.md`
14+20. `20-version-control-phase-2-source-control-provider-foundation.md`

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t