feat(server-core): make dev mode opt-in and stop respecting NODE_ENV - #11959
paveltiunov wants to merge 64 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
github-actions
Bot
added
docs
|
Claude finished @paveltiunov's task in 4m 46s —— View job Review complete — the security fix is right; 1 high, 3 medium, 4 low. 5 inline comments posted.Full review
The core change is correct and the call-site sweep is complete — High — a bare instance now boots as a refresh worker
With neither
Before, Either fold
Medium — the one config this PR targets gets no diagnosticThe warning at which says nothing about dev mode, so the upgrade reads as an unrelated driver-config regression. Worth appending a clause to that throw when Medium — see inline
Low
Test/build verificationNot run here — the sandbox refused the chained Review threadsNo prior threads — the listing returned 0 of my own threads, 0 resolved, 0 re-affirmed. First round on this PR. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @paveltiunov's task in 5m 20s —— View job Review: 2 new inline findings, 1 cross-cutting question — no blockers beyond the dev-server overrideFull review
The core of the change is right: Posted this round
On the two deliberately-deferred Fold in
Tests Coverage of the new I could not run the suites here — Review threads: the listing returned 5 threads of mine, all still live; 0 resolved. 3 of this round's findings were re-affirmed rather than re-posted as duplicates of existing threads — |
|
Claude finished @paveltiunov's task in 4m 46s —— View job Review: 2 new findings (1 high, 1 medium), 2 prior threads re-affirmedFull review
VerdictThe direction is right and the sweep is thorough — after this change the only Two things the sweep missed. 1. 2. Checked and fine
Not verifiedI could not run the test suites from this environment (the sandbox declined the Review threadsThe listing returned 2 of my own threads; 0 resolved, 2 re-affirmed as still live ( |
|
Claude finished @paveltiunov's task in 5m 48s —— View job 1 medium, 2 low — the security tightening is right and the CLI/Docker paths check out; the deprecation warning misses the one population it exists for. Review details
Findings posted inline
What I verified and found sound
Not verifiedCould not run the test suites — Review threadsThe listing returned 2 of my own threads; 0 resolved, 2 re-affirmed as still live:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11959 +/- ##
==========================================
+ Coverage 80.52% 80.60% +0.08%
==========================================
Files 508 508
Lines 107359 107445 +86
Branches 4043 4072 +29
==========================================
+ Hits 86450 86608 +158
+ Misses 20337 20259 -78
- Partials 572 578 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
On the item the last review listed under "Considered and not posted" — My guard asked whether the variable was non-blank. cubesql's // The spellings cubesql's env_parse_bool honours. Anything else - empty,
// `1`, a typo - it warned about and fell back on, so it chose nothing
let redaction_chosen = env::var("CUBEJS_LOG_REDACTION")
.is_ok_and(|v| matches!(v.trim().to_lowercase().as_str(), "true" | "false"));The "parsing it again would warn twice" reasoning you cite as adjudicating it is about not re-parsing, which this still doesn't do — it only asks whether a usable value is there. The two are separable, and conflating them is what left the half-fix in place. The reachability caveat you noted stands and is unchanged: a server-core process throws in Same push takes your wording for the block comment, per my reply on that thread — it keeps the sentence saying why the override exists, which mine had dropped.
Generated by Claude Code |
|
Claude finished @paveltiunov's task in 7m 37s —— View job Review: no blocking issues — 1 medium, 3 low, posted inlineFull review
Findings
What I checked and found soundDev-mode resolution is single-sourced.
The The schema pin holds up under the concurrency it claims to. Symbol-keyed shares mean a repeated
Docs. Not verifiedNo Review threadsThe listing returned 0 unresolved threads (both pages empty), so none of mine were resolved and none re-affirmed. All four findings above are new and were posted inline. |
The cores built there take the pin in their constructor and are never shut down, so the first case with CUBEJS_DEV_MODE=true left dev_pre_aggregations set for the rest of the file. Every later case resolving its own schema then hit the conflict branch, printing the "already set for this process" warning into the suite output - one occurrence before this, none after. Behaviour was unaffected, since userPreAggregationsSchema() hides a value this process pinned, but a warning that fires because of test bookkeeping is one nobody will read when it means something. Uses the two lines OptsHandler.test.ts already has. Also corrects a comment in container.test.js that still named OptsHandler as the site that pins. It has not been since the pin moved to the end of the CubejsServerCore constructor, which is what two of the OptsHandler tests are about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
Review in progress
|
The native guard re-matched `true`/`false` itself to decide whether CUBEJS_LOG_REDACTION was chosen, while ConfigObjImpl decides the same thing by parsing it. Two copies that are only correct while they agree: widen the parse on the cubesql side and a value it honours would read as unchosen here, so `c.log_redaction = !dev_mode` would overwrite a preference the user did express - quietly, since cubesql's own parse succeeded. cubesql now has one parse_bool behind both env_parse_bool and a new env_bool_is_set, and the bridge calls the latter. The `pub` is on a function that has an external caller, which is what the earlier round objected to about env_parse_bool. Also documents what `devServer: false` with CUBEJS_DEV_MODE=true does to the SQL API. That paragraph listed the Cube Store loss and the schema move but not the half with security consequences: the SQL API keys off the variable alone, so the Postgres endpoint still comes up on 15432 accepting any credentials while the REST and GraphQL APIs now enforce JWT. Before this change the instance was in development mode throughout and the open SQL API matched an equally open HTTP API; now it presents as authenticated while that port is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
|
Claude finished @paveltiunov's task in 8m 4s —— View job Review round complete — no correctness defects found; 3 low findings posted inline (0 high, 0 medium, 3 low)Full review
Findings
Considered and not posted
What I verified
Review threadsThe listing returned 0 of my own unresolved threads (paged to the end, both pages empty of unresolved nodes) — nothing to resolve and nothing to re-affirm. All three findings above are new. Not run
|
|
CI triage: Five That line is Why it isn't the diff: I ran those five tests here and they pass, but I'll be precise about what that does and doesn't show: Not spending the re-run on Generated by Claude Code |
The message ended with "run one Cube instance per process", which is no help to the likeliest way of reaching it: one instance, a CUBEJS_PRE_AGGREGATIONS_SCHEMA the user set, and a CreateOptions.preAggregationsSchema that overrules it through `...opts`. There is no second instance to separate, and the other half of the advice - align the variable - names the wrong fix, since dropping either setting works. pinnedPreAggregationsSchema being set is exactly "this process pinned it for an earlier instance", so the two cases are one condition apart. Each now ends with the remedy that applies to it. Both branches are on the tests that already cover them, asserting the advice they get and that they do not get the other one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
Review in progress
|
Three blocks restated what the line under them says or what another site already argues. In lookupConfiguration the "measures the user's own config" sentence is the two statements below it read aloud, and "kept in sync for user config code" repeats the devServer block three lines up; the take-back's second line described its own condition. The field JSDoc in server-core re-argued release-by-identity, which env.ts and shutdown() each state at the point they rely on it - three copies of one rationale, stale together the day it changes. What is left at each site is the part that is not recoverable from the code: the ordering constraint, that `cube.js` sees NODE_ENV, the scoping of the take-back, and what the field holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
Review in progress
|
…NODE_ENV The previous commit cut that note from container.ts along with the restatement around it, which left the limitation recorded nowhere: a `cube.js` that assigns `development` itself cannot be told apart from this run's own write, so the take-back claims it. Without that, this test reads as asserting an arbitrary choice, and it is the case someone would try to "fix". It belongs on the test rather than back in the source, where the line it sat on now says only what it scopes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
|
Claude finished @paveltiunov's task in 7m 20s —— View job No functional defects this round — 0 high, 0 medium, 4 low (all comment/test hygiene)Review detail
ThreadsThe listing returned 0 unresolved threads authored by me (both pages; What I checked and found cleanDev-mode resolution agrees everywhere it should.
The schema pin's ordering holds. The pin does not leak into anything else that scans the environment. The NODE_ENV take-back in Rust.
Findings
Not verified
|
The previous commit added a note that the test already carried in different words, leaving two blocks stacked above it saying the same thing. My check for whether the point survived an earlier trim grepped for one word rather than the idea, so it came back empty and I restored something that was never gone. Merged into the one sentence that is load-bearing: the two writes are indistinguishable, and the config settles which one wins. Which readers still consult NODE_ENV belongs to refreshWorkerMode and detectQueueAndCacheDriver, where it is already said, not to this test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
Review in progress
|
…hey belong
Batched, so the CI matrix restarts once rather than four times.
- container.test.js dropped a `stubConfigurationFile({})` that the test overwrites
twelve lines later with the same argument before rebinding the loader over it.
Nothing asserted depended on it.
- The header comment about `lookupConfiguration` sat above `TestServerContainer`,
which the paragraph under it explains. Moved to the helper it describes, so a
reader at the class is not handed two unrelated justifications.
- env.ts said twice, above the value and above the ternary, what the `remedy`
branches name themselves. Only the keying survives, at the call it explains: it
is per message, so a constant key lets one instance silence the next.
- The DatabricksDriver fallback said "It pins CUBEJS_PRE_AGGREGATIONS_SCHEMA
above" - an ambiguous subject pointing at another package. Names server-core as
the writer and the branch above as the reader.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s
|
Claude finished @paveltiunov's task in 5m 54s —— View job Review complete — no findings. 0 high, 0 medium, 0 low.Details — what was checked, and the notes that did not rise to findings
Dev mode resolutionAll four sites resolve identically —
The
|
This branch was successfully deployed
3 participants
Check List
Description of Changes Made
Development mode was on whenever
NODE_ENVwas anything butproduction, so aninstance with neither
NODE_ENVnorCUBEJS_DEV_MODEset came up in dev mode —an authentication bypass — without anyone asking for it.
Dev mode is now decided by
CUBEJS_DEV_MODEalone and defaults tofalse.NODE_ENVis deprecated for this decision and ignored; a one-time warning is printed when a
non-production
NODE_ENVis seen withCUBEJS_DEV_MODEunset.The decisions that keyed off
NODE_ENVas a stand-in for dev mode now follow theresolved dev mode:
env.tsdevModeCUBEJS_DEV_MODEonly, but callers OR'd inNODE_ENVNODE_ENVuseOptsHandler.isDevMode()NODE_ENV !== 'production' || devModeCreateOptions.devServer ?? devModeCUBEJS_LOG_REDACTIONdefaultNODE_ENV=productionCubejsServerCoreloggerNODE_ENV=productionApiGateway.enforceSecurityChecksNODE_ENV === 'production'!devServerNODE_ENV=productionSQLServernative loggerNODE_ENV=productionDatabricksDriverpre-agg schemadev_pre_aggregationsunlessNODE_ENV=productionConfigObjImpl::default(Rust)isDevModerule, feeding the SQL API'sCUBEJS_LOG_REDACTIONdefaultCUBEJS_DEV_MODEonlyCreateOptions.devServeris authoritative where an embedder set it, and the gateway,CubejsServerCoreandOptsHandlerall resolve it the same way, so no two of them candisagree about whether an instance is a dev server.
cubejs dev-server. It asks for dev mode throughCreateOptions.devServer, not bywriting
CUBEJS_DEV_MODE. That matters because the variable also gates the SQL API'sdefault port and its password check: writing it would have served an unauthenticated SQL
API wherever a port is configured, which master never did. With it left unset, everything
that follows the resolved dev mode sees a dev server, and everything still keyed on the
variable —
pgSqlPort, the SQL password check, the Databricks schema — behaves exactlyas on master. An explicit
CUBEJS_DEV_MODEwins over the command, and adevServerincube.jswins over both.NODE_ENVis synced from the resolved value as a compatibilityshim for user config code, and the CLI calls
markDevModeResolvedByCaller()so thedeprecation warning does not fire against Cube's own
NODE_ENV.DevServer.initDevEnvonly runs in dev mode, so itsNODE_ENV-branched banner collapsedto the single "authentication checks are disabled" line.
Behavior changes worth calling out
server-corewith no env vars now throwsEither CUBEJS_DB_TYPE or CreateOptions.driverFactory must be specifiedinstead ofsilently starting a dev server. That is the point of the change, but it is a visible
break for anyone who relied on the implicit dev mode.
devServer: trueunderNODE_ENV=productionwithCUBEJS_DEV_MODEunset loses JWT enforcement. On master that config mountedPlayground while the data APIs stayed authenticated; dev mode now follows the option,
so
/v1/loadand GraphQL accept requests with no token, GraphiQL and stack traces areserved, log redaction defaults off and pre-aggregations move to
dev_pre_aggregations.This is the only change here that turns authentication off where it was on, and it has
its own paragraph in
DEPRECATION.md.devServer: falsewithCUBEJS_DEV_MODE=true, loses the bundledCube Store default and moves from
dev_pre_aggregationstoprod_pre_aggregations.It also keeps the SQL API on
CUBEJS_DEV_MODE, so the Postgres endpoint stays open on15432while the REST and GraphQL APIs start enforcing JWT — called out inDEPRECATION.mdwith both remedies.one JSON object per line, which breaks anything that greps Cube's stdout.
CUBEJS_DEV_MODE=truecombined with an explicitNODE_ENV=productionno longerenforces JWT checks. Under the CLI and the official Docker images that combination
already behaved this way, so only direct
server-coreembedders with thatcontradictory config are affected.
All of these are documented in
DEPRECATION.md.The pre-aggregation schema pin
A driver never receives
CreateOptions, soDatabricksDriver.getPreAggrSchemaName()—the only reader of
CUBEJS_PRE_AGGREGATIONS_SCHEMAoutside server-core, and only when acatalogis configured — would resolve its own schema fromCUBEJS_DEV_MODEand couldname a different one than the instance.
CubejsServerCorenow writes the schema itresolved into that variable when the user has not set one, as the last statement of its
constructor, and releases it on shutdown. Shares are keyed by identity, so a repeated
shutdown cannot spend another instance's share and a reload's drop invalidates the shares
it drops. A user-set value is never overwritten, and a per-tenant
preAggregationsSchemafunction has no single schema to write, so that case is left alone and warned about.
Deliberately left alone
NODE_ENVstill drives two defaults that are not the dev-mode decision:refreshWorkerMode(env.ts, background refresh on whenNODE_ENV !== 'production')and
detectQueueAndCacheDriver(QueryOrchestrator.ts, cubestore vs memory). Switchingthose would silently stop background refresh or change the queue driver for bare setups.
Both sites carry a comment saying so.
Two reads stay on
CUBEJS_DEV_MODEalone and so can disagree with the resolved value fora
devServerembedder: the SQL API password check (sql-server.ts), which is what keepscubejs dev-serveron the generate-a-password path, andDatabricksDriver.getPreAggrSchemaName(), which cannot seeCreateOptionsat all — adriver receives no
preAggregationsSchema, so fixing it properly means changing thedriver construction contract repo-wide. Both sites are commented and the Databricks case
is in
DEPRECATION.mdwith theCUBEJS_PRE_AGGREGATIONS_SCHEMAworkaround. Happy toopen the driver-config PR separately.
Docs
DEPRECATION.mdcovers theNODE_ENVremoval, the pre-aggregation schema move, thebundled Cube Store loss, the log format change, the SQL API asymmetry and both
CreateOptions.devServerdirections. The dev-mode warning snippets, theCUBEJS_DEV_MODE,CUBEJS_LOG_REDACTIONandCUBEJS_PRE_AGGREGATIONS_SCHEMAreference entries, the Coredeployment pages and the dev-mode page were updated to drop the
NODE_ENVguidance.Testing
CI is green on the head of this branch:
lint,unit (24.x, 3.13),unit (26.x, 3.13),unit-core,build-cubestore,Check fmt/clippy,Unit (Rewrite Engine), CodeQL, thenative build matrix, every integration job and every driver suite.
Locally:
yarn tscandyarn lintclean;cubejs-backend-shared549/549,cubejs-server22/22,cubejs-server-coreOptsHandler48/48 andindex46/46;cargo fmt --all -- --checkandcargo clippy --locked --all-targets -- -D warningsclean in both
rust/cubesqlandpackages/cubejs-backend-native. (cubejs-api-gatewayis covered by the
unitjob rather than locally — its native-dependent suite cannot runin my container.)
New coverage for
getEnv('devMode'), the deprecation warning, the log-redaction defaultand its caller-resolved override,
CreateOptions.devServerin both directions,enforceSecurityChecksresolution, the fullcubejs dev-server/cubejs servermatrixincluding the SQL API port sources, the
NODE_ENVtake-back in all four directions, andthe pre-aggregation schema pin's lifecycle — a refused pin, a throw before and after the
options resolve, a repeated shutdown, a share invalidated by a reload's drop, and the
conflict warning's two remediation branches.
Earlier red runs on intermediate commits were environmental and are triaged in the
comments: live-warehouse contention (Snowflake, BigQuery, MSSQL, Firebolt, one Databricks
shard), a crates.io outage on an aarch64 runner, a testcontainer that could not reach the
Debian repos, and
RefreshScheduler › Exponential backoff, whose assertion this PR doesnot touch — a patch for that one is proposed in a comment, for a separate PR.
🤖 Generated with Claude Code
https://claude.ai/code/session_01LTfc44Gdfu7dEACHzqQD3s