Repository navigation
test(workspace): run one contract table on the git engine and a change-id fake - #620
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fake writes during reads contrary to the binding specification, and two contract behaviors remain incorrectly covered.
3 open findings
What changed in this PR
Adds a reusable workspace engine contract suite and runs it against Git and a change-ID fake.
Changes:
- Adds nine engine contract scenarios and fixtures.
- Introduces
ChangesWithfor engine injection. - Updates architecture documentation and structure gates.
| File | Description |
|---|---|
internal/workspace/workspacetest/fake.go |
Implements the fake engine. |
internal/workspace/workspacetest/contract.go |
Defines the contract table. |
internal/workspace/workspace.go |
Adds engine injection. |
internal/workspace/workspace_test.go |
Runs both engine fixtures. |
internal/workspace/gitengine.go |
Exports GitEngine. |
internal/gates/structure.go |
Updates engine-list ownership. |
internal/gates/structure_test.go |
Updates the structure fixture. |
docs/architecture.md |
Documents the new contract design. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
andybons
force-pushed
the
feat/workspace-engine-list
branch
from
October 14, 2026 03:55
70e3bba to
94d14c0
Compare
andybons
force-pushed
the
feat/workspace-contract-table
branch
from
October 14, 2026 03:58
d527955 to
0b99850
Compare
andybons
force-pushed
the
feat/workspace-engine-list
branch
from
October 14, 2026 04:11
94d14c0 to
98a50bd
Compare
andybons
force-pushed
the
feat/workspace-contract-table
branch
from
October 14, 2026 04:11
0b99850 to
b23a542
Compare
…e-id fake Problem: no test states what an Engine must do, so a second engine has nothing to prove itself against, and a snapshotting engine could add an operation on every read. Design: workspacetest.Run(t, Factory) holds nine engine-neutral rows. Fake is an engine over a plain directory with change IDs, no .git, and an operation counter that rises only when the files change. ChangesWith takes the engine list so a row runs one engine through the real Changes path. GitEngine is exported for the git fixture. The git fixture holds an index.lock during the read-never-writes row, and compares the real index and the loose objects. Semantic change: none for clients. Changes calls ChangesWith with the engine list. Verification: the table ran first on a fake that adds an operation on every read, and the rows for no added operation and for reads never write failed (operations after another read = 4, want 2). They pass with the fix, and the git engine passes all nine rows. go build, go vet, go test -race ./..., golangci-lint, gofmt, HARNESS_E2E_RUNTIME=1 go test -race ./e2e/.
Problem: the no-operation row repeated only the branch read, the fake treated every nested file named like its state file as state, the git footprint missed references, HEAD, and config, and the contract did not say how a snapshotting engine meets the read rule. Design: the no-operation row repeats each scope. The fake skips only the state file in its root. The git footprint adds HEAD, config, packed-refs, and every file of refs, and the reads-never-write row runs with and without a planted index.lock. The Fake and Fixture docs state that the first read after an edit records an operation in the engine's own state, that a read of unchanged files records nothing, and that Diff gives one answer for both scopes. Semantic change: none for clients. Verification: go build, go vet, golangci-lint, go test -race for internal/workspace and internal/gates.
andybons
force-pushed
the
feat/workspace-contract-table
branch
from
October 14, 2026 04:19
b23a542 to
1bd232e
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
No test states what an
Enginemust do, so a second engine has nothing to prove itself against. A snapshotting engine could also add an operation on every read. This is VCS step 6, PR 4 ofvcs-engine-plan.mdsection 5, with the nine rows of its section 7. It is based on #619.Design
Spec: docs/architecture.md
Internal packages(theinternal/workspace/workspacetestrow) andworkspace(ChangesWith).workspacetest.Run(t, Factory)holds the nine engine-neutral rows: a read after an edit reports the edit; two reads with no edit add no operation; an untracked file is reported; a deleted file is reported; a rename is reported as a rename or as a delete and an add; a directory outside any repository isnot_a_repo; an unborn head gives nohead_revand no error; a missing base isno_base; reads never write.Fakeis an engine over a plain directory: its revisions arechange:<n>, it has no.git, and its operation counter rises only when the digest of the files changes.ChangesWithtakes the engine list, andChangespasses the real list to it, so a row runs one engine through the realChangespath.GitEngineis exported for the git fixture. The table runs fromworkspace_test.go, whichtestdata/test-exceptions.txtalready lists, and noOptionsseam is added.The git fixture checks that a read never writes: the row runs with and without a planted
.git/index.lock, so a write to the real index would fail, and it compares the real index,HEAD, the config, the packed references, and every file ofrefsand of the real object directory before and after both scopes. The no-operation row repeats each scope. TheFakemodels an engine that snapshots the working copy on read: the first read after an edit records an operation in its own state file, which no user sees, and a read of unchanged files records nothing. The spec read rule says the same.Semantic change
None for clients.
ChangescallsChangesWithwith the engine list.Verification
The table ran first on a fake that adds an operation on every read. The rows failed for the named reason:
operations after another read = 4, want 2andthe uncommitted read wrote to the repository: 3 want 2. They pass with the fix, and the git engine passes all nine rows.golangci-lint run ./...reports 0 issues,HARNESS_E2E_RUNTIME=1 go test -race ./e2e/passes, andgo build ./... && go vet ./... && go test -race ./... && test -z "$(gofmt -l .)"passes. One CLI e2e row (TestContractCLIRunPrintsEachOutputOfATaskChild...) failed once under the load of the full local run and passes alone with-count=3; it does not touch this change.Known flake, not fixed here:
TestChanges/a_dirty_submodule_is_not_walkedcan pass the 28 s budget under heavy load and fail withtoo_many_changes.CI job times
No job grew by more than 5 s. The 18 rows add about 2 s to the workspace package, which runs in parallel inside Unit.