fix: scrub blocked URLs from system[] instead of relocating all content#198
fix: scrub blocked URLs from system[] instead of relocating all content#198s2bomb wants to merge 1 commit into
Conversation
bvironn
left a comment
bvironn
left a comment
There was a problem hiding this comment.
Tested locally — 214/214 tests pass. The empirical probe table in the PR body is excellent: it isolates the server-side trigger to one substring.
Two follow-ups worth considering:
-
Empty replacement leaves text artifacts: replacing
github.com/anomalyco/opencodewith""can produce dangling phrases like"Visit for help"in the prompt. Consider replacing withopencode.ai(functionally equivalent project URL — passes the validator per your probe row 11) to keep the prompt grammatically intact. -
Blocklist extensibility: a single hardcoded string is fine today, but future Anthropic additions would force another release cycle. An optional
OPENCODE_CLAUDE_AUTH_BLOCKED_STRINGS(comma-separated) merged with the built-in list would let users self-mitigate. Not blocking.
Strict improvement over v1.4.8 either way. Resolves #154 and #210.
…list - Replace 'github.com/anomalyco/opencode' with 'opencode.ai' instead of empty string so surrounding prose stays grammatical (no double spaces or dangling phrases). - Add OPENCODE_CLAUDE_AUTH_BLOCKED_STRINGS env var (comma-separated, optional 'pattern=replacement' form) so users can self-mitigate future server-side additions without a release cycle. - Restate scrub-in-place comment for clarity. - Add tests for grammatical replacement, realistic concatenated prompt, and env-var parser. Fix lint formatting flagged by oxfmt --check. Addresses review feedback from @bvironn on griffinmartin#198.
9b897cb to
a10b2b6
Compare
|
Getting Force pushed your original commit, can bring mine back in if this is resolved. |
Greptile SummaryThis PR narrows the v1.4.8 workaround for Anthropic's 400 rejection: instead of moving all non-core system content to the first user message, it now scrubs only the specific blocked URL (
Confidence Score: 4/5Safe to merge; the scrubbing logic is correct for the described real-world payload and the regression risk over v1.4.8 is minimal. The core change is straightforward and well-tested. The two gaps are: no filter for entries reduced to empty strings after scrubbing, and one stale test name that contradicts its assertions. Neither is a blocking regression. Files Needing Attention: Both changed files warrant a second look:
|
| Filename | Overview |
|---|---|
| src/transforms.ts | Replaces wholesale system-entry relocation with targeted in-place URL scrubbing; correct for real-world payloads but missing a guard for entries reduced to empty strings after scrubbing. |
| src/transforms.test.ts | Tests updated to match new system[]-preserving behaviour; one test description was not renamed and now contradicts its assertions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[transformBody called] --> B{body is string?}
B -- No --> Z[return body as-is]
B -- Yes --> C[Parse JSON]
C --> D[Inject billing header as system_0]
D --> E[Split identity prefix into separate entry]
E --> F{each system entry contains blocked URL?}
F -- Yes --> G[Scrub blocked substring in-place]
F -- No --> H[Leave entry in system unchanged]
G --> H
H --> I[Strip effort for unsupported models]
I --> J[PascalCase-prefix tool names]
J --> K[Repair orphaned tool pairs]
K --> L[Return JSON.stringify result]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/transforms.ts:191-200
**Empty system entry left after full-URL scrub**
If a system entry contains *only* the blocked URL (e.g. `{ type: "text", text: "github.com/anomalyco/opencode" }`), scrubbing reduces `entry.text` to `""`. That empty-text entry is then forwarded to Anthropic's API, which may reject it or silently alter billing/validation logic. The previous implementation explicitly checked `txt.length > 0` before including text in `movedTexts`, preventing this. The new code has no equivalent guard — after the mutation loop, entries with `entry.text === ""` remain in `parsed.system` unchanged.
```suggestion
for (const entry of parsed.system) {
if (
entry.type === "text" &&
typeof entry.text === "string" &&
isBlocked(entry.text)
) {
for (const blocked of BLOCKED_SYSTEM_STRINGS) {
entry.text = entry.text.split(blocked).join("")
}
}
}
// Remove any entries that were reduced to empty strings by scrubbing.
parsed.system = parsed.system.filter(
(e) => !(e.type === "text" && e.text === ""),
)
```
### Issue 2 of 2
src/transforms.test.ts:256
**Stale test description contradicts new behavior**
The test is named `"transformBody relocates multiple non-core system entries to user message as content blocks"`, but the body now asserts the exact opposite: that all entries stay in `system[]` (`parsed.system.length === 4`). This will confuse anyone reading the test output or running it in watch mode — a failing test's name would describe behavior that no longer applies.
```suggestion
it("transformBody keeps multiple safe non-core system entries in system[]", () => {
```
Reviews (1): Last reviewed commit: "fix: only relocate blocked URLs from sys..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const entry of parsed.system) { | ||
| if ( | ||
| entry.type === "text" && | ||
| typeof entry.text === "string" && | ||
| isBlocked(entry.text) | ||
| ) { | ||
| for (const blocked of BLOCKED_SYSTEM_STRINGS) { | ||
| entry.text = entry.text.split(blocked).join("") | ||
| } | ||
| } |
There was a problem hiding this comment.
If a system entry contains only the blocked URL (e.g. { type: "text", text: "github.com/anomalyco/opencode" }), scrubbing reduces entry.text to "". That empty-text entry is then forwarded to Anthropic's API, which may reject it or silently alter billing/validation logic. The previous implementation explicitly checked txt.length > 0 before including text in movedTexts, preventing this. The new code has no equivalent guard — after the mutation loop, entries with entry.text === "" remain in parsed.system unchanged.
| for (const entry of parsed.system) { | |
| if ( | |
| entry.type === "text" && | |
| typeof entry.text === "string" && | |
| isBlocked(entry.text) | |
| ) { | |
| for (const blocked of BLOCKED_SYSTEM_STRINGS) { | |
| entry.text = entry.text.split(blocked).join("") | |
| } | |
| } | |
| for (const entry of parsed.system) { | |
| if ( | |
| entry.type === "text" && | |
| typeof entry.text === "string" && | |
| isBlocked(entry.text) | |
| ) { | |
| for (const blocked of BLOCKED_SYSTEM_STRINGS) { | |
| entry.text = entry.text.split(blocked).join("") | |
| } | |
| } | |
| } | |
| // Remove any entries that were reduced to empty strings by scrubbing. | |
| parsed.system = parsed.system.filter( | |
| (e) => !(e.type === "text" && e.text === ""), | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/transforms.ts
Line: 191-200
Comment:
**Empty system entry left after full-URL scrub**
If a system entry contains *only* the blocked URL (e.g. `{ type: "text", text: "github.com/anomalyco/opencode" }`), scrubbing reduces `entry.text` to `""`. That empty-text entry is then forwarded to Anthropic's API, which may reject it or silently alter billing/validation logic. The previous implementation explicitly checked `txt.length > 0` before including text in `movedTexts`, preventing this. The new code has no equivalent guard — after the mutation loop, entries with `entry.text === ""` remain in `parsed.system` unchanged.
```suggestion
for (const entry of parsed.system) {
if (
entry.type === "text" &&
typeof entry.text === "string" &&
isBlocked(entry.text)
) {
for (const blocked of BLOCKED_SYSTEM_STRINGS) {
entry.text = entry.text.split(blocked).join("")
}
}
}
// Remove any entries that were reduced to empty strings by scrubbing.
parsed.system = parsed.system.filter(
(e) => !(e.type === "text" && e.text === ""),
)
```
How can I resolve this? If you propose a fix, please make it concise.
| assert.ok(parsed.system.some((e) => e.text === "prompt")) | ||
| }) | ||
|
|
||
| it("transformBody relocates multiple non-core system entries to user message as content blocks", () => { |
There was a problem hiding this comment.
The test is named "transformBody relocates multiple non-core system entries to user message as content blocks", but the body now asserts the exact opposite: that all entries stay in system[] (parsed.system.length === 4). This will confuse anyone reading the test output or running it in watch mode — a failing test's name would describe behavior that no longer applies.
| it("transformBody relocates multiple non-core system entries to user message as content blocks", () => { | |
| it("transformBody keeps multiple safe non-core system entries in system[]", () => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/transforms.test.ts
Line: 256
Comment:
**Stale test description contradicts new behavior**
The test is named `"transformBody relocates multiple non-core system entries to user message as content blocks"`, but the body now asserts the exact opposite: that all entries stay in `system[]` (`parsed.system.length === 4`). This will confuse anyone reading the test output or running it in watch mode — a failing test's name would describe behavior that no longer applies.
```suggestion
it("transformBody keeps multiple safe non-core system entries in system[]", () => {
```
How can I resolve this? If you propose a fix, please make it concise.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!
3 participants
Problem
v1.4.8 moved ALL non-core system content to the first user message to avoid Anthropic's 400 rejection. This regresses instruction priority and prompt-cache efficiency — AGENTS.md, env blocks, skills all lose system[] attention priority.
Root cause
Server-side blocklist. One URL triggers rejection:
github.com/anomalyco/opencode.Reproduces with Claude Code itself:
Not a client fingerprint. Not an auth issue. Server-side content filter.
Probe results
15 targeted requests varying system[] content. Same OAuth, same billing, same model.
github.com/anomalyco/opencodeopencode.aiURLSingle URL. Nothing else.
Fix
Scrub blocked URL substring in-place. Entry stays in system[]. No content relocation.
OpenCode concatenates the full prompt (identity + agent prompt + env + AGENTS + skills) into one system[] entry. Old approach relocated the whole entry on match → frontloaded everything into user message. This fix removes only the URL string.
Wire dump verification
Before (v1.4.10):
"You are OpenCode, the best coding agent..."← frontloadedAfter:
"What tools do you have? List 3."← cleanTests
214 pass. No new deps.