feat(dashmate): configure docker build args via config#3764
Conversation
Adds a `dockerBuild.buildArgs` map on the dashmate config schema so
per-image Docker build args can be pinned in the config file (and
overridden per-invocation through shell env), without having to bake
them into the compose YAML by hand or into yarn scripts.
Pieces:
- **Schema** (`src/config/configJsonSchema.js`): `dockerBuild.buildArgs`
is `{ [string]: string }`. Common keys today are `CARGO_BUILD_PROFILE`
(Rust profile) and `SDK_TEST_DATA` (compile-time cfg flag), but it's
open-ended.
- **Config helpers** (`src/config/Config.js`,
`src/commands/config/set.js`): traverse the schema correctly when the
user does `dashmate config set
platform.drive.abci.docker.build.buildArgs.SDK_TEST_DATA true`
(the original pre-check rejected map-shaped paths; this PR fixes that
and pins it with `Config.spec.js::buildArgs`).
- **Env forwarding** (`src/config/generateEnvsFactory.js`): both
`platform.drive.abci.docker.build.buildArgs` and
`platform.dapi.rsDapi.docker.build.buildArgs` are merged and exported
as env vars at compose-spawn time so the compose `${KEY:-default}`
substitution picks them up. Dashmate config is the single source of
truth — `process.env[key]` is not consulted as fallback.
- **Compose YAML** (`docker-compose.build.drive_abci.yml`,
`docker-compose.build.rs-dapi.yml`): forward `CARGO_BUILD_PROFILE` to
the Dockerfile `ARG` (`${CARGO_BUILD_PROFILE:-dev}`).
- **Default config**
(`configs/defaults/getBaseConfigFactory.js`): `buildArgs: {}` on both
rs-dapi and drive-abci build blocks so the schema accepts overrides.
Yarn-script side (`scripts/setup_local_network.sh`):
- The local-network setup script pins `SDK_TEST_DATA=true` on each
`local_N` config so the genesis SDK test data runs at devnet
bring-up. `CARGO_BUILD_PROFILE` is deliberately NOT pinned in the
script — operators set release per-invocation via
`CARGO_BUILD_PROFILE=release yarn start`, or persist it via
`yarn dashmate config set` directly. Keeping the profile out of the
script means the same `yarn setup` works for both dev iteration
loops (default `dev` profile) and SDK_TEST_DATA bakes (release).
Docs:
- `docs/shielded-seeder-performance.md` documents why release profile
is recommended when SDK_TEST_DATA is on at the default N=1_000_000
bake (~13× faster Sinsemilla on release vs debug). Referenced from
`configJsonSchema.js` and `getBaseConfigFactory.js`.
Tests: `Config.spec.js` adds 15 cases covering the map-shaped path
traversal — including a regression test for the original
`buildArgs.SDK_TEST_DATA` failure.
📝 WalkthroughWalkthroughThis PR extends Dashmate's configuration system to support Docker build arguments. It adds schema validation for configuration paths, integrates path validation into the config command, initializes and migrates build argument settings, documents build argument forwarding in docker-compose files, tests template rendering, and applies build arguments to local network setup. ChangesDocker Build Arguments Control Flow
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/dashmate/test/unit/config/Config.spec.js (1)
87-102: ⚡ Quick winAdd a regression test for empty path segments (
a..b,a.b.).Current edge-case tests miss malformed dotted paths with empty segments, which are important for the new schema walker behavior.
🤖 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 `@packages/dashmate/test/unit/config/Config.spec.js` around lines 87 - 102, Add regression tests to cover malformed dotted paths with empty segments for Config.isSchemaPathAllowed: add assertions that Config.isSchemaPathAllowed('a..b') and Config.isSchemaPathAllowed('a.b.') (and optionally '.a' and 'a..') return false so the schema walker rejects empty path segments; place them alongside the existing edge cases in the 'edge cases' describe block near the other isSchemaPathAllowed tests to ensure these malformed paths are explicitly covered.
🤖 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 `@docs/shielded-seeder-performance.md`:
- Line 44: Update the docs to match the implementation: replace references to
platform.drive.abci.docker.build.cargoBuildProfile with the actual config key
platform.drive.abci.docker.build.buildArgs.CARGO_BUILD_PROFILE; remove the claim
that scripts/setup_local_network.sh sets or restores CARGO_BUILD_PROFILE and
instead document that scripts/setup_local_network.sh only sets
SDK_TEST_DATA=true (which is auto-pinned), and state that CARGO_BUILD_PROFILE is
optional and must be set by the user either per-invocation or in their config if
they desire a non-default build profile.
In `@packages/dashmate/src/config/Config.js`:
- Around line 70-72: In isSchemaPathAllowed, add a validation after splitting
the dot-path that rejects any empty segments so paths like "foo." or "a..b"
return false; locate the split operation (path.split('.')) inside the
isSchemaPathAllowed method and ensure you check every segment for === '' (or
length === 0) and return false if any are empty before continuing the existing
allowed-key checks.
In `@packages/dashmate/src/config/configJsonSchema.js`:
- Around line 49-52: The buildArgs object currently allows any property name
(via additionalProperties) so invalid or empty keys can slip through; update the
schema for buildArgs to restrict property names to valid environment-variable
identifiers by replacing additionalProperties with patternProperties using a
regex like ^[A-Z_][A-Z0-9_]*$ for keys and keep the value type as string, and
set additionalProperties: false to reject any names that don't match; target the
buildArgs schema block to apply this change.
In `@packages/dashmate/src/config/generateEnvsFactory.js`:
- Around line 124-130: The loop copying mergedBuildArgs into envs can overwrite
reserved runtime keys; update the assignment in the block that iterates
mergedBuildArgs (the for..of using mergedBuildArgs, getBuildArgs) to skip any
keys in a reserved set (e.g.,
["COMPOSE_FILE","CONFIG_NAME","CONFIG_PATH","NODE_ENV","PATH"] or whatever
project-specific core keys) and optionally log or warn when a user-provided
buildArg is ignored; leave non-reserved keys assigned to envs as before.
---
Nitpick comments:
In `@packages/dashmate/test/unit/config/Config.spec.js`:
- Around line 87-102: Add regression tests to cover malformed dotted paths with
empty segments for Config.isSchemaPathAllowed: add assertions that
Config.isSchemaPathAllowed('a..b') and Config.isSchemaPathAllowed('a.b.') (and
optionally '.a' and 'a..') return false so the schema walker rejects empty path
segments; place them alongside the existing edge cases in the 'edge cases'
describe block near the other isSchemaPathAllowed tests to ensure these
malformed paths are explicitly covered.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17f30280-ab11-4adf-9e82-ca15acf967cc
📒 Files selected for processing (10)
docs/shielded-seeder-performance.mdpackages/dashmate/configs/defaults/getBaseConfigFactory.jspackages/dashmate/docker-compose.build.drive_abci.ymlpackages/dashmate/docker-compose.build.rs-dapi.ymlpackages/dashmate/src/commands/config/set.jspackages/dashmate/src/config/Config.jspackages/dashmate/src/config/configJsonSchema.jspackages/dashmate/src/config/generateEnvsFactory.jspackages/dashmate/test/unit/config/Config.spec.jsscripts/setup_local_network.sh
| static isSchemaPathAllowed(path) { | ||
| if (typeof path !== 'string' || path.length === 0) return false; | ||
|
|
There was a problem hiding this comment.
Reject dot-paths with empty segments.
At Line 90, path.split('.') allows empty segments (e.g., trailing dot), which can make invalid keys schema-allowed under additionalProperties. Add an explicit empty-segment check.
Suggested fix
static isSchemaPathAllowed(path) {
if (typeof path !== 'string' || path.length === 0) return false;
+ const segments = path.split('.');
+ if (segments.some((segment) => segment.length === 0)) return false;
@@
- for (const segment of path.split('.')) {
+ for (const segment of segments) {Also applies to: 90-91
🤖 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 `@packages/dashmate/src/config/Config.js` around lines 70 - 72, In
isSchemaPathAllowed, add a validation after splitting the dot-path that rejects
any empty segments so paths like "foo." or "a..b" return false; locate the split
operation (path.split('.')) inside the isSchemaPathAllowed method and ensure you
check every segment for === '' (or length === 0) and return false if any are
empty before continuing the existing allowed-key checks.
| buildArgs: { | ||
| type: 'object', | ||
| additionalProperties: { type: 'string' }, | ||
| }, |
There was a problem hiding this comment.
Constrain buildArgs keys to valid env-var identifiers.
At Line 49, buildArgs currently accepts any key, including empty/invalid names that won’t map reliably to ${NAME} substitutions. Please validate key names in schema to prevent silent misconfiguration.
Suggested schema hardening
buildArgs: {
type: 'object',
+ propertyNames: {
+ type: 'string',
+ pattern: '^[A-Z_][A-Z0-9_]*$',
+ },
additionalProperties: { type: 'string' },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buildArgs: { | |
| type: 'object', | |
| additionalProperties: { type: 'string' }, | |
| }, | |
| buildArgs: { | |
| type: 'object', | |
| propertyNames: { | |
| type: 'string', | |
| pattern: '^[A-Z_][A-Z0-9_]*$', | |
| }, | |
| additionalProperties: { type: 'string' }, | |
| }, |
🤖 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 `@packages/dashmate/src/config/configJsonSchema.js` around lines 49 - 52, The
buildArgs object currently allows any property name (via additionalProperties)
so invalid or empty keys can slip through; update the schema for buildArgs to
restrict property names to valid environment-variable identifiers by replacing
additionalProperties with patternProperties using a regex like
^[A-Z_][A-Z0-9_]*$ for keys and keep the value type as string, and set
additionalProperties: false to reject any names that don't match; target the
buildArgs schema block to apply this change.
| const mergedBuildArgs = { | ||
| ...getBuildArgs('platform.dapi.rsDapi.docker.build.buildArgs'), | ||
| ...getBuildArgs('platform.drive.abci.docker.build.buildArgs'), | ||
| }; | ||
| for (const [key, value] of Object.entries(mergedBuildArgs)) { | ||
| envs[key] = value; | ||
| } |
There was a problem hiding this comment.
Prevent buildArgs from overriding reserved runtime env keys.
At Line 129, direct assignment can clobber core envs (COMPOSE_FILE, CONFIG_NAME, etc.) if a user sets the same key in buildArgs. Add a collision guard before writing.
Suggested guard
const mergedBuildArgs = {
...getBuildArgs('platform.dapi.rsDapi.docker.build.buildArgs'),
...getBuildArgs('platform.drive.abci.docker.build.buildArgs'),
};
for (const [key, value] of Object.entries(mergedBuildArgs)) {
+ if (Object.prototype.hasOwnProperty.call(envs, key)) {
+ continue;
+ }
envs[key] = value;
}🤖 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 `@packages/dashmate/src/config/generateEnvsFactory.js` around lines 124 - 130,
The loop copying mergedBuildArgs into envs can overwrite reserved runtime keys;
update the assignment in the block that iterates mergedBuildArgs (the for..of
using mergedBuildArgs, getBuildArgs) to skip any keys in a reserved set (e.g.,
["COMPOSE_FILE","CONFIG_NAME","CONFIG_PATH","NODE_ENV","PATH"] or whatever
project-specific core keys) and optionally log or warn when a user-provided
buildArg is ignored; leave non-reserved keys assigned to envs as before.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds a generic buildArgs map to dashmate's dockerBuild schema and wires it through to docker compose, plus a set.js fix to support map-shaped (additionalProperties) schema paths. The implementation is sound — coerceTypes in AJV correctly handles boolean→string coercion for config set ... true flows, and tests for isSchemaPathAllowed are thorough. The remaining issues are documentation inconsistencies introduced by this PR: contradictory claims about shell-env precedence and an invalid config-path reference.
🟡 3 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `docs/shielded-seeder-performance.md`:
- [SUGGESTION] docs/shielded-seeder-performance.md:44: Doc references a config path that does not exist
Line 44 says the release profile is plumbed through dashmate as `platform.drive.abci.docker.build.cargoBuildProfile`, but the schema in `configJsonSchema.js` only adds a generic `buildArgs` map — there is no `cargoBuildProfile` field. The same document uses the correct path (`buildArgs.CARGO_BUILD_PROFILE`) later on lines 113 and 121. An operator following the line-44 instruction will hit `InvalidOptionPathError` or set an inert field.
In `packages/dashmate/src/config/configJsonSchema.js`:
- [SUGGESTION] packages/dashmate/src/config/configJsonSchema.js:35-48: Schema comment claims shell env overrides config, but the opposite is true
The schema comment says `Shell env overrides any value set here (so CARGO_BUILD_PROFILE=release yarn start wins per-invocation)`. The actual precedence is the reverse: `DockerCompose.js#createOptionsWithEnvs` (lines 676–679) constructs the spawn env as `{ ...process.env, ...envs }`, so the values `generateEnvsFactory` injects from `buildArgs` shadow same-named shell env vars. The implementation comment in `generateEnvsFactory.js` (lines 112–116) and the PR design (dashmate config = single source of truth) confirm dashmate wins. Shell env still flows through as a fallback for keys NOT set in `buildArgs`, but it does not override. Reconcile the schema comment with the implementation, or callers will be surprised when `CARGO_BUILD_PROFILE=release yarn start` is silently dropped.
In `packages/dashmate/src/config/generateEnvsFactory.js`:
- [SUGGESTION] packages/dashmate/src/config/generateEnvsFactory.js:112-116: Comment overstates the single-source-of-truth claim
The comment says `do NOT fall back to process.env[key]. Operators who need a per-invocation override should yarn dashmate config set ... rather than FOO=bar yarn start.` In practice `DockerCompose.js#createOptionsWithEnvs` still spreads `process.env` first into the spawn env, so shell-provided values do reach docker-compose for any buildArg key the dashmate config has not set. The accurate semantics are: dashmate config wins on collision; shell env still works as a fallback for unset keys. Stating this precisely prevents a future contributor from removing the `process.env` spread in the name of single-source enforcement, which would silently break existing `FOO=bar yarn start` patterns.
| - **Speedup:** ~20–50× for the Sinsemilla phase; ~10× overall. | ||
| - **Effort:** one config option in dashmate (see below) + one Docker build arg. | ||
| - **Tradeoff:** slower image builds (release optimizations take longer to compile), larger debug-info-stripped binary remains ~50–100 MB vs ~700 MB for debug. | ||
| - **Status:** plumbed through dashmate as `platform.drive.abci.docker.build.cargoBuildProfile` — set to `release` when you want fast seeding. |
There was a problem hiding this comment.
🟡 Suggestion: Doc references a config path that does not exist
Line 44 says the release profile is plumbed through dashmate as platform.drive.abci.docker.build.cargoBuildProfile, but the schema in configJsonSchema.js only adds a generic buildArgs map — there is no cargoBuildProfile field. The same document uses the correct path (buildArgs.CARGO_BUILD_PROFILE) later on lines 113 and 121. An operator following the line-44 instruction will hit InvalidOptionPathError or set an inert field.
| - **Status:** plumbed through dashmate as `platform.drive.abci.docker.build.cargoBuildProfile` — set to `release` when you want fast seeding. | |
| - **Status:** plumbed through dashmate as `platform.drive.abci.docker.build.buildArgs.CARGO_BUILD_PROFILE` — set to `release` when you want fast seeding. |
source: ['claude']
| // Extra build args forwarded to `docker compose build` for this image. | ||
| // Each key becomes an env var with the same name, picked up by the | ||
| // `${KEY:-default}` substitutions in `docker-compose.build.*.yml` | ||
| // which mirror them into the Dockerfile `ARG`s. Shell env overrides | ||
| // any value set here (so `CARGO_BUILD_PROFILE=release yarn start` | ||
| // wins per-invocation). | ||
| // | ||
| // Common keys (when meaningful for the image's target): | ||
| // - CARGO_BUILD_PROFILE: "dev" | "release" — Rust profile for | ||
| // drive-abci / rs-dapi. Release is required for SDK_TEST_DATA | ||
| // shielded seeding at N > a few thousand; see | ||
| // `docs/shielded-seeder-performance.md`. | ||
| // - SDK_TEST_DATA: "true" — enable the SDK test-data cfg flag in | ||
| // the binary at compile time. |
There was a problem hiding this comment.
🟡 Suggestion: Schema comment claims shell env overrides config, but the opposite is true
The schema comment says Shell env overrides any value set here (so CARGO_BUILD_PROFILE=release yarn start wins per-invocation). The actual precedence is the reverse: DockerCompose.js#createOptionsWithEnvs (lines 676–679) constructs the spawn env as { ...process.env, ...envs }, so the values generateEnvsFactory injects from buildArgs shadow same-named shell env vars. The implementation comment in generateEnvsFactory.js (lines 112–116) and the PR design (dashmate config = single source of truth) confirm dashmate wins. Shell env still flows through as a fallback for keys NOT set in buildArgs, but it does not override. Reconcile the schema comment with the implementation, or callers will be surprised when CARGO_BUILD_PROFILE=release yarn start is silently dropped.
source: ['claude', 'codex']
| // Dashmate config is the single source of truth for build args — do NOT | ||
| // fall back to `process.env[key]`. Operators who need a per-invocation | ||
| // override should `yarn dashmate config set ...` rather than `FOO=bar | ||
| // yarn start`. Keeping this single-source matches the `yarn setup` flow | ||
| // that writes SDK_TEST_DATA into the local config automatically. |
There was a problem hiding this comment.
🟡 Suggestion: Comment overstates the single-source-of-truth claim
The comment says do NOT fall back to process.env[key]. Operators who need a per-invocation override should yarn dashmate config set ... rather than FOO=bar yarn start. In practice DockerCompose.js#createOptionsWithEnvs still spreads process.env first into the spawn env, so shell-provided values do reach docker-compose for any buildArg key the dashmate config has not set. The accurate semantics are: dashmate config wins on collision; shell env still works as a fallback for unset keys. Stating this precisely prevents a future contributor from removing the process.env spread in the name of single-source enforcement, which would silently break existing FOO=bar yarn start patterns.
source: ['claude']
| # Note: the seeder runs at `docker build` time inside the Dockerfile's bake | ||
| # stage, so a release-mode binary is highly recommended when SDK_TEST_DATA is | ||
| # on (see `docs/shielded-seeder-performance.md`). Set it per-invocation via |
There was a problem hiding this comment.
💬 Nitpick: Comment says seeder runs at docker build time, but it runs at InitChain
The comment claims the seeder runs at docker build time inside the Dockerfile's bake stage. Per docs/shielded-seeder-performance.md (line 1) the SDK_TEST_DATA seeder runs at InitChain — at runtime in drive-abci, not during image build. What happens at build time is the cfg-gated compilation of the seeder code into the binary (controlled by the SDK_TEST_DATA build arg). The release-profile recommendation is still right, but the stated reason is wrong and will confuse anyone debugging why their image build is slow versus why their node's first block is slow.
source: ['claude']
| const getBuildArgs = (configPath) => ( | ||
| config.has(configPath) ? (config.get(configPath) || {}) : {} | ||
| ); | ||
| const mergedBuildArgs = { | ||
| ...getBuildArgs('platform.dapi.rsDapi.docker.build.buildArgs'), | ||
| ...getBuildArgs('platform.drive.abci.docker.build.buildArgs'), | ||
| }; | ||
| for (const [key, value] of Object.entries(mergedBuildArgs)) { | ||
| envs[key] = value; | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: No unit test for buildArgs forwarding / merge precedence
Config.spec.js covers isSchemaPathAllowed thoroughly, but the new buildArgs-merge behavior in generateEnvsFactory has no unit coverage. A small spec — drive-abci wins on collision with rs-dapi (matches the comment), absent buildArgs does not shadow convertObjectToEnvs output, empty/undefined buildArgs is tolerated — would lock in the documented behavior and protect against a future regression that flipped the spread order.
source: ['claude']
…rough
Replaces the env-var-based forwarding (which hardcoded every supported
arg key in `docker-compose.build.*.yml` as `${KEY:-default}` and
re-exported them from `generateEnvsFactory`) with a generic
dynamic-compose render path. Now `dockerBuild.buildArgs` is open-ended
on the schema and any key/value the operator pins via `dashmate config
set platform.{drive.abci|dapi.rsDapi}.docker.build.buildArgs.X Y` is
emitted as a `services.<svc>.build.args.X: "Y"` entry in the
per-config `dynamic-compose.yml` and merged with the static build
compose by docker compose.
Changes:
- **dynamic-compose.yml.dot**: iterate over
`platform.drive.abci.docker.build.buildArgs` and
`platform.dapi.rsDapi.docker.build.buildArgs` and emit a `build.args`
block per service. The drive_abci service block is only emitted when
either driveLogs or driveBuildArgs has entries; rs_dapi gains a
conditional build block alongside its existing expose stanza.
- **docker-compose.build.drive_abci.yml** /
**docker-compose.build.rs-dapi.yml**: drop the hardcoded
`CARGO_BUILD_PROFILE: ${CARGO_BUILD_PROFILE:-dev}` and (for
drive_abci) `SDK_TEST_DATA: ${SDK_TEST_DATA}` lines. Static compose
now only carries args sourced from shell env (sccache config); all
config-sourced args flow through dynamic-compose.
- **generateEnvsFactory.js**: drop the env-var passthrough block.
- **configJsonSchema.js** / **getBaseConfigFactory.js**: trim the
comment to one line — the schema doesn't need example keys or doc
references.
- **getConfigFileMigrationsFactory.js**: 3.1.0 migration backfills
`buildArgs: {}` onto both build blocks for pre-existing configs.
- **scripts/setup_local_network.sh**: replace the verbose multi-line
comment with one sentence.
Removed `docs/shielded-seeder-performance.md` — it was added by mistake
to this PR (lives with the shielded-pool work, not the dashmate
plumbing).
Tests:
- `dynamicComposeBuildArgs.spec.js`: 3 new tests covering drive_abci
emission, rs_dapi emission, and the empty-buildArgs no-op path.
- `Config.spec.js`: 15 tests still passing.
- 101/101 unit tests pass on `yarn mocha test/unit/**/*.spec.js`.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js (1)
15-17: ⚡ Quick winRestore doT
templateSettings.stripafter mutating it in the test
Inpackages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js,render()setsdot.templateSettings.strip = falseand doesn’t restore the previous value, leaking global doT configuration across tests. Save the prior value and restore it in afinallyblock after rendering.🤖 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 `@packages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js` around lines 15 - 17, The test mutates global doT config by setting dot.templateSettings.strip = false in render(); capture the original value before changing it (e.g., const prev = dot.templateSettings.strip), perform the template work (reading TEMPLATE_PATH and calling dot.template), and then restore the original setting in a finally block (dot.templateSettings.strip = prev) so the mutation does not leak to other tests; update the render() helper in dynamicComposeBuildArgs.spec.js to implement this save/restore around the tpl/fn template operations.
🤖 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 `@packages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js`:
- Around line 7-12: The test uses underscored locals __filename and __dirname
which violate camelCase lint rules; rename them to camelCase (e.g., filename and
dirname or filePath and dirPath) and update their initializations (replace
__filename = fileURLToPath(import.meta.url) and __dirname =
path.dirname(__filename) with the new names) and any subsequent references
(including TEMPLATE_PATH resolution) so all identifiers are camelCase and the
tests still resolve the template path correctly.
---
Nitpick comments:
In `@packages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js`:
- Around line 15-17: The test mutates global doT config by setting
dot.templateSettings.strip = false in render(); capture the original value
before changing it (e.g., const prev = dot.templateSettings.strip), perform the
template work (reading TEMPLATE_PATH and calling dot.template), and then restore
the original setting in a finally block (dot.templateSettings.strip = prev) so
the mutation does not leak to other tests; update the render() helper in
dynamicComposeBuildArgs.spec.js to implement this save/restore around the tpl/fn
template operations.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa58173b-1c8b-4acf-ace9-42a79a40edc1
⛔ Files ignored due to path filters (1)
packages/dashmate/templates/dynamic-compose.yml.dotis excluded by!**/*.dot
📒 Files selected for processing (7)
packages/dashmate/configs/defaults/getBaseConfigFactory.jspackages/dashmate/configs/getConfigFileMigrationsFactory.jspackages/dashmate/docker-compose.build.drive_abci.ymlpackages/dashmate/docker-compose.build.rs-dapi.ymlpackages/dashmate/src/config/configJsonSchema.jspackages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.jsscripts/setup_local_network.sh
✅ Files skipped from review due to trivial changes (1)
- packages/dashmate/docker-compose.build.rs-dapi.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/dashmate/configs/defaults/getBaseConfigFactory.js
- packages/dashmate/src/config/configJsonSchema.js
- scripts/setup_local_network.sh
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const TEMPLATE_PATH = path.resolve( | ||
| __dirname, | ||
| '../../../templates/dynamic-compose.yml.dot', | ||
| ); |
There was a problem hiding this comment.
Rename underscored module-path locals to pass lint and naming rules.
Line 7 and Line 8 use dangle-underscore names (__filename, __dirname), which triggers ESLint and breaks the camelCase guideline for JS variables.
🔧 Proposed fix
-const __filename = fileURLToPath(import.meta.url);
-const __dirname = path.dirname(__filename);
+const fileName = fileURLToPath(import.meta.url);
+const dirName = path.dirname(fileName);
const TEMPLATE_PATH = path.resolve(
- __dirname,
+ dirName,
'../../../templates/dynamic-compose.yml.dot',
);As per coding guidelines: "Use camelCase for variables and functions in JS/TS" and "Use ESLint with Airbnb/TypeScript rules for JS/TS files".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = path.dirname(__filename); | |
| const TEMPLATE_PATH = path.resolve( | |
| __dirname, | |
| '../../../templates/dynamic-compose.yml.dot', | |
| ); | |
| const fileName = fileURLToPath(import.meta.url); | |
| const dirName = path.dirname(fileName); | |
| const TEMPLATE_PATH = path.resolve( | |
| dirName, | |
| '../../../templates/dynamic-compose.yml.dot', | |
| ); |
🧰 Tools
🪛 ESLint
[error] 7-7: Unexpected dangling '_' in '__filename'.
(no-underscore-dangle)
[error] 8-8: Unexpected dangling '_' in '__dirname'.
(no-underscore-dangle)
🤖 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 `@packages/dashmate/test/unit/templates/dynamicComposeBuildArgs.spec.js` around
lines 7 - 12, The test uses underscored locals __filename and __dirname which
violate camelCase lint rules; rename them to camelCase (e.g., filename and
dirname or filePath and dirPath) and update their initializations (replace
__filename = fileURLToPath(import.meta.url) and __dirname =
path.dirname(__filename) with the new names) and any subsequent references
(including TEMPLATE_PATH resolution) so all identifiers are camelCase and the
tests still resolve the template path correctly.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta moves buildArgs forwarding from env passthrough into the per-config dynamic-compose.yml template and rewrites the schema/comments accordingly. All five prior findings are resolved. One blocking regression remains: dashmate config set ...buildArgs.* true (used by this PR's own scripts/setup_local_network.sh) JSON-parses true into a boolean, which fails schema validation that now requires string-typed values. A secondary suggestion covers unescaped YAML interpolation of buildArgs values.
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/dashmate/src/commands/config/set.js`:
- [BLOCKING] packages/dashmate/src/commands/config/set.js:54-60: `dashmate config set ...buildArgs.SDK_TEST_DATA true` is rejected because the CLI JSON-parses the value into a boolean
`configJsonSchema.js` lines 40–42 now requires every `docker.build.buildArgs.*` entry to be `type: 'string'`, but `set.js` unconditionally runs `JSON.parse(optionValue)` before calling `config.set(...)`. For the inputs introduced by this PR — `scripts/setup_local_network.sh` line 24 (`... buildArgs.SDK_TEST_DATA "true"`) and the analogous `CARGO_BUILD_PROFILE release` example — the shell strips the quotes and dashmate receives the bare token `true`/`release`. `JSON.parse("true")` returns boolean `true`, `Config.set` runs AJV validation (Config.js:151), and AJV rejects the boolean against the string-typed `additionalProperties`, throwing `InvalidOptionError`. The setup script in this same PR therefore fails at line 23–24, and the documented operator workflow is broken end-to-end. Skip JSON-parsing for the open-string-map buildArgs path (or, equivalently, fall back to the raw string when the parsed value's type doesn't match the schema).
In `packages/dashmate/templates/dynamic-compose.yml.dot`:
- [SUGGESTION] packages/dashmate/templates/dynamic-compose.yml.dot:30: buildArgs values are interpolated into YAML without escaping
Lines 30 and 58 render each pair as `{{=pair[0]}}: "{{=pair[1]}}"`. doT's `{{=...}}` is raw interpolation and the schema only constrains values to `type: 'string'` with no character restriction, so a value containing `"`, `\`, or a newline produces invalid or altered YAML that fails the next dashmate render with a confusing parse error rather than at `config set` time. The PR opens this surface as a public, open-ended `Record<string, string>` API, so worth either YAML-escaping (single-quoted form with doubled single quotes) or constraining values via a safe-character regex in the schema.
| build: | ||
| args: | ||
| {{~ driveBuildArgs :pair }} | ||
| {{=pair[0]}}: "{{=pair[1]}}" |
There was a problem hiding this comment.
🟡 Suggestion: buildArgs values are interpolated into YAML without escaping
Lines 30 and 58 render each pair as {{=pair[0]}}: "{{=pair[1]}}". doT's {{=...}} is raw interpolation and the schema only constrains values to type: 'string' with no character restriction, so a value containing ", \, or a newline produces invalid or altered YAML that fails the next dashmate render with a confusing parse error rather than at config set time. The PR opens this surface as a public, open-ended Record<string, string> API, so worth either YAML-escaping (single-quoted form with doubled single quotes) or constraining values via a safe-character regex in the schema.
source: ['claude', 'codex']
2 participants
Issue being fixed or feature implemented
Adds a
dockerBuild.buildArgsmap on the dashmate config schema so per-image Docker build args (e.g.CARGO_BUILD_PROFILE,SDK_TEST_DATA) can be pinned per config and forwarded to compose's${KEY:-default}substitution. Previously the only way to set these at build time was to either export them as shell envs every invocation or hand-edit the compose YAML — neither survives `yarn setup` cleanly.What was done?
configJsonSchema.js): `dockerBuild.buildArgs` is `{ [string]: string }`, open-ended.Yarn-script side (`setup_local_network.sh`): pins `SDK_TEST_DATA=true` per local_N (the genesis test-data flag). `CARGO_BUILD_PROFILE` is deliberately NOT pinned — operators set release per-invocation via `CARGO_BUILD_PROFILE=release yarn start` or persist via `dashmate config set`. Keeping the profile out of the script means the same `yarn setup` works for both dev iteration (default `dev` profile) and SDK_TEST_DATA bakes (release).
Docs: `docs/shielded-seeder-performance.md` documents why release is recommended when SDK_TEST_DATA is on at the default bake size. Referenced from the schema + defaults.
How Has This Been Tested?
Breaking Changes
None — schema is additive. Existing dashmate configs without `buildArgs` continue to work; the default is an empty map.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes