fix: surgically relocate only OpenCode-fingerprinted system entries#156
fix: surgically relocate only OpenCode-fingerprinted system entries#156griffinmartin wants to merge 4 commits into
Conversation
v1.4.8 (#148) worked around Anthropic's OAuth content validation by bulk-relocating all non-core system entries to the first user message. This caused a regression in long conversations (#154) because AGENTS.md, environment metadata, and skills blocks lost system-level attention priority and prompt-cache efficiency. Replace bulk relocation with a feature-based detector that only moves entries containing OpenCode fingerprints (\bopencode\b, anomalyco, 'Workspace root folder', <directories>), keeping all other system content in system[] where it belongs. Empirical probing against api.anthropic.com confirmed the classifier is multi-feature rather than substring-based — AGENTS.md, skills, and Claude Code-format env blocks all pass validation cleanly when kept in system[]. Fixes #154
|
I traced this end-to-end against the actual OpenCode runtime path and there is one important gap in the current approach.
Runtime path:
That means the current I verified this locally before patching by feeding a runtime-shaped combined system blob through the current code path. What worked locallyI added a small follow-up fix in
With that in place, the runtime-shaped flow behaved the way this PR intends:
I also added runtime-shaped tests locally for:
So I think the missing piece here is not in Happy to share the exact shape/tests if helpful, but the key point is: the current tests prove the surgical relocation works on synthetic separated |
|
I posted a new issue #210 before figuring out this PR existed. Running a fork with this PR code now. oauth working fine so far. I'd suggest merging this sooner rather than later. The plugins I'm using and developing have system prompts which when relocated degrades agent task performance. It also was making developing context management and state systems for OpenCode a lot more complicated. |
OpenCode joins the provider prompt, input.system entries, and input.user.system into a single string before triggering experimental.chat.system.transform (anomalyco/opencode packages/opencode/src/session/llm.ts:88-103). The earlier surgical relocation logic then saw one giant blob carrying OpenCode fingerprints and moved it wholesale into the first user message — taking AGENTS.md, skills, and the safe portion of the env block with it. The unit tests shipped with #156 used a synthetic separated system[] shape that the runtime never produces, so the regression #154 was not actually fixed. This change: - Adds splitOpenCodeSystemBlob(text) to recover the original segments using stable upstream anchors (env-block sentinel, <available_skills> wrapper, 'Instructions from:' headers). - Adds normalizeEnvBlock(env) to separate the OpenCode-only opener and 'Workspace root folder' line into branded extras while reconstructing a Claude-Code-shaped <env> block that can stay in system[]. - Wires both into the system.transform hook in src/index.ts. Replacing output.system also bypasses OpenCode's rejoin guard at llm.ts:107-111. - Tightens the brand regex to PascalCase \bOpenCode\b plus an explicit opencode.ai URL pattern, so working-directory paths like /Users/foo/dev/opencode-claude-auth and skill plugin file:// locations no longer false-positive as branded. - Adds runtime-shaped tests: splitter, env normalizer, and an end-to-end pipeline test that asserts AGENTS.md, the safe env, and a generic skills block all survive in system[] after hook + transformBody. Credit @ihabwahbi for surfacing the runtime/test mismatch in PR review.
|
Confirmed and fixed in 832105d. Your trace was right end-to-end:
Fix matches what you described:
Added a runtime-shaped pipeline test ( Thanks for the catch — the unit-test-only validation was the real gap. |
|
Update: 832105d closes a runtime gap in the original commit on this PR (the surgical relocation worked on synthetic input but, because OpenCode pre-joins all system pieces into one string before the plugin hook runs, the joined blob still got relocated as a single unit at runtime — taking AGENTS.md and skills with it). The new commit adds a splitter + env-block normalization that recovers AGENTS.md, safe env, and generic skills back into If you've been running daa6425, please rebuild against 832105d and confirm — your plugin-development scenario is exactly the case the runtime-shape test was added to lock down. Will move on merge once CI passes. |
# Conflicts: # src/transforms.test.ts
bvironn
left a comment
bvironn
left a comment
There was a problem hiding this comment.
Technical Review — APPROVE ✅
Checked out branch, ran full suite (250/250 pass), lint (0 issues), build (clean). Traced splitter logic for edge cases.
What this PR does right:
- Solves the runtime gap @ihabwahbi identified: OpenCode pre-joins system entries. splitOpenCodeSystemBlob() peels the blob back into pieces so transformBody classifies each independently.
- isOpenCodeBrandedEntry() with tightened PascalCase matching eliminates path false positives.
- Zero regression risk for non-OpenCode users. Graceful degradation for unrecognized shapes.
- Excellent test coverage including runtime pipeline integration test.
Minor observation (not blocking):
INSTRUCTIONS_HEADER_RE at line 94 uses [/~] which won't match Windows absolute paths. Low severity. Removing the char class to just /Instructions from: / would make it platform-agnostic.
This fixes #154 and #210. Most mature and impactful open PR. Ship it.
Greptile SummaryThis PR fixes a runtime regression (#154) where the v1.4.8 bulk-relocation workaround was defeating itself: OpenCode joins all
Confidence Score: 4/5Safe to merge with the two edge-case caveats understood; the happy-path pipeline is well-tested and the fallback degrades rather than crashes.
Files Needing Attention:
|
| Filename | Overview |
|---|---|
| src/transforms.ts | Adds five new exports; contains a case-sensitivity inconsistency in looksLikeOpenCodeJoinedBlob vs OPENCODE_FEATURE_PATTERNS, and a silent data-loss path in normalizeEnvBlock when ENV_OPENER_RE fails to match. |
| src/index.ts | Wires blob-detection and splitting into the experimental.chat.system.transform hook; logic is correct for the expected common case. |
| src/transforms.test.ts | Adds 16 new tests covering looksLikeOpenCodeJoinedBlob, normalizeEnvBlock, splitOpenCodeSystemBlob, and the full runtime pipeline; correctly updates existing tests to reflect the new surgical-relocation semantics. |
Comments Outside Diff (1)
-
src/transforms.ts, line 828-855 (link)Silent data loss when
ENV_OPENER_REdoesn't matchnormalizeEnvBlockis called with the full env-block string captured byENV_BLOCK_RE(which requires the "You are powered by the model named…" opener). IfENV_CONTAINER_REmatches (the<env>…</env>tags are present) butENV_OPENER_REfails — e.g. OpenCode changes the second line from"Here is some useful information…"to any other phrase —openeris set to"". The opener lines that precede<env>are never captured inopener, and they are not included insafe(which is built only from the<env>body). Those lines are simply dropped: they appear in neithersafenorbranded, and nothing is passed through for them.In v1.4.8, the whole joined blob would be relocated to the first user message, so the model would still see the content. Here the text silently vanishes, which contradicts the stated invariant "never worse than v1.4.8 bulk relocation." A minimal guard — returning
{ safe: null, branded: envBlock }whenENV_OPENER_REfails (mirroring the!containerMatchpath) — would restore the v1.4.8 fallback for this case.Prompt To Fix With AI
This is a comment left during a code review. Path: src/transforms.ts Line: 828-855 Comment: **Silent data loss when `ENV_OPENER_RE` doesn't match** `normalizeEnvBlock` is called with the full env-block string captured by `ENV_BLOCK_RE` (which requires the "You are powered by the model named…" opener). If `ENV_CONTAINER_RE` matches (the `<env>…</env>` tags are present) but `ENV_OPENER_RE` fails — e.g. OpenCode changes the second line from `"Here is some useful information…"` to any other phrase — `opener` is set to `""`. The opener lines that precede `<env>` are never captured in `opener`, and they are not included in `safe` (which is built only from the `<env>` body). Those lines are simply dropped: they appear in neither `safe` nor `branded`, and nothing is passed through for them. In v1.4.8, the whole joined blob would be relocated to the first user message, so the model would still see the content. Here the text silently vanishes, which contradicts the stated invariant "never worse than v1.4.8 bulk relocation." A minimal guard — returning `{ safe: null, branded: envBlock }` when `ENV_OPENER_RE` fails (mirroring the `!containerMatch` path) — would restore the v1.4.8 fallback for this case. How can I resolve this? If you propose a fix, please make it concise.
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:218-223
`looksLikeOpenCodeJoinedBlob` still uses the case-insensitive `/\bopencode\b/i` pattern that the rest of this PR explicitly replaces with the tighter PascalCase `/\bOpenCode\b/`. A system entry whose provider prompt begins with "You are powered by the model named …" and whose working directory contains a lowercase `opencode` path component (e.g. `/home/dev/opencode-project`) would satisfy both conditions here and incorrectly trigger the blob-splitter. Downstream `isOpenCodeBrandedEntry` would still classify the split pieces correctly (nothing would be wrongly relocated), but the unnecessary activation undermines the stated goal of the case-sensitivity tightening.
```suggestion
export function looksLikeOpenCodeJoinedBlob(text: string): boolean {
return (
/^You are powered by the model named/m.test(text) &&
OPENCODE_FEATURE_PATTERNS.some((p) => p.test(text))
)
}
```
### Issue 2 of 2
src/transforms.ts:828-855
**Silent data loss when `ENV_OPENER_RE` doesn't match**
`normalizeEnvBlock` is called with the full env-block string captured by `ENV_BLOCK_RE` (which requires the "You are powered by the model named…" opener). If `ENV_CONTAINER_RE` matches (the `<env>…</env>` tags are present) but `ENV_OPENER_RE` fails — e.g. OpenCode changes the second line from `"Here is some useful information…"` to any other phrase — `opener` is set to `""`. The opener lines that precede `<env>` are never captured in `opener`, and they are not included in `safe` (which is built only from the `<env>` body). Those lines are simply dropped: they appear in neither `safe` nor `branded`, and nothing is passed through for them.
In v1.4.8, the whole joined blob would be relocated to the first user message, so the model would still see the content. Here the text silently vanishes, which contradicts the stated invariant "never worse than v1.4.8 bulk relocation." A minimal guard — returning `{ safe: null, branded: envBlock }` when `ENV_OPENER_RE` fails (mirroring the `!containerMatch` path) — would restore the v1.4.8 fallback for this case.
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function looksLikeOpenCodeJoinedBlob(text: string): boolean { | ||
| return ( | ||
| /^You are powered by the model named/m.test(text) && | ||
| /\bopencode\b/i.test(text) | ||
| ) | ||
| } |
There was a problem hiding this comment.
looksLikeOpenCodeJoinedBlob still uses the case-insensitive /\bopencode\b/i pattern that the rest of this PR explicitly replaces with the tighter PascalCase /\bOpenCode\b/. A system entry whose provider prompt begins with "You are powered by the model named …" and whose working directory contains a lowercase opencode path component (e.g. /home/dev/opencode-project) would satisfy both conditions here and incorrectly trigger the blob-splitter. Downstream isOpenCodeBrandedEntry would still classify the split pieces correctly (nothing would be wrongly relocated), but the unnecessary activation undermines the stated goal of the case-sensitivity tightening.
| export function looksLikeOpenCodeJoinedBlob(text: string): boolean { | |
| return ( | |
| /^You are powered by the model named/m.test(text) && | |
| /\bopencode\b/i.test(text) | |
| ) | |
| } | |
| export function looksLikeOpenCodeJoinedBlob(text: string): boolean { | |
| return ( | |
| /^You are powered by the model named/m.test(text) && | |
| OPENCODE_FEATURE_PATTERNS.some((p) => p.test(text)) | |
| ) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/transforms.ts
Line: 218-223
Comment:
`looksLikeOpenCodeJoinedBlob` still uses the case-insensitive `/\bopencode\b/i` pattern that the rest of this PR explicitly replaces with the tighter PascalCase `/\bOpenCode\b/`. A system entry whose provider prompt begins with "You are powered by the model named …" and whose working directory contains a lowercase `opencode` path component (e.g. `/home/dev/opencode-project`) would satisfy both conditions here and incorrectly trigger the blob-splitter. Downstream `isOpenCodeBrandedEntry` would still classify the split pieces correctly (nothing would be wrongly relocated), but the unnecessary activation undermines the stated goal of the case-sensitivity tightening.
```suggestion
export function looksLikeOpenCodeJoinedBlob(text: string): boolean {
return (
/^You are powered by the model named/m.test(text) &&
OPENCODE_FEATURE_PATTERNS.some((p) => p.test(text))
)
}
```
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!
4 participants
Summary
Fixes #154 — a two-step fix for OpenCode's OAuth
system[]content-classifier rejection (#147) that preserves attention priority and prompt-cache stability for AGENTS.md, skills, and the safe portion of the env block.The original surgical relocation in this PR (commit daa6425) worked correctly on synthetic separated
system[]shapes, but did not deliver its stated outcome at runtime: OpenCode joins the provider prompt + everyinput.systementry +input.user.systeminto a single string before triggeringexperimental.chat.system.transform(see anomalyco/opencodepackages/opencode/src/session/llm.ts:88-103).transformBody()then saw one giant blob carrying OpenCode fingerprints and relocated it wholesale into the first user message — taking AGENTS.md, skills, and the safe part of env with it. Credit @ihabwahbi for catching this in review.The follow-up commit (832105d) closes that runtime gap.
What changed
src/transforms.tssplitOpenCodeSystemBlob(text)— recovers the original segments from OpenCode's joined blob using stable upstream anchors:^You are powered by the model named ... <env>...</env>(from upstreampackages/opencode/src/session/system.tsenvironment())<available_skills>wrapper for the skills blockInstructions from: ...headers (from upstreampackages/opencode/src/session/instruction.tssystem()) for AGENTS.md / CLAUDE.mdnormalizeEnvBlock(env)— separates the OpenCode-only opener (You are powered by the model named ...) andWorkspace root folder: ...line into a relocatable "branded extras" entry while reconstructing a Claude-Code-shaped<env>block (Working directory,Is directory a git repo,Platform,Today's date) that stays insystem[]. Closes the env portion of known-limitation Configure npm publishing #1 from the original PR description.OPENCODE_FEATURE_PATTERNS— the brand regex was previously/\bopencode\b/i, which false-positively matched lowercase path components like/Users/foo/dev/opencode-claude-auth/...(this very repo!) and/Users/foo/.cache/opencode/skills/.... Tightened to\bOpenCode\b(case-sensitive PascalCase), plus an explicitopencode\.aiURL pattern, plus the existing\banomalyco\b,Workspace root folder, and<directories>patterns. anthropic.txt and other branded prose use PascalCase consistently, so brand detection is unchanged in practice; what changes is that working-directory paths and skill plugin file:// URLs no longer trigger relocation.src/index.tsexperimental.chat.system.transformnow detects the joined-blob shape vialooksLikeOpenCodeJoinedBlob()and, when found, runssplitOpenCodeSystemBlob()and replacesoutput.systemwith[identity, ...split-pieces]. Replacing the array (rather than mutating in place after index 0) also bypasses OpenCode's rejoin guard atllm.ts:107-111, which would otherwise fold our split entries back into a single string whensystem.length > 2 && system[0] === header.system[]arrays the hook keeps the original behavior (only prepend identity).src/transforms.test.tsThree new
describeblocks —looksLikeOpenCodeJoinedBlob,normalizeEnvBlock,splitOpenCodeSystemBlob— plus a runtime-shaped pipeline test (the gap):This builds a realistic joined blob (anthropic-style provider prompt + the upstream env block template + a generic
<available_skills>block + anInstructions from: …/CLAUDE.mdblock), runs it through the splitter and the body transform, and asserts:system[]Workspace root folder-stripped) env remains insystem[]system[]Workspace root folder:line are prefixed onto the first user messagesystem[]Two existing brand-detection tests were updated to reflect PascalCase semantics, and an explicit "no false-positives on common path/URL patterns" test was added.
Behavior table at runtime (this PR vs v1.4.8 vs daa6425)
Workspace root folderTest plan
pnpm run buildcleanpnpm run lint— 0 warnings, 0 errors, formatting cleanKnown limitations / follow-ups
Env block still relocates today.Resolved in 832105d vianormalizeEnvBlock.OpenCodementions in user AGENTS.md. If a user's AGENTS.md prose contains the brandOpenCode(PascalCase), that entry gets relocated. Acceptable — content still reaches the model, just at user-message priority.Workspace root folder, removes the</env>close tag, or restructures the joined order, the splitter falls back to passthrough andtransformBodyreverts to v1.4.8-style bulk relocation. Never worse than today.Fixes #154