Close JSON Schema gap-analysis findings by bergundy · Pull Request #169 · temporalio/nexgen

bergundy · 2026-08-31T21:56:16Z

Summary

reconcile the JSON Schema specifications with the completed gap-analysis audit
close parser, planning, naming, reference, generated-layout, documentation-safety, and validation gaps
align Go, Java, Python, and TypeScript generation and runtime behavior, regenerate checked-in outputs, and add regression and conformance coverage

Breaking changes

limit materialized time and date-time offsets to the portable -18:00 through +18:00 range and use Java OffsetDateTime
reject colliding stable TypeScript DEFAULT_<FIELD> exports instead of renaming an existing constant when another model is added

Validation

cargo validate (Node 26, JDK 21, .NET 9)

tconley1428 · 2026-09-01T18:23:09Z

+    origins: &mut BTreeMap<PathBuf, PathBuf>,
+    path: PathBuf,
+    contents: String,
+    origin: PathBuf,


I don't think we should represent origin as a pathbuf if it often isn't a real path.

tconley1428 · 2026-09-01T18:24:09Z

@@ -59,6 +59,31 @@ impl GeneratedFiles {
    }
 }

+/// Inserts one generated file while retaining enough provenance for P7.2's


I don't like referencing the json principles in the generic code. What are we actually trying to accomplish here?

tconley1428 · 2026-09-01T18:24:46Z

@@ -808,6 +826,38 @@ fn insert_generated_file(
    Ok(())
 }

+fn insert_generated_file_with_origin(


Why is this different from the one defined in mod.rs?

tconley1428 · 2026-09-01T18:25:50Z

@@ -85,11 +87,23 @@ fn generate_tree(
 ) -> Result<GeneratedFiles> {
    let model_hoists = tree_model_hoists(branch)?;
    let mut files = BTreeMap::new();
+    let mut origins = BTreeMap::new();


I don't really understand the whole origins thing here. We're adding a lot of context, best I can tell is so that we don't add the same file twice? But because of different sources or something?

tconley1428 · 2026-09-01T18:34:16Z

+/// input leaf/support fragment, or a descriptive virtual path for a fixed
+/// generator-owned artifact.
+pub(crate) fn insert_generated_file_with_origin(
+    files: &mut BTreeMap<PathBuf, String>,


If I've come to grasp the point here correctly, I don't think a separate origin map makes sense. Why not just store the source of the file as part of the file itself?
files: &mut BTreeMap<PathBuf, String>, into files: &mut BTreeMap<PathBuf, (String, String)>, though probably with a struct instead.

Potentially there's enough complexity there that really it is a new object with some helpers for FileMap or whatever. Assuming the same thing is shared between langs, which it's unclear to me is the case, or why this is defined in the root mod.

tconley1428 · 2026-09-01T20:48:34Z

+        path,
+        contents,
+        source,
+        "rename one input file or directory so the generated Python paths differ",


I don't see how it knows this remedy in this location, is this just sorta the default?

tconley1428 · 2026-09-01T20:51:42Z

+/// strings/templates and regex literals, so this small lexer can distinguish a
+/// real `Long` type occurrence without treating the exact const string
+/// `"Long"` (or a pattern containing it) as a package dependency.
+fn contains_identifier_outside_literals(source: &str, identifier: &str) -> bool {


This whole thing seems pretty sketchy. Some kind of hand built parser for the typescript?

You're right. I missed this while skimming the generated code. I got used to not paying much attention to the code in this project.

tconley1428 · 2026-09-01T20:52:28Z

@@ -3470,6 +3619,7 @@ fn render_support_module(support_source: &str) -> String {
 fn render_index_module(
    services: &[RenderedService<'_>],
    model_type_names: &BTreeSet<String>,
+    has_json_runtime_module: bool,


This seems like an abstraction leak, although one that partially already existed. Why does this generator know about Violation?

It also knows about Long, which is proto / WIT specific FTR. I will clean this instance up though.

tconley1428 · 2026-09-01T20:54:06Z

+            &mut files,
+            path.clone(),
+            contents.clone(),
+            "<generated Python recursive-model hoist>",


How are these synthetic sources helpful? Won't they just create confusing error messages? Seems like this would need to know the original file it was hoisted from or something.

tconley1428 · 2026-09-01T20:58:46Z

-        && tree_support_files.contains_key(&PathBuf::from("definitions.ts"));
-    insert_branch_index_file(&mut files, branch, has_json_runtime_module)?;
-    insert_files(&mut files, tree_support_files)?;
+    let additional_exports: &[&str] = if branch_has_json_models(branch) {


Explicitly checking for json models is a leak here as well.

I don't think we were very methodical about avoiding leaks so far. You'll notice the leak is pre-existing.

Yep. Pointing it out, if we decide not to fix it now, that's okay.

I've been looking for them, but it is hard not to miss some. Filing an issue maybe, just as a reminder to come back and do some clean up.

Review of 18a5609 found three places where a spec had been relaxed so that a known code defect became conformant, and a set of findings that were not reached at all. The commit predates roughly half the adversarial review, so some of what it encoded had since been retracted. Reversals — the spec was amended in the wrong direction: * format.md no longer lists Java among the nanosecond-resolution targets for `time`, and no longer prescribes canonicalizing through OffsetTime/LocalTime. Java holds `time` in a String; a String carrier has no resolution of its own, so the truncation is the artificial common-denominator floor P1 forbids. Only Python's microsecond limit is a licensed loss. Also drops the stale claim at the head of the section that `time` round-trips losslessly. * allOf.md scopes the raw pre-merge gate to checks decidable on one branch that a merge could silently discard, and states that naming and satisfiability checks are not among them. The previous wording licensed running P15 value-constant checks per authored branch, which over-rejects schemas whose merged form is clean, and contradicted the delegation this file states twice elsewhere. * dependentRequired.md restores the rewrite fix-it for the draft-07 `dependencies` array form, matching the exclusiveMaximum precedent. Reader- directed porting guidance dissolved the missing-diagnostic defect instead of recording it. * generated-file-layout.md no longer asserts that reserved-name and P15 diagnostics are actionable — two findings show they are not — and instead makes applicability the obligation. * PRINCIPLES.md P1 no longer folds the binary64 exception under "provably cannot carry a wire distinction"; it is a domain restriction owned by type. Decisions the specs did not previously make, each of which blocked a well-defined code fix: * oneOf.md: a non-annotation sibling of a `oneOf` rejects, with a fix-it naming the branch. Silent discard is what P7.1 exists to prevent, and the sibling additionally suppresses two unrelated type guards. * nullability.md: constraints belong on the non-null branch, `default` on the wrapper; each rejects on the other node. Records that a wrapper default lowers against the branch's type. * PRINCIPLES.md P11: names the boundaries the failure must survive, and states that Go does not meet the clause today because the SDK's payload converter severs the error chain, inverting non-retryability. * PRINCIPLES.md 11a: defines the Violation.path grammar, including escaping for arbitrary map keys and the join for a bracket-leading segment, which the four targets currently disagree on. * min/maxProperties.md: the count is over the wire object at each boundary, never over in-memory fields; states the asymmetry the optional+nullable collapse implies rather than leaving it to be discovered. * min/maxProperties.md: the count keywords own satisfiability reconciliation against every keyword that bounds the key space, not only `required`. * ref.md: bounds the input root by the invocation root, rejects absolute refs, and defines what "reproducible" promises — module and package names must not encode the checkout's location. * PRINCIPLES.md P15: the namespace of an emitted file includes what it imports and what the emitter synthesizes into it, covering the service file's SDK imports and Go's synthesized <Service>Client. * pattern.md / maxLength.md: a materialized node's assertion measures the canonical wire form at both boundaries, and a literal is canonicalized before it is checked. Smaller corrections: const.md's Java block no longer uses the arrow switch that does not compile under --release 8; multipleOf.md stops claiming UnmarshalJSON calls the shared Validate; pattern.md's residual-risk sentence names the edges actually open; description.md reconciles "does not reflow" with the wrapping section and records that wrapping can move a doc tag into a position its tool treats as meaningful; PRINCIPLES.md P12.2 states that sharing is a requirement on the predicate rather than on the call graph, which the per-keyword rewrites in the parent commit had left it contradicting.

A whole-diff audit of 18a5609 found five further places where the amendment made a reproduced P1 conformant or unreachable, none of them caught by the scoped reviews, plus a set of self-contradictions the edits introduced. Reversals: * PRINCIPLES.md no longer says Python's last-import-wins shadowing is "not reachable generated output". It is reachable: two service names differing only in case fold to one origin label, the loader accepts, and the emitted module defines the same class twice. The counterfactual framing holds only for the cross-module model-name case the loader rejects up front. * pattern.md gate rule 7 restores the "outside a character class" scoping. Its absence asserted that `[]]`, `[]a]`, `[^]]` and `[]-a]` reject; they pass, and Node in `u` mode throws on import. The hole is now stated as a hole. * pattern.md gate rule 8 no longer claims the D7 rule keeps evaluation linear. It fires only when the outer quantifier is unbounded, so `a{2}*` still panics at Go package init and `^(a|b)*$` still overflows Java's stack. Both are named as open. The neighbouring "complete gate" and "all handled" claims are softened to match. * maxItems.md keeps the 2^53-1 ceiling but states the obligation the ceiling does not discharge: a bound in [2^31, 2^53-1] requires Java to emit a `long` literal, or the package does not compile. The ceiling read as though it made the range safe, which is the claim the finding disproves. * deprecated.md scopes the "where the tooling supports it" hedge to TypeScript and Python, where the limit is genuine. It was covering Go's inert service/operation marker, which is a generator placement bug — one blank line fixes it — not a toolchain limit. Self-contradictions introduced by the parent commit: * PRINCIPLES.md TS §4 said converters join a per-module namespace and, two lines later, that they are validated run-wide. Replaced rather than appended. * additionalProperties.md said key order is preserved and, in the same sentence, that member order is not guaranteed. "key order" dropped from the preserved list. * type.md's `"null"` type-mapping row still read "only inside nullability pattern" after the loader bullet was widened to any `oneOf` branch. * contentEncoding.md rejected the materializing-format combination one-sidedly; format.md had no counterpart bullet, leaving the question unadjudicated from the other side. Mirrored. * The inert-`$ref`-sibling carve-out was written into allOf.md and comment.md but not ref.md, which still stated siblings merge unconditionally. Propagated, and all three now carry a Status: unimplemented marker — the fold gate admits only the four name keywords today, so the clause is the contract and the gate is the defect. Overstatements narrowed: type.md's "only a free-form oneOf object branch" (an un-hoisted array branch lands on the same fallback); enum.md stating the union-variant leg as fact when its own report calls it unreproduced; format.md's ecosystem row claiming such documents are never accepted, when the dialect gate is conditional on a declared `$schema`.

tconley1428 · 2026-09-02T21:53:52Z

+        &mut self,
+        path: impl Into<PathBuf>,
+        contents: String,
+        source: impl Into<String>,


I think I would either make source optional, or add another insert method which does not take a source, like insert_system_file or something which has a default error message.

I'm like medium on the idea though. If you don't want to do that, throw a comment on this function describing that source can be a placeholder for a generated file, and that failing should be unexpected, indicating a validation failure upstream.

bergundy marked this pull request as ready for review September 1, 2026 18:06

bergundy requested a review from a team as a code owner September 1, 2026 18:06

tconley1428 reviewed Sep 1, 2026

View reviewed changes

bergundy requested a review from tconley1428 September 1, 2026 19:10

tconley1428 reviewed Sep 1, 2026

View reviewed changes

bergundy added 17 commits September 2, 2026 12:13

docs: align JSON Schema specs with gap analysis

16b36d2

docs: audit and complete the gap-analysis spec amendments

65d26e6

Fix emitted name collision scopes

ceff92e

Fix Python recursive model hoisting

ba68701

Complete P15 value token planning

046f29f

Fix reviewed emitted-name scope defects

31e695a

Fix default collision documentation

71705f0

fix typescript JSON wire key handling

5034e5b

tighten portable regex gate

9418585

fix JSON runtime validation parity

f763ddb

fix(json-schema): tighten pattern and materialization gates

13c4c95

fix(json-schema): close parser normalization gaps

646363b

fix(json-schema): correct parser edge cases

d78bc02

fix(json-schema): reconcile parser integration cases

7b3559d

fix(json-schema): preserve integer literal spelling across merges

f10636f

bergundy added 28 commits September 2, 2026 12:15

fix(go): make schema regex helpers collision-safe

435f184

fix(go): reject outbound temporal offsets beyond wire range

fc5b137

fix(java): preserve full temporal offset range

e3f800b

test(python): type negative item witness explicitly

1ebf142

fix(python): silence static-only runtime guard diagnostics

8dae72c

test(typescript): format violation path witness

7d61e63

fix(go): emit semantic helpers once per package

b2fe771

fix(java): qualify temporal carrier imports

f594ac2

fix(java): annotate qualified temporal carriers

c607eb0

fix(java): separate collection and element nullability

29a39a2

fix(go): align semantic helper planning after integration

5e5e2b2

chore(typescript): regenerate integrated JSON samples

7eed360

docs(json-schema): mark Java count literals implemented

6a638a0

fix: reconcile gap branch with generator config

62a0bc9

fix(typescript): keep payload validation helper internal

4bb3d57

fix(typescript): omit empty validation branches

a78f021

Remove generated Python lint suppressions

f0764c0

fix(go): simplify output collision remedy

a2d5633

Rename Python barrel references to __init__.py

4b8b1eb

refactor: isolate frontend-specific generator behavior

e9aca67

fix(json-schema): limit materialized temporal offsets

517de86

Remove JSON Schema term from Java diagnostic

f32208a

fix(json-schema): drop numeric lexeme preflight

7b87c74

refactor: consolidate generated file tracking

35780bd

refactor(typescript): track Long imports structurally

bf84a06

refactor(typescript): move runtime exports to backend

ad9e10d

fix(python): preserve generated file provenance

b5c226b

chore(java): regenerate stable example headers

78e64d2

bergundy force-pushed the fix/all-gap-analysis branch from cc2a5b7 to 78e64d2 Compare September 2, 2026 21:42

tconley1428 reviewed Sep 2, 2026

View reviewed changes

Uh oh!

Conversation

bergundy commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bergundy commented Aug 31, 2026 •

edited

Loading