Skip to content

Commit 699915c

Browse files
khaliqgantkhaliqgant
authored
Merge pull request #64 from AgentWorkforce/fix/bulk-write-per-file-revisions
feat(bulk): return per-file revisions from /fs/bulk
2 parents b1a868c + 105b859 commit 699915c

11 files changed

Lines changed: 895 additions & 99 deletions

‎docs/bulk-per-file-revisions-boundary.md‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
# Bulk Per-File Revisions — Boundary Spec
2+
3+
This document scopes the server-side change to `/fs/bulk` so the response
4+
includes per-file revisions, and the matching simplification in the mount
5+
reconciler so it no longer needs a follow-up `ReadFile` to discover revisions.
6+
7+
## 1. Files touched
8+
9+
| File | Create/Modify | Reason |
10+
| ---- | ------------- | ------ |
11+
| `internal/httpapi/server.go` | Modify | `handleBulkWrite` must build and return a `results` array with `{path, revision, contentType}` for every successfully written file. |
12+
| `internal/httpapi/server_test.go` | Modify | Add a test asserting the new `results` field is populated for successful writes and that failed entries are absent from `results` (they continue to appear in `errors`). |
13+
| `internal/relayfile/store.go` | Modify | `BulkWrite` and `BulkWriteFork` already compute per-file revisions internally; change their signatures to surface those revisions (and content types) so `handleBulkWrite` can forward them. |
14+
| `internal/relayfile/types.go` (or wherever `BulkWriteFile` / `BulkWriteError` live) | Modify | Add a shared `BulkWriteResult` struct (`Path`, `Revision`, `ContentType`) so server and mount wire types stay aligned. |
15+
| `internal/mountsync/types.go` | Modify | `BulkWriteResponse.Results` already exists; verify field names and JSON tags match the server shape exactly (`results`, `path`, `revision`, `contentType`). Remove the `Files []RemoteFile` branch if it is only there as a compatibility shim — keep it only if it is still populated by another code path. |
16+
| `internal/mountsync/syncer.go` | Modify | `reconcileBulkWrite`: when the bulk response already provides a revision for the path, use it directly and skip the `ReadFile` round-trip. Retain the `ReadFile` fallback only for the defensive empty-revision case. |
17+
| `internal/mountsync/syncer_test.go` | Modify | Tighten `TestBulkMigrationReducesHTTPCalls` to assert zero total requests on `/fs/file` (not just zero `POST`s). Remove the now-redundant `fsFilePostCount` counter. |
18+
19+
## 2. New wire shape for `/fs/bulk` response
20+
21+
```json
22+
{
23+
"written": 3,
24+
"errorCount": 1,
25+
"errors": [
26+
{
27+
"path": "locked.txt",
28+
"code": "permission_denied",
29+
"message": "agent lacks write permission"
30+
}
31+
],
32+
"results": [
33+
{
34+
"path": "docs/a.md",
35+
"revision": "rev_01HW...",
36+
"contentType": "text/markdown"
37+
},
38+
{
39+
"path": "docs/b.md",
40+
"revision": "rev_01HW...",
41+
"contentType": "text/markdown"
42+
},
43+
{
44+
"path": "src/x.go",
45+
"revision": "rev_01HW...",
46+
"contentType": "text/x-go"
47+
}
48+
],
49+
"correlationId": "corr_..."
50+
}
51+
```
52+
53+
Notes:
54+
55+
- `results` contains one entry per successfully written file, in the same
56+
order `BulkWrite` processed them. Entries in `errors` are **not** present in
57+
`results` (and vice versa).
58+
- `contentType` is emitted with `omitempty`; absent when the store did not
59+
resolve one.
60+
- `written` continues to equal `len(results)`.
61+
62+
## 3. Backward compatibility
63+
64+
- **Old SDK / client reads new response:** Extra top-level `results` field is
65+
ignored by standard JSON decoders (all existing SDKs use struct decoding with
66+
unknown-field tolerance or `map[string]any`). No existing field changes
67+
meaning, so old clients keep working.
68+
- **New mount reads old server response:** `BulkWriteResponse.Results` is
69+
`omitempty` on the wire and decodes to `nil` / empty. `revisionsByPath()`
70+
returns an empty map, and `reconcileBulkWrite` falls back to the existing
71+
`ReadFile` code path to recover the revision. This makes the rollout safe in
72+
either order (server-first or client-first).
73+
- No version gate, feature flag, or capability negotiation is required.
74+
75+
## 4. `Store.BulkWrite` signature change
76+
77+
Current:
78+
79+
```go
80+
func (s *Store) BulkWrite(workspaceID string, files []BulkWriteFile) (int, []BulkWriteError)
81+
func (s *Store) BulkWriteFork(workspaceID, forkID string, files []BulkWriteFile) (int, []BulkWriteError)
82+
```
83+
84+
Proposed:
85+
86+
```go
87+
func (s *Store) BulkWrite(workspaceID string, files []BulkWriteFile) (written int, results []BulkWriteResult, errors []BulkWriteError)
88+
func (s *Store) BulkWriteFork(workspaceID, forkID string, files []BulkWriteFile) (written int, results []BulkWriteResult, errors []BulkWriteError)
89+
```
90+
91+
Where `BulkWriteResult` is:
92+
93+
```go
94+
type BulkWriteResult struct {
95+
Path string `json:"path"`
96+
Revision string `json:"revision"`
97+
ContentType string `json:"contentType,omitempty"`
98+
}
99+
```
100+
101+
- `written == len(results)` is an invariant maintained by both methods.
102+
- `BulkWriteFork` receives the identical treatment — same struct, same
103+
ordering guarantee, same invariant. This keeps fork and main-branch writes
104+
symmetric for the mount code that consumes them.
105+
- The new `BulkWriteResult` type lives next to `BulkWriteFile` /
106+
`BulkWriteError` in the `relayfile` package and is re-aliased from
107+
`internal/mountsync/types.go` the same way those two are today.
108+
109+
## 5. Mount-side change
110+
111+
In `reconcileBulkWrite`:
112+
113+
- Before calling `ReadFile`, consult the `revisionsByPath()` map built from
114+
`BulkWriteResponse.Results`. If the map contains a non-empty revision for
115+
`pendingWrite.remotePath`, use it directly and skip the `ReadFile` call.
116+
- Preserve the `ReadFile` fallback for the defensive case where the server
117+
returned no `results` entry or an empty revision (covers rolling deploys and
118+
future server-side bugs).
119+
- `contentType` resolution follows the same pattern: prefer the value from
120+
`BulkWriteResponse.Results`, fall back to `ReadFile` only if the bulk
121+
response did not supply one *and* the local snapshot lacks one.
122+
- The function's signature and the caller in the syncer loop do not change;
123+
only the internal branching changes.
124+
125+
Net effect: the happy path goes from `POST /fs/bulk` + N × `GET /fs/file` down
126+
to a single `POST /fs/bulk`.
127+
128+
## 6. Test updates
129+
130+
### Server (`internal/httpapi/server_test.go`)
131+
132+
Add a test (or extend an existing one in `TestBulkWriteEndpoint`) that:
133+
134+
- Submits a bulk write with at least one writable file and one file that will
135+
fail (e.g. permission denied or invalid path).
136+
- Asserts `results` contains exactly the successful paths, each with a
137+
non-empty `revision` and the expected `contentType`.
138+
- Asserts the failed file is present in `errors` and **absent** from
139+
`results`.
140+
- Asserts `len(results) == written` and `len(errors) == errorCount`.
141+
142+
### Mount (`internal/mountsync/syncer_test.go`)
143+
144+
Tighten `TestBulkMigrationReducesHTTPCalls`:
145+
146+
- Replace the `fsFilePostCount atomic.Int32` POST-only counter with a single
147+
counter (or path slice) that records **every** request whose path matches
148+
`/fs/file` regardless of method.
149+
- Assert that counter is `0` after the bulk sync completes.
150+
- Remove the now-dead `fsFilePostCount` declaration, the POST filter inside
151+
the handler, and the post-check log line that reports `GET /fs/file`
152+
read-backs (there should be none).
153+
- Keep the assertion that exactly one `POST /fs/bulk` was made.
154+
155+
## 7. Deliberately out of scope
156+
157+
- FUSE mount path (`cmd/relayfile-fuse`, `internal/fuse/...`) — unchanged.
158+
- SDK / TypeScript bindings — no wire-type regeneration in this change; JS
159+
consumers continue to ignore the extra field until a separate follow-up.
160+
- Observer / dashboard surfaces — they consume events, not the bulk response;
161+
no change.
162+
- Any change to `errors` shape, `correlationId` semantics, or the `forkID`
163+
branching logic.
164+
165+
## 8. Acceptance gates
166+
167+
All three must pass, exactly as written:
168+
169+
```bash
170+
go test ./internal/httpapi/... -count=1 -timeout 120s
171+
go test ./internal/mountsync/... -count=1 -timeout 120s
172+
go build ./...
173+
```
174+
175+
BOUNDARY_READY

‎docs/bulk-per-file-revisions-review.md‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
# Bulk Per-File Revisions — Review Verdict
2+
3+
Reviewed against `docs/bulk-per-file-revisions-boundary.md` at branch
4+
`fix/bulk-write-per-file-revisions`.
5+
6+
## 1. Scope / files modified
7+
8+
Boundary-listed files touched (all present):
9+
10+
- `internal/httpapi/server.go` ✅
11+
- `internal/httpapi/server_test.go` ✅
12+
- `internal/relayfile/store.go` ✅ — `BulkWriteResult` added here (boundary
13+
allowed "wherever `BulkWriteFile` / `BulkWriteError` live")
14+
- `internal/mountsync/types.go` ✅ — alias to `relayfile.BulkWriteResult`,
15+
dropped the compatibility `Files []RemoteFile` branch as the boundary
16+
allowed
17+
- `internal/mountsync/syncer.go` ✅
18+
- `internal/mountsync/syncer_test.go` ✅
19+
20+
Additional files touched (minor scope creep, non-blocking):
21+
22+
- `internal/relayfile/store_test.go` — expanded to cover the new
23+
`results` return; reasonable companion to the store change even though the
24+
boundary did not enumerate it.
25+
- `internal/relayfile/adapters_test.go` — cosmetic `gofmt` alignment only
26+
(no functional change). Outside boundary scope but harmless.
27+
- `package-lock.json` — unrelated npm lockfile churn; not required by this
28+
feature.
29+
- `.trajectories/index.json`, `.trajectories/active/…`, `.trajectories/completed/…`,
30+
`workflows/05[9]-*.ts`, `workflows/061-bulk-write-per-file-revisions.ts` —
31+
trajectory / workflow bookkeeping, not production code.
32+
33+
Nothing functionally drifts outside the feature surface.
34+
35+
## 2. Backward compatibility
36+
37+
Wire response is purely additive. `handleBulkWrite` still emits
38+
`written`, `errorCount`, `errors`, `correlationId` with unchanged names,
39+
types, and semantics. `results` is a new sibling key that older clients
40+
using struct decoding (or `map[string]any`) will simply drop. Confirmed by
41+
re-reading `internal/httpapi/server.go:1597-1615` — only additive changes.
42+
43+
Mount side decodes `results` with `omitempty` on the response struct and
44+
falls back to the existing `ReadFile` path when the map is empty or the
45+
revision is blank (`internal/mountsync/syncer.go:684` retains the `ReadFile`
46+
call when `result.Revision` is missing). Rollout remains safe in either
47+
order.
48+
49+
## 3. Signature change coverage
50+
51+
`Store.BulkWrite` and `Store.BulkWriteFork` now return
52+
`(int, []BulkWriteResult, []BulkWriteError)`. Every call site is updated:
53+
54+
- `internal/httpapi/server.go:1603` (fork) and `:1605` (main).
55+
- `internal/httpapi/server_test.go:904, :945, :983` (export tests use `_`).
56+
- `internal/relayfile/store_test.go:444, :493, :547, :560, :589, :630`.
57+
58+
Grep for `\.BulkWrite(Fork)?\(` shows no stale 2-return call sites.
59+
60+
## 4. Mount `reconcileBulkWrite`
61+
62+
`flushPendingBulkWrites` builds `resultsByPath` via the new
63+
`resultsByPath()` helper in `internal/mountsync/types.go:23-36`, which
64+
normalizes paths and trims whitespace. When an entry exists, `ContentType`
65+
is copied into the pending snapshot before the reconcile call, and the
66+
revision is passed as the third argument to `reconcileBulkWrite`. An empty
67+
revision still triggers the defensive `ReadFile` fallback inside
68+
`reconcileBulkWrite` (unchanged), preserving the rolling-deploy safety net
69+
the boundary called out.
70+
71+
## 5. Test coverage
72+
73+
- **Success path carries `results`**: `TestBulkWriteEndpoint`
74+
(`internal/httpapi/server_test.go:823+`) asserts `Results` has exactly
75+
one entry with matching `Path`, non-empty `Revision`, and the expected
76+
`ContentType`.
77+
- **Error path omits failed entry from `results`**: same test submits a
78+
second file with `encoding: utf16` that fails validation; it asserts the
79+
failed path is in `Errors` (code `invalid_encoding`) and verifies no
80+
entry in `Results` shares that path.
81+
- **Invariants**: `len(results) == written` and `len(errors) == errorCount`
82+
are both asserted.
83+
- **Mount skips `ReadFile` when revision is present**:
84+
`TestBulkMigrationReducesHTTPCalls`
85+
(`internal/mountsync/syncer_test.go:660+`) now records *every* request on
86+
`/fs/file` and asserts `len(fsFileRequests) == 0` after the bulk sync.
87+
The old `fsFilePostCount` POST-only counter and its apologetic comment
88+
block are removed, as the boundary required.
89+
- **Store-level results shape** is exercised by the new
90+
`assertBulkWriteResults` helper (`internal/relayfile/store_test.go:203`)
91+
and wired into every `BulkWrite` test case in that file.
92+
93+
## 6. E2E tightening
94+
95+
`TestBulkMigrationReducesHTTPCalls` no longer inspects only POSTs — it
96+
asserts zero total requests on `/fs/file`. The POST-filter inside the mock
97+
handler was removed; any method on `/fs/file` now fails the test. This is
98+
exactly the tightening the boundary mandated (section 6, "Mount").
99+
100+
## 7. Build / format / vet
101+
102+
- `gofmt -l internal/{httpapi,mountsync,relayfile}` → clean.
103+
- `go build ./...` → clean.
104+
- `go test ./internal/httpapi/... ./internal/mountsync/... ./internal/relayfile/... -count=1 -timeout 180s`
105+
→ all three packages `ok`.
106+
- `go vet ./...` → one failure in `internal/mountfuse/fuse_test.go:76`
107+
(`fakeRemoteClient` missing `WriteFilesBulk`). Verified pre-existing by
108+
running `go vet` on the stashed pre-change tree; it fails identically.
109+
`internal/mountfuse` is the FUSE mount path the boundary explicitly
110+
excludes from scope (section 7, "FUSE mount path … unchanged"), so this
111+
is out of scope for this review. Recommend tracking it as a separate
112+
cleanup issue.
113+
114+
## 8. Nit (non-blocking)
115+
116+
When `BulkWrite` returns on an early input-validation error
117+
(`workspaceID` blank), `results` is returned as `nil`, and the server
118+
response therefore marshals `"results": null` rather than `[]`. The
119+
boundary doc does not specify empty-array vs. null semantics, and current
120+
clients tolerate either, so this is not blocking — but emitting
121+
`[]BulkWriteResult{}` in the handler when `results == nil` would make the
122+
wire shape more predictable for future JS/SDK consumers.
123+
124+
## Verdict
125+
126+
All seven checklist items pass. The new wire field is additive, signatures
127+
are updated consistently, the mount prefers response-supplied revisions
128+
with a preserved `ReadFile` fallback, tests cover both the success and
129+
error halves of the response, and the E2E proof is tightened to assert
130+
zero `/fs/file` requests of any method. Pre-existing `go vet` failure in
131+
`internal/mountfuse` is unrelated to this branch.
132+
133+
REVIEW_APPROVED

‎internal/httpapi/server.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1597,17 +1597,19 @@ func (s *Server) handleBulkWrite(w http.ResponseWriter, r *http.Request, workspa
15971597
}
15981598

15991599
var written int
1600+
var results []relayfile.BulkWriteResult
16001601
var storeErrors []relayfile.BulkWriteError
16011602
if forkID != "" {
1602-
written, storeErrors = s.store.BulkWriteFork(workspaceID, forkID, allowed)
1603+
written, results, storeErrors = s.store.BulkWriteFork(workspaceID, forkID, allowed)
16031604
} else {
1604-
written, storeErrors = s.store.BulkWrite(workspaceID, allowed)
1605+
written, results, storeErrors = s.store.BulkWrite(workspaceID, allowed)
16051606
}
16061607
errorsOut = append(errorsOut, storeErrors...)
16071608
writeJSON(w, http.StatusAccepted, map[string]any{
16081609
"written": written,
16091610
"errorCount": len(errorsOut),
16101611
"errors": errorsOut,
1612+
"results": results,
16111613
"correlationId": correlationID,
16121614
})
16131615
}

0 commit comments

Comments
 (0)