fix(131): address P1 review findings (tool-call, Dockerfile, fly.toml, CI)

[andreiships-bot]

Summary

Retroactive fixes for P1 findings from post-merge review of PRs #1, #2, #3.

• tool-call.ts: Agent.defaultAgent() returns a name string, not Agent.Info — now resolved with Agent.get() before passing to ToolRegistry.tools(); agent.id on a string is undefined, fixed to pass agentName; use agent's configured model instead of hardcoded opencode/default
• Dockerfile: Quote $(find ...) + add existence check; remove irrelevant BUN_RUNTIME_TRANSPILER_CACHE_PATH (compiled binary doesn't use Bun's transpiler)
• fly.toml: min_machines_running: 0 → 1 to avoid cold starts on interactive sessions
• smoke test: Replace hardcoded /tmp/ paths with mktemp tmpdir + EXIT trap for parallel-CI safety
• build-push.yml: Gate :latest on stable tags only; add provenance: false; explicit platforms: linux/amd64

Test coverage

The tool-call.ts bugs were type-level, not behavioral:

• No tool reads Tool.Context.agent, so agent: undefined causes no runtime failure
• ToolRegistry.tools() receiving a string instead of Agent.Info also fails silently at runtime

Existing coverage that protects against regressions:

• typecheck.yml — catches the Agent.Info/string type mismatch (would have blocked the original PRs if they had gone through PR flow)
• packages/opencode/test/server/tool-call.test.ts — 4 integration tests covering 404, 400, unknown tool, and successful glob execution (added in PR [131-1] feat: add POST /session/:id/tool/call direct tool execution endpoint #1)

No new tests added — the existing test file is sufficient and bun typecheck is the right guard for this category of type-correctness fix.

Related

Closes findings from retroactive review:

• PR [131-1] feat: add POST /session/:id/tool/call direct tool execution endpoint #1 inline comments: FINDING-1 (Agent.Info type mismatch), FINDING-2 (unused session model)
• PR [131-2] feat: Dockerfile (Bun), fly.toml, and integration smoke test #2 inline comments: FINDING-1 (unquoted find), FINDING-2 (BUN env var), FINDING-3 (cold starts), FINDING-4 (tmp collisions)
• PR [131-3] feat: CI pipeline — build and push opencode Docker image on tag #3 inline comments: FINDING-1 (:latest on pre-releases), FINDING-2 (provenance), FINDING-3 (platforms)

Test plan

• bun typecheck passes (catches the Agent.Info fix)
• bun test passes in packages/opencode (existing 4 tests)
• Tag a v0.0.1-rc1 → verify :latest is NOT pushed
• Tag a v0.0.1 → verify :latest IS pushed

[github-actions]

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

1. Open an issue describing the bug/feature (if one doesn't exist)
2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

[andreiships-bot]

Claude Single-Pass Review

Summary

This PR correctly and cleanly addresses all five P1 findings from the post-merge retroactive review. All five fixes are technically sound — the tool-call type correction, Dockerfile hardening, fly.toml cold-start fix, smoke test tmpdir isolation, and CI :latest gating are all correct.

Findings

[FINDING-1] nit: P2 | .github/workflows/build-push.yml:37 | When latest_tag output is empty (pre-release tags), the tags: block renders with a blank line: registry.fly.io/...:v0.1.0-rc1\n\n. This is harmless — docker/build-push-action ignores blank entries — but it is worth noting the behavior is intentional and tested per the PR test plan.

Code Quality

• Correctness: Agent.defaultAgent() → Agent.get() chain correctly resolves string → Agent.Info; ToolRegistry.tools() signature is agent?: Agent.Info so passing undefined on a missing agent is safe
• Correctness: agent: agentName (string) replaces agent: agent.id (was undefined on a string value) — fixed
• Correctness: Unused session variable removed cleanly
• Correctness: modelCtx falls back to { providerID: "opencode", modelID: "default" } when agent has no configured model — matches the original hardcoded value, correct
• Dockerfile: Binary-not-found now exits with a clear error message; quoting prevents word-splitting on paths with spaces
• Dockerfile: BUN_RUNTIME_TRANSPILER_CACHE_PATH removal is correct — compiled binary doesn't use Bun runtime transpiler
• fly.toml: min_machines_running: 1 prevents cold starts — appropriate for interactive sessions requiring low latency
• Smoke test: mktemp -d + trap ... EXIT eliminates /tmp/ collision risk in parallel CI runs
• Smoke test: grep used directly instead of cat | grep — correct UUOC elimination
• CI: :latest promotion correctly gated on [[ "$tag" != *-* ]] — handles -alpha, -rc1, -beta patterns
• CI: provenance: false + explicit platforms: linux/amd64 correct for Fly.io registry compatibility
• Tests: No new tests added — justified; type bugs are caught by bun typecheck, behavioral coverage exists in tool-call.test.ts

Recommendation

[x] Approve | All P1 findings from the retroactive review are properly resolved. The single nit (blank line in tags block) is cosmetic and harmless.

[andreiships-bot]

Claude Review

See inline comments for details.

[andreiships-bot]

Claude Single-Pass Review

Summary

This PR is a well-scoped retroactive fix addressing P1 findings from post-merge reviews of PRs #1-3, plus a CI runner migration from Blacksmith to Ubicloud. The substantive logic fixes in tool-call.ts, Dockerfile, fly.toml, and the smoke test are all correct and improve reliability. No new issues introduced.

Findings

[FINDING-1] nit: P2 | packages/opencode/src/server/routes/tool-call.ts:491 | Agent.get() returns Info | undefined (lookup by key in a Record), so agentInfo can be undefined. The optional chain agentInfo?.model correctly handles the model fallback, and ToolRegistry.tools(modelCtx, agentInfo) receives undefined when the agent name is not in the registry. If ToolRegistry.tools has a non-null assertion on its second argument this silently degrades. The PR author acknowledged this is currently safe at runtime, but a defensive check if (!agentInfo) throw new NotFoundError(...) would make the failure explicit.

[FINDING-2] nit: P2 | .github/workflows/build-push.yml:50 | When steps.version.outputs.latest_tag is empty (pre-release tag), the tags block becomes a two-line string where the second line is a blank value. Docker Build Push action trims blank tags, so this works, but it's worth noting the implicit dependency on that trimming behavior. An explicit if: condition on the :latest tag push step would be clearer than relying on blank-line filtering in the multi-line string.

[FINDING-3] nit: P2 | .github/workflows/publish.yml:287 | Windows target now uses windows-latest (GitHub-hosted) instead of blacksmith-4vcpu-windows-2025. This means Windows CI builds will accrue GitHub Actions minutes. The commit message acknowledges "no Ubicloud Windows runner" — this is correct and acceptable, just noting the billing implication is not documented in the PR description.

Code Quality

• Correctness verified: tool-call.ts type fixes are sound — Agent.defaultAgent() returns string, Agent.get(string) returns Info | undefined, optional chaining handles the undefined case
• Dockerfile: set -e + quoted $binary + existence check is a strict improvement over the unquoted $(find ...) in the cp argument
• fly.toml min_machines_running: 1 is appropriate for interactive sessions that should not cold-start
• Smoke test: mktemp -d + trap 'rm -rf' EXIT pattern is correct for parallel-CI-safe temp isolation; cat /tmp/... | grep → grep file useless-use-of-cat cleanup is a bonus improvement
• CI runner migration: all blacksmith-* labels correctly mapped to ubicloud-standard-* equivalents per the project's Ubicloud Runners rule
• nit: Agent.get() return type is Info | undefined — passing undefined to ToolRegistry.tools() as the agent argument is currently safe but not explicitly guarded

Recommendation

[x] Approve with changes — the P2 nits above are optional hardening; the core fixes are correct and CI is green. Safe to merge.

[andreiships-bot]

Claude Review - Out-of-Diff Findings

The following findings are on lines outside the PR diff:

packages/opencode/src/server/routes/tool-call.ts:491
[FINDING-1] nit: P2 | Agent.get() returns Info | undefined (lookup by key in a Record), so agentInfo can be undefined. The optional chain agentInfo?.model correctly handles the model fallback, and ToolRegistry.tools(modelCtx, agentInfo) receives undefined when the agent name is not in the registry. If ToolRegistry.tools has a non-null assertion on its second argument this silently degrades. The PR author acknowledged this is currently safe at runtime, but a defensive check if (!agentInfo) throw new NotFoundError(...) would make the failure explicit.

.github/workflows/build-push.yml:50
[FINDING-2] nit: P2 | When steps.version.outputs.latest_tag is empty (pre-release tag), the tags block becomes a two-line string where the second line is a blank value. Docker Build Push action trims blank tags, so this works, but it's worth noting the implicit dependency on that trimming behavior. An explicit if: condition on the :latest tag push step would be clearer than relying on blank-line filtering in the multi-line string.

.github/workflows/publish.yml:287
[FINDING-3] nit: P2 | Windows target now uses windows-latest (GitHub-hosted) instead of blacksmith-4vcpu-windows-2025. This means Windows CI builds will accrue GitHub Actions minutes. The commit message acknowledges "no Ubicloud Windows runner" — this is correct and acceptable, just noting the billing implication is not documented in the PR description.

[andreiships-bot]

Gemini Deep Review

Summary

The PR successfully addresses several P1 review findings across CI workflows, Docker configuration, and the direct tool-call endpoint. Key improvements include the transition to Ubicloud runners, enhanced Docker binary detection, and fixing a type mismatch in the tool execution route.

Findings

[gemini-1] nit: P2 | .github/workflows/build-push.yml:32 | Bash-ism in CI script
The if [[ "$tag" != *-* ]]; then block uses [[ which is a non-POSIX bash extension. While GitHub Actions defaults to bash on Linux, using [ is more portable and consistent with the project's preference for POSIX shell scripts where possible.
Fix: Change [[ to [ and != to != (or use a portable string comparison). Actually, [[ is fine if bash is guaranteed, but [ is safer. Given the context, this is a minor nit.

[gemini-2] issue: P1 | packages/opencode/src/server/routes/tool-call.ts:64 | Potential undefined agentInfo
The code calls Agent.get(agentName) which could return undefined. While the modelCtx handles this via optional chaining (agentInfo?.model ?? ...), agentInfo is passed as the second argument to ToolRegistry.tools(modelCtx, agentInfo).
Verified via diff: const tools = await ToolRegistry.tools(modelCtx, agentInfo)
Fix: Ensure ToolRegistry.tools gracefully handles an undefined agent, or add a check if a valid agent is strictly required for tool resolution.

[gemini-3] nit: P2 | scripts/ci/test-opencode-integration.sh:78 | Fragile grep for SESSION_ID
The extraction of SESSION_ID uses grep -o '"id":"[^"]*"' ... | cut -d'"' -f4. This assumes a specific JSON format and may be fragile if the response contains other "id" fields or different spacing.
Fix: Use jq -r '.id' for robust JSON parsing, consistent with other parts of the codebase.

PR Metadata

Suggested PR Title: fix(opencode): address P1 findings for tool-call, Docker, and CI
Suggested Description Update: Added specific mention that the tool-call route was fixed to correctly resolve agent identity before fetching tools, addressing a type mismatch.

Questions

• Does the POST /session/:id/tool/call endpoint in the opencode server have authentication middleware applied? Since it allows direct tool execution (including shell and filesystem access), it must be protected by the OPENCODE_SERVER_PASSWORD as specified in Spec 132.

Recommendation

[ ] Approve | [X] Approve with changes | [ ] Request changes