fix(spec): address map-file schema review comments - #218
Merged
Conversation
- Remove required constraint from AWS if/then so validation applies when provider is absent (default=aws) - Reject unknown dollar-prefixed keys via patternProperties: false (except schema and config) Parser fixes and schema tests follow in subsequent commits. Ref: #217
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR treats all top-level ChangesReserved Control Key Filtering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
8 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Envilder map-file v1 JSON Schema to better align validation behavior with the intended defaults and reserved-key rules, and republishes the same changes to the website-served schema copy.
Changes:
- Adjust
$config.provider=awsconditional so AWS rules also apply whenprovideris omitted (default aws). - Disallow unknown top-level
$-prefixed keys (allowing only$schemaand$config). - Apply the same schema changes to both the source spec and the published website schema.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/map-file.v1.json | Fixes AWS-default conditional behavior and rejects unknown $-prefixed keys in the canonical schema. |
| src/website/public/schema/map-file.v1.json | Mirrors the same schema fixes in the website-published copy. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the JSON schema for map files in both the specification and the website public directory. It removes the requirement for the provider property in the AWS validation block, allowing AWS to be the default provider. Additionally, it restricts top-level reserved keys starting with $ to only $schema and $config using a negative lookahead regex. I have no feedback to provide.
- General: empty, mappings-only, /, $-prefix rejection, unknown config fields, metadata, non-string values - AWS: profile allowed, vaultUrl/projectId/namespace/path forbidden, default behavior when provider absent - Azure: vaultUrl required, profile/projectId/namespace/path forbidden - GCP: projectId required, profile/vaultUrl/namespace/path forbidden - HashiCorp: vaultUrl required, namespace allowed, profile/projectId/path forbidden - File: path required, profile/vaultUrl/projectId/namespace forbidden Also adds ajv + ajv-formats as devDependencies for schema validation.
All four parsers (TS core, Node SDK, .NET SDK, Python SDK) previously used exact-match on the config key, letting keys like schema leak into variable mappings. Changed to startsWith prefix check so all reserved keys are excluded. - FileVariableStore.getParsedMapping(): filter dollar-prefixed entries - MapFileParser.parse() (Node SDK): startsWith guard - MapFileParser.Parse() (.NET SDK): StartsWith guard - MapFileParser.parse() (Python SDK): startswith guard - Rebuild GHA bundle (dist/index.js)
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/sdks/nodejs/application/map-file-parser.test.ts (1)
116-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd test coverage for the cross-provider validation error paths.
Once the cross-provider checks are added to
parseConfig(see the comment inmap-file-parser.ts), two tests are needed here to ensure they exercise the error paths:+ it('Should_ThrowError_When_ProviderIsAzureAndProfilePresent', () => { + // Arrange + const json = JSON.stringify({ + $config: { + provider: 'azure', + vaultUrl: 'https://my-vault.vault.azure.net', + profile: 'production', + }, + }); + const sut = new MapFileParser(); + + // Act + const act = () => sut.parse(json); + + // Assert + expect(act).toThrow( + "Cross-provider conflict: 'profile' is not supported for the Azure provider", + ); + }); + + it('Should_ThrowError_When_ProviderIsAwsAndVaultUrlPresent', () => { + // Arrange + const json = JSON.stringify({ + $config: { + provider: 'aws', + vaultUrl: 'https://vault.example.com', + }, + }); + const sut = new MapFileParser(); + + // Act + const act = () => sut.parse(json); + + // Assert + expect(act).toThrow( + "Cross-provider conflict: 'vaultUrl' is not supported for the AWS provider", + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sdks/nodejs/application/map-file-parser.test.ts` around lines 116 - 129, Add two unit tests in map-file-parser.test.ts that exercise the cross-provider validation paths introduced in MapFileParser.parse / parseConfig: one test where $config.provider is "aws" but the JSON includes a provider-specific key for "azure" (or another non-aws key) and assert parse throws the expected cross-provider error message, and a second test where $config.provider is "azure" but the JSON includes an "aws"-specific key and assert the corresponding error; locate tests near the existing Should_ThrowError_When_ProviderIsUnknown case and follow the same Arrange/Act/Assert pattern using new MapFileParser() and expect(act).toThrow with the exact error strings produced by parseConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/envilder/core/infrastructure/variableStore/FileVariableStore.ts`:
- Around line 37-40: In the Object.entries loop inside FileVariableStore where
you build the mappings record (variables: mappings, rest), ensure you only
assign runtime string values: check if typeof value === "string" (and value !==
null) before doing mappings[key] = value; skip or ignore any non-string values
so mappings remains Record<string,string> and callers of getMapping() can't
receive non-string data.
In `@src/sdks/nodejs/src/application/map-file-parser.ts`:
- Around line 56-76: In parseConfig, add cross-provider validation: after
computing providerStr and config.provider, if config.provider ===
PROVIDER_MAP['azure'] and obj.profile is a string throw an Error like "profile
is not allowed for provider: azure"; similarly if config.provider ===
PROVIDER_MAP['aws'] and obj.vaultUrl is a string throw an Error like "vaultUrl
is not allowed for provider: aws"; reference the parseConfig function,
providerStr, config.provider and PROVIDER_MAP, and also add unit tests named
Should_ThrowError_When_ProviderIsAzureAndProfilePresent and
Should_ThrowError_When_ProviderIsAwsAndVaultUrlPresent to cover both failure
cases.
In `@src/sdks/python/envilder/application/map_file_parser.py`:
- Around line 50-52: The code currently calls _deserialize_config for every
$config object without rejecting cross-provider fields; modify
_deserialize_config (or validate right before calling it where key ==
_CONFIG_KEY) to determine the effective provider (use the explicit provider
value or default to "aws" if missing) and then enforce cross-provider rules: if
provider == "azure" and the config dict contains "profile" (or "profile" key),
raise a ValueError; if provider == "aws" and the config dict contains "vaultUrl"
or "vault_url", raise a ValueError; ensure the error messages mention the
offending field and provider so that callers of _deserialize_config (referenced
here as config = _deserialize_config(value)) will reject invalid Azure+profile
or AWS+vaultUrl combinations.
---
Outside diff comments:
In `@tests/sdks/nodejs/application/map-file-parser.test.ts`:
- Around line 116-129: Add two unit tests in map-file-parser.test.ts that
exercise the cross-provider validation paths introduced in MapFileParser.parse /
parseConfig: one test where $config.provider is "aws" but the JSON includes a
provider-specific key for "azure" (or another non-aws key) and assert parse
throws the expected cross-provider error message, and a second test where
$config.provider is "azure" but the JSON includes an "aws"-specific key and
assert the corresponding error; locate tests near the existing
Should_ThrowError_When_ProviderIsUnknown case and follow the same
Arrange/Act/Assert pattern using new MapFileParser() and expect(act).toThrow
with the exact error strings produced by parseConfig.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb5fdefd-e6c8-4eb2-96d8-059af5bdafc6
⛔ Files ignored due to path filters (5)
.github/agents/tdd-coach.agent.mdis excluded by none and included by nonegithub-action/dist/index.jsis excluded by!**/dist/**,!github-action/dist/**and included by nonepackage.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yamland included by nonespec/map-file.v1.jsonis excluded by none and included by none
📒 Files selected for processing (15)
src/envilder/core/infrastructure/variableStore/FileVariableStore.tssrc/sdks/dotnet/Application/MapFileParser.cssrc/sdks/nodejs/src/application/map-file-parser.tssrc/sdks/python/envilder/application/map_file_parser.pysrc/website/public/schema/map-file.v1.jsontests/envilder/core/infrastructure/variableStore/fileVariableStore.test.tstests/sdks/dotnet/Application/MapFileParserTests.cstests/sdks/nodejs/application/map-file-parser.test.tstests/sdks/python/application/test_map_file_parser.pytests/spec/map-file-schema.aws.test.tstests/spec/map-file-schema.azure.test.tstests/spec/map-file-schema.file.test.tstests/spec/map-file-schema.gcp.test.tstests/spec/map-file-schema.hashicorp.test.tstests/spec/map-file-schema.test.ts
…E2E helpers - Bump CLI/GHA to 0.10.0, Node SDK to 0.2.0, .NET SDK to 0.3.0, Python SDK to 0.4.0 - Update all READMEs and docs with `` field in map-file examples - Update changelogs for all components - Add `` to sample map files - Fix E2E readMappings() helpers to filter all `$`-prefixed keys - Rebuild GHA bundle
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/cli.test.ts (1)
64-79: ⚡ Quick winAdd
$schemato the dynamic test fixtures so the new filter is actually exercised.The dynamically-written map files created in
beforeAll(mapFilePathandmapFileWithConfigPath) contain no$schemakey. As a result, the updatedstartsWith('$')filter inreadMappingsis never triggered against a$schemavalue during these E2E runs. If production code accidentally leaked$schemainto mappings and tried to resolve its URL as an SSM path, these tests would not catch it.Adding
$schemato both fixtures mirrors the real-world map files shown in the sample fixtures (param-map.json,param-map-with-aws-config.json) and closes the coverage gap for the core fix in this PR.♻️ Proposed fixture update
await Promise.all([ writeFile( mapFilePath, - JSON.stringify({ TOKEN_SECRET: `${ssmPrefix}/Token` }, null, 2), + JSON.stringify( + { + $schema: 'https://envilder.com/schema/map-file.v1.json', + TOKEN_SECRET: `${ssmPrefix}/Token`, + }, + null, + 2, + ), ), writeFile( mapFileWithConfigPath, JSON.stringify( { + $schema: 'https://envilder.com/schema/map-file.v1.json', $config: { provider: 'aws' }, TOKEN_SECRET: `${ssmPrefix}/Token`, }, null, 2, ), ), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cli.test.ts` around lines 64 - 79, The dynamic test fixtures written in beforeAll (the writeFile calls that create mapFilePath and mapFileWithConfigPath) must include a "$schema" key so the readMappings code path that filters keys with startsWith('$') is exercised; update the JSON objects passed to those writeFile invocations to add a "$schema" entry (matching the shape of real fixtures, e.g., a schema URL string) in both the plain map and the map with $config so the startsWith('$') logic in readMappings runs during the E2E tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/cli.test.ts`:
- Around line 64-79: The dynamic test fixtures written in beforeAll (the
writeFile calls that create mapFilePath and mapFileWithConfigPath) must include
a "$schema" key so the readMappings code path that filters keys with
startsWith('$') is exercised; update the JSON objects passed to those writeFile
invocations to add a "$schema" entry (matching the shape of real fixtures, e.g.,
a schema URL string) in both the plain map and the map with $config so the
startsWith('$') logic in readMappings runs during the E2E tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e79b7658-8f5a-4031-9f96-0a5a0da2fa7c
⛔ Files ignored due to path filters (4)
README.mdis excluded by none and included by nonegithub-action/README.mdis excluded by none and included by nonepackage.jsonis excluded by none and included by nonesecrets-map.jsonis excluded by none and included by none
📒 Files selected for processing (18)
docs/changelogs/cli.mddocs/changelogs/gha.mddocs/changelogs/sdk-dotnet.mddocs/changelogs/sdk-nodejs.mddocs/changelogs/sdk-python.mddocs/pull-command.mddocs/push-command.mde2e/cli.test.tse2e/gha.test.tse2e/sample/param-map-with-aws-config.jsone2e/sample/param-map-with-azure-config.jsone2e/sample/param-map.jsonsrc/sdks/dotnet/Envilder.csprojsrc/sdks/dotnet/README.mdsrc/sdks/nodejs/README.mdsrc/sdks/nodejs/package.jsonsrc/sdks/python/README.mdsrc/sdks/python/pyproject.toml
✅ Files skipped from review due to trivial changes (16)
- e2e/sample/param-map-with-aws-config.json
- src/sdks/python/pyproject.toml
- src/sdks/nodejs/package.json
- src/sdks/dotnet/Envilder.csproj
- e2e/sample/param-map.json
- docs/changelogs/gha.md
- docs/changelogs/sdk-dotnet.md
- docs/changelogs/sdk-python.md
- src/sdks/dotnet/README.md
- docs/changelogs/sdk-nodejs.md
- e2e/sample/param-map-with-azure-config.json
- docs/pull-command.md
- docs/changelogs/cli.md
- src/sdks/nodejs/README.md
- docs/push-command.md
- src/sdks/python/README.md
getParsedMapping() now skips entries where typeof value !== 'string', preventing numbers, objects, booleans, or nulls from leaking into downstream secret resolution. Closes review comments about unsafe 'value as string' cast.
References src/sdks/*/README.md (per-SDK READMEs) instead of non-existent src/sdks/README.md.
1 task
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.
Pull Request
What does this PR do?
Addresses review comments from #217 (merged prematurely):
equired: [provider]\ from AWS if/then so validation applies when provider is absent (default=aws)
Related issues
Type of change
Checklist
Notes for reviewer
Schema fixes are committed. Parser fixes and schema tests are in progress via TDD.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation