Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Cloudflare Durable Object worker and local dev tooling, replaces hardcoded Durable Object URL/secret with environment/Nomad secrets for the Unyt scenario, and provides Nix and shell helpers to run the Durable Object store locally. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
|
The following will be added to the changelog [0.6.1] - 2026-03-09Features
Bug Fixes
Miscellaneous Tasks
Documentation
|
cdunster
force-pushed
the
547-hcf-to-manage-the-durable-object-storage-and-dont-hard-code-the-url-and-secret
branch
from
d73e89e to
814c729
Compare
cdunster
commented
Mar 9, 2026
This comment was marked as outdated.
This comment was marked as outdated.
cdunster
force-pushed
the
547-hcf-to-manage-the-durable-object-storage-and-dont-hard-code-the-url-and-secret
branch
from
311b595 to
60ce170
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cdunster
commented
Mar 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/local-durable-objects.sh (1)
1-7: Add defensive shell options and environment variable validation.The script assumes
UNYT_DURABLE_OBJECTS_URLandUNYT_DURABLE_OBJECTS_SECRETare set and that the URL contains a port. Consider adding validation to provide clearer error messages when prerequisites are missing.🛡️ Proposed defensive improvements
#!/usr/bin/env bash +set -euo pipefail run_local_durable_object_store() { + if [[ -z "${UNYT_DURABLE_OBJECTS_URL:-}" ]]; then + echo "Error: UNYT_DURABLE_OBJECTS_URL is not set" >&2 + return 1 + fi + if [[ -z "${UNYT_DURABLE_OBJECTS_SECRET:-}" ]]; then + echo "Error: UNYT_DURABLE_OBJECTS_SECRET is not set" >&2 + return 1 + fi + local port=${UNYT_DURABLE_OBJECTS_URL##*:} + if [[ ! "$port" =~ ^[0-9]+$ ]]; then + echo "Error: Could not extract valid port from UNYT_DURABLE_OBJECTS_URL" >&2 + return 1 + fi wrangler dev --types --config="$(pwd)/durable_object_store/wrangler.jsonc" --local --port "$port" --var "SECRET_KEY:$UNYT_DURABLE_OBJECTS_SECRET" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/local-durable-objects.sh` around lines 1 - 7, Add defensive shell options and validate required env vars inside run_local_durable_object_store: enable strict mode (set -euo pipefail) at the top of the script, check that UNYT_DURABLE_OBJECTS_URL and UNYT_DURABLE_OBJECTS_SECRET are defined and non-empty, verify that UNYT_DURABLE_OBJECTS_URL contains a colon and a valid port after extracting it (using the current extraction logic with ${UNYT_DURABLE_OBJECTS_URL##*:}), and on any failure print a clear error to stderr and exit non-zero; keep the call to wrangler dev in run_local_durable_object_store but ensure variables are quoted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@durable_object_store/src/index.ts`:
- Around line 51-57: The handler unconditionally returns 200 after calling
env.RUN_STORE.idFromName/run_store.get and executing
stub.fetch("https://internal/set"), so write failures aren't propagated; update
the code that calls stub.fetch (the fetch call on the stub) to await and inspect
the returned Response, and if the response is not ok (status >= 400) return or
throw a Response that propagates the status and body (or an error message)
instead of always returning 200; ensure you reference the stub.fetch call and
use idFromName/get to locate the logic to implement this check and return the
stub's error status/body when it fails.
- Around line 42-60: In handlePost, stop using truthiness checks for
run_id/value/secret and instead validate types and presence: parse the JSON and
if parsing fails return 400, then verify run_id is a non-empty string (typeof
run_id === "string" && run_id.length > 0), verify secret is a string and matches
env.SECRET_KEY, and verify the request actually includes a value key (use
Object.prototype.hasOwnProperty.call(body, "value") or "value" in body) so falsy
values like 0/false/"" are accepted; keep the env.RUN_STORE.idFromName/get/fetch
logic but return 400 for bad input and 403 for secret mismatch, preserving the
catch block for unexpected errors.
- Around line 64-86: handleGet currently returns records to anyone with run_id;
add the same shared-secret validation used in handlePost before fetching the
stored record. In handleGet, read the incoming secret (e.g., from Authorization
header or a "secret" param) and compare it to env.SHARED_SECRET (or the same
validation helper used by handlePost); if missing or mismatched return 401/403
immediately, otherwise proceed to env.RUN_STORE.idFromName/run stub.fetch.
Ensure you reuse the same validation logic/function used by handlePost to keep
behavior consistent.
In `@nomad/run_scenario.tpl.hcl`:
- Around line 135-148: The Durable Objects URL is hard-coded into the env
(UNYT_DURABLE_OBJECTS_URL); replace the literal with a configuration-driven
value fetched from Nomad-managed config/secrets (e.g., use
secret.job_secrets.UNYT_DURABLE_OBJECTS_URL or a template variable from ds
"vars" such as index (ds "vars") "unyt_durable_objects_url" with a sensible
default) so the connection endpoint is sourced from the existing secret
"job_secrets" or template data rather than baked into UNYT_DURABLE_OBJECTS_URL.
- Line 147: The HCL2 attribute assignment for UNYT_DURABLE_OBJECTS_URL currently
ends with an invalid trailing semicolon; remove the semicolon after the URL
value so the line reads as a plain HCL attribute assignment (i.e.,
UNYT_DURABLE_OBJECTS_URL =
"https://wind-tunnel-durable-objects.holochain.workers.dev") to fix the template
validation error.
---
Nitpick comments:
In `@scripts/local-durable-objects.sh`:
- Around line 1-7: Add defensive shell options and validate required env vars
inside run_local_durable_object_store: enable strict mode (set -euo pipefail) at
the top of the script, check that UNYT_DURABLE_OBJECTS_URL and
UNYT_DURABLE_OBJECTS_SECRET are defined and non-empty, verify that
UNYT_DURABLE_OBJECTS_URL contains a colon and a valid port after extracting it
(using the current extraction logic with ${UNYT_DURABLE_OBJECTS_URL##*:}), and
on any failure print a clear error to stderr and exit non-zero; keep the call to
wrangler dev in run_local_durable_object_store but ensure variables are quoted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c17c813-3229-4f58-a6a9-36dfba79a738
📒 Files selected for processing (12)
.gitignoredurable_object_store/package.jsondurable_object_store/src/index.tsdurable_object_store/tsconfig.jsondurable_object_store/wrangler.jsoncflake.nixnomad/run_scenario.tpl.hclscenarios/unyt_chain_transaction/README.mdscenarios/unyt_chain_transaction/src/behaviour/smart_agreements.rsscenarios/unyt_chain_transaction/src/durable_object.rsscenarios/unyt_chain_transaction/src/main.rsscripts/local-durable-objects.sh
durable_object_store/src/index.ts
Outdated
Comment on lines
+64
to
+86
| async function handleGet(url: URL, env: Env): Promise<Response> { | ||
| const run_id = url.searchParams.get("run_id"); | ||
| if (!run_id) { | ||
| return new Response(JSON.stringify({ error: "Missing run_id" }), { status: 400 }); | ||
| } | ||
| const id = env.RUN_STORE.idFromName(run_id); | ||
| const stub = env.RUN_STORE.get(id); | ||
| const resp = await stub.fetch("https://internal/get"); | ||
| const data = await resp.json<{ error?: string; value?: unknown }>(); | ||
| if (data.error) { | ||
| return new Response(JSON.stringify(data), { status: 404 }); | ||
| } | ||
| return new Response(JSON.stringify(data), { status: 200 }); | ||
| } | ||
|
|
||
| export default { | ||
| async fetch(request: Request, env: Env): Promise<Response> { | ||
| const url = new URL(request.url); | ||
| const method = request.method; | ||
| if (method === "POST") { | ||
| return await handlePost(request, env); | ||
| } else if (method === "GET") { | ||
| return await handleGet(url, env); |
There was a problem hiding this comment.
Apply the shared secret to reads as well.
handleGet currently makes every stored record readable to any caller that knows run_id. If these payloads are not intentionally public, this endpoint needs the same secret check as handlePost.
🔐 Proposed fix
-async function handleGet(url: URL, env: Env): Promise<Response> {
+async function handleGet(request: Request, env: Env): Promise<Response> {
+ const url = new URL(request.url);
+ const secret = request.headers.get("x-secret");
+ if (secret !== env.SECRET_KEY) {
+ return new Response(JSON.stringify({ error: "Unauthorized" }), { status: 403 });
+ }
const run_id = url.searchParams.get("run_id");
if (!run_id) {
return new Response(JSON.stringify({ error: "Missing run_id" }), { status: 400 });
}
@@
const url = new URL(request.url);
const method = request.method;
if (method === "POST") {
return await handlePost(request, env);
} else if (method === "GET") {
- return await handleGet(url, env);
+ return await handleGet(request, env);
} else {
return new Response("Method Not Allowed", { status: 405 });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@durable_object_store/src/index.ts` around lines 64 - 86, handleGet currently
returns records to anyone with run_id; add the same shared-secret validation
used in handlePost before fetching the stored record. In handleGet, read the
incoming secret (e.g., from Authorization header or a "secret" param) and
compare it to env.SHARED_SECRET (or the same validation helper used by
handlePost); if missing or mismatched return 401/403 immediately, otherwise
proceed to env.RUN_STORE.idFromName/run stub.fetch. Ensure you reuse the same
validation logic/function used by handlePost to keep behavior consistent.
cdunster
commented
Mar 9, 2026
Comment on lines
+37
to
+40
| let base_url = std::env::var("UNYT_DURABLE_OBJECTS_URL") | ||
| .expect("UNYT_DURABLE_OBJECTS_URL needs to be set for this scenario to run"); | ||
| let secret = std::env::var("UNYT_DURABLE_OBJECTS_SECRET") | ||
| .expect("UNYT_DURABLE_OBJECTS_SECRET needs to be set for this scenario to run"); |
Contributor
Author
There was a problem hiding this comment.
I'm tempted to make these WT_DURABLE_OBJECTS_URL and WT_DURABLE_OBJECTS_SECRET and make them generic to Wind Tunnel itself so that other scenarios could use them in the future but then we should probably move all of the Durable Object stuff into common code so maybe that should come later.
Member
There was a problem hiding this comment.
Fine either way, no objection to these being scenario specific
Contributor
Author
There was a problem hiding this comment.
I'll leave them as they are for now then. If more scenarios use them then we can make the Durable Object stuff common
mattyg
previously approved these changes
Mar 10, 2026
cdunster
force-pushed
the
547-hcf-to-manage-the-durable-object-storage-and-dont-hard-code-the-url-and-secret
branch
from
6758f8e to
a3fae8d
Compare
cdunster
force-pushed
the
547-hcf-to-manage-the-durable-object-storage-and-dont-hard-code-the-url-and-secret
branch
from
af3285b to
b9ab6b0
Compare
cdunster
commented
Mar 12, 2026
cdunster
commented
Mar 12, 2026
| wrangler secret put SECRET_KEY | ||
| ``` | ||
|
|
||
| After updating the `SECRET_KEY`, the value of `UNYT_DURABLE_OBJECTS_SECRET` in |
Contributor
Author
There was a problem hiding this comment.
I may need to add a bit about being careful of special characters but I'm really not sure what/where the problems were with the special characters not working in the secret.
ThetaSinner
approved these changes
Mar 12, 2026
mattyg
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds the necessary wrangler project that can be deployed to Cloudflare to create a Durable Object store. This store is currently just used by the Unyt Chain Transaction scenario but has been kept someone general as it just stores JSON objects so can be used by other scenarios in the future.
FYI, if all of wrangler stuff gets accepted here then we no longer need holochain/hc-github-config#72 as that was going to be a fresh repo just for these files but I thought that they seemed small enough to keep in this repo and this means that the devs have everything they need to run the scenario locally in this repo. I'm open to suggestions though.
I also renamed the
NUMBER_OF_LINKS_TO_PROCESSscenario environment variable toUNYT_NUMBER_OF_LINKS_TO_PROCESSto make it clear that it is only used in the Unyt Chain Transaction scenario and added the missingadd_capture_env("UNYT_NUMBER_OF_LINKS_TO_PROCESS")call.Closes #547.
TODO:
Note that all commits in a PR must follow Conventional Commits before it can be merged, as these are used to generate the changelog
Summary by CodeRabbit
New Features
Documentation
Chores