fix(sql): wire sql.engine/allowFullScan/maxSortRows/maxHashRows to real config - #2484
Open
kriszyp wants to merge 11 commits into
Open
fix(sql): wire sql.engine/allowFullScan/maxSortRows/maxHashRows to real config#2484kriszyp wants to merge 11 commits into
kriszyp wants to merge 11 commits into
Conversation
sqlEngine/config.ts read engine/allowFullScan/maxSortRows/maxHashRows from globalThis.harperConfig.sql, which nothing in production ever assigned — only three unit-test files set it as scaffolding. A value set under sql.* in harperdb-config.yaml never reached the SQL engine; sql.engine only appeared to work because it also has a HARPER_SQL_ENGINE env fallback. Register the four keys in CONFIG_PARAMS (utility/hdbTerms.ts) and read them via configUtils.getConfigValue(), the same accessor every other config domain uses. getConfigValue() returns undefined pre-boot rather than eagerly initializing from disk, preserving the "works without a fully booted config" property the globalThis branch existed for, and it self-initializes correctly per worker thread with no new boot hook to wire in. Delete the globalThis branch entirely. Switch the three scaffolding test files (join/mutation/aggregate) from mutating globalThis.harperConfig to configUtils.updateConfigObject(), the already-sanctioned in-memory config override unit tests use elsewhere. Add router.test.js coverage proving sql.engine/allowFullScan/ maxSortRows/maxHashRows are actually read from Harper config (not just the env var), and that the env var still wins for sql.engine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
Addresses a plan-mode cross-model review (Framing-Verdict: better-alternative-exists) of the prior commit's accessor-swap fix: - Add a scoped `sql` Joi schema (validation/configValidator.ts) so a malformed sql.* value (bad engine enum, wrong-typed allowFullScan, non-positive/non-integer row caps, an unknown key) is rejected loudly at boot or on set_configuration, instead of silently keeping the default — the top-level schema's allowUnknown:true previously let an entire malformed `sql:` section through unvalidated. - Correct sqlEngine/PLAN.md's stale `sql.engine.allowFullScan` / `sql.engine.maxSortRows` / `sql.engine.maxHashRows` phrasing to match the actual sibling-key shape SqlEngineConfig has always used — the review flagged this as a real doc/code contradiction an operator could be misled by. - Switch the sql.* test scaffolding (join/mutation/aggregate.test.js, and this fix's own router.test.js coverage) from blindly resetting to `undefined` to snapshot/restore, so a suite doesn't clobber a value set by another one sharing the same mocha process. - Add getSqlEngineConfig() coverage for wrong-typed/unrecognized config values (defense-in-depth: Joi's coercion at validate time is never written back into flatConfigObj, so the accessor's own typeof guards are what actually protect a live read). - Add registration + set_configuration rejection tests (unitTests/config/setConfigurationSql.test.js) and Joi schema tests (unitTests/validation/configValidator.test.js), following existing precedents (replicationReceiveQueueParam.test.js's registration pattern, the blob-gap-floor schema tests) rather than exercising setConfiguration()'s full success path, which would write to this box's shared on-disk test config. Deliberately not adopted, with disqualifiers recorded in the PR body's "For the human reviewer" section: resolving one config snapshot per SQL statement (the review's hot-path suggestion), and a full HTTP integration boot test for sql.engine/allowFullScan specifically (the 'auto' engine's legacy fallback masks the config-driven difference at the HTTP-observable level, so a naive version of that test would pass on both the fixed and the reverted code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
Adopts the concrete, fixable findings from the full pre-push review (gemini + cursor-composer + Harper domain adjudication) of the previous two commits: - sqlSchema now sets convert:false, so a quoted allowFullScan:"true" or maxSortRows:"500" in harperdb-config.yaml is REJECTED at boot instead of silently passing Joi (which coerces it) and then being dropped by getSqlEngineConfig()'s typeof guard — validateConfig() never writes the coerced value back into configDoc for sql the way it does for threads/logging/storage, so leaving convert:true on would have made the new schema's strictness a no-op for exactly the scenario it exists to catch. - Tighten maxSortRows/maxHashRows's defense-in-depth guard from typeof === 'number' to isPositiveInteger (rejects NaN/negative/ fractional caps too — NaN in particular defeats PhysicalSort's `buf.length >= cap` guard entirely, since every comparison against NaN is false). - router.test.js: clear the four sql.* keys before each test instead of only snapshotting, so the default-value assertions can't go red on a machine whose own harper-config.yaml already sets one of them; add a flattenConfig() unit test covering the nested-to-flat key derivation the other tests bypass via updateConfigObject(). - join.test.js: drop three per-test SQL_ALLOWFULLSCAN=true reassignments already covered by the describe's beforeEach. - Trim added comments that narrated intent/history rather than documenting a non-obvious invariant, per Harper's zero-new-comments default. Not adopted, both already covered as open decisions carried into the PR body's "For the human reviewer": resolving one config snapshot per SQL statement instead of per-call reads (unchanged from the plan review — no per-statement context exists at the router/optimizer layer to hang it on), and registering sql_engine's bare-env-var reachability (SQL_ENGINE), which the domain leg flagged as colliding with a common external convention (e.g. Django) — a real, if graduated, availability risk shared in kind with ~150 other existing bare CONFIG_PARAMS names, surfaced to the task owner rather than resolved unilaterally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
Both the codex and gemini legs of the delta review flagged several added comments as narrating test/bug history or restating what the test names already say rather than documenting a non-obvious invariant. Trims those; keeps the two comments codex specifically called out as explaining a real invariant (the sql Joi schema's convert:false rationale, and why the set_configuration rejection test needs no on-disk config fixture). Also independently re-verified (not adopted) two other delta-round gemini findings against the actual code and the passing test suite: the claimed ReferenceError from bare string/boolean/number in configValidator.ts (destructured from Joi.types() at the top of the file — 508 tests exercising that schema all pass) and the claimed Joi abortEarly:true truncating the maxSortRows/maxHashRows rejection test (validateConfig() explicitly passes abortEarly: false — the test asserting both messages together already passes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
Codex's third delta round flagged the remaining setConfigurationSql.test.js preamble as restating what the parameterized test names already say. Also independently verified (not adopted) gemini's round-3 "blocker" claim that getConfigValue()/flattenConfig() have a casing mismatch — both explicitly lowercase before the flatConfigObj lookup (config/configUtils.ts's getConfigValue return line and flattenConfig's squashObj), and this PR's own flattenConfig() derivation test already exercises and passes that exact path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
Contributor
There was a problem hiding this comment.
Code Review
This pull request flattens the SQL engine configuration parameters (such as engine, allowFullScan, maxSortRows, and maxHashRows) under the main configuration utility instead of nesting them under globalThis.harperConfig. It updates configuration resolution, validation schemas, and associated unit tests to use these new flat keys. Feedback on the changes suggests importing plain 'node:assert' instead of 'node:assert/strict' in the new test file to comply with the repository's linting rules.
gemini-code-assist flagged unitTests/config/setConfigurationSql.test.js's node:assert/strict import as against repo house style (.gemini/styleguide.md: plain node:assert + explicit assert.strictEqual/deepStrictEqual). The file already only calls .strictEqual/.rejects, so the swap is semantically a no-op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytbJQ44LZsFyZ1C1fFdNk
kriszyp
marked this pull request as ready for review
September 3, 2026 22:09
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
reviewed
Sep 3, 2026
Snapshot and clear HARPER_SQL_ENGINE alongside the sql.* config keys so external environment settings cannot override the config-integration assertions. Also isolate the existing router tests from a machine-local sql.engine value now that the router reads live Harper config. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
…boot
`sql` is now a validated core config section, but the root config namespace is
shared with application entries: `deploy_component project=sql package=x` wrote
`sql: {package: x}` and reported success, and the next restart failed config
validation with no way out but hand-editing the YAML.
Reserve the name at every ingress that creates a component under it — the
deploy/add validators, `set_component_file` (creation only), and
`set_configuration`'s `<component>_package`/`_port` escape, which maps straight
into a root entry without passing through either operation. `force` does not buy
the name: there is no core component to overwrite, only config to break.
An application deployed under the name before it was reserved still boots. The
`sql` entry validates as an application entry when it carries one of the keys a
deploy writes, and as the settings schema otherwise, so a typo'd setting still
fails loudly; boot warns to rename. The two shapes cannot be mixed.
Co-Authored-By: Claude Opus <noreply@anthropic.com>
…rved name Review round 2. `set_env_value` creates the project directory the same way `set_component_file` does, and `harper deploy setup=true` would seal a credential for a component name the server then refuses — both now go through the reservation. The grandfather check treats an unresolvable components root as "not there" so it fails closed to the reservation instead of erroring, which is also what the Windows unit job (no ambient install) exercises. Widens the legacy-application key list to every deployment key componentLoader reads off a root entry, so a grandfathered entry cannot be mistaken for engine settings and fail boot. Drops the sinon/rewire tests the house style forbids: the deploy handler cases now call the real operation, and the file-writer cases pin a temporary components root and cover both sides of the grandfather check. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…ation Review round 3. `sql: false` (and `sql:`) is how componentLoader spells a disabled component, so an operator who had already turned a pre-reservation `sql` application off would have hit the very boot failure this change exists to prevent; both are now accepted. The reservation matches case, like the config param lookup it protects. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…rotects Review round 4: matching case-insensitively refused a redeploy of an existing component named `SQL` — a distinct root key that collides with nothing — and did it with a message naming a configuration section that does not exist. Co-Authored-By: Claude Opus <noreply@anthropic.com>
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
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.
sqlEngine/config.tsreadsql.engine/allowFullScan/maxSortRows/maxHashRowsfromglobalThis.harperConfig, which nothing in production ever assigned — only three unit-test files set it as scaffolding. A value undersql:inharperdb-config.yamlnever reached the SQL engine;sql.engineonly appeared to work because it also has aHARPER_SQL_ENGINEenv fallback, and the other three caps had no fallback at all and were permanently stuck at their compiled-in defaults. This registers the four keys with the real config layer (CONFIG_PARAMS+configUtils.getConfigValue(), the same accessor every other config domain uses) and deletes the deadglobalThisbranch, plus adds Joi validation for the section so a malformed value fails loudly instead of silently keeping the default.Making
sql:a validated core section collided with the fact that Harper's root config namespace also holds application entries:deploy_component project=sql package=…wrotesql: {package: …}, reported success, and made the next boot fail config validation with no recovery but hand-editing the YAML. Sosqlis now a reserved component name, refused at every ingress that creates a component under it, and an application already deployed under the name is grandfathered by a shape-switched schema plus a rename warning.For the human reviewer
Framing: the planning gate returned
better-alternative-existsand the alternative was adopted, rewriting the chosen approach: the reservation is a shared predicate inutility/componentNames.ts(the module the CLI and server already share for name derivation) applied to every component-creation ingress, rather than a check on the two obvious operations. That is what caught theset_configuration sql_package=xbypass —findUnrecognizedParams()deliberately admits arbitrary<component>_package/_portnames andupdateConfigValue()maps them straight into a root entry, so the deploy guard alone would not have held.sql, or every core root section? Onlysqlis reserved. Every other section (storage,node,replication,models…) inherits the top-levelallowUnknown: true, so a component deployed under one of those names validates and boots today — it silently replaces that section's settings instead of failing, which is a real but different bug. Widening the set is additive and cheap later; each widening is a new rejection for someone who has such a component deployed now. Left for a follow-up rather than decided here (not yet filed as an issue).deploy_componentalready refusesgraphql,http, … with a 409 thatforce: trueoverrides. This adds a distinct rule with a 400 thatforcecannot override, because there is no core component to overwrite — forcing would only let the operator write config that breaks their next boot. Two mechanisms with differentforcesemantics now coexist; collapsing them later changes whatforcemeans for operators.sqlapplication is frozen for deploys, editable for migration.set_component_fileandset_env_valuerefuse the name only when the project directory does not exist yet — a filesystem probe inside a Joi validator, and a second "is this legacy?" heuristic beside the config-key one. The uniform alternative (refuse always, migrate through drop/redeploy) is simpler but leaves an operator unable to patch the app they are being told to migrate.sqlapplication is frozen for redeploys. It keeps loading, but it cannot be redeployed or reconfigured —deploy_componentrefuses the name unconditionally,forceincluded. That is deliberate friction to force the rename, and the boot warning is the only migration guidance shipped. The alternative (allow redeploy while the entry exists) keeps the ambiguity alive indefinitely.false/null, componentLoader's spelling for a disabled component, is accepted so an operator who had already turned such an app off still boots. The pre-push review asked for historical mixed entries to be accepted and new ones blocked in mutation validation instead; declined, because "historical" is only distinguishable at a mutation — a hand-edited YAML, a restored backup or a replicated write reaching boot validation has no prior state to diff against, so the rule would be unenforceable exactly where the ambiguity does its damage. No supported path produces a mixed entry:deploy_componentrebuilds the whole entry from request fields, andsql.enginewas unreachable in production before this PR.sql.engine.*.sql.engineships as its own scalar mode string, withallowFullScan/maxSortRows/maxHashRowsas siblings undersql— not nested underengineassqlEngine/PLAN.md(now corrected) used to describe. This matchesSqlEngineConfig's pre-existing shape and every existing test's scaffolding. Reversible only before this ships: once a scalarsql.engineis in a customer's YAML, moving to asql.engine.*section later is a breaking rename.getSqlEngineConfig()is still read fresh at each of its ~5 call sites (router, scan validator, hash-join/hash-aggregate build, sort) rather than resolved once into a per-statement snapshot. The alternative — attaching a resolved config toSqlEngineContext— only covers the physical-execution half of the pipeline; the router and the optimizer'svalidateScannablerule run before anySqlEngineContextexists, so it would need new plumbing invented for those two call sites too. Cost of not doing this: aset_configurationwrite landing mid-query could theoretically seeallowFullScan/caps change between planning and execution within one statement — accepted, since config is restart-required everywhere else in Harper already, and the per-call cost is a handful of map lookups per statement, not per row.sql.*Joi validation usesconvert: false.validateConfig()only writes Joi's coerced value back intoconfigDocforthreads/componentsRoot/logging/storage/operationsApi— neversql— so leaving Joi's defaultconvert: trueon would have accepted a quotedallowFullScan: "true"and then silently dropped it (the accessor'stypeofguard rejects the still-unconverted string).convert: falsemakes that combination fail loudly at boot/set_configurationinstead.sql_enginealso exposes the bare env varSQL_ENGINE. EveryCONFIG_PARAMSentry is reachable as an unprefixed environment variable onharper run(bin/run.ts'sassignCMDENVVariables), andSQL_ENGINEis also a common external convention (Django, other ORM tooling) that could plausibly already be set, unrelated to Harper, in a shared.env/compose file — with the new Joi enum validation, such a collision now makesharper runabort instead of silently no-op. Discussed directly with the task owner: accepted as consistent with ~150 other existing bareCONFIG_PARAMSnames carrying the same class of risk; a general "opt a param out of the bare-env-var scan" mechanism is left for a follow-up rather than special-cased here (not yet filed as an issue).set_configuration sql_*still requires a restart, like every other config param: the write lands in the file without updating the in-memory flat config. The engine flag is the kind of knob an operator might expect to flip live during a cutover, but no config domain in Harper propagates live across threads.sqlthat already exists (it has no root-config entry, so nothing fails validation; its operator finds out at the next redeploy, which is refused), andHARPER_BUILTIN_COMPONENTS=sql, which registers a trusted plugin by name and bypasses operation validation by design.Verification
npm run build,npm run lint:required,npm run format:writeclean.npm run test:unit:main: 5334 passing, 1 failing — a pre-existing domain-socket-path-length assertion that resolves a relative rootPath againstprocess.cwd()and fails from any agent worktree, identical onorigin/main.npm run test:unit:resources2004 passing / 0 failing;npm run test:unit:bin252 passing.npm run test:integration -- "integrationTests/deploy/**/*.test.ts": 61 passing / 0 failing — real deploys still run through the new validator unchanged.RESERVED_COMPONENT_NAMESturns all 10 reservation assertions red acrossunitTests/components/reservedComponentName.test.js,unitTests/config/setConfigurationSql.test.jsand the deploy handler cases; the grandfather schema cases fail on the previous commit, where asql: {package}entry is rejected with'sql.package' is not allowed. The foursql.*accessor assertions inunitTests/sqlEngine/router.test.jsstay red onorigin/main.deploy_componentwith an explicitsql, a canonicalizedsql.tgz, a package-derivedsql, the payload form, andforce: true(400, through the real handler, which derivesprojectbefore validating);add_component;set_component_fileandset_env_valueagainst a pinned temporary components root, covering both sides of the grandfather check;set_configuration sql_package/sql_port; and the CLIdeploy setup=truename check.engineenum, a quotedallowFullScan, non-positive/non-integer row caps, an unknown key,{},false/null (componentLoader's disabled-component spelling, which the closed settings schema would otherwise have failed at boot — found by the pre-push review), an application-shaped entry, and a mixed entry.sql_*value from aset_configurationbody throughcastConfigValue/flattenConfigtogetSqlEngineConfig(); each side is covered in isolation, and a break there fails loudly at validation rather than silently. The boot rename warning is covered only through its predicate —validateConfig()is not exported and AGENTS.md forbids newsinon/rewireuses, so the onelogger.warncall behind the tested predicate has no direct test. No API-to-restart integration test was added.Complexity: medium
Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=9 @ 6a66d45
Human-Review-Need: 3 @ 6a66d45