fix(core): context value for feature flag should be parsed for boolean

[kumsmrit]

Issue # (if applicable)

Closes #30583.

Reason for this change

Integration tests fail when feature flag snapshots are incorrectly updated to true for flags defaulted to false when they are being passed as string values.

Description of changes

Fixed an issue where feature flags are incorrectly computed when passed as the string value 'false'.
The current implementation uses Boolean('false') which evaluates to true here; this caused integration test snapshots to be updated with incorrect values (true) for feature flags that should default to false.

This change properly parses string representations of boolean values to ensure feature flags maintain their intended state, resolving the integration test failures.

Description of how you validated changes

• Added unit test to validate 'false' is parsed correctly as boolean- false
• Ran the integration tests successfully- also deleted snapshots and validate the snapshot update

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

✅ A exemption request has been requested. Please wait for a maintainer's review.

[kumsmrit]

Exemption Request: This change correctly parses feature flag and has been validated through unit tests; Also existing integration tests were successfully executed; additional integration test should not be needed.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: ce35077
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[vishaalmehrishi]

This change properly parses string representations of boolean values to ensure feature flags maintain their intended state, resolving the integration test failures.

Is it possible to ensure that the boolean feature flags are always specified as booleans instead of strings? This would resolve the issue at its source.

[kumsmrit]

This change properly parses string representations of boolean values to ensure feature flags maintain their intended state, resolving the integration test failures.

Is it possible to ensure that the boolean feature flags are always specified as booleans instead of strings? This would resolve the issue at its source.

While it would be ideal to ensure feature flags are always specified as booleans, there could be couple of challenges:

• Feature flags can be set through various methods including CDK context from cdk.json, CLI parameters, and environment variables, which often come in as strings.
• Context values passed from the command line are always strings (interface here)
• Existing users may already be using string values like 'true'/'false' in their configurations, so a strict enforcement could introduce breaking changes

This fix takes a pragmatic approach by handling string representations properly while maintaining backward compatibility, to addresses the immediate issue without disrupting existing workflows.

[IkeNefcy]

Exemption Request: This change correctly parses feature flag and has been validated through unit tests; Also existing integration tests were successfully executed; additional integration test should not be needed.

I would argue that a new integ is needed, having no test is what lead to #30583. Plus since you are introducing a new feature flag, an integ that at least tests with the ff enabled and disabled should be created (usually enabled and disabled ff is also applied to the existing tests if applicable too). I understand what you are saying about the unit test, but a unit test is exactly that, just a unit test, integ tests ensure that your change is able to be deployed to cfn.

[kumsmrit]

Exemption Request: This change correctly parses feature flag and has been validated through unit tests; Also existing integration tests were successfully executed; additional integration test should not be needed.

I would argue that a new integ is needed, having no test is what lead to #30583. Plus since you are introducing a new feature flag, an integ that at least tests with the ff enabled and disabled should be created (usually enabled and disabled ff is also applied to the existing tests if applicable too). I understand what you are saying about the unit test, but a unit test is exactly that, just a unit test, integ tests ensure that your change is able to be deployed to cfn.

Thank you for the feedback. I would like to call out that this PR doesn’t introduce any new feature flag; it only fixes how existing flags are parsed when passed as a string. The issue in #30583 occurred precisely because existing integration tests were failing due to incorrect parsing of string feature flag values; these integration tests cover resources with feature flags both enabled and disabled.
This PR addresses the root cause by fixing the parsing logic in the core library. I have added a unit test that specifically verifies the string 'false' is correctly parsed as boolean false, which directly targets the issue that caused the integration test failures. All of the existing integration tests were successfully executed after this change.
I think adding new integration tests specifically for this parsing fix would be redundant when we already have integration tests that exercise this code path with ff enabled and disabled.

[rix0rrr]

I'm with Vishaal. This issue should be addressed where the flag value is set, not where it is read.

Please point me to where is this specific "false" value is coming from?

• Yes, the CLI only allows passing string context. That was a pragmatic decision 5 years ago, and until now no-one has complained they weren't able to pass booleans via the CLI. We can still fix it, but it must not be that important if nobody has complained in 5 years.
• Other context sources are represented in JSON, so are perfectly capable of representing the distiction between "false" and false.

If the "false" is coming from our code base, I'd rather have a linter check saying "this value looks supiciously like a stringified boolean, are you sure that's what you mean?"

[rix0rrr]

I think adding new integration tests specifically for this parsing fix would be redundant when we already have integration tests that exercise this code path with ff enabled and disabled.

I agree with that!

However you're also making an assumption here 🤓. Not your change, but do we actually have integ tests for both flag values?

[kumsmrit]

Please point me to where is this specific "false" value is coming from?

Here's where the "false" is coming from:
The integ-test-runner calls into runner-base.ts:getContext(), which merges in the “recommended” feature-flags JSON, where each flag is a boolean (true/false) here.
But after getContext() hands off this object to the wrapper, the cdk-wrapper:renderMapArrayArg() forces every value through a template literal making it string "@aws-cdk/aws-lambda:recognizeLayerVersion=false".
And the parse-command-line-arguments treats each --context argument as a raw string given the type is array and it doesn't convert feature flag value to boolean, instead just passes that entire "featureFlag=boolean" as string.

If I understand correctly, CLI’s entire context-passing mechanism relies on serialising every entry to a "KEY=VALUE" string, and there is no way to keep a raw boolean through renderMapArrayArg(). In-order to retain the boolean-typed context flags, we would require rewriting how CLI parses --context which could be non-trivial and backward incompatible change.
Please let me know your thoughts or if there is an alternative solution to this.

[kumsmrit]

I think adding new integration tests specifically for this parsing fix would be redundant when we already have integration tests that exercise this code path with ff enabled and disabled.

I agree with that!

However you're also making an assumption here 🤓. Not your change, but do we actually have integ tests for both flag values?

We do have integ tests where the flag values are picked from the recommended-feature-flags and the recommended values have both true and false; one such flag being logApiResponseDataPropertyTrueDefault where the flag value is being passed 'false' for the integration tests.
Please let me know if you think we should add additional integration tests also.

[mrgrain]

Here's where the "false" is coming from: The integ-test-runner calls into runner-base.ts:getContext(), which merges in the “recommended” feature-flags JSON, where each flag is a boolean (true/false) here. But after getContext() hands off this object to the wrapper, the cdk-wrapper:renderMapArrayArg() forces every value through a template literal making it string "@aws-cdk/aws-lambda:recognizeLayerVersion=false". And the parse-command-line-arguments treats each --context argument as a raw string given the type is array and it doesn't convert feature flag value to boolean, instead just passes that entire "featureFlag=boolean" as string.

If I understand correctly, CLI’s entire context-passing mechanism relies on serialising every entry to a "KEY=VALUE" string, and there is no way to keep a raw boolean through renderMapArrayArg(). In-order to retain the boolean-typed context flags, we would require rewriting how CLI parses --context which could be non-trivial and backward incompatible change. Please let me know your thoughts or if there is an alternative solution to this.

This is an excellent summary and investigation! Thank you. Sounds like it is pretty much the feature that @rix0rrr was pointing out:

Yes, the CLI only allows passing string context. That was a pragmatic decision 5 years ago, and until now no-one has complained they weren't able to pass booleans via the CLI. We can still fix it, but it must not be that important if nobody has complained in 5 years.

To me, we should fix this issue. It will not only be an issue for us but for others as well. I don't think detecting numbers and booleans and correctly typing them would be highly controversial or backward incompatible. But I can appreciate that it goes beyond the immediate problem and how a more "hacky" solution could be more appropriate.

What do you both think?

[kumsmrit]

To me, we should fix this issue. It will not only be an issue for us but for others as well. I don't think detecting numbers and booleans and correctly typing them would be highly controversial or backward incompatible. But I can appreciate that it goes beyond the immediate problem and how a more "hacky" solution could be more appropriate.

What do you both think?

I agree that this is worth fixing, but for this PR I was leaning towards fixing the immediate problem keeping the CLI behaviour consistent.

I can follow up in a separate PR for having first‐class support for typed context values. The potential solution for fixing this could be patching the deploy case where we can parse the parameters correctly in packages/aws-cdk/lib/cli/cli.ts; modifying the split loop to below code:

if (typeof parameter === 'string') {
  const [key, ...val] = parameter.split('=');
  const rawVal = val.join('=');

  // JSON.parse will turn "true"→true, "123"→123, '{"foo":42}'→{foo:42}
  // on error we leave it as a string
   let keyValue: any = rawVal;
   try {
     keyValue = JSON.parse(rawVal);
   } catch {
     /* leave as-is */
   }
  parameterMap[key] = keyValue;
}

This should retain the type of context value as "featureFlag"=boolean while still preserving non-JSON values as strings.
Please let me know your thoughts on this.

[mrgrain]

I can follow up in a separate PR for having first‐class support for typed context values. The potential solution for fixing this could be patching the deploy case where we can parse the parameters correctly in packages/aws-cdk/lib/cli/cli.ts; modifying the split loop to below code:

Parameters are something entirely different. you are looking for this function: https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/lib/cli/user-configuration.ts#L332

[kumsmrit]

Parameters are something entirely different. you are looking for this function: https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/lib/cli/user-configuration.ts#L332

Thank you for the reference; I have created the PR for supporting the typed context values: aws/aws-cdk-cli#595

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.