fix(opencode): harden opencode plugin against abort, orphan, and arg edge cases - #728
Conversation
…edge cases Address the six still-open findings from alibaba#702 in the opencode plugin: - M1: wrap the telemetry app.log call so a failing log service no longer blocks tool registration - M2: guard context.abort.aborted for hosts that omit an abort signal - M3: reject resume combined with commit or a from/to range - M4: spawn OCR in its own process group and kill the group on timeout or cancel so git/LLM grandchildren cannot orphan - L1: treat empty stdout as no changes instead of misleading invalid JSON - L2: drop the dead ReviewInput.repo field and pass cwd as the repo arg directly Adds regression tests for each fix, including a process-group test that asserts grandchildren are reaped on cancellation.
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| test("plugin still registers tools when the telemetry log call fails", async () => { | ||
| const hooks = await OpenCodeReviewPlugin({ | ||
| client: { | ||
| app: { | ||
| log: async () => { | ||
| throw new Error("log service unavailable") | ||
| }, | ||
| }, | ||
| }, | ||
| worktree: "/tmp/project", | ||
| }) | ||
| assert.deepEqual(Object.keys(hooks.tool).sort(), ["ocr_health", "ocr_review"]) | ||
| }) |
There was a problem hiding this comment.
[test · medium]
The test asserts that tools are registered when the telemetry log call fails, but it doesn't verify that the error was properly handled or that no unhandled promise rejection occurred. If the production code only catches the error synchronously but the log call is fire-and-forget async, the test could pass while an unhandled rejection still leaks. Consider adding an assertion that verifies the error was caught, such as checking that the promise resolved without throwing or listening for unhandled rejections during the test.
| test("ocr_review reports no output instead of invalid JSON when OCR prints nothing", async () => { | ||
| await withFakeOcr("", async (worktree) => { | ||
| const { hooks } = await loadPlugin(worktree) | ||
| const output = await hooks.tool.ocr_review.execute({}, toolContext(worktree)) | ||
| assert.match(output, /No changes detected/) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
[test · low]
The test verifies that when OCR produces no output, the result matches /No changes detected/, but it doesn't explicitly assert that the output does NOT contain a JSON parse error message (which was the previous behavior being fixed). Consider adding a negative assertion to ensure the error message about invalid JSON is not present, making the test more robust against regressions.
Suggestion:
| test("ocr_review reports no output instead of invalid JSON when OCR prints nothing", async () => { | |
| await withFakeOcr("", async (worktree) => { | |
| const { hooks } = await loadPlugin(worktree) | |
| const output = await hooks.tool.ocr_review.execute({}, toolContext(worktree)) | |
| assert.match(output, /No changes detected/) | |
| }) | |
| }) | |
| test("ocr_review reports no output instead of invalid JSON when OCR prints nothing", async () => { | |
| await withFakeOcr("", async (worktree) => { | |
| const { hooks } = await loadPlugin(worktree) | |
| const output = await hooks.tool.ocr_review.execute({}, toolContext(worktree)) | |
| assert.match(output, /No changes detected/) | |
| assert.doesNotMatch(output, /invalid JSON/i) | |
| }) | |
| }) |
| test("ocr_health works when no abort signal is provided", async () => { | ||
| await withFakeOcr( | ||
| [ | ||
| "if (process.argv[2] === 'version') {", | ||
| " console.log('OpenCodeReview 1.2.3')", | ||
| "} else {", | ||
| " console.error('missing LLM credentials')", | ||
| " process.exitCode = 7", | ||
| "}", | ||
| ].join("\n"), | ||
| async (worktree) => { | ||
| const { hooks } = await loadPlugin(worktree) | ||
| const output = await hooks.tool.ocr_health.execute( | ||
| {}, | ||
| { worktree, directory: worktree }, | ||
| ) | ||
| assert.match(output, /OpenCodeReview 1\.2\.3/) | ||
| assert.match(output, /LLM connection check failed: missing LLM credentials/) | ||
| }, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
[test · medium]
This test covers the no-abort-signal scenario for ocr_health, but the same scenario is not tested for ocr_review, which has more complex cancellation logic involving process group killing. If ocr_review also accesses abortSignal without a guard, it could crash in production when called without a signal. Consider adding a similar test for ocr_review to ensure both tools handle the absence of an abort signal gracefully.
2 participants
Fixes #702 (the six findings not covered by #717).
Implements M1, M2, M3, M4, L1, L2 from the issue:
H1 is intentionally left to #717.
Tests: ran
npm run checkin plugins/open-code-review/opencode, all 21 tests pass. New tests cover each fix; the process-group test fails without the detached/kill-group change.