Opencode-ify#8
Conversation
Run the full Worker → EngineActor → SocketActor pipeline locally in workerd via Miniflare. Tests cover the HTTP entrypoint, engine.io open packet, DO storage persistence, and AlarmTimer heartbeat flush. - demo-server/vitest.config.mts: cloudflareTest plugin reading wrangler.toml - demo-server/test/worker-entry.spec.ts: 4 HTTP entrypoint tests - demo-server/test/durable-objects.spec.ts: 4 DO state + alarm tests - demo-server/test/tsconfig.json: cloudflare:test workers types - demo-server/package.json: test / test:watch scripts, new devDeps - AGENTS.md: document the test setup and available APIs
Checkout submodules, fetch the socket.io@4.8.1 tag so make patch-upstream resolves, build the library, then run pnpm run --filter ./demo-server test. Also fix the pnpm filter in AGENTS.md (--filter ./demo-server by directory, not by package name).
📝 WalkthroughWalkthroughAdds repository development guidance, Cloudflare Worker integration tests, CI validation, upstream socket.io bundling workarounds, and a Terraform deployment migration reference. ChangesUpstream compatibility and build configuration
Integration testing and continuous integration
Repository development guidance
Terraform deployment migration reference
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
AGENTS.md (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifier to fenced code block.
Fenced code blocks should have a language specified to ensure proper rendering and to resolve the
markdownlintwarning.♻️ Proposed refactor
-``` +```text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 7, Update the fenced code block in AGENTS.md to include an explicit text language identifier, using ```text instead of an untyped fence, while preserving the block’s existing contents.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 9-14: Update the test job workflow to declare read-only GitHub
token permissions and configure the actions/checkout@v4 step with credential
persistence disabled. Keep the existing job and checkout behavior otherwise
unchanged.
In @.opencode/commands/patch-upstream.md:
- Around line 7-8: Update the patch-upstream instructions to fetch the pinned
socket.io@4.8.1 upstream ref after initializing the submodule and before running
make patch-upstream, using the same fetch command or step already used by CI.
In `@demo-server/test/durable-objects.spec.ts`:
- Around line 79-93: Add a hibernation-recovery test in the durable-object test
suite using evictDurableObject, alongside the existing runInDurableObject and
runDurableObjectAlarm coverage. Establish or inspect state through the
EngineActor stub, evict the Durable Object, then invoke it again and assert the
persisted state is restored successfully; import evictDurableObject from
cloudflare:test and preserve socket cleanup.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 7: Update the fenced code block in AGENTS.md to include an explicit text
language identifier, using ```text instead of an untyped fence, while preserving
the block’s existing contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02e161d-4aaa-4039-b8e2-1b0895a8fa3b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.github/workflows/test.yaml.opencode/commands/build.md.opencode/commands/check.md.opencode/commands/demo-node.md.opencode/commands/format.md.opencode/commands/lint.md.opencode/commands/patch-upstream.md.opencode/commands/typecheck.mdAGENTS.mddemo-server/package.jsondemo-server/test/durable-objects.spec.tsdemo-server/test/tsconfig.jsondemo-server/test/worker-entry.spec.tsdemo-server/vitest.config.mtsmise.tomlopencode.json
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Apply least-privilege permissions and avoid credential persistence.
The workflow currently uses the default permissions, which can grant unnecessary write access to the repository depending on organization settings. Additionally, actions/checkout persists the default GitHub token in the local git config, which could be exposed if later steps execute compromised code.
Please restrict the workflow permissions to read-only and disable credential persistence in the checkout step.
🔒 Proposed security improvements
jobs:
test:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
timeout-minutes: 10
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false
# init submodule (pinned to the socket.io@4.8.1 tag commit) and fetch the📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: | |
| - uses: actions/checkout@v4 | |
| jobs: | |
| test: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| timeout-minutes: 10 | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yaml around lines 9 - 14, Update the test job
workflow to declare read-only GitHub token permissions and configure the
actions/checkout@v4 step with credential persistence disabled. Keep the existing
job and checkout behavior otherwise unchanged.
Source: Linters/SAST tools
| 1. If not already initialized: `git submodule update --init` | ||
| 2. Apply patches: `make patch-upstream` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fetch the pinned upstream ref before applying patches.
On a fresh checkout, git submodule update --init does not guarantee that the local socket.io@4.8.1 ref exists, but make patch-upstream resets to that ref. Add the fetch step used by CI before invoking the Make target.
Proposed documentation update
1. If not already initialized: `git submodule update --init`
-2. Apply patches: `make patch-upstream`
+2. Fetch the pinned ref: `git -C socket.io fetch --depth 1 origin socket.io@4.8.1`
+3. Apply patches: `make patch-upstream`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. If not already initialized: `git submodule update --init` | |
| 2. Apply patches: `make patch-upstream` | |
| 1. If not already initialized: `git submodule update --init` | |
| 2. Fetch the pinned ref: `git -C socket.io fetch --depth 1 origin socket.io@4.8.1` | |
| 3. Apply patches: `make patch-upstream` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.opencode/commands/patch-upstream.md around lines 7 - 8, Update the
patch-upstream instructions to fetch the pinned socket.io@4.8.1 upstream ref
after initializing the submodule and before running make patch-upstream, using
the same fetch command or step already used by CI.
| it('runs the EngineActor alarm without throwing', async () => { | ||
| const { ws } = await openSocketIoConnection() | ||
| try { | ||
| await new Promise((r) => setTimeout(r, 250)) | ||
| const id = env.engineActor.idFromName('singleton') | ||
| const stub = env.engineActor.get(id) | ||
| // runDurableObjectAlarm returns true if an alarm was scheduled and ran. | ||
| // AlarmTimer schedules a 30s alarm on each new connection; this flushes it. | ||
| const ran = await runDurableObjectAlarm(stub) | ||
| expect(typeof ran).toBe('boolean') | ||
| } finally { | ||
| ws.close() | ||
| } | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add tests for hibernation recovery using evictDurableObject.
The current test suite exercises runInDurableObject and runDurableObjectAlarm, but does not test hibernation recovery. As per coding guidelines, you must use Cloudflare test APIs such as runInDurableObject, runDurableObjectAlarm, and evictDurableObject to inspect storage, trigger alarms, and test hibernation recovery.
Consider importing evictDurableObject from cloudflare:test and adding a test case that verifies state is successfully restored when the Durable Object wakes up from hibernation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@demo-server/test/durable-objects.spec.ts` around lines 79 - 93, Add a
hibernation-recovery test in the durable-object test suite using
evictDurableObject, alongside the existing runInDurableObject and
runDurableObjectAlarm coverage. Establish or inspect state through the
EngineActor stub, evict the Durable Object, then invoke it again and assert the
persisted state is restored successfully; import evictDurableObject from
cloudflare:test and preserve socket cleanup.
Source: Coding guidelines
Rename test.yaml -> check.yaml. The job now runs: - make patch-upstream lib-build - dprint check on socket.io-serverless - tsc --noEmit on library (non-blocking, pre-existing upstream errors) - wrangler deploy --dry-run on demo-server - vite build on demo-client - vitest integration tests on demo-server Also apply dprint fmt to socket.io-serverless/build.mjs (target: "es2023" -> 'es2023') so 'dprint check' passes clean.
Update the git submodule pin, regenerate the patch against the new tag, and adjust make patch-upstream to fetch the tag before resetting. Notable upstream change: socket.io/lib/client.ts already uses 'import x from "..."' syntax, so that hunk is no longer needed in the patch. The remaining patch hunks (removing export maps from package.json files and stripping module.exports from index.ts) still apply cleanly. All 8 integration tests pass against 4.8.3.
Add docs/terraform-migration.md explaining the concrete differences between wrangler and terraform, a full wrangler.toml -> HCL field mapping, required repo changes (Makefile, package.json, CI), local-dev options, DO migration semantics, and a ready-to-adapt main.tf.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/check.yaml (1)
10-14: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply least-privilege permissions and avoid credential persistence.
The workflow currently uses the default permissions, which can grant unnecessary write access to the repository depending on organization settings. Additionally,
actions/checkoutpersists the default GitHub token in the local git config, which could be exposed if later steps execute compromised code.Please restrict the workflow permissions to read-only and disable credential persistence in the checkout step.
🔒 Proposed security improvements
check: runs-on: ubuntu-latest + permissions: + contents: read timeout-minutes: 10 steps: - uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check.yaml around lines 10 - 14, Restrict the workflow’s top-level GITHUB_TOKEN permissions to read-only, and update the actions/checkout@v4 step to disable credential persistence. Keep the existing runner, timeout, and checkout behavior unchanged.Source: Linters/SAST tools
🧹 Nitpick comments (1)
docs/terraform-migration.md (1)
76-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRebase the example on the currently recommended Workers resources.
The current provider documentation still supports
cloudflare_workers_script, but recommendscloudflare_worker,cloudflare_worker_version, andcloudflare_workers_deploymentfor direct control. Either update this future migration guide to those resources or explicitly pin and justify the legacy resource choice. (registry.terraform.io)Also applies to: 238-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/terraform-migration.md` around lines 76 - 78, The migration guide’s Terraform example references the legacy cloudflare_workers_script resource without justification. Update the documented demo-server/infra/main.tf migration target to use cloudflare_worker, cloudflare_worker_version, and cloudflare_workers_deployment, including the related provider and variable guidance, or explicitly pin the provider version and explain why the legacy resource remains required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/terraform-migration.md`:
- Line 34: Update the fenced code block containing the PUT request in the
migration documentation to include an appropriate language identifier,
preferably http or text, while preserving its request content.
- Around line 111-120: Update the `deploy:cf` and `build:cf` entries in the
`demo-server/package.json` migration table to invoke the repository-root
Makefile from the `demo-server` working directory, using `make -C .. tf-apply`
and `make -C .. tf-plan` (or an equivalent explicit directory change). Keep the
existing `dev:cf` command and bundling clarification unchanged.
- Around line 61-63: Update the Terraform migration guide’s `dist/cf.js`
references, including the table entry and Makefile guidance, to consistently use
`../../socket.io-serverless/dist/cf.js` from `demo-server/infra`. Remove the
incorrect `${path.module}/dist/cf.js` and
`${path.module}/../socket.io-serverless/dist/cf.js` paths while preserving the
existing build instructions.
- Around line 79-81: Update the Terraform environment-variable guidance in the
migration documentation to use the correctly spelled Cloudflare account variable
and the Terraform-compatible TF_VAR_account_id naming for the account_id input,
while keeping the API token guidance accurate.
---
Duplicate comments:
In @.github/workflows/check.yaml:
- Around line 10-14: Restrict the workflow’s top-level GITHUB_TOKEN permissions
to read-only, and update the actions/checkout@v4 step to disable credential
persistence. Keep the existing runner, timeout, and checkout behavior unchanged.
---
Nitpick comments:
In `@docs/terraform-migration.md`:
- Around line 76-78: The migration guide’s Terraform example references the
legacy cloudflare_workers_script resource without justification. Update the
documented demo-server/infra/main.tf migration target to use cloudflare_worker,
cloudflare_worker_version, and cloudflare_workers_deployment, including the
related provider and variable guidance, or explicitly pin the provider version
and explain why the legacy resource remains required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1996dcc7-a31b-412b-94ee-b31735a0222f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.github/workflows/check.yamlAGENTS.mdMakefiledocs/terraform-migration.mdpatches/0001-workarounds-to-upstream-socket.io.patchsocket.iosocket.io-serverless/build.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
| Both `wrangler deploy` and `terraform apply` end at the same Cloudflare REST | ||
| endpoint: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced code block.
This triggers markdownlint MD040. Use ```http or ```text.
Proposed fix
-```
+```http
PUT /accounts/{account_id}/workers/scripts/{script_name}🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/terraform-migration.md` at line 34, Update the fenced code block
containing the PUT request in the migration documentation to include an
appropriate language identifier, preferably http or text, while preserving its
request content.
Source: Linters/SAST tools
| | `name = "sio-serverless-demo"` | `script_name = "sio-serverless-demo"` | | | ||
| | `main = "src/cf/main.ts"` | `main_module = "cf.js"` + `content = file("${path.module}/dist/cf.js")` | TF does not bundle; you point `content` at the **already-built** `dist/cf.js` from `make lib-build`. | | ||
| | `compatibility_date = "2024-08-21"` | `compatibility_date = "2024-08-21"` | Same value. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one valid dist/cf.js path throughout the guide.
Because Terraform is placed in demo-server/infra, ${path.module}/dist/cf.js and ${path.module}/../socket.io-serverless/dist/cf.js point to incorrect locations. The sample HCL’s ../../socket.io-serverless/dist/cf.js is the consistent path; update the table and Makefile guidance to match it.
Also applies to: 106-109, 268-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/terraform-migration.md` around lines 61 - 63, Update the Terraform
migration guide’s `dist/cf.js` references, including the table entry and
Makefile guidance, to consistently use `../../socket.io-serverless/dist/cf.js`
from `demo-server/infra`. Remove the incorrect `${path.module}/dist/cf.js` and
`${path.module}/../socket.io-serverless/dist/cf.js` paths while preserving the
existing build instructions.
| - `demo-server/infra/terraform.tfvars` — or pull `account_id` / | ||
| `api_token` from the `CLOUDFLARE_API_TOKEN` / `CLOUDflare_ACCOUNT_ID` | ||
| environment variables (recommended). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the TF_VAR_ prefix for Terraform inputs.
CLOUDflare_ACCOUNT_ID is misspelled, and Terraform won’t map account_id from CLOUDFLARE_ACCOUNT_ID; input variables need TF_VAR_account_id.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/terraform-migration.md` around lines 79 - 81, Update the Terraform
environment-variable guidance in the migration documentation to use the
correctly spelled Cloudflare account variable and the Terraform-compatible
TF_VAR_account_id naming for the account_id input, while keeping the API token
guidance accurate.
| ### 4.3 `demo-server/package.json` | ||
|
|
||
| | Script | Current | After the switch | | ||
| |---|---|---| | ||
| | `deploy:cf` | `wrangler deploy` | `make tf-apply` (or `cd infra && terraform apply`) | | ||
| | `build:cf` | `wrangler deploy --dry-run --outdir ./build` | `make tf-plan` (validates config; does NOT bundle — bundling is now `make lib-build`'s job) | | ||
| | `dev:cf` | `wrangler dev` | **Unchanged** — keep `wrangler dev` for local dev (see §5) | | ||
|
|
||
| `wrangler` would remain a `devDependency` for `wrangler dev` and `wrangler types` | ||
| even after the switch. The deploy-script would no longer call it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the package-script commands runnable from demo-server.
Package scripts execute with demo-server as their working directory, but the Makefile is at the repository root. Therefore make tf-plan and make tf-apply will not find the root Makefile. Use make -C .. tf-plan / make -C .. tf-apply, or explicitly cd .. first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/terraform-migration.md` around lines 111 - 120, Update the `deploy:cf`
and `build:cf` entries in the `demo-server/package.json` migration table to
invoke the repository-root Makefile from the `demo-server` working directory,
using `make -C .. tf-apply` and `make -C .. tf-plan` (or an equivalent explicit
directory change). Keep the existing `dev:cf` command and bundling clarification
unchanged.
1 participant
Summary by CodeRabbit
Documentation
Tests
Chores