Mark page pulls destructive in MCP - #216
Conversation
|
Self-review completed for
|
|
@claude review once |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _ = json.Unmarshal(raw, &arguments) | ||
| return runtime.ToValue(map[string]any{"executed": false, "requires_confirmation": true, "operation": strings.Join(operation.Path, "."), "arguments": arguments}) | ||
| } | ||
| output, err := r.catalog.Execute(ctx, operation, raw) |
There was a problem hiding this comment.
The five-second execution context only triggers a JavaScript interrupt, while this call receives the original request context. If a Gumroad API request is slow, the native operation can continue until the request or HTTP client's longer timeout expires. Code Mode therefore does not enforce its advertised per-call deadline. Pass the derived execution context through the installed operations and into Catalog.Execute.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/codemode/runtime.go
Line: 156
Comment:
**Deadline bypasses CLI operations**
The five-second execution context only triggers a JavaScript interrupt, while this call receives the original request context. If a Gumroad API request is slow, the native operation can continue until the request or HTTP client's longer timeout expires. Code Mode therefore does not enforce its advertised per-call deadline. Pass the derived execution context through the installed operations and into `Catalog.Execute`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if operation.ReadOnly { | ||
| if len(call.Arguments) == 2 { | ||
| panic(runtime.NewTypeError("read-only operations do not accept options")) | ||
| } | ||
| } else if !confirmed { |
There was a problem hiding this comment.
The confirmation boundary trusts the legacy MCP read-only annotation, but pages.pull is classified as read-only even though it creates and replaces a local file. Calling gumroad.pages.pull(...) therefore writes to the server filesystem without {confirm: true}, contrary to Code Mode's rule that mutations first produce a no-write plan. Classify operations using both remote and local side effects before using this annotation as an execution gate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/codemode/runtime.go
Line: 147-151
Comment:
**File writes bypass confirmation**
The confirmation boundary trusts the legacy MCP read-only annotation, but `pages.pull` is classified as read-only even though it creates and replaces a local file. Calling `gumroad.pages.pull(...)` therefore writes to the server filesystem without `{confirm: true}`, contrary to Code Mode's rule that mutations first produce a no-write plan. Classify operations using both remote and local side effects before using this annotation as an execution gate.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| } | ||
|
|
||
| func (r Runtime) Execute(ctx context.Context, source string) (any, error) { | ||
| runtime := goja.New() |
There was a problem hiding this comment.
The 64 KiB limits bound source text and serialized output, but not the JavaScript heap or the exported result before serialization. A short call such as return "x".repeat(1_000_000_000) can allocate a very large value before the result-size check runs, potentially exhausting memory in the shared stdio server. Agent-supplied code needs an enforceable memory boundary rather than only input, time, and post-execution output checks.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/codemode/runtime.go
Line: 29
Comment:
**JavaScript memory is unbounded**
The 64 KiB limits bound source text and serialized output, but not the JavaScript heap or the exported result before serialization. A short call such as `return "x".repeat(1_000_000_000)` can allocate a very large value before the result-size check runs, potentially exhausting memory in the shared stdio server. Agent-supplied code needs an enforceable memory boundary rather than only input, time, and post-execution output checks.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if len(call.Arguments) == 0 || goja.IsUndefined(call.Argument(0)) { | ||
| operations := make([]string, 0) | ||
| for _, operation := range r.catalog.Operations() { | ||
| operations = append(operations, strings.Join(operation.Path, ".")) |
There was a problem hiding this comment.
gumroad.help() reports raw command paths such as offer-codes.list, while the installed JavaScript namespace is gumroad.offer_codes.list. The discovery API therefore returns names that cannot be used as shown, and help("offer_codes.list") rejects the actual callable name. Returning the normalized JavaScript path, or both the CLI and JavaScript names, would keep discovery consistent with execution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/codemode/runtime.go
Line: 41
Comment:
**Help returns uncallable paths**
`gumroad.help()` reports raw command paths such as `offer-codes.list`, while the installed JavaScript namespace is `gumroad.offer_codes.list`. The discovery API therefore returns names that cannot be used as shown, and `help("offer_codes.list")` rejects the actual callable name. Returning the normalized JavaScript path, or both the CLI and JavaScript names, would keep discovery consistent with execution.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
da641e9 to
0efee25
Compare
keithah
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| case "pull": | ||
| if len(path) != 2 || path[0] != "pages" { | ||
| annotations.ReadOnlyHint = true | ||
| } else { | ||
| value := true | ||
| annotations.DestructiveHint = &value | ||
| } |
There was a problem hiding this comment.
This change marks pages_pull as destructive, but the canonical agent guidance in skills/gumroad/SKILL.md still says pull-style MCP tools are read-only. The repository requires command behavior changes to be reflected in the skill documentation, so this requirement must be satisfied before merging to prevent agents from relying on incorrect annotation guidance.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/cmd/mcp/mcp.go
Line: 157-163
Comment:
**Agent guidance remains stale**
This change marks `pages_pull` as destructive, but the canonical agent guidance in `skills/gumroad/SKILL.md` still says pull-style MCP tools are read-only. The repository requires command behavior changes to be reflected in the skill documentation, so this requirement must be satisfied before merging to prevent agents from relying on incorrect annotation guidance.
**Context Used:** CLAUDE.md ([source](https://github.com/antiwork/gumroad-cli/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in d5dee02. The canonical MCP guidance now distinguishes non-page pull commands from pages_pull, which carries destructiveHint because it writes a local HTML file. Focused MCP and embedded-skill tests passed.
1 participant
What
Marks
pages_pullas destructive in the existing MCP tool catalog.Why
pages pulluses a GET request, but it also creates or replaces a local HTML file. The MCP annotation must describe all side effects, not only the Gumroad API method, so clients can request confirmation before the tool writes to disk.Before/After
Before:
pages_pullwas marked read-only even though it writes<slug>.htmlor the requested output path.After:
pages_pullcarriesdestructiveHint; it is no longer advertised as read-only.Test Results
At
0efee25f4f1344e650758f4aede878669ce5dcc2:go test ./internal/cmd/mcp -run TestEnumerationAndMetadata -count=1 -v— passed.make test-cover— passed; MCP coverage: 99.3%.PATH=/Users/hermes/work/bin:$PATH make lint— passed.PATH=/Users/hermes/work/bin:$PATH make test-race— passed.Scope
This replaces the earlier oversized Code Mode proposal with one independently useful safety correction: 10 additions and 3 deletions across two MCP files. The broader Code Mode work is deliberately excluded for later small PRs.
AI disclosure
This PR was implemented with GPT-5.6-terra via OpenAI Codex.
Prompts that directed the implementation:
The author inspected and verified the final diff and local checks.