fix: close opencode upstream intake#155
Merged
Merged
Conversation
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdaptor Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant AuthService as Auth.Service
participant AppRuntime
participant Adaptor
Workspace->>AuthService: request auth context (Auth.Service.use(...auth.all()))
AuthService-->>AppRuntime: auth payload (async)
AppRuntime-->>Workspace: resolved OPENCODE_AUTH_CONTENT
Workspace->>Adaptor: create(config, env{OPENCODE_AUTH_CONTENT, OPENCODE_WORKSPACE_ID, OPENCODE_EXPERIMENTAL_WORKSPACES, OTEL_*}, from?)
Adaptor-->>Workspace: Promise<void>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
83cdf40 to
8a9d380
Compare
Astro-Han
commented
Apr 22, 2026
Astro-Han
left a comment
Owner
Author
There was a problem hiding this comment.
Nitpicky review of the upstream-intake closeout. Findings labeled P0–P3; nothing looks blocking for the stated scope, but a few of the P1/P2 items (auth env fanout, null! on Accordion.Item, and the unbounded turn scan) feel worth a second look before merge.
- P1: 3
- P2: 4
- P3: 3
8a9d380 to
ff1efc2
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/session-review.tsx (1)
496-507:⚠️ Potential issue | 🟠 MajorInclude media files in the renderability gate.
Line 496 currently treats all
0/0diffs as non-renderable, which also disables media diffs (oftenadditions=0,deletions=0) even though they can still be previewed. This makes those rows non-interactive (trigger/view-button/chevron all gated off).💡 Suggested fix
- const diffCanRender = () => diff.additions !== 0 || diff.deletions !== 0 + const diffCanRender = () => !!mediaKind() || diff.additions !== 0 || diff.deletions !== 0Downstream impact: update
packages/ui/src/components/session-review-diff-render.test.tsexpectation that currently hardcodes the olddiffCanRenderexpression.Also applies to: 516-516, 557-561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/session-review.tsx` around lines 496 - 507, The renderability gate currently implemented in diffCanRender (used in the Accordion.Trigger and related UI) treats diffs with additions===0 && deletions===0 as non-renderable and therefore disables media diffs; change diffCanRender to also return true for media files by adding a media check (e.g., check the file variable or a diff.isBinary/diff.isImage flag or a small helper isMediaFile(file) that matches common media extensions) so the expression becomes: additions !== 0 || deletions !== 0 || isMediaFile(file) (or diff.isMedia). Update the related occurrences (diffCanRender usage around Accordion.Trigger and the other two spots called out) and adjust the test expectation in packages/ui/src/components/session-review-diff-render.test.ts to match the new logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/util/media.ts`:
- Around line 32-35: The current ftyp detection in
packages/opencode/src/util/media.ts only checks the major brand at offset 8 and
misses compatible brands; update the ftyp handling (where startsWithAt and ascii
are used against bytes) to read the ftyp box size (uint32BE at offset 0) and
iterate 4-byte brand entries starting at offset 8 up to the box size, checking
each brand for any of the known variants (e.g.,
"avif","avis","heic","hevc","heix","hevx","mif1","msf1") and returning the
correct MIME ("image/avif" or "image/heic") when a match is found; add a
regression test covering files whose compatible-brand list (not major brand)
contains these variants.
---
Outside diff comments:
In `@packages/ui/src/components/session-review.tsx`:
- Around line 496-507: The renderability gate currently implemented in
diffCanRender (used in the Accordion.Trigger and related UI) treats diffs with
additions===0 && deletions===0 as non-renderable and therefore disables media
diffs; change diffCanRender to also return true for media files by adding a
media check (e.g., check the file variable or a diff.isBinary/diff.isImage flag
or a small helper isMediaFile(file) that matches common media extensions) so the
expression becomes: additions !== 0 || deletions !== 0 || isMediaFile(file) (or
diff.isMedia). Update the related occurrences (diffCanRender usage around
Accordion.Trigger and the other two spots called out) and adjust the test
expectation in packages/ui/src/components/session-review-diff-render.test.ts to
match the new logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 53c671dc-878d-4374-be8b-fb3ed96a1ae0
📒 Files selected for processing (19)
packages/opencode/src/control-plane/types.tspackages/opencode/src/control-plane/workspace.tspackages/opencode/src/plugin/github-copilot/copilot.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/tool/read.tspackages/opencode/src/tool/skill.tspackages/opencode/src/tool/webfetch.tspackages/opencode/src/util/media.tspackages/opencode/test/plugin/github-copilot-models.test.tspackages/opencode/test/plugin/workspace-adaptor.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/tool/read.test.tspackages/plugin/src/index.tspackages/ui/src/components/message-part-stale.test.tspackages/ui/src/components/message-part.tsxpackages/ui/src/components/session-review-diff-render.test.tspackages/ui/src/components/session-review.tsxpackages/ui/src/components/session-turn-parent.test.tspackages/ui/src/components/session-turn.tsx
ff1efc2 to
0d27acb
Compare
Astro-Han
commented
Apr 22, 2026
Astro-Han
commented
Apr 22, 2026
Astro-Han
commented
Apr 22, 2026
Astro-Han
commented
Apr 22, 2026
Astro-Han
commented
Apr 22, 2026
0d27acb to
c36e423
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/session-review.tsx (1)
496-507:⚠️ Potential issue | 🟠 MajorGate expansion/rendering, not only trigger affordances.
Line 507 disables the trigger, but non-renderable rows can still be opened through controlled state (
openprop / “expand all”), which makesexpanded()true and still mounts diff content. That violates the “non-renderable” contract for metadata-only rows.Suggested fix
@@ - const expanded = createMemo(() => open().includes(file)) + const diffCanRender = () => diff.additions !== 0 || diff.deletions !== 0 + const expanded = createMemo(() => diffCanRender() && open().includes(file)) @@ - const diffCanRender = () => diff.additions !== 0 || diff.deletions !== 0 + // moved above so render/mount logic can use it tooAlso update expand-all to only include renderable files:
const renderableFiles = createMemo(() => items() .filter((d) => d.additions !== 0 || d.deletions !== 0) .map((d) => d.file), ) const handleExpandOrCollapseAll = () => { const next = open().length > 0 ? [] : renderableFiles() handleChange(next) }Also applies to: 557-561
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/session-review.tsx` around lines 496 - 507, The Accordion currently only disables the trigger via Accordion.Trigger when diffCanRender() is false, but expanded state (controlled via open/expanded) can still mount diff content for non-renderable rows; update the gating so the diff content is not rendered when diffCanRender() is false by using diffCanRender() (or its memoized equivalent) to prevent expansion and to exclude non-renderable files from expand-all logic: add a memo like renderableFiles based on items().filter(d => d.additions !== 0 || d.deletions !== 0).map(d => d.file), use that in handleExpandOrCollapseAll to only expand renderable files (call handleChange with renderableFiles() or []), and ensure Accordion item expansion checks diffCanRender() before rendering content (e.g., block expanded/open for that file in the expanded() or open prop handling) so non-renderable rows never mount their diff content.
♻️ Duplicate comments (1)
packages/opencode/src/control-plane/workspace.ts (1)
207-217:⚠️ Potential issue | 🟠 Major
OPENCODE_AUTH_CONTENTis still over-broadly exposed to adaptor code.
Workspace.createforwards serialized auth for all providers into plugin adaptor env by default. That keeps credential exposure wider than necessary for third-party adaptors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/control-plane/workspace.ts` around lines 207 - 217, The code currently injects OPENCODE_AUTH_CONTENT with full serialized auth into every adaptor via Workspace.create -> adaptor.create; change this so the full auth blob is NOT forwarded by default: obtain the complete auth via AppRuntime.runPromise(Auth.Service.use(...)) but filter it down to only the providers the target adaptor actually needs (e.g., consult adaptor manifest or a new adaptor.requiresAuthProviders list on the config) and only set OPENCODE_AUTH_CONTENT when the filteredAuth is non-empty; implement this by replacing the current Object.entries(...) block with a step that computes filteredAuth = pick(fullAuth, requiredProviders) and only JSON.stringify(filteredAuth) into env when present, otherwise omit OPENCODE_AUTH_CONTENT before calling adaptor.create(config, env).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/plugin/workspace-adaptor.test.ts`:
- Around line 154-156: The test currently assumes OTEL env vars are unset which
makes it flaky; before constructing or inspecting created.env in
workspace-adaptor.test (the test that sets/inspects the variable named created),
explicitly remove any CI/dev OTEL variables by deleting
process.env.OTEL_EXPORTER_OTLP_HEADERS, process.env.OTEL_EXPORTER_OTLP_ENDPOINT
and process.env.OTEL_RESOURCE_ATTRIBUTES (or set them to undefined) at the start
of the test, or alternatively stub/memoize process.env and restore afterwards,
so expect(created.env).not.toHaveProperty(...) assertions are deterministic
across environments.
---
Outside diff comments:
In `@packages/ui/src/components/session-review.tsx`:
- Around line 496-507: The Accordion currently only disables the trigger via
Accordion.Trigger when diffCanRender() is false, but expanded state (controlled
via open/expanded) can still mount diff content for non-renderable rows; update
the gating so the diff content is not rendered when diffCanRender() is false by
using diffCanRender() (or its memoized equivalent) to prevent expansion and to
exclude non-renderable files from expand-all logic: add a memo like
renderableFiles based on items().filter(d => d.additions !== 0 || d.deletions
!== 0).map(d => d.file), use that in handleExpandOrCollapseAll to only expand
renderable files (call handleChange with renderableFiles() or []), and ensure
Accordion item expansion checks diffCanRender() before rendering content (e.g.,
block expanded/open for that file in the expanded() or open prop handling) so
non-renderable rows never mount their diff content.
---
Duplicate comments:
In `@packages/opencode/src/control-plane/workspace.ts`:
- Around line 207-217: The code currently injects OPENCODE_AUTH_CONTENT with
full serialized auth into every adaptor via Workspace.create -> adaptor.create;
change this so the full auth blob is NOT forwarded by default: obtain the
complete auth via AppRuntime.runPromise(Auth.Service.use(...)) but filter it
down to only the providers the target adaptor actually needs (e.g., consult
adaptor manifest or a new adaptor.requiresAuthProviders list on the config) and
only set OPENCODE_AUTH_CONTENT when the filteredAuth is non-empty; implement
this by replacing the current Object.entries(...) block with a step that
computes filteredAuth = pick(fullAuth, requiredProviders) and only
JSON.stringify(filteredAuth) into env when present, otherwise omit
OPENCODE_AUTH_CONTENT before calling adaptor.create(config, env).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ea3d73d4-c917-4f9f-baad-a8cb93b81192
📒 Files selected for processing (19)
packages/opencode/src/control-plane/types.tspackages/opencode/src/control-plane/workspace.tspackages/opencode/src/plugin/github-copilot/copilot.tspackages/opencode/src/session/message-v2.tspackages/opencode/src/tool/read.tspackages/opencode/src/tool/skill.tspackages/opencode/src/tool/webfetch.tspackages/opencode/src/util/media.tspackages/opencode/test/plugin/github-copilot-models.test.tspackages/opencode/test/plugin/workspace-adaptor.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/tool/read.test.tspackages/plugin/src/index.tspackages/ui/src/components/message-part-stale.test.tspackages/ui/src/components/message-part.tsxpackages/ui/src/components/session-review-diff-render.test.tspackages/ui/src/components/session-review.tsxpackages/ui/src/components/session-turn-parent.test.tspackages/ui/src/components/session-turn.tsx
c36e423 to
0fa3753
Compare
Owner
Author
|
Handled the CodeRabbit outside-diff review body items from the latest review:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/control-plane/workspace.ts`:
- Around line 209-211: The OPENCODE_WORKSPACE_ID env var is being set from
mutable runtime config.id but should come from the persisted workspace record
(info.id) so adaptors that mutate id in configure don't cause drift; update the
env construction that sets OPENCODE_WORKSPACE_ID to use the persisted workspace
identity (info.id / id from the loaded workspace record) instead of config.id
and ensure the code references the variable that holds the stored workspace info
(e.g., info) when building the env object in the same scope as where
AppRuntime.runPromise(...) is used.
In `@packages/opencode/test/plugin/github-copilot-models.test.ts`:
- Around line 165-232: Add a focused regression test that mirrors the existing
"chat.params" tests but calls hooks["chat.headers"] instead: instantiate
CopilotAuthPlugin, call hooks["chat.headers"] with a github-copilot model whose
api.npm is "@ai-sdk/anthropic" and assert the headers/options include the
Copilot-specific change (e.g., toolStreaming disabled), then call
hooks["chat.headers"] with a github-copilot OpenAI model and a non-copilot
anthropic model and assert their headers/options are unchanged (no
toolStreaming); reference CopilotAuthPlugin and hooks["chat.headers"] to locate
where to hook the new test.
In `@packages/ui/src/components/session-review.tsx`:
- Around line 266-274: The bulk-toggle logic in handleExpandOrCollapseAll
currently bases its decision on open().length which can include non-renderable
items; update handleExpandOrCollapseAll to use the same renderable-open
predicate used for the header (hasOpenRenderableFiles()) so the toggle computes
next as either [] when there are open renderable files or renderableFiles() when
none are open, and then call handleChange(next); this ensures the label and
action match the actual renderable diff rows (refer to renderableFiles(),
handleExpandOrCollapseAll, open(), handleChange, and hasOpenRenderableFiles()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 865d7880-fa7a-4d77-8364-2116f38567a3
📒 Files selected for processing (13)
packages/opencode/src/control-plane/types.tspackages/opencode/src/control-plane/workspace.tspackages/opencode/src/plugin/github-copilot/copilot.tspackages/opencode/src/tool/skill.tspackages/opencode/test/plugin/github-copilot-models.test.tspackages/opencode/test/plugin/workspace-adaptor.test.tspackages/plugin/src/index.tspackages/ui/src/components/message-part-stale.test.tspackages/ui/src/components/message-part.tsxpackages/ui/src/components/session-review-diff-render.test.tspackages/ui/src/components/session-review.tsxpackages/ui/src/components/session-turn-parent.test.tspackages/ui/src/components/session-turn.tsx
0fa3753 to
3443d08
Compare
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced Apr 23, 2026
This was referenced Apr 25, 2026
This was referenced May 3, 2026
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
readpromotes disguised image/PDF files while keeping SVG/text paths readable.Why
This is the final scoped #92 closeout after the session runtime lane in #140 landed. Desktop hardening remains tracked separately in #141/#152 and is not blocking this upstream intake closeout.
Related Issue
Closes #92
How To Verify
Screenshots or Recordings
Not applicable. The UI changes are narrow primitive guards and sticky offset constants covered by focused source tests plus the app build in
test:ci.Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Bug Fixes
Improvements
Tests