Skip to content

Commit c9c4e74

Browse files
committed
fix: address pr51 review findings
1 parent 23f7fef commit c9c4e74

6 files changed

Lines changed: 206 additions & 46 deletions

File tree

‎plugins/opencode/commands/rescue.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
description: Delegate investigation, an explicit fix request, or follow-up rescue work to the OpenCode rescue subagent
33
argument-hint: "[--background|--wait] [--worktree] [--resume|--fresh] [--model <provider/model>] [--agent <build|plan>] [what OpenCode should investigate, solve, or continue]"
44
context: fork
5-
allowed-tools: Bash(node:*)
5+
allowed-tools: Bash(node:*), AskUserQuestion
66
---
77

88
Route this request to the `opencode:opencode-rescue` subagent.

‎plugins/opencode/commands/setup.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ allowed-tools: Bash(node:*), Bash(npm:*), Bash(brew:*), Bash(curl:*), AskUserQue
77
Run:
88

99
```bash
10-
node "${CLAUDE_PLUGIN_ROOT}/scripts/opencode-companion.mjs" setup --json $ARGUMENTS
10+
node "${CLAUDE_PLUGIN_ROOT}/scripts/opencode-companion.mjs" setup --json "$ARGUMENTS"
1111
```
1212

1313
If the result says OpenCode is unavailable:
@@ -25,7 +25,7 @@ npm install -g opencode-ai
2525
- Then rerun:
2626

2727
```bash
28-
node "${CLAUDE_PLUGIN_ROOT}/scripts/opencode-companion.mjs" setup --json $ARGUMENTS
28+
node "${CLAUDE_PLUGIN_ROOT}/scripts/opencode-companion.mjs" setup --json "$ARGUMENTS"
2929
```
3030

3131
If OpenCode is already installed:

‎plugins/opencode/scripts/lib/prompts.mjs‎

Lines changed: 53 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,24 @@ import {
1616
getPrDiff,
1717
} from "./git.mjs";
1818

19-
// Inline-diff thresholds. When a review exceeds either, we fall back to a
20-
// "self-collect" context that omits the full diff and asks OpenCode to
21-
// inspect the change itself via read-only git commands. See issue #40 and
22-
// openai/codex-plugin-cc#179.
19+
// Inline-diff thresholds. When a review exceeds either, we keep the prompt
20+
// bounded by including a diff excerpt instead of the full diff. The review
21+
// agent is intentionally shell-disabled, so the prompt must contain the
22+
// evidence the model is allowed to use.
2323
const DEFAULT_INLINE_DIFF_MAX_FILES = 5;
2424
const DEFAULT_INLINE_DIFF_MAX_BYTES = 256 * 1024;
2525

26-
function buildCollectionGuidance(includeDiff) {
27-
return includeDiff
26+
function buildCollectionGuidance(diffIsComplete) {
27+
return diffIsComplete
2828
? "Use the repository context below as primary evidence."
29-
: "The repository context below is a lightweight summary. The full diff is intentionally omitted — inspect the target yourself with read-only git commands (git diff, git log, git show) before finalizing findings.";
29+
: "The repository context below contains a bounded diff excerpt, not the complete diff. Only report findings supported by the provided excerpt, metadata, status, and changed-file list; explicitly say when omitted diff content prevents a conclusion.";
30+
}
31+
32+
function truncateUtf8(text, maxBytes) {
33+
if (!text) return text;
34+
const buf = Buffer.from(text, "utf8");
35+
if (buf.length <= maxBytes) return text;
36+
return buf.subarray(0, maxBytes).toString("utf8").replace(/\uFFFD$/, "");
3037
}
3138

3239
/**
@@ -57,28 +64,31 @@ export async function buildReviewPrompt(cwd, opts, pluginRoot) {
5764
diffStat = await getDiffStat(cwd, { base: opts.base });
5865
}
5966

60-
// Classify the review scope. Large reviews fall back to a lightweight
61-
// context (stat + changed-files list + status) so the prompt doesn't blow
62-
// past model/provider input limits. OpenCode is then instructed to
63-
// inspect the diff itself via read-only git commands.
6467
const diffBytes = Buffer.byteLength(diff || "", "utf8");
6568
const maxFiles = Number.isFinite(opts.maxInlineDiffFiles)
6669
? opts.maxInlineDiffFiles
6770
: DEFAULT_INLINE_DIFF_MAX_FILES;
6871
const maxBytes = Number.isFinite(opts.maxInlineDiffBytes)
6972
? opts.maxInlineDiffBytes
7073
: DEFAULT_INLINE_DIFF_MAX_BYTES;
71-
const includeDiff = changedFiles.length <= maxFiles && diffBytes <= maxBytes;
72-
const collectionGuidance = buildCollectionGuidance(includeDiff);
74+
const overFileLimit = changedFiles.length > maxFiles;
75+
const overByteLimit = diffBytes > maxBytes;
76+
const diffIsComplete = !overByteLimit;
77+
const diffForPrompt = overByteLimit ? truncateUtf8(diff, maxBytes) : diff;
78+
const collectionGuidance = buildCollectionGuidance(diffIsComplete);
7379

7480
const targetLabel = prInfo
7581
? `Pull request #${prInfo.number} "${prInfo.title}" (${prInfo.headRefName} -> ${prInfo.baseRefName})`
7682
: opts.base
7783
? `Branch diff against ${opts.base}`
7884
: "Working tree changes";
7985

80-
const reviewContext = buildReviewContext(diff, status, changedFiles, prInfo, {
81-
includeDiff,
86+
const reviewContext = buildReviewContext(diffForPrompt, status, changedFiles, prInfo, {
87+
diffIsComplete,
88+
originalDiffBytes: diffBytes,
89+
maxInlineDiffBytes: maxBytes,
90+
overFileLimit,
91+
overByteLimit,
8292
diffStat,
8393
});
8494

@@ -112,7 +122,7 @@ function buildStandardReviewPrompt(diff, status, changedFiles, opts) {
112122

113123
const reviewContext =
114124
opts.reviewContext
115-
?? buildReviewContext(diff, status, changedFiles, opts.prInfo, { includeDiff: true });
125+
?? buildReviewContext(diff, status, changedFiles, opts.prInfo, { diffIsComplete: true });
116126
const collectionGuidance = opts.collectionGuidance ?? buildCollectionGuidance(true);
117127

118128
return `You are performing a code review of ${targetLabel}.
@@ -136,13 +146,11 @@ ${reviewContext}`;
136146
/**
137147
* Build the repository context block for review prompts.
138148
*
139-
* When `opts.includeDiff` is false (large-review fallback), the full `<diff>`
140-
* block is omitted and a `<diff_stat>` summary is emitted instead. OpenCode
141-
* is expected to inspect the diff itself via read-only git commands — the
142-
* collection-guidance line above the context makes this explicit.
149+
* When `opts.diffIsComplete` is false, the `<diff>` block is a bounded
150+
* excerpt. The surrounding note tells the model not to invent findings from
151+
* omitted content.
143152
*/
144153
function buildReviewContext(diff, status, changedFiles, prInfo, opts = {}) {
145-
const includeDiff = opts.includeDiff !== false;
146154
const sections = [];
147155

148156
if (prInfo) {
@@ -166,16 +174,31 @@ function buildReviewContext(diff, status, changedFiles, prInfo, opts = {}) {
166174
sections.push(`<changed_files>\n${changedFiles.join("\n")}\n</changed_files>`);
167175
}
168176

169-
if (includeDiff) {
170-
if (diff) {
171-
sections.push(`<diff>\n${diff}\n</diff>`);
177+
if (opts.overFileLimit || opts.overByteLimit) {
178+
const reasons = [];
179+
if (opts.overFileLimit) reasons.push(`file count ${changedFiles.length}`);
180+
if (opts.overByteLimit) {
181+
reasons.push(`diff size ${opts.originalDiffBytes} bytes`);
172182
}
173-
} else {
174-
const statBody =
175-
opts.diffStat && opts.diffStat.length > 0
176-
? opts.diffStat
177-
: `(diff too large to inline: ${changedFiles.length} file(s))`;
178-
sections.push(`<diff_stat>\n${statBody}\n</diff_stat>`);
183+
const budget = opts.overByteLimit && opts.maxInlineDiffBytes
184+
? `; excerpt budget ${opts.maxInlineDiffBytes} bytes`
185+
: "";
186+
const note = opts.diffIsComplete === false
187+
? "Diff context is bounded"
188+
: "Review spans a broad changed-file set, but the diff below is complete";
189+
sections.push(
190+
`<diff_note>\n` +
191+
`${note} (${reasons.join(", ")}${budget}). ` +
192+
`Findings must be supported by the diff evidence below.\n` +
193+
`</diff_note>`
194+
);
195+
if (opts.diffStat) {
196+
sections.push(`<diff_stat>\n${opts.diffStat}\n</diff_stat>`);
197+
}
198+
}
199+
200+
if (diff) {
201+
sections.push(`<diff>\n${diff}\n</diff>`);
179202
}
180203

181204
return sections.join("\n\n");

‎plugins/opencode/scripts/opencode-companion.mjs‎

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,14 @@ import { isOpencodeInstalled, getOpencodeVersion, spawnDetached, getConfiguredPr
4646
import { isServerRunning, ensureServer, createClient, connect } from "./lib/opencode-server.mjs";
4747
import { resolveWorkspace } from "./lib/workspace.mjs";
4848
import { loadState, updateState, upsertJob, generateJobId, jobDataPath } from "./lib/state.mjs";
49-
import { buildStatusSnapshot, resolveResultJob, resolveCancelableJob, enrichJob, reconcileAllJobs } from "./lib/job-control.mjs";
49+
import {
50+
buildStatusSnapshot,
51+
resolveResultJob,
52+
resolveCancelableJob,
53+
enrichJob,
54+
reconcileAllJobs,
55+
matchJobReference,
56+
} from "./lib/job-control.mjs";
5057
import { createJobRecord, runTrackedJob, getClaudeSessionId } from "./lib/tracked-jobs.mjs";
5158
import {
5259
renderStatus,
@@ -589,7 +596,11 @@ async function handleWorktreeCleanup(argv) {
589596

590597
const workspace = await resolveWorkspace();
591598
const state = loadState(workspace);
592-
const job = state.jobs?.find((j) => j.id === jobId || j.id.startsWith(jobId));
599+
const { job, ambiguous } = matchJobReference(state.jobs ?? [], jobId);
600+
if (ambiguous) {
601+
console.error("Ambiguous job reference. Please provide a more specific ID prefix.");
602+
process.exit(1);
603+
}
593604
if (!job) {
594605
console.error(`No job found for ${jobId}.`);
595606
process.exit(1);
@@ -631,24 +642,53 @@ async function handleWorktreeCleanup(argv) {
631642
async function handleTaskWorker(argv) {
632643
const { options } = parseArgs(argv, {
633644
valueOptions: ["job-id", "workspace", "task-text", "agent", "model", "resume-session"],
634-
booleanOptions: ["write"],
645+
booleanOptions: ["write", "worktree"],
635646
});
636647

637648
const workspace = options.workspace;
638649
const jobId = options["job-id"];
639650
const taskText = options["task-text"];
640651
const agentName = options.agent ?? "build";
641652
const isWrite = !!options.write;
653+
const useWorktree = Boolean(options.worktree);
642654
const resumeSessionId = options["resume-session"];
643655

644656
if (!workspace || !jobId || !taskText) {
645657
process.exit(1);
646658
}
647659

660+
let worktreeSession = null;
661+
let effectiveCwd = workspace;
662+
if (useWorktree) {
663+
try {
664+
worktreeSession = await createWorktreeSession(workspace);
665+
effectiveCwd = worktreeSession.worktreePath;
666+
upsertJob(workspace, {
667+
id: jobId,
668+
worktreeSession: {
669+
worktreePath: worktreeSession.worktreePath,
670+
branch: worktreeSession.branch,
671+
repoRoot: worktreeSession.repoRoot,
672+
baseCommit: worktreeSession.baseCommit,
673+
timestamp: worktreeSession.timestamp,
674+
},
675+
});
676+
} catch (err) {
677+
upsertJob(workspace, {
678+
id: jobId,
679+
status: "failed",
680+
phase: "failed",
681+
completedAt: new Date().toISOString(),
682+
errorMessage: `Failed to create worktree: ${err.message}`,
683+
});
684+
process.exit(1);
685+
}
686+
}
687+
648688
try {
649689
await runTrackedJob(workspace, { id: jobId }, async ({ report, log }) => {
650690
report("starting", "Background worker connecting to OpenCode...");
651-
const client = await connect({ cwd: workspace });
691+
const client = await connect({ cwd: effectiveCwd });
652692

653693
let sessionId;
654694
if (resumeSessionId) {
@@ -663,17 +703,48 @@ async function handleTaskWorker(argv) {
663703

664704
const prompt = buildTaskPrompt(taskText, { write: isWrite });
665705
report("investigating", "Running task...");
706+
log(`Agent: ${agentName}, Write: ${isWrite}, Worktree: ${worktreeSession?.branch ?? "no"}`);
666707

667708
const response = await client.sendPrompt(sessionId, prompt, {
668709
agent: agentName,
669710
});
670711

671712
const text = extractResponseText(response);
713+
714+
let worktreeDiff = null;
715+
if (worktreeSession) {
716+
try {
717+
worktreeDiff = await diffWorktreeSession(worktreeSession);
718+
} catch (err) {
719+
log(`Failed to compute worktree diff: ${err.message}`);
720+
}
721+
}
722+
672723
report("finalizing", "Done");
673724

674-
return { rendered: text, summary: text.slice(0, 500) };
725+
return {
726+
rendered: worktreeSession
727+
? renderWorktreeTaskOutput(text, worktreeSession, worktreeDiff, jobId)
728+
: text,
729+
summary: text.slice(0, 500),
730+
worktreeSession: worktreeSession
731+
? {
732+
worktreePath: worktreeSession.worktreePath,
733+
branch: worktreeSession.branch,
734+
repoRoot: worktreeSession.repoRoot,
735+
baseCommit: worktreeSession.baseCommit,
736+
}
737+
: null,
738+
};
675739
});
676740
} catch (err) {
741+
if (worktreeSession) {
742+
try {
743+
await cleanupWorktreeSession(worktreeSession, { keep: false });
744+
} catch {
745+
// best-effort
746+
}
747+
}
677748
// Error is already logged by runTrackedJob
678749
process.exit(1);
679750
}

‎tests/companion-cli.test.mjs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { describe, it, beforeEach, afterEach } from "node:test";
2+
import assert from "node:assert/strict";
3+
import fs from "node:fs";
4+
import path from "node:path";
5+
import { fileURLToPath } from "node:url";
6+
import { createTmpDir, cleanupTmpDir, setupTestEnv } from "./helpers.mjs";
7+
import { runCommand } from "../plugins/opencode/scripts/lib/process.mjs";
8+
import { saveState } from "../plugins/opencode/scripts/lib/state.mjs";
9+
10+
const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
11+
const companionScript = path.join(repoRoot, "plugins", "opencode", "scripts", "opencode-companion.mjs");
12+
13+
let tmpDir;
14+
let workspace;
15+
16+
beforeEach(() => {
17+
tmpDir = createTmpDir("companion-cli");
18+
workspace = path.join(tmpDir, "workspace");
19+
fs.mkdirSync(workspace);
20+
setupTestEnv(tmpDir);
21+
});
22+
23+
afterEach(() => {
24+
cleanupTmpDir(tmpDir);
25+
});
26+
27+
describe("opencode-companion CLI", () => {
28+
it("worktree-cleanup rejects ambiguous job ID prefixes", async () => {
29+
saveState(workspace, {
30+
config: {},
31+
jobs: [
32+
{
33+
id: "task-abc",
34+
type: "task",
35+
worktreeSession: {
36+
worktreePath: path.join(tmpDir, "wt-a"),
37+
branch: "opencode/a",
38+
repoRoot: workspace,
39+
},
40+
},
41+
{
42+
id: "task-abd",
43+
type: "task",
44+
worktreeSession: {
45+
worktreePath: path.join(tmpDir, "wt-b"),
46+
branch: "opencode/b",
47+
repoRoot: workspace,
48+
},
49+
},
50+
],
51+
});
52+
53+
const result = await runCommand(
54+
"node",
55+
[companionScript, "worktree-cleanup", "task-a", "--action", "discard"],
56+
{ cwd: workspace, env: { CLAUDE_PLUGIN_DATA: tmpDir } }
57+
);
58+
59+
assert.equal(result.exitCode, 1);
60+
assert.match(result.stderr, /Ambiguous job reference/);
61+
assert.equal(result.stdout, "");
62+
});
63+
});

0 commit comments

Comments
 (0)