Conversation
…ssions Implements full MCP event client pipeline using Effect.ts: notification handler publishes to Bus, EventQueue service buffers with priority-aware draining and TTL expiry, prepareStep injects urgent events between tool calls, loop boundary injects high/normal events. Per-server permission config controls which effect types are allowed (inject_context defaults deny, notify_user defaults allow). Also fixes pre-existing test timeout flakes in prompt-effect tests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces extensive updates across the repository, including enhanced PR and issue templates, expanded documentation for AI agents, and a new Trust & Vouch system for contributors. Technical improvements feature a refactored provider connection flow, updated terminal serialization logic, and a major expansion of the E2E testing suite. Feedback focused on the potential for suppressed errors in GitHub Actions to mask failures and the non-deterministic nature of the automated issue triage tool, which currently relies on randomness for assignments.
| ${{ runner.os }}-bun- | ||
|
|
||
| - name: Install setuptools for distutils compatibility | ||
| run: python3 -m pip install setuptools || pip install setuptools || true |
There was a problem hiding this comment.
The use of || true to suppress errors in pip install can mask genuine installation failures. It is better to check if the package is already installed or handle the error explicitly if it is expected to fail.
| - If title + body does not contain "zen", do not add the "zen" label | ||
| - If "nix" label is added but title + body does not mention nix/nixos, the tool will drop "nix" | ||
| - If title + body mentions nix/nixos, assign to `rekram1-node` | ||
| - If "desktop" label is added, the tool will override assignee and randomly pick one Desktop / Web owner |
There was a problem hiding this comment.
The determinism rule mentions that the tool will 'randomly pick' a Desktop/Web owner. Relying on randomness for issue assignment can lead to inconsistent accountability. Consider using a round-robin approach or a more deterministic selection based on current workload.
| function pick<T>(items: readonly T[]) { | ||
| return items[Math.floor(Math.random() * items.length)]! | ||
| } |
There was a problem hiding this comment.
The pick function uses Math.random() for assignment. This is non-deterministic and makes it difficult to track or audit why a specific user was assigned to an issue. Consider a more deterministic selection method.
Imports event Zod schemas from axiomantic/typescript-sdk fork instead of maintaining local copies in event-schemas.ts. Deletes the local schema file entirely.
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
@gemini-code-assist please re-review |
There was a problem hiding this comment.
Code Review
This pull request introduces extensive updates across the repository, including administrative configurations, AI agent definitions, and a comprehensive set of locale-specific glossaries. Significant changes were made to the Nix packaging logic and the frontend application, which now features a refactored entry point, improved routing, and a new performance debug bar. The E2E testing suite was also greatly expanded. Feedback on the changes highlights a resource-intensive "busy-wait" loop in the new health check logic, an inconsistency in session routing where a keyed boundary is missing despite being required by the new implementation spec, and a potential state persistence bug in the provider connection dialog when the component is reused.
| while (true) { | ||
| const res = yield* Effect.promise(() => checkServerHealth(http)) | ||
| if (res.healthy) return true | ||
| if (checkMode() === "background" || type === "http") return false | ||
| } |
There was a problem hiding this comment.
The health check loop in ConnectionGate performs a "busy-wait" on the network when the server is unreachable. If checkServerHealth returns quickly (e.g., due to an immediate connection refusal), this loop will spam network requests without any delay, consuming client resources and potentially flooding the network. A small delay should be added between retries using yield* Effect.sleep("1 second").
while (true) {
const res = yield* Effect.promise(() => checkServerHealth(http))
if (res.healthy) return true
if (checkMode() === "background" || type === "http") return false
yield* Effect.sleep("1 second")
}
| const SessionRoute = () => ( | ||
| <SessionProviders> | ||
| <Session /> | ||
| </SessionProviders> | ||
| ) |
There was a problem hiding this comment.
The SessionRoute component renders SessionProviders without a keyed boundary. According to the create-effect-simplification-spec.md added in this PR, session-scoped UI and state should be keyed by a stable identity to ensure a clean reset when switching between sessions. Without a keyed boundary (e.g., using <Show when={key()} keyed>), providers like TerminalProvider or FileProvider may persist stale state or fail to re-initialize correctly when navigating between different sessions.
| let auto = false | ||
| createEffect(() => { | ||
| if (auto) return | ||
| if (loading()) return | ||
| if (methods().length === 1) { | ||
| auto = true | ||
| selectMethod(0) | ||
| } | ||
| document.addEventListener("keydown", handleKey) | ||
| onCleanup(() => { | ||
| document.removeEventListener("keydown", handleKey) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The auto flag used to prevent multiple auto-selections is a local variable in the component closure. If the DialogConnectProvider component is reused for a different provider (e.g., if the parent doesn't use a keyed boundary when switching props.provider), the auto flag will remain true, preventing auto-selection for subsequent providers. It is safer to check if a method is already selected via store.methodIndex === undefined instead of relying on a manual flag.
|
@gemini-code-assist please re-review |
There was a problem hiding this comment.
Code Review
This pull request introduces several infrastructure and configuration updates, including updates to GitHub Actions for Bun setup, refinements to issue templates and contributor management files, and improvements to the project's CI/CD and development environment configuration. I have provided feedback on cleaning up duplicate entries in the VOUCHED list and improving error handling for setuptools installation in the CI workflow.
| -danieljoshuanazareth | ||
| -danieljoshuanazareth |
There was a problem hiding this comment.
This username is listed twice. Please remove the duplicate entry to keep the file clean.
-danieljoshuanazareth
| ${{ runner.os }}-bun- | ||
|
|
||
| - name: Install setuptools for distutils compatibility | ||
| run: python3 -m pip install setuptools || pip install setuptools || true |
There was a problem hiding this comment.
Using || true suppresses all errors from the preceding commands, which can make it difficult to debug CI issues if setuptools fails to install for a legitimate reason. A more robust approach would be to explicitly check for Python and pip, and log a warning on failure rather than silently continuing. This would provide better visibility without failing the build.
Consider replacing this with a multi-line script for better error handling and logging, for example:
- name: Install setuptools for distutils compatibility
run: |
if command -v python3 &>/dev/null; then
python3 -m pip install setuptools || echo "Warning: Failed to install setuptools with python3."
elif command -v python &>/dev/null; then
python -m pip install setuptools || echo "Warning: Failed to install setuptools with python."
else
echo "Warning: Python not found, skipping setuptools installation."
fi
shell: bash
|
@gemini-code-assist please re-review |
There was a problem hiding this comment.
Code Review
This extensive pull request introduces a wide range of updates, including CI/CD pipeline enhancements, a refactored Nix build system, and a new E2E testing framework with LLM mocking. The subagent ecosystem is expanded with translation glossaries and changelog generation, while the web application's entry point is refactored to include a server health gate. Additionally, the agent style guide is updated to enforce stricter naming conventions. Feedback identifies a critical mismatch in TanStack Query hook usage for Solid.js, unhandled errors in the provider authorization flow, and potential recursion risks in error formatting, alongside suggestions for more balanced triage assignment logic.
| import { Dialog } from "@opencode-ai/ui/dialog" | ||
| import { IconButton } from "@opencode-ai/ui/icon-button" | ||
| import { ProviderIcon } from "@opencode-ai/ui/provider-icon" | ||
| import { useMutation } from "@tanstack/solid-query" |
There was a problem hiding this comment.
The import useMutation from @tanstack/solid-query appears to be incorrect for the Solid.js version of the library (v5), which typically uses createMutation. Using useMutation (the React hook name) will likely result in a runtime error as it is not exported by the Solid adapter.
| import { useMutation } from "@tanstack/solid-query" | |
| import { createMutation } from "@tanstack/solid-query" |
|
|
||
| const disabledProviders = globalSync.data.config.disabled_providers ?? [] | ||
| const nextDisabled = disabledProviders.filter((id) => id !== result.providerID) | ||
| const saveMutation = useMutation(() => ({ |
There was a problem hiding this comment.
As noted in the import feedback, useMutation should be replaced with createMutation to align with the @tanstack/solid-query API for Solid.js.
| const saveMutation = useMutation(() => ({ | |
| const saveMutation = createMutation(() => ({ |
| } | ||
| dispatch({ type: "auth.pending" }) | ||
| const start = Date.now() | ||
| await globalSDK.client.provider.oauth |
There was a problem hiding this comment.
The authorize call is awaited but not wrapped in a try/catch block. If the initial authorization request fails (e.g., due to a network error), the error will be unhandled, and the UI will remain in a pending state with a spinner indefinitely. Consider wrapping this call in a try/catch and dispatching an auth.error action on failure.
| function formatError(value: unknown, fallback: string): string { | ||
| if (value && typeof value === "object" && "data" in value) { | ||
| const data = (value as { data?: { message?: unknown } }).data | ||
| if (typeof data?.message === "string" && data.message) return data.message | ||
| } | ||
| if (value && typeof value === "object" && "error" in value) { | ||
| const nested = formatError((value as { error?: unknown }).error, "") | ||
| if (nested) return nested | ||
| } | ||
| if (value && typeof value === "object" && "message" in value) { | ||
| const message = (value as { message?: unknown }).message | ||
| if (typeof message === "string" && message) return message | ||
| } | ||
| if (value instanceof Error && value.message) return value.message | ||
| if (typeof value === "string" && value) return value | ||
| return fallback | ||
| } |
There was a problem hiding this comment.
The formatError function is recursive and could potentially lead to a stack overflow if an error object contains a circular reference to itself in the error property. While unlikely with standard API responses, adding a check for circularity or limiting the recursion depth would make this more robust.
| results.push("Dropped label: nix (issue does not mention nix)") | ||
| } | ||
|
|
||
| const assignee = nix ? "rekram1-node" : web ? pick(TEAM.desktop) : args.assignee |
There was a problem hiding this comment.
The pick(TEAM.desktop) function uses Math.random() to select an assignee. While simple, this can lead to an uneven distribution of issues if the random seed is unlucky. Consider implementing a simple round-robin or load-based assignment if more balanced triaging is required as the team grows.
1 participant
Summary
Context
Client-side integration for MCP events SEP reference implementation.
Uses Effect.ts patterns throughout (Service, Layer, serviceOption, PubSub).
Test plan
bun test packages/opencode/test/session/event-queue.test.ts packages/opencode/test/mcp/event-handler.test.ts