Stage 1: SYNC_MODE dry-run safety guard - #12
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
Comment |
`npm run worker` against real credentials destroys real data: the contacts
job DELETEs every in-game contact under STANDINGS_LABEL that isn't a member
(130 in one production run), the wanderer job destructively reconciles a
live ACL, and token-health rotates EVE refresh tokens — and EVE rotates on
use, so refreshing against a copy of production silently invalidates
production's stored tokens. Nothing warned a developer before now.
Adds a required SYNC_MODE env var (live | dry-run, no zod default) enforced
at the boundaries a job cannot bypass:
- the three integration client factories (ESI contacts, Discord roles,
Wanderer ACL) — mutating methods only; reads are never suppressed
- getFreshAccessToken, which refuses the EVE refresh in dry-run
- the ops webhook, so a local worker never pages the real ops channel
Authentication is deliberately NOT guarded: the login and character-link
OAuth exchanges mint new credentials from a fresh authorization code and
invalidate nothing.
Because guarded void methods return normally, the wanderer and discord-roles
jobs would otherwise record success for work that never happened. In dry-run
they now write NO audit rows and report would* counters instead of the
applied-change ones — audit_log is what an operator reconstructs an incident
from and must never claim a mutation that didn't occur.
SYNC_MODE has no default because every alternative has a silent failure
mode: defaulting to dry-run lets a missing production secret turn sync into
an unnoticed no-op, and defaulting to live makes the destructive
configuration the one you get by forgetting.
DEPLOY: `fly secrets set SYNC_MODE=live` BEFORE deploying this. There is no
automatic rollback — fly.toml defines no health checks, and only the worker
validates config at startup.
Design and rationale: docs/superpowers/specs/2026-08-03-local-dev-setup.md
guarzo
force-pushed
the
worktree-local-dev-setup
branch
from
August 3, 2026 18:27
755cf05 to
518c91f
Compare
Owner
Author
Rebased onto
|
| File | Resolution |
|---|---|
src/jobs/wanderer.ts |
formatting only — main's wrapped logAudit calls kept, inside my if (!dry) guard |
src/jobs/discord-roles.ts |
kept main's eslint-disable comments on the as Record<string, number> assertions — they're load-bearing, and my [removedKey] change sits inside them |
tests/{account-view,accounts,eve-sso}.test.ts |
took main's shape (it dropped the as NodeJS.ProcessEnv casts), added SYNC_MODE: "live" |
docs/ops.md |
took main's STANDINGS_LABEL=authgd, added SYNC_MODE=live |
Re-verified after rebase
$ npm run typecheck # clean
$ npm run format:check # clean
$ npm run lint # 0 errors
$ npm test
Test Files 40 passed (40)
Tests 288 passed (288)
npm run test:e2e is still unrun for the reason in the description — port 3111 held by another workstream.
This was referenced Aug 3, 2026
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
…cks (#16) #14 added an http_service check on /api/health, which makes a claim I wrote in #12 false: docs/ops.md said "fly.toml defines no health checks". It now does. The conclusion was right for the wrong reason, and the wrong reason is the dangerous part — a reader who checks fly.toml now sees a health check and reasonably concludes config errors are covered. They are not: /api/health never calls getConfig(). getDb() reads process.env.DATABASE_URL directly (src/db/index.ts) and checkLiveness only runs `select 1` (src/services/health.ts). With SYNC_MODE unset the endpoint returns 200 and the machine stays in rotation while every real page 500s. Replaced the paragraph with a per-component table that says exactly which layer catches what, and names /api/health's 200 as the trap. Also switched the example to `fly secrets set --stage`, which avoids restarting machines for a value the running code does not yet read. Separately, notes .nvmrc and scripts/check-node-version.sh from #13 alongside the existing engines/.npmrc mention, so a future Node bump knows all three pins must agree.
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
EVE SSO rejects the fake client id in .env.example, so there was no way to browse a running dev app as a real account — least of all an admin. The seeding machinery existed but was e2e-only: pinned to the test database and port 3111, and insert-only, because e2e gets a truncated database before every test and never had to survive a second run. npm run db:seed upsert; safe to re-run npm run db:seed -- --reset TRUNCATE first, for a clean slate Seeds six accounts — an admin with two alts, a flygd member with an alt, blue, green, plus cryo and tier_locked so the admin pages have those states to render — and prints a session cookie for each. IDEMPOTENT BY DEFAULT, which is the whole design. Character ids are fixed (a reserved 91_000_0xx block, clear of e2e's 90_000_0xx counter so the two can never collide) and every row is upserted, so a second run converges instead of dying on a duplicate primary key. The account and its main character are upserted in ONE transaction because account_main_character_fk is DEFERRABLE INITIALLY DEFERRED and only checked at commit. Both paths write rows — --reset destructively, the default by adding fixture accounts — and a dev seed has no legitimate remote use, so the script refuses any DATABASE_URL that is not provably loopback. ALLOW_REMOTE_SEED=1 overrides. The host is lower-cased first: `postgres:` is a NON-SPECIAL URL scheme, so WHATWG preserves host case (`http:` would not), and without that postgres://LOCALHOST/... is refused — which would train developers to reach for the override to work around a false rejection. Deliberately NOT shared with e2e/helpers.ts or tests/helpers/seed.ts. The repo already runs two independent seed helpers with different contracts; this is a third. Unifying them would need one helper parameterised for insert-only AND upsert, which costs more than the ~20 duplicated lines and puts the e2e suite at risk to make dev nicer. The session hashing is NOT duplicated — createSession from src/services/session.ts is the real one. Also removes a duplicate "engines" key in package.json: main carried both ">=22" (#13) and ">=22.9" (#12). JSON.parse keeps the last, so it resolved correctly by luck of ordering; a tool taking the first would have read a floor below the one fly.toml's release command depends on. Stage 3 of docs/superpowers/specs/2026-08-03-local-dev-setup.md
guarzo
added a commit
that referenced
this pull request
Aug 3, 2026
* feat: dev seed script with paste-able session cookies EVE SSO rejects the fake client id in .env.example, so there was no way to browse a running dev app as a real account — least of all an admin. The seeding machinery existed but was e2e-only: pinned to the test database and port 3111, and insert-only, because e2e gets a truncated database before every test and never had to survive a second run. npm run db:seed upsert; safe to re-run npm run db:seed -- --reset TRUNCATE first, for a clean slate Seeds six accounts — an admin with two alts, a flygd member with an alt, blue, green, plus cryo and tier_locked so the admin pages have those states to render — and prints a session cookie for each. IDEMPOTENT BY DEFAULT, which is the whole design. Character ids are fixed (a reserved 91_000_0xx block, clear of e2e's 90_000_0xx counter so the two can never collide) and every row is upserted, so a second run converges instead of dying on a duplicate primary key. The account and its main character are upserted in ONE transaction because account_main_character_fk is DEFERRABLE INITIALLY DEFERRED and only checked at commit. Both paths write rows — --reset destructively, the default by adding fixture accounts — and a dev seed has no legitimate remote use, so the script refuses any DATABASE_URL that is not provably loopback. ALLOW_REMOTE_SEED=1 overrides. The host is lower-cased first: `postgres:` is a NON-SPECIAL URL scheme, so WHATWG preserves host case (`http:` would not), and without that postgres://LOCALHOST/... is refused — which would train developers to reach for the override to work around a false rejection. Deliberately NOT shared with e2e/helpers.ts or tests/helpers/seed.ts. The repo already runs two independent seed helpers with different contracts; this is a third. Unifying them would need one helper parameterised for insert-only AND upsert, which costs more than the ~20 duplicated lines and puts the e2e suite at risk to make dev nicer. The session hashing is NOT duplicated — createSession from src/services/session.ts is the real one. Also removes a duplicate "engines" key in package.json: main carried both ">=22" (#13) and ">=22.9" (#12). JSON.parse keeps the last, so it resolved correctly by luck of ordering; a tool taking the first would have read a floor below the one fly.toml's release command depends on. Stage 3 of docs/superpowers/specs/2026-08-03-local-dev-setup.md * docs: testing EVE SSO and Discord linking over a tunnel (#21) Stage 4, documentation only. The seeded cookie from Stage 3 covers most dev work; this covers the case it cannot — changing the login or character-link flows themselves, which needs the real providers to redirect back to your machine. Every concrete claim was verified against the code rather than described from memory: - Redirect URIs are string-concatenated, not URL-joined, so a trailing slash on APP_BASE_URL yields `//auth/eve/callback` and z.string().url() accepts it silently — it surfaces much later as an unexplained redirect mismatch. - One EVE entry covers both flows: /auth/eve/login and /auth/eve/link both call buildEveAuthorizeUrl, so they share /auth/eve/callback. - EVE sends redirect_uri only on authorize; exchangeEveCode sends grant_type, code and code_verifier. Discord sends it TWICE — authorize and token exchange — and they must match. Changing APP_BASE_URL mid-flow therefore breaks Discord linking while EVE login keeps working, which reads like a Discord outage and is not. - Once APP_BASE_URL is https the session cookie is issued Secure, so the browser will not return it over http://localhost — you appear logged out no matter how often you log in. Browse the tunnel origin. Recommends .env.local for the override: it wins over .env, .env* is gitignored apart from .env.example, and switching back is deleting one file. * fix: address CodeRabbit review on #20 Seven findings, all verified against the code first. Two were real bugs: - The non-local refusal message called new URL(cfg.databaseUrl).host to name the host — but that branch is reached BY unparseable URLs, so it threw a TypeError and printed a stack trace instead of the guidance. Confirmed: `new URL("not a url")` throws. Now degrades to "unparseable DATABASE_URL". - The entry-point guard was process.argv[1]?.endsWith("seed-dev.ts"). Rename the file or emit it as .js and main() silently stops running: the command exits 0 having seeded nothing. Now compares resolved paths against fileURLToPath(import.meta.url). The truncation table list was duplicated between scripts/seed-dev.ts and tests/helpers/db.ts. Drift there is silent — a new table missing from one copy leaves stale rows after --reset and fails no test. Both now import src/db/tables.ts, and a new test asserts the list matches information_schema, so adding a table without registering it breaks a test instead of leaking rows. Test fix: the session-reissue assertion compared two Sets for inequality, which passes if only one of six cookies changed. Now asserts full disjointness. Docs: the cookie step said the "value" is the printed string, but the script prints a full name=value assignment and devtools has separate fields — pasting the whole line fails to authenticate. The documented id range said 91_000_0xx while alts use 91_000_1xx; corrected to the reserved 91_000_000-91_999_999 range. And "later file first" was a confusing way to describe env precedence — now states that Next applies its own .env.local-wins rule while the tsx scripts get the same result from flag order.
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
1 participant
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.
SYNC_MODEis required with no default. If it is unset when this code deploys:workergetConfig()atsrc/worker/index.ts:16)webgetConfig()is lazyThere is no automatic rollback.
fly.tomldefines no[checks]or[[http_service.checks]]. Setting the secret first is the safety net, becausefly secrets settriggers its own rolling restart.Why
npm run workeragainst real credentials destroys real data:STANDINGS_LABELthat isn't a member — 130 in one production runtoken-healthrotates EVE refresh tokens, and EVE rotates on use — so a local worker pointed at a copy of production silently invalidates production's stored tokens. No API named "delete" is called; the damage is a side effect of a read-shaped operationNothing warned a developer before now.
Design
Full rationale:
docs/superpowers/specs/2026-08-03-local-dev-setup.md(included in this PR).Enforcement sits at the boundaries a job cannot bypass, not inside each job, so a destructive call added later is guarded the day it's written:
getFreshAccessTokenreason: "dry_run"postOpsWebhookOrThrowresolves, never throws (throwing would make the dead-letter handler retry forever)Audit correctness
Guarded
voidmethods return normally, so the wanderer and discord-roles jobs would otherwise record success for work that never happened. In dry-run they write no audit rows and reportwouldAdd/wouldRemove/wouldUnblock/wouldChangeRoles.audit_logis what an operator reconstructs an incident from and must never claim a mutation that didn't occur.Accepted limitation
Refusing the token refresh means the contacts job can't read contacts, so dry-run shows no contacts diff — every character reports as skipped. Wanderer and Discord preview fully (they use the ACL key and bot token). The alternative — refresh but don't persist — would invalidate the production token and discard its replacement.
Why no default
Every alternative has a silent failure mode. Defaulting to
dry-runlets a missing production secret turn sync into an unnoticed no-op; defaulting tolivemakes the destructive configuration the one you get by forgetting.Verification
Baseline was 271; all originals still pass, plus 17 new safety tests asserting absence — zero requests issued, zero audit rows written — each paired with a live-mode case proving the msw handlers are really wired.
Worker banner, run for real:
npm run test:e2ewas NOT run. Port 3111 was held by another workstream's dev server, andreuseExistingServerwould have run the specs against that build — a meaningless pass. Instead the app was booted on a free port with exactly the playwright env block plusSYNC_MODE=dry-run:GET /login -> HTTP 200. The full e2e suite still needs a run.Reviewer focus
src/jobs/wanderer.tsandsrc/jobs/discord-roles.ts— everylogAuditin a mutation path gated on!dry. Highest-severity area.src/lib/sync-mode.ts— the guard itself.src/jobs/contacts.ts:95—counts.targets--in thedry_runbranch nets the earlier++to zero.Notes
scripts/wanderer-smoke.tsnow refuses to run in dry-run. Without it the suppressed add surfaces as"ADD not visible on re-read", which reads like a broken Wanderer instance rather than a mode mismatch.testConfig(), which is why one new env var touched 23 files. Left alone — out of scope, but a real maintenance tax.tests/helpers/config.tsdefaults tolivedeliberately: 13 test files assert live behavior through it, and defaulting to dry-run would suppress the requests they exist to verify while leaving them green.Out of scope
sync_run.modecolumn — a migration; deferred pending discussion.purgedeletes rows directly and no HTTP-boundary guard can help. Documented indocs/ops.md— never put a productionDATABASE_URLin a local.env.🤖 Generated with Claude Code