Skip to content

Commit a3c79b4

Browse files
committed
fix(opencode): address final-review findings
Follow-ups from the branch-level code review. All low-risk fixes that came up during verification/review of the otherwise-ready feature. - RemoteHost.launch_opencode: read the opencode server password from a mode-0600 file in the workdir via `$(cat ...)` rather than inlining it into the command string. Previously any local user on the remote host could read the basic-auth password from the bash process's argv via `ps aux` during the launch window. Env variables (asyncssh create_process env=) would be cleaner but OpenSSH's default AcceptEnv won't forward arbitrary names, so a mode-0600 file the command substitutes at exec time is the portable fix. - RemoteHost.terminate_opencode(aggressive=True): use proc.kill() (SIGKILL) instead of proc.terminate() (SIGTERM). The spec's cancellation path expects SIGKILL; with SIGTERM an opencode that blocks on shutdown would eat the 5-second shutdown grace period. LocalHost was already correct. - LocalHost.tail_log / RemoteHost.tail_log: document why we use `tail -F -n +1` instead of the spec's `-n 0`. Fresh workdir + race- free at-least-once delivery (an earlier `-n 0` attempt silently dropped log lines that opencode wrote between subprocess spawn and our tail subscribing). - Remove unused `aiofiles>=23.0` dependency (the module docstring mentioned it but the implementation spawns a `tail` subprocess; aiofiles was never imported). Update host.py's module docstring. - AGENTS.md: add a "Where the fork lives" paragraph pointing at github.com/csillag/opencode #csillag/make-web-embeddable-in-iframes, the upstream PR anomalyco/opencode#23912, and the build command so someone picking up this branch cold can populate OPTIO_OPENCODE_BINARY_DIR without hunting for the right repo. Deferred review items (no change in this commit): remote-mode cancellation test (spec Section 9 coverage gap), routing install stderr through ctx.report_progress (nice-to-have), cosmetic file-touch idiom, demo-task inner.execute clarification, SFTP-over- SSH retry path not exercised by tests, negative-cache comment consolidation.
1 parent d0e22fa commit a3c79b4

3 files changed

Lines changed: 45 additions & 9 deletions

File tree

packages/optio-opencode/AGENTS.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,17 @@ This is a bridge feature for shipping an iframe-embeddability fork of
8888
opencode until those fixes land upstream; once upstream ships, the env
8989
var's only remaining use is pinning to a specific build.
9090

91+
**Where the fork lives:** the patched binaries are built from
92+
<https://github.com/csillag/opencode> on branch
93+
`csillag/make-web-embeddable-in-iframes` — three small orthogonal
94+
patches (relative vite base, CSP `'unsafe-eval'`, `getCurrentUrl`
95+
honoring localStorage). Upstream PR:
96+
<https://github.com/anomalyco/opencode/pull/23912>. To populate
97+
`OPTIO_OPENCODE_BINARY_DIR`: check out that branch, run
98+
`bun install && bun run --cwd packages/opencode build`, and point the
99+
env var at the resulting `packages/opencode/dist/` directory (which
100+
contains per-target subdirs such as `opencode-linux-x64/bin/opencode`).
101+
91102
## Testing
92103

93104
Unit + local integration: `pytest tests/` (needs MongoDB via Docker).

packages/optio-opencode/pyproject.toml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ requires-python = ">=3.11"
1111
dependencies = [
1212
"optio-core",
1313
"asyncssh>=2.14",
14-
"aiofiles>=23.0",
1514
]
1615

1716
[project.optional-dependencies]

packages/optio-opencode/src/optio_opencode/host.py

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
33
Two concrete implementations:
44
5-
* ``LocalHost`` — the optio worker itself. Uses asyncio subprocess + aiofiles.
5+
* ``LocalHost`` — the optio worker itself. Uses asyncio subprocess.
66
* ``RemoteHost`` — a remote machine reachable over SSH. Uses asyncssh,
77
multiplexing command exec + SFTP + local port forwarding over a single
88
connection.
@@ -278,6 +278,14 @@ async def establish_tunnel(self, opencode_port: int) -> int:
278278

279279
async def tail_log(self) -> AsyncIterator[str]:
280280
log_path = os.path.join(self.workdir, "optio.log")
281+
# `-n +1` reads from line 1, not EOF. The design spec suggested
282+
# `-n 0` (EOF) to avoid reprocessing a truncated earlier run, but
283+
# our workdir is always fresh (setup_workdir creates the log
284+
# empty just before this) so there's nothing to reprocess, and
285+
# `-n 0` has a race: lines written to the log before tail
286+
# subscribes (e.g. opencode eagerly appending STATUS right after
287+
# launch) are silently skipped. `-n +1` + always-empty initial
288+
# log gives us at-least-once delivery.
281289
self._tail_proc = await asyncio.create_subprocess_exec(
282290
"tail", "-F", "-n", "+1", log_path,
283291
stdout=asyncio.subprocess.PIPE,
@@ -495,17 +503,30 @@ async def launch_opencode(
495503
ready_timeout_s: float,
496504
extra_args: list[str] | None = None,
497505
) -> LaunchedProcess:
498-
assert self._conn is not None
506+
assert self._conn is not None and self._sftp is not None
507+
# Write the password to a mode-0600 file in the workdir and have
508+
# the remote shell read it via command substitution at launch
509+
# time. Interpolating the literal password into the command
510+
# string would leak it to any local user on the remote host who
511+
# runs `ps` — the bash process executing our command has the full
512+
# command line (including the password) in its argv. With `$(cat
513+
# .opencode-password)`, the command line only shows the expansion
514+
# syntax, not the value; the file itself is readable only by our
515+
# user. The file is swept with the rest of the workdir on
516+
# teardown.
517+
pw_file = ".opencode-password"
518+
await self.write_text(pw_file, password)
519+
await self._conn.run(f"chmod 600 {self.workdir}/{pw_file}", check=True)
499520
cmd = (
500521
f"cd {self.workdir} && "
501-
f"OPENCODE_SERVER_PASSWORD={password} "
522+
f"OPENCODE_SERVER_PASSWORD=\"$(cat ./{pw_file})\" "
502523
f"BROWSER=true "
503524
# 2>&1 merges stderr into stdout because opencode prints the
504525
# "Web interface:" URL line via UI.println → stderr.
505-
# Single-quote the command. `$opencode` is safely substituted
506-
# here (Python f-string) before the remote shell sees anything;
507-
# the remote bash just sees the absolute path or the literal
508-
# word "opencode".
526+
# Single-quote the inner command. `$opencode` is safely
527+
# substituted here (Python f-string) before the remote shell
528+
# sees anything; the remote bash just sees the absolute path
529+
# or the literal word "opencode".
509530
f"bash -lc '{self._opencode_exec} web --port=0 --hostname=127.0.0.1 2>&1'"
510531
)
511532
self._launch_proc = await self._conn.create_process(cmd)
@@ -543,6 +564,8 @@ async def establish_tunnel(self, opencode_port: int) -> int:
543564
async def tail_log(self) -> AsyncIterator[str]:
544565
assert self._conn is not None
545566
log_path = f"{self.workdir}/optio.log"
567+
# See LocalHost.tail_log for why `-n +1` rather than the spec's
568+
# `-n 0`: fresh workdir + race-free at-least-once delivery.
546569
self._tail_proc = await self._conn.create_process(
547570
f"tail -F -n +1 {log_path}"
548571
)
@@ -564,7 +587,10 @@ async def terminate_opencode(
564587
if proc.returncode is not None:
565588
return
566589
if aggressive:
567-
proc.terminate()
590+
# Spec requires SIGKILL in the cancellation path — .terminate()
591+
# sends SIGTERM, which opencode may handle and block on, blowing
592+
# our shutdown grace budget.
593+
proc.kill()
568594
# Best-effort: do not wait.
569595
return
570596
proc.terminate()

0 commit comments

Comments
 (0)