Repository navigation
fix: replace regex and string hacks with real parsers across ports - #511
Merged
Merged
Conversation
…sers across ports (audit findings 5, 7-15, 17, 18) Currency parsing via Intl, SQL splitter, view comparators, Python YAML 1.2, Mustache tag scanning, forgiving XML reader, JSONC-aware upgrade rewriter, leak-scan renames, Java authored-name serialization, ERR_COLUMN_NAME_COLLISION, pnpm workspace parsing and assorted smaller instances.
…allowlist generators An ejected copy is package-renamed, so same-package bare-name resolution stopped compiling once #503 started calling the support class there.
…te-segment collision (audit finding 18 follow-ups)
…ter glob grammar in Java ERR_ENUM_NORMALIZE_COLLISION in all ports; CSV formula guard skips strict numeric literals; <filters> use Scope.compilePattern semantics and fail generation when a pattern would select fewer objects than before.
…n extract; make eject --undo order-sensitive The forgiving JSON reader dropped \uXXXX, \b and \f escapes and the fence matcher closed at the first inner triple backtick, in all five ports. eject --undo treated a copy whose only edit was a reorder as stock (TS, Python, C#). Python LONG coercion no longer rounds through float. Adds three shared extract-conformance cases.
…workspace walk, splitter dialect, constraint fallback)
…verify, JPY parse
…licit handoff note
…owups change: Java <filters> upgrade example (java.md and CHANGELOG), eject reorder wording (own-your-codegen.md), CSV numeric exemption (reporting.md). Edits verified by grep and diff stat.
…h.round claim from docs/ports/typescript-client.md, the parseCurrency header in currency.ts, and the currency-input.tsx comment. Doc and comment edits only; no remaining findings.
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.
Notes for reviewers
Scope. This PR carries both
fm/mo-regex-blockersand the follow-up branch (audit findings 1, 2, 4, 5, 7-15, 17, 18). It is the third pipeline run on the branch: run 1 was aborted over a stale intent line, run 2 was cancelled after review rounds because the intent still described a removed C# detector, and run 3 is this one, started from run 2's head.C# namespaces and handoff (captain's direction). The C# port no longer reads source code to infer configuration. Namespace inference via Roslyn is removed, and Roslyn is gone from
MetaObjects.Codegen. Namespaces now come from the package bindings in the codegen config (type override,PackageNamespaces,Convention,UnmappedStrategy), withcsharp.namespace/--namespaceas the single flat namespace for the defaultFlattenstrategy. Every resolved namespace is checked as a dotted C# identifier path at the resolver, naming the metadata package and the bad segment. Thecodegen/Program.cssource check was removed on the captain's direction, not converted to a parser; a 1.0 project whoseProgram.csbuilds its own generator list gets a CHANGELOG upgrade step (replace the list withGenerators.All). A sweep of the TS, Java, Kotlin, Python and C# ports found no other code that reads generated or user source to infer configuration (JavaFmtSupport.detectPackagereads metadata JSON and is fine).Behaviour notes.
@patternwith\p{L}now fails atgen(Pythonrehas no such class); generated Pydantic models use ASCII-flag compiled patterns, so the Pydantic minimum is raised to 2.8 (CHANGELOG upgrade note).<filters>moved to the shared scope grammar: breaking, with a CHANGELOG upgrade note, and a filter that would now select less fails with an error that says "since 8.2.0".scripts/leak-scan.test.tsis not wired intoci-local.sh.uvdev environment (--version,test_format, jinja2); they pass underuv run --frozen --extra integration pytest.Test evidence at the run-3 pipeline head (exported tree, run per port; the pipeline's own test step was inconclusive because the full TypeScript suite timed out):
mvn clean install, isolated repo)tests/integrationexcluded)The TypeScript
integration-tests(Testcontainers) package did not complete in that run. Typecheck and build are clean.Hand-driven scenarios, all passing:
userId/userIDcollision error naming both fields; pnpm workspace parity (tools/buildfound,test/testsskipped); currency sign and strict grammar; Python YAML 1.2 (0777is 777,yes/no/off/1:30stay strings, duplicate key errors); Java<filters>*vs**and the vanishing-filter error; C# bindings (config-only namespace, invalid segment errors, nothing-to-emit not erroring) through their test classes;eject --undoby hand in TypeScript and Python and through the suites in Java, Kotlin and C#.What Changed
\uXXXXwith surrogate pairs,\b,\f,\/) in every port, and fenced replies close on a line-anchored fence, so an inner ``` no longer truncates them. Currency parsing inruntime-webuses `Intl.NumberFormat` parts with a strict grammar, and `eject --undo` compares owned copies as ordered text, so a reorder no longer deletes a hand-edited file.userId/userIDstyle column collisions fail at load naming both fields, and Python YAML uses the YAML 1.2 core schema. C# namespaces come from the package bindings in the codegen config. The SQL statement splitter handles SQLite triggers and PostgresE''strings, and view comparison covers literal-only changes./Userspaths. Java<filters>use theScope.compilePatterngrammar and fail generation when a filter selects fewer objects than before (see the CHANGELOG upgrade note).Validation
Risk Assessment
GenConfig.Namespaceerror handled in gen/verify, pnpm test dirs, zero-decimal currency locale), but the change is 284 files and I read only the recent fix-round hunks, not the whole diff.Testing
Baseline ts-fast/ts-unit was green per context. I drove six TypeScript scenarios live against source (reply-parser escapes, fence close, eject --undo reorder refusal and untouched removal, enum collision load errors, SQL splitter trigger and E-string); all passed. Not driven: userId collision, pnpm test-dir skip, currency, and the other ports. The full TypeScript regression suite timed out at 590s without a summary, so broad regression is unverified. Verdict inconclusive.
bun testin server/typescript exceeded the 590s limit with no summary.Superseding evidence (gathered after this table, on the run-3 pipeline head, outside the pipeline). The rows above marked untested, and the timed-out suite, are closed:
meta verifyfails withERR_COLUMN_NAME_COLLISIONnaminguserId,userIDand columnuser_id.tools/**andpackages/**patterns,tools/buildis found andtools/test,packages/a/test,packages/a/testsare skipped.-$1,234.50,(1,234.50),$-5.00,5.00-give negative minor units;-5.00-,12abc,1,2,3,1e3,12.50 (2 items)give null; de-DE1.234,50 €gives 123450.0777loads as 777,yes/no/off/1:30stay strings, a duplicate key is an error. Full Python suite: 4001 passed, 0 failed.<filters>:acme::*and bare*againstacme::shop::Orderraise the "since 8.2.0" error,acme::**,acme::shop::*and**select it. Full Java and Kotlin reactor: 5040 tests, 0 failed, 1 skipped.CodegenNamespaceTestsandPackageBindingResolverTests(config-only namespace then bare verify clean; nothing to emit does not error; invalid segments such asmy-appand2fa, and keywords, fail for an override and for a convention). Full C# suite: 3982 passed, 0 failed, 1 skipped.integration-testspackage did not complete.Evidence: TS reply parser extraction results
Evidence: Enum normalize collision load errors
Evidence: SQL statement splitter output
Evidence: Partial full TS test run (timed out)
Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
🔧 **Rebase** - 1 issue found → auto-fixed ✅
CHANGELOG.md- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
server/typescript/packages/sdk/src/workspace.ts:138- The round-4 fix for review-3 was told to match pnpm by checking pnpm's real behaviour. It skips onlynode_modulesandbower_components(WORKSPACE_IGNORED_DIRS). pnpm's ownfindPackages(pnpm/pnpmworkspace/projects-reader/src/findPackages.ts) usesDEFAULT_IGNORE = ['**/node_modules/**', '**/bower_components/**', '**/test/**', '**/tests/**']. Concrete input:packages: ['packages/*']with a manifest atpackages/test/package.json, ortools/**withtools/x/tests/fixture/package.json. pnpm does not treat these as workspace members, butdiscoverWorkspacereturns them. A test fixture whose manifest carries ametaobjectspackage therefore joins the model or collides, and nothing errors. Fix: addtestandteststo the ignored set. The check must apply to any path segment, both literal segments (expandSegments, line ~234) and wildcard and globstar descent, as pnpm's**/x/**ignore does. Addpackages/test/*andtestscases toworkspace.test.ts, which currently asserts only the bower/dist/build cases. Also correct CHANGELOG.md:455-456, which says "onlynode_modulesandbower_componentsare never entered". Fixed on the head:WORKSPACE_IGNORED_DIRSnow includestestandtests, matching pnpm's default ignore list; tests covertools/buildas a member and the skipped test dirs.server/csharp/MetaObjects.Codegen/PackageBindingResolver.cs:407- Round 7'sValidNamespacewas meant to ensure every produced namespace is a valid C# identifier path. It checks only the character classes, so a reserved keyword passes. Concrete input:Convention { Case = PackageCase.Lowercase }(orPreserveorCamelCase) and metadata packageacme::event,acme::objectoracme::default. The resolver returnsacme.event, validation accepts it, andgenexits 0 writingnamespace acme.event;. The adopter's build then fails with CS1001. The same gap exists for a flatcsharp.namespace/--namespacesuch asAcme.public(Generator.cs:336, the GenConfig.Namespace getter) and for the FR-019 provided-enum path (Fr019SharedEnum.cs:137). All of these route through the one helper. Fix there once: reject a segment that is a C# reserved keyword (not the contextual ones), or emit it@-escaped. Add a case to PackageBindingResolverTests and CodegenNamespaceTests. Fixed on the head: the resolver's namespace validity check now rejects C# reserved keywords (ReservedKeywords), for overrides, conventions and the flat namespace.docs/CONFORMANCE.md:29- Inline fix: the corpus table and section headers still gave pre-branch counts. This branch adds 8 metamodel fixtures and 2 render fixtures. I updated the metamodel corpus from 413 to 421 at lines 29 and 287, render from 16 to 18 at lines 33 and 322, and the orphaned-fixtures summary at line 612 (metamodel 421, verify 45, render 18).docs/CONFORMANCE.md:324- The inline fix changed the render-conformance section header to (18), but the sentence under it still says "All 16 fixtures". The section now gives two different counts. The breakdown that follows (4 end-to-end examples and 11 Mustache-semantics fixtures) also does not cover the two fixtures this branch adds: render-comment-with-partial and render-csv-numeric-exempt. Change "All 16 fixtures" to "All 18 fixtures", and update the breakdown so it accounts for the two new fixtures. Leave the corrected header and the table counts as they are.🔧 Fix applied.
1 info still open:
docs/CONFORMANCE.md:29- Inline fix: the corpus table and section headers still gave pre-branch counts. This branch adds 8 metamodel fixtures and 2 render fixtures. I updated the metamodel corpus from 413 to 421 at lines 29 and 287, render from 16 to 18 at lines 33 and 322, and the orphaned-fixtures summary at line 612 (metamodel 421, verify 45, render 18).server/typescript- Full TypeScript regression suite (cd server/typescript && bun test) did not complete: killed by 590s timeout with no pass/fail summary. Broad regression is unverified.bun testin server/typescript exceeded the 590s limit with no summary.scripts/ci-local.sh --only ts-fast --only ts-unit --strict-toolchainsbun extract driver script against render/src/extract/extract.tsdisposable project:meta init,meta eject entity, field reorder thenmeta eject --undo entity(refused), untouched copymeta eject --undo entity(removed), via cli/src/index.ts run()bun enum-collision driver (enum-collision-drive.ts) against metadata MetaDataLoaderbun split-statements driver (split-drive.ts)fullbun testin server/typescript (timed out at 590s)🔧 **Document** - 1 issue found → auto-fixed ✅
docs/ports/typescript-client.md:347- Claim that float drift is bounded by a single Math.round in parseCurrency was not checked against the rewritten parseCurrency (Intl.NumberFormat parts).🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.