chore(ci): set KUBO_API_URL for staging and document the local v2 stack - #1228
Conversation
WalkthroughThe PR aligns local, staging, and desktop E2E environments with ChangesEnvironment and setup alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
The staging deploy wrote v1-era pin-store names and never set the one the v2 API reads, so every hosted write answered 503 — uploads and folder creates alike, since a record's head block goes through the same endpoint. - staging `.env.staging` now sets `KUBO_API_URL=http://ipfs:5001` and `ROUTING_V1_URL=http://someguy:8190`. `ROUTING_V1_URL` was unset too, so the republisher walk resolved every name to null and re-PUT nothing. - drop `IPFS_PROVIDER`, `IPFS_LOCAL_API_URL`, `IPFS_LOCAL_GATEWAY_URL`, `DELEGATED_ROUTING_URL` and `DELEGATED_ROUTING_FALLBACK_URL`; no source file reads any of them. - `KuboPinStore` reports an unset `KUBO_API_URL` at construction, so a misconfiguration is visible at deploy time rather than under load. Logged rather than thrown: an unconfigured store is a supported shape. - the root README carries one full local recipe — Postgres, Kubo, someguy, the record store, the API, the web bundle — with the environment inline instead of `.env.example` files the repo has not shipped for months. - the local stack resolves records through `mock-ipns-routing`, not someguy: hermetic, instant, and no test vault's names on the public DHT. - v1 `docs/` recipes that led readers into a redis/tee-worker stack are retired to stubs pointing at the README; `CONFIGURATION.md` keeps its catalogue with the dead names corrected. Closes #1209 Closes #1216
…cipe Self-review follow-ups. - `RoutingV1RecordTransport` reports an unset `ROUTING_V1_URL` at boot, the same way the pin store now does. Its degraded path is the quieter of the two: `runOnce` returns before it can alert, so nothing surfaces until names start expiring. - trim the pin-store comment to the one non-obvious why; the fail-closed rationale already lives on the class doc. - use the repo's existing Logger-spy idiom in the new tests. - mark the recipe's example secrets local-only, and note that the dev compose binds Kubo's unauthenticated admin RPC to all interfaces. - repoint the last `docs/ARCHITECTURE.md` link that still sent readers to the emptied DEVELOPMENT.md.
…store - `desktop-e2e.yml` set the same four dead v1 names and neither live one; swap them for `KUBO_API_URL` and `ROUTING_V1_URL`. The workflow already runs a real `ipfs daemon` on 5001, so the values are unchanged. - the content slice binds the registry's exported `PinStore` instead of constructing a second `KuboPinStore`, so an unconfigured store is reported once at boot rather than once per module.
16b60df to
4f413de
Compare
The README told a reader to `cp apps/api/.env.example` and the file was not there. Restore both templates and make them the single place the local stack's configuration is written down. - `apps/api/.env.example` and `apps/web/.env.example` carry values that match the compose stack, so they run as copied. Every name in them has a reader in the code; the optional web names are commented out rather than blank. - `VITE_READ_ACCELERATOR_URL` ships commented out. Dormant is the content gateway's fail-closed state, so copying the template must not switch it on, and `config.ts` still gives it no default. - the README stops repeating the variable list and copies the templates instead, so the two cannot disagree. - drop the now-false claims that the repo ships no templates. - the desktop section named `VITE_GOOGLE_CLIENT_ID`, `VITE_TEST_LOGIN_SECRET` and `CIPHERBOX_API_URL` for a shell that has no sources and reads no environment; removed rather than given a template with no readers.
…nfig-and-local-stack
…nfig-and-local-stack
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/republisher/record-transport.ts (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the routing configuration diagnostic.
No test covers
RoutingV1RecordTransportconstruction withROUTING_V1_URLunset. Add one that asserts the constructor logs one error containingROUTING_V1_URL, and add a case for configured construction that makes no logger calls.🤖 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 `@apps/api/src/republisher/record-transport.ts` around lines 62 - 66, Add regression tests for RoutingV1RecordTransport construction: verify an unset ROUTING_V1_URL causes exactly one logger error containing ROUTING_V1_URL, and verify configured construction makes no logger calls. Reuse the existing transport and logger test setup without changing production behavior.Source: Path instructions
🤖 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 `@apps/web/.env.example`:
- Around line 25-28: Update the Web3Auth configuration table in
docs/CONFIGURATION.md to include VITE_WEB3AUTH_VERIFIER and mark both
VITE_WEB3AUTH_CLIENT_ID and VITE_WEB3AUTH_VERIFIER as required, matching the
requirements documented in apps/web/.env.example.
In `@README.md`:
- Around line 128-130: Add a bounded readiness gate between the Docker Compose
status check and Step 2 in the README instructions. Wait until Postgres, Kubo,
and routing services report healthy, fail after the configured timeout if any
remain starting or unhealthy, then proceed to migrations and API commands.
---
Nitpick comments:
In `@apps/api/src/republisher/record-transport.ts`:
- Around line 62-66: Add regression tests for RoutingV1RecordTransport
construction: verify an unset ROUTING_V1_URL causes exactly one logger error
containing ROUTING_V1_URL, and verify configured construction makes no logger
calls. Reuse the existing transport and logger test setup without changing
production behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 750af86e-d912-4471-a5e8-14d4fe145209
📒 Files selected for processing (15)
.github/workflows/deploy-staging.yml.github/workflows/desktop-e2e.ymlCONTRIBUTING.mdREADME.mdapps/api/.env.exampleapps/api/src/content/content.module.tsapps/api/src/registry/pin-store.test.tsapps/api/src/registry/pin-store.tsapps/api/src/registry/registry.module.tsapps/api/src/republisher/record-transport.tsapps/web/.env.exampledocs/ARCHITECTURE.mddocs/CONFIGURATION.mddocs/DEVELOPMENT.mddocs/GETTING-STARTED.md
…th verifier The recipe told the reader to wait for health and then handed them `docker compose ps`, which only prints the current status — so the migration could run against a Postgres still starting. Every service in the compose file defines a healthcheck, so `--wait` is the gate, with a timeout to bound it. CONFIGURATION.md marked only the client ID required and omitted the verifier, while `loginEnv` refuses a session missing either. A reader following the table alone built a UI that renders and cannot log in. The routing transport's boot report gains the regression tests its pin-store twin already had: the unset variable is named exactly once, and a configured endpoint stays silent.
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
docs/CONFIGURATION.md omits VITE_WEB3AUTH_VERIFIER |
Taken. Verified against engine/config.ts first — both names are in LOGIN_ENV and loginEnv throws when either is missing, so the omission really did leave a reader with a bundle that cannot log in. Added as a Yes row stating the coupling. |
README.md readiness gate before migrations |
Taken, differently. The prose already said "wait for the services to report healthy" above a docker compose ps that does not wait. All four compose services define healthchecks, so the fix is up -d --wait --wait-timeout 180 rather than a hand-rolled loop — it bounds the wait, fails non-zero, and made the recipe shorter. |
Nitpick (1 of 1) — taken
| Nitpick | Disposition |
|---|---|
No regression coverage for the RoutingV1RecordTransport boot report |
Taken. This was a real asymmetry: the PR added the report to mirror KuboPinStore, and the pin-store twin already had exactly these two tests. record-transport.test.ts now asserts the unset case logs once, names ROUTING_V1_URL, and states the consequence, and that a configured endpoint stays silent. Verified as a true negative control — deleting the logger.error line fails the first test, restoring it passes. |
Unchanged, and why
The two desktop-e2e.yml gaps the PR body already records stay filed rather than fixed here — the dead Redis install steps and the five non-existent package builds. Both are more than the name swap this PR scoped, and the file is marked DORMANT at line 1 so it cannot run today regardless. #1229 (republisher walk observability) is likewise unchanged; the review did not raise it.
Gates on ff32bb6
pnpm --filter @cipherbox/api test 0 — 183 passed (24 files), up from 181 by the two added above · pnpm lint 0 · pnpm typecheck 0 · pnpm lint:tracker-refs 0 · markdownlint on both touched files 0.
Generated by Claude Code
Nothing in v2 reads REDIS_HOST or REDIS_PORT, and blueprint/deploy.md records redis as dead: nothing queues, and throttling is in-process. The workflow still installed and health-checked a server on all three OSes and set both variables at three sites, so every run paid for a service no code would ever open. Removal only — the file stays DORMANT and its stale package builds are untouched.
Scope addition: the dead Redis plumbing is now removedThis PR previously recorded the Verified unread before deleting, rather than trusting the earlier claim: a repo-wide search for What went:
Pure deletion, 44 lines, no substitutions. Every run of that workflow was installing and health-checking a service no code would ever open. Checked rather than assumed: the file still parses as YAML with its single job and all 31 remaining steps, and the deleted Linux step carried its own Gates on 779c592: Still not fixed there and out of scope: the workflow builds Generated by Claude Code |
The step built five packages that do not exist — crypto, core, api-client, sdk-core and sdk — and then ran vite in apps/desktop, which carries no vite config, no vite dependency and no sources. The v2 shell embeds a checked-in static index.html through tauri.conf.json's frontendDist, so the cargo build needs nothing built ahead of it. The whole step goes rather than its first five lines, since the vite invocation was as dead as the packages. Nothing downstream consumed its output. No replacement is guessed here: what the mounted-desktop matrix needs is decided when the workflow is rewired.
Scope addition: the legacy frontend build is removedRemoved in f548d52. The PR body previously recorded this as knowingly left alone; it has been updated. The whole step went, not just the five package builds. Verifying before deleting turned up that the
So trimming only the first five lines would have left a step that still could not run. Checked rather than assumed:
No replacement is guessed. If the v2 desktop frontend eventually needs Gates on f548d52: With this and 779c592, the two Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/api/src/republisher/record-transport.test.ts`:
- Around line 182-183: Strengthen the assertion for the republisher diagnostic
in the test near the existing ROUTING_V1_URL check: update the `errorSpy`
expectation so it matches the fail-closed consequence phrase “walk will not run”
(or the complete diagnostic), rather than the ambiguous substring “walk”.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76f079b5-99c7-4e2e-9737-cf72e4f2adfe
📒 Files selected for processing (4)
.github/workflows/desktop-e2e.ymlREADME.mdapps/api/src/republisher/record-transport.test.tsdocs/CONFIGURATION.md
💤 Files with no reviewable changes (1)
- .github/workflows/desktop-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/CONFIGURATION.md
stringContaining('walk') passed on any message carrying that substring,
so a report that named the variable and dropped what goes wrong still
satisfied a test whose name promises the consequence. It now matches the
phrase itself.
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
record-transport.test.ts asserts stringContaining('walk') rather than the consequence |
Taken. A fair catch on a test this PR added one round earlier: the test's own name promises "names the unset variable and its consequence", and 'walk' checked no consequence — a message saying the walk will run would have passed. Now matches the full diagnostic. Verified as a discrimination check, not just a re-run: weakening the production message to one that still contains walk but drops the consequence fails the new assertion and passed the old one. |
Nitpicks / outside-diff-range: none. The review body carried no nitpick section — the single actionable comment was its whole content. Stated explicitly because those sections create no threads.
Not re-raised, and unchanged: the Redis removal (779c592) and the legacy frontend-build removal (f548d52) drew no findings; CodeRabbit marked desktop-e2e.yml as having no reviewable changes, both being pure deletions, and skipped README.md and docs/CONFIGURATION.md as similar to previously reviewed changes.
Gates on 3abf0b6: pnpm --filter @cipherbox/api test 0 (24 files, 183 tests) · pnpm lint 0 · pnpm typecheck 0 · pnpm lint:tracker-refs 0. CI on the previous head f548d52 was green across all 25 jobs, and this commit touches one test file.
Generated by Claude Code
2 participants
What was broken
The staging deploy wrote v1-era pin-store variable names and never set the one the v2 API
actually reads.
KuboPinStorereads exactly one name —KUBO_API_URL— and unset,rpc()throws
ServiceUnavailableException. That is not only an upload failure: a record's headblock is uploaded through the same endpoint, so folder create fails too. Every hosted write
answered 503.
Separately, no recipe in the repo booted a full v2 stack locally. The compose file already
carried every service; the recipes that would use it told you to copy
.env.examplefilesthat have not existed in the repo for months.
Changes
Staging deploy
KUBO_API_URL=http://ipfs:5001— the compose-internal Kubo RPC address.ROUTING_V1_URL=http://someguy:8190. Not in either issue, found while verifyingthem: the deploy wrote the dead
DELEGATED_ROUTING_URLwhile the live name therepublisher reads was unset, so
RoutingV1RecordTransport.configuredwas false — the walkresolved every name to null and re-PUT nothing. Same defect, one variable over.
IPFS_PROVIDER,IPFS_LOCAL_API_URL,IPFS_LOCAL_GATEWAY_URL,DELEGATED_ROUTING_URLandDELEGATED_ROUTING_FALLBACK_URL. Confirmed by repo-wide grepthat no source file reads any of the five.
THROTTLE_BYPASS_SECRETis also unread by any source file, butblueprint/deploy.mdlistsits staging secret under Keep, so it stays.
Pin-store configuration is reported at boot
KuboPinStorenow logs an error at construction whenKUBO_API_URLis unset, naming thevariable and its consequence.
Logged, not thrown. The class doc already sanctions an unconfigured store as a supported
shape —
unpindeliberately no-ops for BYO-only deployments and unit tests — so a hard bootfailure would turn a supported configuration into a crash loop. This also matches the repo's
existing precedent for the identical failure mode:
content.service.tslogs'DB pool too small ... hosted uploads will be refused with 503'and degrades rather thandying.
JWT_SECRETthrows because a missing secret makes tokens forgeable — a silentsecurity hole. A missing
KUBO_API_URLis a loud functional outage, not a security hole.RoutingV1RecordTransportgets the mirrored report, and its failure is the quieter of thetwo —
runOncereturns beforealerter.walkComplete, so there is no error, no warn, and nozero-count until IPNS names hit EOL.
One local recipe
The root
README.md"Getting started" section now carries the full stack: Postgres, Kubo,someguy, the record store, the API, and the web bundle, start to finish.
The
.env.exampletemplates are restored and are the single source of truth.apps/api/.env.exampleandapps/web/.env.exampleare checked in with values that match thecompose stack, so
cp apps/api/.env.example apps/api/.envruns as written. The README nolonger repeats the variable list — it copies the templates — so the two cannot disagree.
Every name in both templates has a verified reader in the code, which is the whole point of
#1209: a template full of dead names would recreate that defect somewhere more discoverable.
The API's twelve resolve to
app.module.ts,data-source.ts,main.ts,auth.module.ts,test-auth.service.ts,pin-store.tsandrecord-transport.ts; the web's eight toconfig.tsandintrospection.ts.VITE_READ_ACCELERATOR_URLships commented out, not blank — dormant is the contentgateway's fail-closed state, so copying the template must not silently switch it on.
config.tsis untouched and still gives it no default. Verified in the built bundle: zerooccurrences of the gateway URL after a build from the template.
No
apps/desktop/.env.examplewas created. That shell has no TypeScript sources and reads noenvironment variable at all, so a template there would have no readers whatsoever.
Record resolution:
mock-ipns-routing(3001), not someguy. Documented with the reason —it is hermetic and in-memory, so a locally published record resolves immediately and
deterministically and no test vault's IPNS names reach the public DHT. someguy joins the real
accelerated DHT and stays for staging parity. The recipe notes both variables must name the
same backend or the republisher re-PUTs into a store the client never reads.
The stack is started with
--wait --wait-timeout 180. Every compose service defines ahealthcheck, so this holds until each one passes and exits non-zero otherwise — the migration
step cannot race a Postgres that is still starting.
Stale v1 docs
docs/GETTING-STARTED.mdanddocs/DEVELOPMENT.mdare reduced to stubs pointing at theREADME — keeping a second and third copy of the recipe is how the current rot happened.
docs/CONFIGURATION.mdkeeps its catalogue but loses the Redis, TEE Integration, TEE Workerand Phala sections and has the dead IPFS/routing names corrected. Its web table also gains
VITE_WEB3AUTH_VERIFIER, which it omitted while marking only the client ID required —loginEnvrefuses a session missing either, so a reader following the table alone built a UIthat renders and cannot log in.
CONTRIBUTING.mdsent new contributors straight into the deadrecipes and now points at the README.
desktop-e2e.ymlThe workflow set the same four dead pin-store/routing names and neither live one; swapped for
KUBO_API_URLandROUTING_V1_URL. Values are unchanged — that workflow already runs a realipfs daemonon 5001 and the mock routing store on 3001.The dead Redis plumbing is removed. It installed and health-checked a Redis server on all
three OSes —
brew,apt-get, and Memurai viachocowith aredis-cli pingreadiness loop— and set
REDIS_HOST/REDIS_PORTat three separate sites. Nothing in v2 reads eithervariable, and
blueprint/deploy.mdrecords redis as dead: nothing queues, and throttling isin-process. Every run paid for a service no code would open. 44 lines, pure deletion.
The legacy frontend build is removed. The step built
@cipherbox/crypto,core,api-client,sdk-coreandsdk— none of which exist; the workspace holds onlyclient,api,desktop,webandmock-ipns-routing— and then ranpnpm vite buildinapps/desktop, which carries no vite config, no vite dependency and no sources. The whole stepgoes rather than its first five lines, because the vite invocation was as dead as the packages:
tauri.conf.jsonsetsfrontendDist: "../frontend", a checked-in staticindex.html, socargo build -p cipherbox-desktopneeds nothing built ahead of it. Nothing downstream consumedthe step's output. No replacement is guessed — what the mounted-desktop matrix needs is decided
when this workflow is rewired per
blueprint/testing.md.The two real builds are untouched:
tools/mock-ipns-routingand@cipherbox/api.The file stays marked DORMANT at line 1;
tests/desktop-e2e/was demolished with the v1layout, so the suite it invokes still does not exist and this remains a cleanup of a workflow
that cannot run until it is rewired.
Verification
Followed the recipe end to end against the real stack rather than reasoning about it.
POST /content/uploaddriven directly with a JWT from/auth/test-loginand a CID computedout-of-band from the local Kubo:
KUBO_API_URLKUBO_API_URL is unset; hosted uploads will be refused with 503POST /content/uploadbeforePin store unavailable; upload not durableKUBO_API_URL=http://localhost:5001POST /content/uploadafter{"cid":"bafkr4ibzqsjphufa3r7r4qyep5ybh3xtsjajphh33gbthcqrfi3rafdici","size":41}pin/ls{"Type":"direct"}— pinned direct, not recursive, as the blueprint requiresPOST /content/upload201Gates
pnpm lintpnpm typecheckpnpm --filter @cipherbox/api testmarkdownlinton every touched.mdpnpm lint:tracker-refszizmor --no-online-auditson.github/prettier --checkon the touched workflowFour tests cover the two boot reports: each names its variable and its consequence when unset,
and stays silent when configured. The routing pair was verified as a true negative control —
deleting the
logger.errorline fails the unset test.Self-review gates
/simplify(4 agents) and/security-reviewwere run on this diff; findings folded back in.simplify — trimmed the pin-store comment to its one non-obvious why (the rest restated
the class doc, which
AGENTS.mdforbids); switched the new tests to the repo's existingvi.spyOn(Logger.prototype, ...)idiom instead of a hand-rolled capture array; removed threeredundant restatements from the README; repointed the last
docs/ARCHITECTURE.mdlink thatstill sent readers to the emptied
DEVELOPMENT.md.The altitude pass argued the diff was under-generalized, which is why
RoutingV1RecordTransportgot the mirrored boot report. It also concluded a config validationschema would be the wrong altitude here:
KUBO_API_URLunset is a supported configuration,so a presence-check would either over-reject or stay silent.
security-review — no HIGH or MEDIUM. Confirmed both new log lines are static literals that
interpolate no config value; no fail-closed path weakened (
rpc()still throws unconfigured);staging's
5001is bound127.0.0.1and someguy's8190is not host-published, so pointingat them adds no exposure. One LOW fixed: the recipe's literal
JWT_SECRET/TEST_LOGIN_SECRETare now marked local-only, since
test-loginis hard-blocked in production but not instaging. Also added a caution that the dev compose binds Kubo's unauthenticated admin RPC to
all interfaces (pre-existing, but the README is now the single local-dev entry point).
Coordinator follow-ups
Double boot log: fixed.
RegistryModulenow exportsPinStoreandContentModuleimportsit instead of declaring a second
{ provide: PinStore, useClass: KuboPinStore }. Three lines,no cycle (
RegistryModuledoes not importContentModule), and it matches whatContentModule's own doc already claims ("Reuses the registry's pin-store port"). Verifiedempirically rather than by inspection: typecheck 0, tests pass, the app boots, each
unset-variable error now prints exactly once, and
POST /content/uploadstill returns 201through the shared instance.
Republisher walk observability: filed, not built — #1229. Chose "boot report stays, walk
signal is a separate decision".
The two options contained in
apps/apiare both wrong on the merits. EmittingwalkComplete(0, 0)on the unconfigured path sets the same gauges a configured republisherwith zero registered names sets, so it manufactures a healthy-looking signal for an unhealthy
state. Throwing contradicts an explicitly supported deployment shape —
record-transport.tsdocuments "unit deploys, BYO-only" and the early return exists precisely to avoid
alert-storming them; that is the same mistake this PR avoided for
KUBO_API_URL.The correct fix is a distinct signal an operator can alert on separately from a zero-count
walk (a
republisher_configuredgauge, or a walk-outcome label). That adds to theRepublisherAlertercontract and toMetricsService, and changes what dashboards and alertrules receive — the stated bar for filing rather than guessing.
What is still blocked
Interactive Web3Auth login needs
VITE_WEB3AUTH_CLIENT_IDandVITE_WEB3AUTH_VERIFIER,which a clean checkout does not carry; the README states this instead of leaving a reader to
discover it at the login screen.
Notes for review
cipherboxdatabase polluted by earlier sessions;re-running against a fresh database succeeded, so this is local state, not a recipe defect.
Closes #1209
Closes #1216
Note
Set
KUBO_API_URLandROUTING_V1_URLin staging CI and document the local v2 dev stackIPFS_*andDELEGATED_ROUTING_*env vars withKUBO_API_URLandROUTING_V1_URLin the staging deploy and desktop E2E workflows.Logger.errortoKuboPinStoreandRoutingV1RecordTransportwhen their required URLs are unset, so misconfiguration is surfaced immediately rather than at request time.PinStorefromRegistryModuleand removes the redundant binding inContentModule.Macroscope summarized 3abf0b6.
Summary by CodeRabbit
Documentation
Bug Fixes
Tests