feat(execd): add replacedCount feedback to Replace File Content API#991
feat(execd): add replacedCount feedback to Replace File Content API#991Pangjiping wants to merge 3 commits into
Conversation
The Replace File Content API (`POST /files/replace`) previously returned HTTP 200 with an empty body, making it impossible for callers to know whether replacements actually occurred. This change adds a `replacedCount` field per file in the response so callers can detect no-match scenarios. Closes opensandbox-group#982 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb55dcab0d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return e.client.doRequest(ctx, http.MethodPost, "/files/replace", req, nil) | ||
| func (e *ExecdClient) ReplaceInFiles(ctx context.Context, req ReplaceRequest) (ReplaceResponse, error) { | ||
| var resp ReplaceResponse | ||
| err := e.client.doRequest(ctx, http.MethodPost, "/files/replace", req, &resp) |
There was a problem hiding this comment.
When this SDK talks to an execd version from before this API addition, /files/replace still returns 200 OK with an empty body because the old controller called RespondSuccess(nil). Passing a non-nil result here makes doRequest run json.NewDecoder(resp.Body).Decode(&resp), so those otherwise-successful replacements now fail with decode response: EOF; Python/Kotlin explicitly handle this compatibility case, but the Go path does not.
Useful? React with 👍 / 👎.
| var response = await _client.PostAsync<Dictionary<string, ReplaceFileContentResult>>( | ||
| "/files/replace", body, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When the C# SDK is used against an execd build that predates replacedCount, /files/replace returns 200 OK with no response body (RespondSuccess(nil) in the old handler). Switching to the generic PostAsync<T> path makes HandleResponseAsync<T> throw Unexpected empty response body, so successful replacements on older sandboxes now surface as SDK failures instead of returning an empty result list.
Useful? React with 👍 / 👎.
|
|
||
| // ReplaceInFiles performs text replacement in files. | ||
| func (s *Sandbox) ReplaceInFiles(ctx context.Context, req ReplaceRequest) error { | ||
| func (s *Sandbox) ReplaceInFiles(ctx context.Context, req ReplaceRequest) (ReplaceResponse, error) { |
There was a problem hiding this comment.
Changing this public SDK method from error to (ReplaceResponse, error) breaks every existing Go caller that currently does err := sandbox.ReplaceInFiles(...) or passes the method as a func(...) error. Since the replacement counts are additive feedback, this should keep the existing method signature and expose the counted response through a new API or other backward-compatible path.
Useful? React with 👍 / 👎.
| /// <exception cref="InvalidArgumentException">Thrown when request values are invalid.</exception> | ||
| /// <exception cref="SandboxException">Thrown when the execd service request fails.</exception> | ||
| Task ReplaceContentsAsync( | ||
| Task<IReadOnlyList<ContentReplaceResult>> ReplaceContentsAsync( |
There was a problem hiding this comment.
This interface change requires every in-repo ISandboxFiles implementation to return Task<IReadOnlyList<ContentReplaceResult>>, but StubFiles in sdks/sandbox/csharp/tests/OpenSandbox.Tests/SandboxEgressLifecycleTests.cs:370 still implements Task ReplaceContentsAsync(...). As a result, the C# test project no longer compiles once this signature is changed.
Useful? React with 👍 / 👎.
| if (body.isNullOrBlank()) { | ||
| emptyList() | ||
| } else { | ||
| val parsed = kotlinx.serialization.json.Json.decodeFromString<Map<String, kotlinx.serialization.json.JsonObject>>(body) |
There was a problem hiding this comment.
This call relies on the reified kotlinx.serialization.decodeFromString extension, but that extension is not imported in this file; the member overload on Json requires an explicit deserializer. In the Kotlin SDK build this leaves the new replace response parsing line without a matching decodeFromString overload, so :sandbox:compileKotlin fails before the SDK can be published.
Useful? React with 👍 / 👎.
Pangjiping
left a comment
Pangjiping
left a comment
There was a problem hiding this comment.
Self-review: found 6 issues (2× P1, 2× P2, 2× P3). P1 backward compat bugs in Go and C# SDKs need fixing before merge.
- P1: Go SDK — handle io.EOF when old execd returns empty body
- P1: C# SDK — catch JsonException on empty body deserialization
- P2: execd handler — validate item.Old != "" to prevent misleading
replacedCount from strings.Count("", s) returning len+1
- P2: Python SDK — move backward compat from codegen file to adapter
layer (catch JSONDecodeError instead of patching generated code)
- P3: Kotlin — use jsonPrimitive.intOrNull instead of toString().toIntOrNull()
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2be6f1989
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except JSONDecodeError: | ||
| return [] |
There was a problem hiding this comment.
This catch wraps asyncio_detailed, whose generated parser calls response.json() for 400 and 500 responses as well as 200 responses. If execd/Gin or an intermediary returns an empty or non-JSON 400/500 body, this returns [] before handle_api_error runs, so a failed replacement is reported as a successful no-result replacement; the sync adapter repeats the same pattern. Limit the empty-body compatibility path to successful responses only.
Useful? React with 👍 / 👎.
- C# StubFiles: update ReplaceContentsAsync return type to match interface - Kotlin: apply spotless formatting (line breaks, indentation) - Java E2E: fix method name writeFiles → write to match Kotlin interface Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ca9909396
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if item.Old == "" { | ||
| c.RespondError(http.StatusBadRequest, model.ErrorCodeInvalidRequest, "old content must not be empty") | ||
| return |
There was a problem hiding this comment.
For clients that send old: "", this now returns 400 even though the public contract still defines old as a plain required string with no minLength (checked specs/execd-api.yaml), and the SDK models still allow constructing that request. The previous handler passed the value through to strings.ReplaceAll, so this is an undocumented breaking protocol change; either preserve the old behavior or update the spec/SDK validation with a migration story.
Useful? React with 👍 / 👎.
1 participant
Summary
replacedCountfield per file in thePOST /files/replaceresponse, so callers can detect no-match and multi-match scenariosreplacedCount=0), multi-match (replacedCount>1), and batch replace across all test suitesCloses #982
Changes
execd-api.yaml— 200 response schema +ReplaceFileContentResultstrings.Count()before replace, return per-file results (unix + windows)ContentReplaceResultmodel, adapter/service returnlist[ContentReplaceResult]ContentReplaceResultinterface, adapter/service returnContentReplaceResult[]ReplaceResult/ReplaceResponsetypes,ReplaceInFiles()returns(ReplaceResponse, error)ContentReplaceResultdata class, adapter uses direct OkHttp call to parse responseContentReplaceResultclass, adapter returnsIReadOnlyList<ContentReplaceResult>list[ContentReplaceResult]instead ofStatusResponseBackward compatibility
[]; Go decodes empty → nil mapvoid/Noneto result list — callers not using return value are unaffectedTest plan
go build ./...incomponents/execd— passed locallypnpm run buildinsdks/sandbox/javascript— passed locally./gradlew buildto regenerate API from spec🤖 Generated with Claude Code