[codex] Refactor enum handling through domain commands#243
Open
eviltester wants to merge 21 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ChangesEnum Canonicalization and Parity Retirement
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45626bae67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/core-ui/src/tests/interaction/matrix/app-schema-interaction-matrix.test.js`:
- Around line 32-35: The smoke scenario selection in
app-schema-interaction-matrix.test.js currently hides missing scenario IDs by
filtering out falsy results, which can silently shrink coverage. Update the
scenario resolution logic around buildUiInteractionScenarios,
findScenarioByLogicalId, and SMOKE_SCENARIO_IDS to fail fast when any ID cannot
be found, instead of using filter(Boolean). Keep the suite generation strict so
a renamed or missing logical ID immediately surfaces as a test failure.
In
`@packages/core-ui/src/tests/interaction/matrix/generator-schema-interaction-matrix.test.js`:
- Around line 32-35: The smoke matrix setup in
generator-schema-interaction-matrix.test.js is silently dropping missing entries
because `SMOKE_SCENARIO_IDS.map(...).filter(Boolean)` hides absent scenarios.
Update the scenario selection around `buildUiInteractionScenarios`,
`findScenarioByLogicalId`, and `SMOKE_SCENARIO_IDS` to fail fast instead: detect
when any curated smoke scenario id cannot be resolved and throw an explicit
error or assertion before building `scenarios`, rather than filtering it out.
In
`@packages/core-ui/src/tests/interaction/matrix/support/schema-interaction-matrix-report.js`:
- Line 54: The report-helper module export list is out of sync with the
generator script: `scripts/generate-schema-interaction-matrix.mjs` still depends
on `renderMatrixSummaryMarkdown` from `schema-interaction-matrix-report.js`.
Restore a compatibility export for `renderMatrixSummaryMarkdown` in that module,
or update the generator script to stop importing it and use the new API, so both
`buildChunkDescriptors` and `formatCommandsForConsole` remain available without
breaking the summary artifact flow.
In `@packages/core/js/data_generation/rulesParser.js`:
- Around line 242-244: The enum rule rendering in rulesParser.js only
canonicalizes datatype.enum(...) and can leave awd.datatype.enum(...) unchanged,
so update the renderer to normalize all supported enum function spellings before
output. In the code paths using EnumParser.isCanonicalDomainEnumRuleSpec and
EnumParser.normalizeToCanonicalSchemaRuleSpec, extend the check/normalization
logic so older awd.datatype.enum(...) inputs also map to the canonical enum(...)
form, and apply the same fix in both affected rendering locations in
rulesParser.js.
In `@packages/core/js/data_generation/schema-conversion.js`:
- Around line 16-18: Broaden the enum normalization in the schema conversion
path so legacy enum spellings are canonicalized too. Update the conditional
around `EnumParser.isCanonicalDomainEnumRuleSpec` in `schema-conversion.js`, and
the matching logic at the other enum handling site, so both `datatype.enum(...)`
and legacy `awd.datatype.enum(...)` are rewritten through
`EnumParser.normalizeToCanonicalSchemaRuleSpec`. This will keep schema emission
consistent with the expanded enum parsing surface and ensure round-tripped
schemas always use `enum(...)`.
In `@packages/core/js/data_generation/schema-rules-adapter.js`:
- Around line 95-97: The domain enum handling in normaliseFakerCommand currently
only matches datatype.enum after lowercasing, so awd.datatype.enum falls through
and serializes incorrectly. Update the condition around
buildEnumRuleSpec(params) to also recognize the awd.datatype.enum alias (or
normalize it before the check) so SOURCE_TYPE_DOMAIN enum rows follow the same
enum rule path consistently.
In `@packages/core/js/data_generation/testDataRulesCompiler.js`:
- Around line 73-74: The compile() flow in testDataRulesCompiler.js only
normalizes enum rules in the auto-detected branch, so predeclared rules with
type set to enum can still bypass canonicalization and produce mixed enum/domain
representations. Update compile() so it also routes the declared-type enum path
through the same normalization logic used by
EnumParser.normalizeToCanonicalDomainRuleSpec, and ensure the rule.type is
consistently set to the canonical domain form when ruleSpec is normalized.
In `@packages/core/js/data_generation/utils/enumParser.js`:
- Around line 61-72: The canonical domain-enum check in
EnumParser.isCanonicalDomainEnumRuleSpec is too loose because it only matches
the datatype.enum( prefix, so malformed ruleSpec strings can still be classified
as enum-like. Tighten this helper to validate the full invocation syntax
end-to-end, and keep EnumParser.isEnumLikeRule aligned with that stricter check.
This will prevent schema-constraint-validator from passing invalid domain rules
into EnumParser.extractEnumValues and turning validation errors into exceptions.
In `@packages/core/js/keywords/domain/datatype/datatype-enum.js`:
- Around line 5-14: The enum argument normalizer is double-wrapping already
formatted input, causing `enum(active,inactive)` to become
`enum(enum(active,inactive))` and break parsing. Update
`validateDatatypeEnumArgs` and the normalization path in `datatype-enum.js` to
detect when the single string argument is already function-shaped (for example,
already starts with `enum(` and ends with `)`) and pass it through unchanged
before calling `EnumParser.extractEnumValues`, so existing enum syntax is parsed
directly instead of wrapped again.
🪄 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 Plus
Run ID: ff4bf96b-fdb3-40a5-9303-b03e76b109cd
📒 Files selected for processing (45)
apps/web/src/tests/browser/generator/functional/schema-edit.spec.jsdocs/frontend-ui-matrix-rationalization-plan.mdpackages/core-ui/js/gui_components/shared/domain-command-help-metadata.jspackages/core-ui/js/gui_components/shared/domain-commands.jspackages/core-ui/js/gui_components/shared/test-data/generation/generation-controller.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-row-mapper.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-runtime.jspackages/core-ui/src/tests/grid/schema/test-data-schema-grid-engine-compat.test.jspackages/core-ui/src/tests/interaction/app-test-data-focused-generation.test.jspackages/core-ui/src/tests/interaction/matrix/app-schema-interaction-matrix.test.jspackages/core-ui/src/tests/interaction/matrix/fixtures/schema-interaction-matrix-summary.mdpackages/core-ui/src/tests/interaction/matrix/fixtures/schema-interaction-matrix.jsonpackages/core-ui/src/tests/interaction/matrix/fixtures/ui-scenario-parity.jsonpackages/core-ui/src/tests/interaction/matrix/generator-schema-interaction-matrix.test.jspackages/core-ui/src/tests/interaction/matrix/schema-interaction-matrix-report.test.jspackages/core-ui/src/tests/interaction/matrix/schema-interaction-runtime-matrix.test.jspackages/core-ui/src/tests/interaction/matrix/support/schema-interaction-matrix-report.jspackages/core-ui/src/tests/interaction/matrix/support/ui-scenario-parity.jspackages/core-ui/src/tests/interaction/matrix/ui-scenario-parity-fixture.test.jspackages/core-ui/src/tests/interaction/matrix/ui-scenario-parity-support.test.jspackages/core-ui/src/tests/interaction/matrix/ui-schema-interaction-parity.test.jspackages/core-ui/src/tests/shared/generation-controller.test.jspackages/core-ui/src/tests/shared/schema-runtime.test.jspackages/core/js/command-help/command-help-validators.jspackages/core/js/data_generation/n-wise/pairwiseTestDataGenerator.jspackages/core/js/data_generation/rulesBasedDataGenerator.jspackages/core/js/data_generation/rulesParser.jspackages/core/js/data_generation/schema-constraint-validator.jspackages/core/js/data_generation/schema-conversion.jspackages/core/js/data_generation/schema-rules-adapter.jspackages/core/js/data_generation/testDataRulesCompiler.jspackages/core/js/data_generation/utils/enumParser.jspackages/core/js/domain/domain-command-metadata.jspackages/core/js/domain/domain-keywords.jspackages/core/js/domain/parser/DomainKeywordParser.jspackages/core/js/keywords/domain/datatype/datatype-enum.jspackages/core/js/keywords/domain/datatype/datatype-keyword-definitions.jspackages/core/js/keywords/domain/datatype/enum-keyword-definition.jspackages/core/src/tests/data_generation/domain-generation-path.test.jspackages/core/src/tests/data_generation/enum-compiler-integration.test.jspackages/core/src/tests/data_generation/enum-format-bug-fix.test.jspackages/core/src/tests/data_generation/enum-surface-parity.test.jspackages/core/src/tests/data_generation/mixed-compiler-integration.test.jspackages/core/src/tests/data_generation/unit/domain/domain-command-metadata.test.jspackages/core/src/tests/data_generation/user-reported-bug-http-method.test.js
💤 Files with no reviewable changes (6)
- packages/core-ui/src/tests/interaction/matrix/schema-interaction-matrix-report.test.js
- packages/core-ui/src/tests/interaction/matrix/ui-scenario-parity-support.test.js
- packages/core-ui/src/tests/interaction/matrix/support/ui-scenario-parity.js
- packages/core-ui/src/tests/interaction/matrix/ui-scenario-parity-fixture.test.js
- packages/core-ui/src/tests/interaction/matrix/ui-schema-interaction-parity.test.js
- packages/core-ui/src/tests/interaction/matrix/fixtures/ui-scenario-parity.json
Greptile SummaryThis PR refactors enum handling so all public spellings (
Confidence Score: 4/5The refactoring is well-structured and extensively tested, but a regex mismatch between the new The empty-parentheses enum normalization path (
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Schema Text / Schema Rows"] --> B["TestDataRulesCompiler"]
B --> C{"isEnumPattern?"}
C -->|"Yes, valid"| D["type='domain'\nruleSpec=normalizeToCanonicalDomainRuleSpec"]
C -->|"Yes, invalid explicit"| E["type='domain'\nruleSpec UNCHANGED\ne.g. enum()"]
C -->|"No"| F["type='faker'/'literal'/'regex'/'domain'"]
D --> G["RulesBasedDataGenerator\n→ DomainTestDataGenerator"]
E --> H["schema-conversion.cloneRule"]
H --> I{"isCanonicalSchemaSerializableEnumRuleSpec?"}
I -->|"true for enum() via isAwdEnumFormat [\\s\\S]*"| J["normalizeToCanonicalSchemaRuleSpec\n→ extractAwdEnumValues (.+)\n⚠️ THROWS Invalid enum format"]
I -->|"false"| K["ruleSpec passed through"]
D --> L["schema-conversion.cloneRule\n→ normalizeToCanonicalSchemaRuleSpec ✓"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Schema Text / Schema Rows"] --> B["TestDataRulesCompiler"]
B --> C{"isEnumPattern?"}
C -->|"Yes, valid"| D["type='domain'\nruleSpec=normalizeToCanonicalDomainRuleSpec"]
C -->|"Yes, invalid explicit"| E["type='domain'\nruleSpec UNCHANGED\ne.g. enum()"]
C -->|"No"| F["type='faker'/'literal'/'regex'/'domain'"]
D --> G["RulesBasedDataGenerator\n→ DomainTestDataGenerator"]
E --> H["schema-conversion.cloneRule"]
H --> I{"isCanonicalSchemaSerializableEnumRuleSpec?"}
I -->|"true for enum() via isAwdEnumFormat [\\s\\S]*"| J["normalizeToCanonicalSchemaRuleSpec\n→ extractAwdEnumValues (.+)\n⚠️ THROWS Invalid enum format"]
I -->|"false"| K["ruleSpec passed through"]
D --> L["schema-conversion.cloneRule\n→ normalizeToCanonicalSchemaRuleSpec ✓"]
Reviews (13): Last reviewed commit: "Fix issue 228 accessibility and validati..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/js/data_generation/utils/enumParser.js`:
- Line 66: The regex in isCanonicalDomainEnumRuleSpec is vulnerable to
polynomial backtracking because the adjacent whitespace quantifiers overlap with
the broad payload match. Simplify the pattern by removing the redundant inner
\s* guards around the [\s\S]+ section so the expression still matches the same
enum rule specs but no longer creates ambiguous whitespace backtracking.
🪄 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 Plus
Run ID: 24d39aa0-6935-4e9d-8cac-75f94cff24f0
📒 Files selected for processing (8)
packages/core/js/data_generation/schema-rules-adapter.jspackages/core/js/data_generation/testDataRulesCompiler.jspackages/core/js/data_generation/utils/enumParser.jspackages/core/js/keywords/domain/datatype/datatype-enum.jspackages/core/src/tests/data_generation/enum-surface-parity.test.jspackages/core/src/tests/data_generation/schema-rules-adapter.test.jspackages/core/src/tests/data_generation/unit/domain/domainKeywords.test.jspackages/core/src/tests/data_generation/unit/enum/enumTestDataGenerator.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/js/keywords/domain/datatype/datatype-enum.js
- packages/core/js/data_generation/testDataRulesCompiler.js
- packages/core/js/data_generation/schema-rules-adapter.js
- packages/core/src/tests/data_generation/enum-surface-parity.test.js
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/js/data_generation/utils/enumParser.js (2)
338-340: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAlign extraction with case-insensitive detection and remove the CodeQL regex shape.
Line 340 rejects
DATATYPE.ENUM(...)even thoughhasAwdEnumInvocationPrefixaccepts it. The same regex keeps the overlapping\s*(.+)\s*pattern flagged by CodeQL; capture the full argument body and trim after matching.Suggested fix
- const match = ruleSpec.match(/^(?:enum|datatype\.enum|awd\.datatype\.enum)\s*\(\s*(.+)\s*\)$/); + const match = ruleSpec.match(/^(?:enum|datatype\.enum|awd\.datatype\.enum)\s*\(([\s\S]+)\)$/i);🤖 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/core/js/data_generation/utils/enumParser.js` around lines 338 - 340, Update extractAwdEnumValues in enumParser.js so it matches the same case-insensitive prefixes accepted by hasAwdEnumInvocationPrefix, including DATATYPE.ENUM and AWD.DATATYPE.ENUM forms. Replace the current enum matching regex shape that uses overlapping \s*(.+)\s* with a safer capture of the full argument body, then trim the captured value after matching before parsing the enum entries.Source: Linters/SAST tools
268-294: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve escaped characters in quoted enum values.
Line 268 strips quote delimiters during tokenization, so Lines 292-293 no longer recognize quoted tokens and never unescape
\"or\\. Canonical values likedatatype.enum("a\"b")round-trip asa\"binstead ofa"b; unterminated quotes are also accepted.Suggested fix
if (char === '"' && (i === 0 || paramsStr[i - 1] !== '\\')) { // Toggle quote state for unescaped quotes inQuotes = !inQuotes; + currentValue += char; } else if (char === ',' && !inQuotes) { // Found separator outside quotes values.push(currentValue.trim()); currentValue = ''; @@ // Add final value values.push(currentValue.trim()); + if (inQuotes) { + throw new Error('Invalid enum format: unterminated quoted value'); + } + if (values.length === 0 || values.every((value) => value.length === 0)) { throw new Error('No valid values found in enum'); }🤖 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/core/js/data_generation/utils/enumParser.js` around lines 268 - 294, The enum tokenization in enumParser.js is stripping quote delimiters too early, so quoted values are no longer recognized later and escaped sequences are not properly restored. Update the parsing flow around the enum value collection and the final normalization in the return map so quoted tokens remain identifiable until after parsing, then unescape quoted values via unescapeQuotedEnumValue only for truly quoted entries. Also make the parser reject unterminated quoted strings instead of accepting them as valid enum values.
🤖 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/core/js/data_generation/enum/enumTestDataRuleValidator.js`:
- Around line 14-22: The fallback in enumTestDataRuleValidator currently turns
every non-explicit parse failure from parseEnumRuleSpec into success via
EnumParser.extractEnumValues, which lets malformed legacy enum specs slip
through. Update the validator to remove that blanket recovery or restrict it to
a clearly identifiable legacy CSV form, and keep parseEnumRuleSpec as the source
of truth for accepted implicit enums. Make the change in
enumTestDataRuleValidator so TestDataRulesCompiler.compile only canonicalizes
truly valid enum inputs.
In `@packages/core/js/keywords/domain/datatype/datatype-enum.js`:
- Around line 73-76: The enum value derivation in `getCandidates` only handles
`datatype.enum`, so `awd.datatype.enum` contexts fall through and return no
candidates. Update the command check in this enum parsing logic to treat
`awd.datatype.enum` the same as `datatype.enum`, and make sure the existing
`EnumParser.extractEnumDisplayValue(params)` and `candidates.push(params)` paths
are applied for both command names.
In `@packages/core/src/tests/data_formats/gherkin-convertor.test.js`:
- Around line 229-241: The GherkinConvertor data-table test only covers truthy
non-string values, so it can miss regressions where falsy primitives are
serialized as empty cells; update the existing `converts non-string values and
escapes gherkin table separators` test in `GherkinConvertor.fromDataTable` to
also assert `false` and `0` are preserved and stringified in the output,
alongside the current pipe-escaping checks.
In `@packages/core/src/tests/data_formats/html-convertor.test.js`:
- Around line 61-73: The regression test in HtmlConvertor.fromDataTable only
covers truthy non-string values, so add explicit assertions for falsy primitives
like false and 0 in the GenericDataTable export path to ensure they are
preserved instead of being dropped by truthiness-based conversion. Update the
existing test case in html-convertor.test.js to include rows or cells that
exercise false and 0, and verify the output contains their stringified values
alongside the existing HtmlConvertor and GenericDataTable coverage.
In `@packages/core/src/tests/data_formats/markdown-convertor.test.js`:
- Around line 347-359: The MarkdownConvertor test only verifies truthy primitive
rendering, so it would miss regressions where falsy values are dropped before
stringification. Tighten the existing fromDataTable test in
markdown-convertor.test.js by adding assertions for falsy non-string values
handled by GenericDataTable rows, using the same MarkdownConvertor output to
confirm false and 0 are preserved as strings alongside the current pipe-escaping
checks.
---
Outside diff comments:
In `@packages/core/js/data_generation/utils/enumParser.js`:
- Around line 338-340: Update extractAwdEnumValues in enumParser.js so it
matches the same case-insensitive prefixes accepted by
hasAwdEnumInvocationPrefix, including DATATYPE.ENUM and AWD.DATATYPE.ENUM forms.
Replace the current enum matching regex shape that uses overlapping \s*(.+)\s*
with a safer capture of the full argument body, then trim the captured value
after matching before parsing the enum entries.
- Around line 268-294: The enum tokenization in enumParser.js is stripping quote
delimiters too early, so quoted values are no longer recognized later and
escaped sequences are not properly restored. Update the parsing flow around the
enum value collection and the final normalization in the return map so quoted
tokens remain identifiable until after parsing, then unescape quoted values via
unescapeQuotedEnumValue only for truly quoted entries. Also make the parser
reject unterminated quoted strings instead of accepting them as valid enum
values.
🪄 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 Plus
Run ID: b94c9a6b-d740-4d1a-aa1b-c5c23dec6103
📒 Files selected for processing (19)
packages/core-ui/js/gui_components/shared/schema-row-rule-mapper.jspackages/core-ui/js/gui_components/shared/test-data/schema/schema-runtime.jspackages/core-ui/src/tests/generator/schema-row-rule-mapper.test.jspackages/core/js/command-help/command-help-validators.jspackages/core/js/data_generation/enum/enumTestDataRuleValidator.jspackages/core/js/data_generation/schema-rules-adapter.jspackages/core/js/data_generation/testDataRulesCompiler.jspackages/core/js/data_generation/utils/enum-rule-detection.jspackages/core/js/data_generation/utils/enumParser.jspackages/core/js/keywords/domain/datatype/datatype-enum.jspackages/core/src/tests/data_formats/export-string-conversion-bug-fix.test.jspackages/core/src/tests/data_formats/gherkin-convertor.test.jspackages/core/src/tests/data_formats/html-convertor.test.jspackages/core/src/tests/data_formats/markdown-convertor.test.jspackages/core/src/tests/data_generation/enum-format-bug-fix.test.jspackages/core/src/tests/data_generation/enum-surface-parity.test.jspackages/core/src/tests/data_generation/enum-value-format-generation.test.jspackages/core/src/tests/data_generation/unit/enum/enumParser.test.jspackages/core/src/tests/data_generation/user-reported-bug-http-method.test.js
💤 Files with no reviewable changes (3)
- packages/core/src/tests/data_formats/export-string-conversion-bug-fix.test.js
- packages/core/src/tests/data_generation/user-reported-bug-http-method.test.js
- packages/core/src/tests/data_generation/enum-format-bug-fix.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/js/data_generation/testDataRulesCompiler.js
Comment on lines
+14
to
+22
| if (!parsed.ok && !parsed.explicit) { | ||
| parsed = { | ||
| ok: true, | ||
| values: EnumParser.extractEnumValues(ruleSpec), | ||
| explicit: false, | ||
| source: 'legacy-enum-type', | ||
| error: '', | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t treat every non-explicit parse failure as a valid legacy enum.
Lines 14-22 currently convert any parseEnumRuleSpec() failure with explicit: false into success by re-running extractEnumValues(). Because TestDataRulesCompiler.compile() canonicalizes anything this validator accepts, malformed non-explicit specs can now be silently normalized into datatype.enum(...) instead of being rejected. parseEnumRuleSpec() already covers the supported implicit CSV forms, so this fallback needs to be removed or narrowed to a predicate that proves a real legacy surface.
Suggested fix
- if (!parsed.ok && !parsed.explicit) {
- parsed = {
- ok: true,
- values: EnumParser.extractEnumValues(ruleSpec),
- explicit: false,
- source: 'legacy-enum-type',
- error: '',
- };
- }
-
if (!parsed.ok) {
this.validationError = parsed.error;
return false;
}📝 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.
Suggested change
| if (!parsed.ok && !parsed.explicit) { | |
| parsed = { | |
| ok: true, | |
| values: EnumParser.extractEnumValues(ruleSpec), | |
| explicit: false, | |
| source: 'legacy-enum-type', | |
| error: '', | |
| }; | |
| } |
🤖 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/core/js/data_generation/enum/enumTestDataRuleValidator.js` around
lines 14 - 22, The fallback in enumTestDataRuleValidator currently turns every
non-explicit parse failure from parseEnumRuleSpec into success via
EnumParser.extractEnumValues, which lets malformed legacy enum specs slip
through. Update the validator to remove that blanket recovery or restrict it to
a clearly identifiable legacy CSV form, and keep parseEnumRuleSpec as the source
of truth for accepted implicit enums. Make the change in
enumTestDataRuleValidator so TestDataRulesCompiler.compile only canonicalizes
truly valid enum inputs.
Comment on lines
+73
to
+76
| if (command === 'datatype.enum' && params.length > 0) { | ||
| candidates.push(`datatype.enum(${EnumParser.extractEnumDisplayValue(params)})`); | ||
| candidates.push(params); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle awd.datatype.enum when deriving values from fieldDefinition.params.
Line 73 only recognizes datatype.enum, while the core-ui mapper now treats awd.datatype.enum as the same enum command. A context with fieldDefinition.command: 'awd.datatype.enum', params: 'active,inactive', and no ruleSpec falls through to [], causing valid enum-member validation to fail.
Proposed fix
- if (command === 'datatype.enum' && params.length > 0) {
+ if ((command === 'datatype.enum' || command === 'awd.datatype.enum') && params.length > 0) {
candidates.push(`datatype.enum(${EnumParser.extractEnumDisplayValue(params)})`);
candidates.push(params);
}📝 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.
Suggested change
| if (command === 'datatype.enum' && params.length > 0) { | |
| candidates.push(`datatype.enum(${EnumParser.extractEnumDisplayValue(params)})`); | |
| candidates.push(params); | |
| } | |
| if ((command === 'datatype.enum' || command === 'awd.datatype.enum') && params.length > 0) { | |
| candidates.push(`datatype.enum(${EnumParser.extractEnumDisplayValue(params)})`); | |
| candidates.push(params); | |
| } |
🤖 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/core/js/keywords/domain/datatype/datatype-enum.js` around lines 73 -
76, The enum value derivation in `getCandidates` only handles `datatype.enum`,
so `awd.datatype.enum` contexts fall through and return no candidates. Update
the command check in this enum parsing logic to treat `awd.datatype.enum` the
same as `datatype.enum`, and make sure the existing
`EnumParser.extractEnumDisplayValue(params)` and `candidates.push(params)` paths
are applied for both command names.
Comment on lines
+229
to
+241
| test('converts non-string values and escapes gherkin table separators', () => { | ||
| let table = new GenericDataTable(); | ||
| table.setHeaders(['Text', 'Number', 'Boolean']); | ||
| table.appendDataRow(['Data with | pipe', 42, true]); | ||
| table.appendDataRow(['plain data', 3.14, false]); | ||
|
|
||
| let output = new GherkinConvertor().fromDataTable(table); | ||
|
|
||
| expect(output).toContain('Data with \\| pipe'); | ||
| expect(output).toContain('42'); | ||
| expect(output).toContain('true'); | ||
| assertNoCommonErrorPatternsInValue(output); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin the false/0 serialization path here as well.
Right now this verifies only truthy non-string values. That misses the common regression where falsy primitives are treated as empty cells before escaping/stringification.
Suggested test tightening
- table.appendDataRow(['plain data', 3.14, false]);
+ table.appendDataRow(['plain data', 0, false]);
@@
expect(output).toContain('42');
+ expect(output).toContain('0');
expect(output).toContain('true');
+ expect(output).toContain('false');
assertNoCommonErrorPatternsInValue(output);📝 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.
Suggested change
| test('converts non-string values and escapes gherkin table separators', () => { | |
| let table = new GenericDataTable(); | |
| table.setHeaders(['Text', 'Number', 'Boolean']); | |
| table.appendDataRow(['Data with | pipe', 42, true]); | |
| table.appendDataRow(['plain data', 3.14, false]); | |
| let output = new GherkinConvertor().fromDataTable(table); | |
| expect(output).toContain('Data with \\| pipe'); | |
| expect(output).toContain('42'); | |
| expect(output).toContain('true'); | |
| assertNoCommonErrorPatternsInValue(output); | |
| }); | |
| test('converts non-string values and escapes gherkin table separators', () => { | |
| let table = new GenericDataTable(); | |
| table.setHeaders(['Text', 'Number', 'Boolean']); | |
| table.appendDataRow(['Data with | pipe', 42, true]); | |
| table.appendDataRow(['plain data', 0, false]); | |
| let output = new GherkinConvertor().fromDataTable(table); | |
| expect(output).toContain('Data with \\| pipe'); | |
| expect(output).toContain('42'); | |
| expect(output).toContain('0'); | |
| expect(output).toContain('true'); | |
| expect(output).toContain('false'); | |
| assertNoCommonErrorPatternsInValue(output); | |
| }); |
🤖 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/core/src/tests/data_formats/gherkin-convertor.test.js` around lines
229 - 241, The GherkinConvertor data-table test only covers truthy non-string
values, so it can miss regressions where falsy primitives are serialized as
empty cells; update the existing `converts non-string values and escapes gherkin
table separators` test in `GherkinConvertor.fromDataTable` to also assert
`false` and `0` are preserved and stringified in the output, alongside the
current pipe-escaping checks.
Comment on lines
+61
to
+73
| test('converts non-string values and escapes html markup characters', () => { | ||
| let table = new GenericDataTable(); | ||
| table.setHeaders(['Text', 'Number', 'Boolean']); | ||
| table.appendDataRow(['Data with < and >', 42, true]); | ||
| table.appendDataRow(['plain data', 3.14, false]); | ||
|
|
||
| let output = new HtmlConvertor().fromDataTable(table); | ||
|
|
||
| expect(output).toContain('Data with < and >'); | ||
| expect(output).toContain('42'); | ||
| expect(output).toContain('true'); | ||
| assertNoCommonErrorPatternsInValue(output); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the falsy primitive cases in this regression test.
Lines 69-72 only prove truthy non-strings survive export. A truthiness regression like value ? String(value) : '' would still pass here, even though false/0 are the values most likely to get dropped. packages/core/js/data_formats/generic-data-table.js already uses that pattern, so this edge case is worth pinning explicitly.
Suggested test tightening
- table.appendDataRow(['plain data', 3.14, false]);
+ table.appendDataRow(['plain data', 0, false]);
@@
expect(output).toContain('42');
+ expect(output).toContain('0');
expect(output).toContain('true');
+ expect(output).toContain('false');
assertNoCommonErrorPatternsInValue(output);📝 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.
Suggested change
| test('converts non-string values and escapes html markup characters', () => { | |
| let table = new GenericDataTable(); | |
| table.setHeaders(['Text', 'Number', 'Boolean']); | |
| table.appendDataRow(['Data with < and >', 42, true]); | |
| table.appendDataRow(['plain data', 3.14, false]); | |
| let output = new HtmlConvertor().fromDataTable(table); | |
| expect(output).toContain('Data with < and >'); | |
| expect(output).toContain('42'); | |
| expect(output).toContain('true'); | |
| assertNoCommonErrorPatternsInValue(output); | |
| }); | |
| test('converts non-string values and escapes html markup characters', () => { | |
| let table = new GenericDataTable(); | |
| table.setHeaders(['Text', 'Number', 'Boolean']); | |
| table.appendDataRow(['Data with < and >', 42, true]); | |
| table.appendDataRow(['plain data', 0, false]); | |
| let output = new HtmlConvertor().fromDataTable(table); | |
| expect(output).toContain('Data with < and >'); | |
| expect(output).toContain('42'); | |
| expect(output).toContain('0'); | |
| expect(output).toContain('true'); | |
| expect(output).toContain('false'); | |
| assertNoCommonErrorPatternsInValue(output); | |
| }); |
🤖 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/core/src/tests/data_formats/html-convertor.test.js` around lines 61
- 73, The regression test in HtmlConvertor.fromDataTable only covers truthy
non-string values, so add explicit assertions for falsy primitives like false
and 0 in the GenericDataTable export path to ensure they are preserved instead
of being dropped by truthiness-based conversion. Update the existing test case
in html-convertor.test.js to include rows or cells that exercise false and 0,
and verify the output contains their stringified values alongside the existing
HtmlConvertor and GenericDataTable coverage.
Comment on lines
+74
to
+81
| const executableRule = | ||
| rule.type === 'enum' | ||
| ? { | ||
| ...rule, | ||
| type: 'domain', | ||
| ruleSpec: EnumParser.normalizeToCanonicalDomainRuleSpec(rule.ruleSpec), | ||
| } | ||
| : rule; |
There was a problem hiding this comment.
EnumParser.normalizeToCanonicalDomainRuleSpec calls extractEnumValues internally, which throws 'Invalid enum format' for malformed ruleSpecs. Because this construction happens before generator.generateFrom, any exception propagates directly to the caller rather than returning a graceful { isError: true } response object. The old EnumTestDataGenerator encapsulated this error path within its own generateFrom handler. A guard here preserves the error-response contract for callers that don't first gate on successful compilation/validation.
Suggested change
| const executableRule = | |
| rule.type === 'enum' | |
| ? { | |
| ...rule, | |
| type: 'domain', | |
| ruleSpec: EnumParser.normalizeToCanonicalDomainRuleSpec(rule.ruleSpec), | |
| } | |
| : rule; | |
| let executableRule = rule; | |
| if (rule.type === 'enum') { | |
| try { | |
| executableRule = { | |
| ...rule, | |
| type: 'domain', | |
| ruleSpec: EnumParser.normalizeToCanonicalDomainRuleSpec(rule.ruleSpec), | |
| }; | |
| } catch { | |
| value = dataResponse(rule.name, '**ERROR**', false, true); | |
| break; | |
| } | |
| } |
Comment on lines
32
to
34
| static isAwdEnumFormat(ruleSpec) { | ||
| return /^(enum|datatype\.enum|awd\.datatype\.enum)\s*\(/.test(ruleSpec); | ||
| return /^(enum|datatype\.enum|awd\.datatype\.enum)\s*\([\s\S]*\)$/i.test(String(ruleSpec || '').trim()); | ||
| } |
There was a problem hiding this comment.
isAwdEnumFormat allows empty parentheses via [\s\S]*, so isCanonicalSchemaSerializableEnumRuleSpec returns true for enum() and datatype.enum(). normalizeToCanonicalSchemaRuleSpec then calls extractAwdEnumValues, whose match regex uses (.+) (one-or-more), which fails for empty parens and throws 'Invalid enum format'. Because neither schema-conversion.cloneRule nor either renderSpecFromRulesWithTokens function wraps that call in a try/catch, any rule whose ruleSpec survived compilation as enum() or datatype.enum() causes parseSchemaText or renderSchemaText to throw an uncaught exception. Aligning isAwdEnumFormat to require at least one character closes the gap.
Suggested change
| static isAwdEnumFormat(ruleSpec) { | |
| return /^(enum|datatype\.enum|awd\.datatype\.enum)\s*\(/.test(ruleSpec); | |
| return /^(enum|datatype\.enum|awd\.datatype\.enum)\s*\([\s\S]*\)$/i.test(String(ruleSpec || '').trim()); | |
| } | |
| static isAwdEnumFormat(ruleSpec) { | |
| return /^(enum|datatype\.enum|awd\.datatype\.enum)\s*\([\s\S]+\)$/i.test(String(ruleSpec || '').trim()); | |
| } |
|
Too many files changed for review. ( |
| */ | ||
| static extractAwdEnumValues(ruleSpec) { | ||
| // Match patterns like: enum(value1,value2) or enum("value1", "value2", "value3") | ||
| const match = ruleSpec.match(/^(?:enum|datatype\.enum|awd\.datatype\.enum)\s*\(\s*([\s\S]*)\s*\)$/); |
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.
Summary
Refactors enum handling so public enum spellings normalize into the domain command execution model via
datatype.enum, while preservingenum(...)as the canonical emitted schema text.Also rationalizes the schema interaction matrix coverage by removing static matrix/parity fixtures and the broad app-vs-generator parity sweep, keeping generated runtime coverage plus small app/generator page-wiring smoke suites.
Closes #228.
Validation
pnpm test -- packages/core-ui/src/tests/interaction/matrix/schema-interaction-scenario-builder.test.js packages/core-ui/src/tests/interaction/matrix/schema-interaction-runtime-matrix.test.js packages/core-ui/src/tests/interaction/matrix/app-schema-interaction-matrix.test.js packages/core-ui/src/tests/interaction/matrix/generator-schema-interaction-matrix.test.js packages/core-ui/src/tests/interaction/matrix/deterministic-scenario-seed.test.jspnpm run verify:localpnpm run verify:uipnpm run verify:localSummary by CodeRabbit
datatype.enumas a first-class keyword with dedicated help/usage, validation, and variadicvaluessupport.datatype.enum(...)form.