mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
fix(opencode): deliver the session URL on OpenCode 2's plan review path too
The first commit fixed only the native command path. The plan path builds its
own client (createV2Client, typed as { app: { agents, log } } with no notifier),
so a remote OpenCode 2 user who reached a review through submit_plan still never
saw the URL: no browser is opened for them and the plugin's console output is
discarded by the host.
The plan path now builds the same bridge client the command path uses, with
toolContext.sessionID, so it carries notifyUrl whenever the host exposes
session.synthetic. That covers both runtimes: the CLI runtime already prefers
notifyUrl inside toastPlannotatorUrl, and the embedded runtime's previously
empty logReady hook is now createPlanReadyNotifier.
That hook still does not log. app.log is console.error, the same stderr
handleServerReady already printed the URL to, so logging there would duplicate
the line in remote mode and add a stray one locally, which is why the hook was
empty. The transcript notice is a different surface, and it is the only one a
remote reviewer can see. Without session.synthetic the hook stays silent exactly
as before.
createV2Client is gone: it duplicated the bridge client's URL-deduped app.log
verbatim, and nothing else used it.
Three tests on the plan path (delivers the notice; stays silent and does not
re-log without synthetic; catches a rejecting notice) plus one that pins the two
wiring seams at source level, since the notifier tests all pass while the plan
path is wired to nothing, which is the shape the bug had.
Also from review: console.error is stubbed across the V2 URL delivery block, so
those tests no longer print URL lines into the suite output. The README bullet
now says the notice covers every way a session opens rather than slash commands
alone.
AI-assisted (Claude) under maintainer direction.
This commit is contained in:
@@ -46,7 +46,7 @@ OpenCode 2 support is experimental while its plugin API is in beta. The core `su
|
||||
- **Command precedence.** OpenCode activates its own config-command loader after package plugins, and the last definition to claim a name wins, so the markdown stubs the installer writes to `~/.config/opencode/commands` would otherwise shadow the native definitions on every normal install. Plannotator re-registers the three names shortly after startup so its own definitions are the ones that run. If that reclaim cannot run, the stubs keep the names and the commands still work through the model-mediated fallback.
|
||||
- **Agent switching.** `ctx.session.switchAgent` arrived with the same plugin API generation. On a host that exposes it, an agent switch chosen in the review UI is applied to the session. On an older host the plan is still approved and a warning is written to the server log; switch to `build` manually before implementation.
|
||||
- **Abort signal.** V2 tool execution still exposes no abort signal. Cancelling a turn cannot stop a running review server or CLI child immediately.
|
||||
- **Session URLs.** OpenCode 2 has a TUI plugin entry point, but it is separate from the server plugin Plannotator registers, so there is no toast to show and the plugin's own console output is discarded by the host unless you start it with `OPENCODE_PRINT_LOGS=1`. Instead, when a slash command runs on a host whose plugin API exposes `session.synthetic`, Plannotator posts the URL into the session transcript as a `Plannotator session ready: <url>` notice. It is injected with `resume: false`, so it appears without waking a model turn. That is the link to open for a remote session, which gets no browser opened for it. On an older host without `session.synthetic` the URL only reaches that discarded console output, so run with `OPENCODE_PRINT_LOGS=1` there.
|
||||
- **Session URLs.** OpenCode 2 has a TUI plugin entry point, but it is separate from the server plugin Plannotator registers, so there is no toast to show and the plugin's own console output is discarded by the host unless you start it with `OPENCODE_PRINT_LOGS=1`. Instead, on a host whose plugin API exposes `session.synthetic`, Plannotator posts the URL into the session transcript as a `Plannotator session ready: <url>` notice, injected with `resume: false` so it appears without waking a model turn. This covers every way a session opens: the three slash commands and the `submit_plan` plan review, whether the review runs on the embedded runtime or the CLI. That is the link to open for a remote session, which gets no browser opened for it. On an older host without `session.synthetic` the URL only reaches that discarded console output, so run with `OPENCODE_PRINT_LOGS=1` there.
|
||||
|
||||
### OpenCode 1
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { describe, expect, mock, test } from "bun:test";
|
||||
import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import {
|
||||
@@ -485,6 +485,17 @@ describe("shared command stubs", () => {
|
||||
describe("V2 session URL delivery", () => {
|
||||
const SESSION_URL = "http://127.0.0.1:19432";
|
||||
|
||||
// cli-bridge logs every forwarded line, and the V2 client's app.log is
|
||||
// console.error, so without this each test here prints a URL into the suite
|
||||
// output. Restored per test so a real failure elsewhere still reports.
|
||||
const originalConsoleError = console.error;
|
||||
beforeEach(() => {
|
||||
console.error = () => {};
|
||||
});
|
||||
afterEach(() => {
|
||||
console.error = originalConsoleError;
|
||||
});
|
||||
|
||||
function makeSyntheticCtx() {
|
||||
const synthetic = mock(async (_input: unknown) => ({}));
|
||||
return { synthetic, ctx: { session: { synthetic } } as any };
|
||||
|
||||
@@ -1,8 +1,12 @@
|
||||
import { afterEach, describe, expect, mock, test } from "bun:test";
|
||||
import serverPlugin, {
|
||||
createPlanReadyNotifier,
|
||||
pushComposedSystemReminder,
|
||||
replacePlanningSystemParts,
|
||||
} from "./server";
|
||||
import { createV2BridgeClient, formatSessionUrlNotice } from "./v2-client";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
|
||||
const originalAllowSubagents = process.env.PLANNOTATOR_ALLOW_SUBAGENTS;
|
||||
|
||||
@@ -373,3 +377,77 @@ describe("system part consolidation (#1114 regression class)", () => {
|
||||
expect(text.indexOf("Host base rules")).toBeLessThan(text.indexOf("Second host part"));
|
||||
});
|
||||
});
|
||||
|
||||
describe("V2 plan review URL delivery", () => {
|
||||
const SESSION_URL = "http://127.0.0.1:19432";
|
||||
|
||||
// Regression: only the slash-command path was fixed at first. The plan path
|
||||
// builds its own client, so a remote reviewer who reached the review through
|
||||
// submit_plan still saw nothing: no browser is opened for them, and the
|
||||
// plugin's console output is discarded by the host.
|
||||
test("the embedded plan path posts the session URL as a transcript notice", async () => {
|
||||
const synthetic = mock(async (_input: unknown) => ({}));
|
||||
const client = createV2BridgeClient({
|
||||
ctx: { session: { synthetic } } as never,
|
||||
getAgents: async () => [],
|
||||
sessionID: "session-1",
|
||||
});
|
||||
|
||||
createPlanReadyNotifier(client)(SESSION_URL);
|
||||
await Promise.resolve();
|
||||
|
||||
expect(synthetic).toHaveBeenCalledTimes(1);
|
||||
expect(synthetic.mock.calls[0]![0]).toMatchObject({
|
||||
sessionID: "session-1",
|
||||
description: formatSessionUrlNotice(SESSION_URL),
|
||||
resume: false,
|
||||
});
|
||||
});
|
||||
|
||||
// Regression: the fallback must stay SILENT, not fall back to app.log. That
|
||||
// is console.error, the same stderr handleServerReady has already printed the
|
||||
// URL to, so logging here would duplicate the line in remote mode and add a
|
||||
// stray one locally. This hook was empty for exactly that reason.
|
||||
test("without session.synthetic the plan path stays silent", () => {
|
||||
const log = mock((_entry: unknown) => {});
|
||||
const client = createV2BridgeClient({
|
||||
ctx: { session: {} },
|
||||
getAgents: async () => [],
|
||||
sessionID: "session-1",
|
||||
});
|
||||
client.app.log = log as never;
|
||||
|
||||
expect(client.notifyUrl).toBeUndefined();
|
||||
expect(() => createPlanReadyNotifier(client)(SESSION_URL)).not.toThrow();
|
||||
expect(log).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Regression: the notifier tests above all pass while the plan path itself is
|
||||
// wired to nothing, which is exactly the shape the bug had. Reaching the real
|
||||
// wiring means running a plan review, so these two facts are pinned at source
|
||||
// level instead: without the session id the client can build no notifier, and
|
||||
// without the ready hook nothing ever calls it. Either one silently restores
|
||||
// the invisible URL with no other symptom.
|
||||
test("the plan path threads the session id and drives the ready hook", () => {
|
||||
const source = readFileSync(path.join(import.meta.dir, "server.ts"), "utf-8");
|
||||
|
||||
// Booleans, not toMatch: a failing regex against a whole source file dumps
|
||||
// the file into the report and buries the one line that matters.
|
||||
expect(/sessionID:\s*toolContext\.sessionID/.test(source)).toBe(true);
|
||||
expect(/logReady:\s*createPlanReadyNotifier\(/.test(source)).toBe(true);
|
||||
});
|
||||
|
||||
// Regression: a rejected notice must not surface as an unhandled rejection
|
||||
// and must not take the plan review down with it.
|
||||
test("a rejecting notice is caught", async () => {
|
||||
const client = createV2BridgeClient({
|
||||
ctx: { session: { synthetic: async () => { throw new Error("session gone"); } } } as never,
|
||||
getAgents: async () => [],
|
||||
sessionID: "session-1",
|
||||
});
|
||||
|
||||
expect(() => createPlanReadyNotifier(client)(SESSION_URL)).not.toThrow();
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -24,7 +24,12 @@ import {
|
||||
} from "./cli-bridge";
|
||||
import { switchV2SessionAgent } from "./agent-switch";
|
||||
import { registerNativeCommands } from "./native-commands";
|
||||
import { normalizeAgentList, type V2ContextLike } from "./v2-client";
|
||||
import {
|
||||
createV2BridgeClient,
|
||||
formatSessionUrlNotice,
|
||||
normalizeAgentList,
|
||||
type V2ContextLike,
|
||||
} from "./v2-client";
|
||||
import { executeSubmitPlan } from "./submit-plan-executor";
|
||||
import type { PlanEdit } from "./plan-edits";
|
||||
import { getPlanningPrompt } from "./planning-prompt";
|
||||
@@ -38,6 +43,13 @@ type V2Client = {
|
||||
agents: () => Promise<{ data: OpenCodeBridgeAgent[] }>;
|
||||
log: (entry: { level: "info" | "error"; message: string }) => void;
|
||||
};
|
||||
/**
|
||||
* Visible delivery for the session URL, present only when the host can post
|
||||
* a transcript notice. `cli-bridge` prefers it over the (absent on V2) toast
|
||||
* for the CLI runtime; `createPlanReadyNotifier` is the embedded runtime's
|
||||
* equivalent. See `createSessionUrlNotifier` in `v2-client.ts`.
|
||||
*/
|
||||
notifyUrl?: (input: { url: string; message: string }) => Promise<unknown>;
|
||||
};
|
||||
|
||||
type EmbeddedRuntimeModule = {
|
||||
@@ -196,7 +208,14 @@ const serverPlugin = {
|
||||
const session = await ctx.session.get({ sessionID: toolContext.sessionID });
|
||||
const directory = session.location.directory;
|
||||
const bridge = await getBridgeContext(getAgents);
|
||||
const client = createV2Client(getAgents);
|
||||
// Same client the native command path uses, and for the same reason:
|
||||
// `sessionID` is what lets the session URL reach a remote reviewer, who
|
||||
// gets no browser opened and cannot see the plugin's console output.
|
||||
const client = createV2BridgeClient({
|
||||
ctx: v2,
|
||||
getAgents,
|
||||
sessionID: toolContext.sessionID,
|
||||
});
|
||||
const result = await executeSubmitPlan({
|
||||
edits: getPlanEdits(input),
|
||||
invokingAgent: toolContext.agent,
|
||||
@@ -252,23 +271,6 @@ function getPlanTimeoutSeconds(): number | null {
|
||||
return parsed === 0 ? null : parsed;
|
||||
}
|
||||
|
||||
function createV2Client(
|
||||
getAgents: () => Promise<OpenCodeBridgeAgent[]>,
|
||||
): V2Client {
|
||||
const loggedUrls = new Set<string>();
|
||||
return {
|
||||
app: {
|
||||
agents: async () => ({ data: await getAgents() }),
|
||||
log: ({ message }) => {
|
||||
const url = /https?:\/\/\S+/.exec(message)?.[0];
|
||||
if (url && loggedUrls.has(url)) return;
|
||||
if (url) loggedUrls.add(url);
|
||||
console.error(message);
|
||||
},
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function allowSubagents(): boolean {
|
||||
const value = process.env.PLANNOTATOR_ALLOW_SUBAGENTS?.trim();
|
||||
return value === "1" || value === "true";
|
||||
@@ -310,6 +312,35 @@ function getPlanHtml(): string {
|
||||
return planHtml;
|
||||
}
|
||||
|
||||
/**
|
||||
* The embedded runtime's ready hook: put the session URL where a reviewer can
|
||||
* see it, and nowhere else.
|
||||
*
|
||||
* This still must NOT go to `client.app.log`. That is `console.error`, the same
|
||||
* stderr stream `handleServerReady` has already printed the URL to, so logging
|
||||
* here would duplicate the line in remote mode and add a stray one locally
|
||||
* (which is why this hook used to be empty). The transcript notice is a
|
||||
* different surface entirely, and on OpenCode 2 it is the only one a remote
|
||||
* reviewer can actually see: the host discards a server plugin's stderr unless
|
||||
* it was started with `OPENCODE_PRINT_LOGS=1`.
|
||||
*
|
||||
* Best-effort in both directions: an older host exposes no `session.synthetic`,
|
||||
* so `notifyUrl` is absent and this stays silent, exactly as before.
|
||||
*/
|
||||
export function createPlanReadyNotifier(client: V2Client): (url: string) => void {
|
||||
return (url: string) => {
|
||||
const notify = client.notifyUrl;
|
||||
if (typeof notify !== "function") return;
|
||||
try {
|
||||
void notify({ url, message: formatSessionUrlNotice(url) }).catch(() => {
|
||||
// A cosmetic notice must never surface as an unhandled rejection.
|
||||
});
|
||||
} catch {
|
||||
// Visible URL delivery is best-effort.
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
async function runPlanReview(input: {
|
||||
client: V2Client;
|
||||
runtime: RuntimeMode;
|
||||
@@ -338,14 +369,7 @@ async function runPlanReview(input: {
|
||||
htmlContent: getPlanHtml(),
|
||||
timeoutSeconds: input.timeoutSeconds,
|
||||
abortSignal: input.abortSignal,
|
||||
// Intentionally empty. OpenCode 2's server-plugin context exposes no log or
|
||||
// tui domain, and the V2 client's app.log falls through to console.error,
|
||||
// which is the same stderr stream handleServerReady already prints to.
|
||||
// Wiring this up would duplicate the session URL in remote mode and add a
|
||||
// stray line locally. V1 does target client.app.log and client.tui.showToast,
|
||||
// which are HTTP surfaces separate from stderr, so V1 never repeats itself.
|
||||
// A real toast here needs an upstream OpenCode API that does not exist yet.
|
||||
logReady: () => {},
|
||||
logReady: createPlanReadyNotifier(input.client),
|
||||
});
|
||||
} catch (error) {
|
||||
if (input.runtime === "embedded") throw error;
|
||||
|
||||
Reference in New Issue
Block a user