feat(dashmate): configure docker build args via config#3764
Merged
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.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPer-image Docker build arguments are forwarded from platform config into a generated dynamic-compose.yml as build.args. Schema comments, defaults, migrations, compose templates, rendering tests, env-generation behavior, Config path validation, and local setup scripts were updated to reflect this flow. ChangesDocker Build Arguments Configuration Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Contributor
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
Collaborator
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.
…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`.
Contributor
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
thepastaclaw
suggested changes
May 28, 2026
Collaborator
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.
Resolve buildArgs feature collision: base v3.1-dev independently shipped the same feature via env-passthrough (generateEnvsFactory). Kept this branch's dynamic-compose mechanism instead, dropping the env-passthrough block, per the "drop env passthrough" design (commit 868ae52). Conflict resolutions: - compose build files: take dynamic-compose approach (no explicit SDK_TEST_DATA/CARGO_BUILD_PROFILE ${...} substitutions) - configJsonSchema.js: dynamic-compose comment + kept base's stricter propertyNames pattern validation - getBaseConfigFactory.js / migrations: buildArgs:{} backfill - setup_local_network.sh: took base (JSON-string quoting + release pinning required at the new N=1,000,000 seed default) - generateEnvsFactory.js: removed base's env-passthrough block (single source of truth is dynamic-compose.yml) 18/18 dashmate unit tests pass.
The local-network setup script no longer forces CARGO_BUILD_PROFILE=release on each masternode's drive-abci build. Operators set the profile per-config when they want a release bake; SDK_TEST_DATA seeding still defaults on. Note: with env-passthrough removed (single source of truth is dynamic-compose buildArgs) and no profile pinned, the Dockerfile ARG default (CARGO_BUILD_PROFILE=dev) applies unless set via `dashmate config set`.
Collaborator
|
✅ Review complete (commit 59cceeb) |
thepastaclaw
suggested changes
Jun 1, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta removes the CARGO_BUILD_PROFILE=release pin from the local setup script and JSON-quotes SDK_TEST_DATA so the path/value combo passes validation. Two prior findings carry forward (set.js still JSON-parses values before schema validation; buildArgs values still render into YAML unescaped). One new regression: with env forwarding and ${CARGO_BUILD_PROFILE:-dev} both removed, the previously-documented CARGO_BUILD_PROFILE=release yarn start per-invocation override no longer reaches Docker builds.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(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: [Carried-forward — STILL VALID] `dashmate config set` JSON-parses build-arg values, so bare string values that look like JSON literals are rejected
The path precheck was correctly swapped to `Config.isSchemaPathAllowed`, but `runWithDependencies` still unconditionally `JSON.parse`s every CLI value. Because `configJsonSchema.js` types `buildArgs` values as `{ type: 'string' }` (with `additionalProperties: { type: 'string' }`), any CLI invocation like `dashmate config set …buildArgs.CARGO_BUILD_PROFILE release` parses `release` fine (string) — but `dashmate config set …buildArgs.SDK_TEST_DATA true` parses to boolean `true` and is rejected by AJV. The new setup script works only because it passes `'"true"'` (a JSON-quoted string). This is the PR's new public API; documented use cases (`SDK_TEST_DATA`, plus future flag-style args like `1`/`0`/`null`) all collide with this typing. Either short-circuit JSON.parse for `…docker.build.buildArgs.…` paths, or fall back to the raw string when JSON.parse yields a value that fails schema validation as a string.
In `packages/dashmate/src/config/generateEnvsFactory.js`:
- [SUGGESTION] packages/dashmate/src/config/generateEnvsFactory.js:106-109: Shell-only `CARGO_BUILD_PROFILE=release yarn start` overrides no longer reach Docker builds
This PR removes both halves of the previous shell-env passthrough: `generateEnvsFactory.js` explicitly stops forwarding `docker.build.buildArgs` into the compose environment, and `docker-compose.build.drive_abci.yml` / `docker-compose.build.rs-dapi.yml` drop their `CARGO_BUILD_PROFILE: ${CARGO_BUILD_PROFILE:-dev}` entries (confirmed in the diff against v3.1-dev). After this change, `CARGO_BUILD_PROFILE=release yarn start` is silently a no-op — the only working path is `dashmate config set …buildArgs.CARGO_BUILD_PROFILE '"release"'`. The setup script's own comment correctly tells operators to use `dashmate config set`, so the script is internally consistent, but the PR description still advertises the shell-env override. Either restore the `${CARGO_BUILD_PROFILE:-dev}` fallback in the two build compose files (so `dynamic-compose.yml` overrides win when set but the shell env still works otherwise), or update the PR description to drop the shell-env claim and point users at `dashmate config set` exclusively.
In `packages/dashmate/templates/dynamic-compose.yml.dot`:
- [SUGGESTION] packages/dashmate/templates/dynamic-compose.yml.dot:30: [Carried-forward — STILL VALID] buildArgs values are raw-interpolated into YAML without escaping or schema char restrictions
Both render sites (`drive_abci` line 30, `rs_dapi` line 58) emit `{{=pair[0]}}: "{{=pair[1]}}"` with no escaping. `configJsonSchema.js` restricts keys via `propertyNames.pattern: '^[A-Za-z_][A-Za-z0-9_]*$'` but values are just `{ type: 'string' }` with no character restriction. A value containing `"`, `\`, or a newline (settable via `dashmate config set …buildArgs.FOO 'a"b'`) renders into malformed YAML and breaks the next compose invocation — there is no subsequent validation step over the generated `dynamic-compose.yml`. Operator-controlled input, so it's a robustness/UX concern rather than security. The cleanest fix is to mirror the key restriction onto values in the schema (any printable-ASCII regex that excludes `"`, `\`, and control chars easily covers realistic build args like `dev`/`release`/`true`/version/path strings). Alternatively, render values via `JSON.stringify` in the .dot template so YAML-quoting handles edge cases automatically.
Merge resolution left a double blank line that tripped @stylistic/no-multiple-empty-lines in the dashmate lint job.
thepastaclaw
reviewed
Jun 1, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at 6f083eb (no new delta vs prior reviewed SHA). Reconciled all four prior findings: the migration blank-line was fixed; the YAML-escaping and shell-env-passthrough findings remain valid as suggestions; the set.js JSON.parse issue is downgraded — set.js is not in this PR's cumulative diff against origin/v3.1-dev and the PR ships an explicit workaround. No blocking issues; review action COMMENT.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/config/generateEnvsFactory.js`:
- [SUGGESTION] packages/dashmate/src/config/generateEnvsFactory.js:106-109: [Carried-forward — STILL VALID] PR description still advertises shell-env CARGO_BUILD_PROFILE override, but env passthrough was removed
`generateEnvsFactory.js` now explicitly stops forwarding `docker.build.buildArgs` into the compose environment (replaced with a NOTE comment), and `docker-compose.build.drive_abci.yml` / `docker-compose.build.rs-dapi.yml` no longer carry `CARGO_BUILD_PROFILE: ${CARGO_BUILD_PROFILE:-dev}`. Only values persisted into dashmate config land in `dynamic-compose.yml`, so a shell-only invocation like `CARGO_BUILD_PROFILE=release yarn start` is silently ignored. This contradicts the PR description's stated workflow ("per-invocation via CARGO_BUILD_PROFILE=release yarn start or persist via dashmate config set"). `scripts/setup_local_network.sh` already reflects the new reality by using `dashmate config set ...buildArgs.CARGO_BUILD_PROFILE '"release"'`. Either update the PR description to match (config-only workflow), or restore a narrowly-scoped env override for ergonomics. Documenting the operator workflow consistently matters because this is the headline UX of the change.
In `packages/dashmate/src/commands/config/set.js`:
- [SUGGESTION] packages/dashmate/src/commands/config/set.js:54-60: [Carried-forward — STILL VALID, downgraded] `dashmate config set` JSON-parses values; new buildArgs string-only schema rejects bare `true`/`false`/`1`/`null`
`runWithDependencies` still runs `JSON.parse(optionValue)` with a string-fallback before `config.set(...)`. For the new `...docker.build.buildArgs.<KEY>` paths, `configJsonSchema.js` requires every value to be a string, so bare CLI inputs that happen to be valid JSON non-strings (`true`, `false`, `null`, `1`, `[…]`, `{…}`) are converted to non-strings and then rejected by AJV. Operators have to know to wrap such values in JSON quotes (e.g. `'"true"'`), which `scripts/setup_local_network.sh` already does. Note that `set.js` itself is NOT modified in this PR's cumulative diff against `origin/v3.1-dev`, so the JSON.parse logic predates this branch; however, the new buildArgs schema is what exposes the sharp edge for the PR's intended workflow. Typical CARGO_BUILD_PROFILE values (`dev`, `release`) parse cleanly via the catch-fallback, so this is an edge case rather than a workflow-breaker — hence suggestion rather than blocking. Consider bypassing JSON.parse for `…buildArgs.*` paths since the schema is string-only.
Address PR review findings on the dynamic-compose buildArgs work: - dynamic-compose.yml.dot: render build.args values via JSON.stringify so a value containing a quote/backslash/newline produces valid YAML instead of breaking the document. Ordinary values (e.g. "true") are unchanged. - Config.isSchemaPathAllowed: reject paths with empty segments (a..b, trailing dot) so an empty key can't slip through a map's additionalProperties descent. - setup_local_network.sh: correct the comment — SDK_TEST_DATA seeding runs at genesis (InitChain, runtime), not at docker build; release matters because the compiled binary runs that seeding ~10x faster. - tests: YAML round-trip escaping test (js-yaml), empty-segment rejection test; rename dangle-underscore locals in the spec.
thepastaclaw
approved these changes
Jun 1, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta resolves the YAML escaping issue (dynamic-compose.yml.dot now wraps buildArg values via JSON.stringify with a js-yaml round-trip regression test) and tightens isSchemaPathAllowed with an empty-segment guard plus unit coverage. The carried-forward 'dashmate config set foot-gun' finding from both agents does not survive verification: Config.ajv is constructed with coerceTypes: true, so bare CLI inputs true/false/1/null are silently coerced into strings before AJV validates against the buildArgs string schema. No in-scope issues remain.
…thAllowed The empty-segment guard's `segments` shadowed the `segments` local inside the resolveRef closure, tripping eslint no-shadow. Renamed to pathSegments.
thepastaclaw
approved these changes
Jun 1, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #3764 against v3.1-dev (head 59cceeb). The latest delta is a no-shadow lint rename in Config.isSchemaPathAllowed — zero behavior change. The full PR cleanly wires per-image docker build-args into dynamic-compose.yml for drive_abci/rs_dapi, with safe JSON.stringify rendering, env-var-identifier propertyNames constraint, empty-segment path rejection, and matching tests. No in-scope blockers or suggestions. One follow-up worth tracking separately about the shared dockerBuild schema exposing buildArgs on two sites that the renderer ignores.
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
2 participants
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.
Issue being fixed or feature implemented
Adds a
dockerBuild.buildArgsmap to 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 the image build. Previously the only way to set these at build time was to export shell envs on every invocation or hand-edit the compose YAML — neither survivesyarn setupcleanly.What was done?
configJsonSchema.js):dockerBuild.buildArgsis{ [string]: string }, with keys constrained to env-var identifiers (^[A-Za-z_][A-Za-z0-9_]*$).Config.js,commands/config/set.js): schema-aware path validation (Config.isSchemaPathAllowed) so map-shaped paths likeplatform.drive.abci.docker.build.buildArgs.SDK_TEST_DATAare accepted — the old existence-based pre-check rejected any key that didn't exist yet. Paths with empty segments are rejected.templates/dynamic-compose.yml.dot): each config'sbuildArgsrender directly into that config's generateddynamic-compose.ymlasbuild.argsentries, with valuesJSON.stringify-escaped so a quote/backslash/newline stays valid YAML. Dashmate config is the single source of truth — there is no shell-env passthrough.buildArgs: {}placeholder on both thedrive.abcianddapi.rsDapibuild blocks; a 3.1.0 migration backfills the field on pre-existing configs.Yarn-script side (
setup_local_network.sh): pinsSDK_TEST_DATA="true"perlocal_N(the genesis test-data flag).CARGO_BUILD_PROFILEis intentionally not pinned — operators set it per config when they want a release bake:Release is strongly recommended when
SDK_TEST_DATAis on: the genesis shielded-pool seeding runs at InitChain (runtime), and a release-compiled binary runs that Sinsemilla work ~10× faster than a debug build.How Has This Been Tested?
Config.spec.js— unit tests for schema-aware path traversal (typedproperties,additionalPropertiesmaps,$refdescent, leaf-primitive and empty-segment rejection) plus a regression test for the originalbuildArgs.SDK_TEST_DATAfailure.dynamicComposeBuildArgs.spec.js— renders the template and assertsbuild.argsare emitted per service, omitted whenbuildArgsis empty, and YAML-escaped (a js-yaml round-trip on a value containing a quote, backslash and newline).yarn setup local --node-count=3 …, then a release bake viadashmate config set …CARGO_BUILD_PROFILE '"release"', produces a releasedrive:localimage with SDK_TEST_DATA seeding active.Breaking Changes
None — the schema change is additive. Existing dashmate configs without
buildArgsare backfilled to an empty map by the 3.1.0 migration and continue to work unchanged.Checklist:
For repository code-owners and collaborators only