Repository navigation
refactor(workspace): move the git diff behind Engine and Repo - #618
Merged
Merged
Conversation
Problem: the spec describes GET /workspace/changes as a git diff, so a second version-control engine has no contract to meet. Design: the spec names the Engine and Repo interfaces that internal/workspace consumes, the ordered engine list that only Changes walks, and the detection marker that each engine owns. Repo carries no storage path. The git behavior moves into a git engine bullet. Branch becomes an optional label. Semantic change: none for clients. head, base.sha, and not_a_git_repo stay deprecated. Verification: go test -race ./internal/gates/.
There was a problem hiding this comment.
🟡 Changes recommended
Changes hard-codes Git instead of walking the ordered engine list required by the architecture specification.
2 open findings
What changed in this PR
Refactors workspace diffing behind repository-owned VCS interfaces while preserving Git behavior.
Changes:
- Adds
Engine,Repo,Head, andBaseabstractions. - Moves Git-specific revision and diff logic into
gitengine.go.
| File | Description |
|---|---|
internal/workspace/workspace.go |
Defines interfaces and delegates workspace changes. |
internal/workspace/gitengine.go |
Implements the interfaces for Git. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Problem: the spec names the Engine operations without signatures or a no-match signal, and it says a read never changes the repository, which an engine that snapshots its working copy cannot meet. Design: the spec shows the Engine, Repo, Head, and Base method sets, states that Open returns ok false with a nil error for a directory that is not its kind and that a non-nil error ends the request, and states that only a missing base wraps ErrNoBase. The read rule becomes engine-neutral: a read may snapshot the working copy, adds no operation when nothing changed, and writes no index, reference, or object a user could see. Two sentences are reworded. Semantic change: none for clients. Verification: go test ./internal/gates/.
Problem: workspace.Changes calls git directly, so a second engine would need a branch in Changes. Design: Engine and Repo are defined in internal/workspace, which consumes them. gitengine.go implements them on the existing git helpers. Changes keeps the scope check, the containment check, and the budget, and builds the answer from Head, Base, and Diff. The branch label and the engine name come from the engine. Semantic change: none. The answer, the error messages, and the subprocess calls are the same. Verification: the existing internal/workspace tests and the e2e rows pass unchanged; go build, go vet, go test -race ./..., gofmt.
Problem: the staticcheck job flags the write of the deprecated Head field. Design: the line carries the same nolint note as the other deprecated writes. Semantic change: none. Verification: golangci-lint run ./internal/workspace/ reports 0 issues.
…ngine-neutral Problem: the not_a_repo message and the ErrNotRepo doc name git, the Changes comment holds a git-only hazard, and the Repo.Base doc says every error wraps ErrNoBase while the git engine also returns context and process errors. Design: the message reads "is not a work tree of a known VCS". The ceiling hazard moves to the doc of the git engine Open. The Base doc says that only the no-base case wraps ErrNoBase. Semantic change: the message text of a not_a_repo error changes, and the e2e golden changes with it. Verification: internal/workspace tests and the e2e workspace rows pass.
andybons
force-pushed
the
feat/workspace-engine-interface
branch
from
October 14, 2026 03:55
c42dedd to
0e3a493
Compare
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
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.
Problem
workspace.Changescalls git directly, so a second version-control engine would need a branch insideChanges. This is VCS step 6, PR 2 ofvcs-engine-plan.mdsection 5. It is based on #617 (the docs PR).Design
Spec: docs/architecture.md
Internal packages(internal/workspace) andworkspace(theEngineandRepobullets).Engine,Repo,Head, andBaseare defined ininternal/workspace, the package that consumes them.gitengine.goimplements them on the existing git helpers;git.goanddiff.goare unchanged.Changeskeeps the scope check, the containment check, and the 28 s budget, and builds the answer fromHead,Base, andDiff.Name()gives theenginefield andHead.Labelgivesbranch.Repoexposes no storage path.Ruling:
Difftakes theHeadas well as theBase, because the caller already holds it and the git engine needs it to seed a missing index and to pick the empty tree. A zeroBasemeans the head.Semantic change
The text of a
not_a_repoerror changes from "is not a git work tree" to "is not a work tree of a known VCS", and the e2e goldengit_changes_refusalschanges with it. The answer and the git subprocess calls are the same.Verification
The existing
internal/workspacetests and the e2e workspace rows pass unchanged (also withHARNESS_E2E_RUNTIME=1).go build ./... && go vet ./... && go test -race ./... && test -z "$(gofmt -l .)"passes.CI job times
No job grew by more than 5 s. The change adds no test.