Skip to content

Add OpenCode execute regression test#1903

Closed
Utkarshbhimte wants to merge 2 commits intopaperclipai:masterfrom
makerdock:fix/opencode-execute-regression
Closed

Add OpenCode execute regression test#1903
Utkarshbhimte wants to merge 2 commits intopaperclipai:masterfrom
makerdock:fix/opencode-execute-regression

Conversation

@Utkarshbhimte
Copy link
Copy Markdown

Summary

  • add a regression test for the OpenCode execute path when AGENT_HOME lives outside the project cwd
  • verify execute() passes a temporary runtime config with permission.external_directory=allow into the spawned OpenCode process
  • assert existing OpenCode config is preserved and the temporary config home is cleaned up after the run

Problem observed

In a real Paperclip heartbeat run, an opencode_local agent failed with repeated auto-rejections for external_directory reads against AGENT_HOME files like HEARTBEAT.md, SOUL.md, and TOOLS.md.

The failing shape was:

  • working directory under /paperclip/.../projects/.../_default
  • AGENT_HOME under /paperclip/.../workspaces/<agent-id>
  • headless OpenCode execution with Skip permissions enabled

That produced logs like:

  • permission requested: external_directory (...); auto-rejecting

Why this PR

The current master branch already contains the runtime-config behavior for opencode_local, but there was no execute-path regression test covering the exact failure mode above. This test locks in that behavior so the headless permission plumbing does not regress silently.

Verification

  • pnpm test:run server/src/__tests__/opencode-local-execute.test.ts
  • pnpm -r typecheck (fails on pre-existing unrelated UI Lexical issues on current upstream master)
  • pnpm test:run (fails on the same pre-existing UI Lexical module issues on current upstream master)
  • pnpm build (fails on the same pre-existing UI Lexical module issues on current upstream master)

This reproduces the headless external_directory failure seen when AGENT_HOME sits outside cwd and verifies that execute() injects a temporary runtime config with permission.external_directory=allow, preserves existing OpenCode config, and cleans up the temp config after the run.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds a single integration-level regression test for the opencode_local execute path, ensuring that when AGENT_HOME lives outside the project cwd, the spawned OpenCode process receives a temporary XDG_CONFIG_HOME with permission.external_directory=allow merged into it — the exact fix that was already shipped in packages/adapters/opencode-local/src/server/runtime-config.ts.\n\nKey points:\n- The test is well-structured: it creates a fake opencode binary, captures what the real execute() passes to it, and asserts the full contract: runtime config contents, XDG_CONFIG_HOME isolation, source-config preservation (theme, permission.read), commandNotes injection message, and temp-dir cleanup.\n- The fake script correctly handles the models subcommand so ensureOpenCodeModelConfiguredAndAvailable passes, and process.env.HOME is safely redirected to the temp root and restored in the finally block, keeping ~/.claude/skills writes out of the real home directory.\n- One minor issue: line 171 uses capture.xdgConfigHome ?? \"\" as the argument to fs.access, which would pass the cleanup assertion vacuously if xdgConfigHome were somehow null.\n- The PR description is missing a "thinking path" in the format described in CONTRIBUTING.md. The description has useful "Problem observed" and "Why this PR" sections, but CONTRIBUTING.md requires a top-down narrative starting from the product level ("Paperclip orchestrates AI agents…") down to the specific fix. Please add a thinking path so reviewers can quickly understand the context at a glance.

Confidence Score: 5/5

Safe to merge — the only change is a new test file with no production code impact.

The PR adds a single test file with no modifications to production code. The one flagged issue (the ?? "" null fallback) is a P2 style/clarity suggestion that does not affect correctness in the happy path. All remaining concerns are non-blocking.

No files require special attention beyond the single minor suggestion on line 171 of the new test file.

Important Files Changed

Filename Overview
server/src/tests/opencode-local-execute.test.ts New regression test for the opencode_local execute path; correctly verifies external_directory permission injection, source-config preservation, XDG_CONFIG_HOME isolation, and temp-dir cleanup. Minor concern: the ?? "" fallback on xdgConfigHome could silently mask a null value in the cleanup assertion.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: server/src/__tests__/opencode-local-execute.test.ts
Line: 171

Comment:
**Null fallback masks assertion intent**

If `capture.xdgConfigHome` is `null` (e.g., because the fake script ran without a set `XDG_CONFIG_HOME`), the expression resolves to `fs.access("")`, which throws `ENOENT` for the wrong reason — making the assertion pass vacuously.

Consider explicitly asserting that `xdgConfigHome` is a non-null string first, and then checking that the path no longer exists:

```suggestion
      expect(capture.xdgConfigHome).not.toBeNull();
      await expect(fs.access(capture.xdgConfigHome!)).rejects.toThrow();
```
This separates the "was a temp dir actually created?" check from the "was it cleaned up?" check, giving a clearer failure message if either invariant is violated.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Add OpenCode execute regression test" | Re-trigger Greptile

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant