Repository navigation
TS emitter correctness + repo hygiene (10-item sweep) - #4
Conversation
The generated index.ts re-exported 12 of 18 runtime modules, omitting
concurrency, dedup, idempotency, middleware, streaming, and cache. So
'import { streamSse } from "./index"' failed (the documented entry
point), forcing consumers to know internal module paths. All 18 are
always emitted, so the re-exports are always valid; verified the SDK
still typechecks (no duplicate-export conflicts).
Same bug class as the Rust/Go ValidationError collision: a spec model named like a built-in SDK type (ApiClient, ResponseCache, ValidationError, Logger, Middleware, ...) would redeclare it and break compilation with a duplicate-export error. Add safe_model_name() in name.rs, applied consistently at all three model-name derivation sites (emit_object, emit_enum, and the file/barrel paths in lib.rs) so the filename, export, and barrel re-export all agree. Colliding names get a 'Model' suffix. Verified: the GitHub TS SDK (which has a ValidationError model) now generates ValidationErrorModel.ts and typechecks clean; petstore unaffected (no colliding names).
#3: Added github_generates_and_typechecks_ts + stripe_generates_and_typechecks_ts to regression.rs (generate → npm install → tsc --noEmit), mirroring the existing go/rust gates. assert_tsc helper skips gracefully when node/npm are absent. This immediately caught a real bug: Stripe's DeletedExternalAccount and DeletedPaymentSource unions both contain DeletedBankAccount/DeletedCard arms, so both emitted top-level isDeletedBankAccount/isDeletedCard guards that collided under the barrel's 'export *' (TS2308). Fix: TS oneOf guards are now union-scoped (is{Union}{Arm}, e.g. isPetEventPetCreated) instead of arm-only (isPetCreated). Rust is unaffected (its guards are enum methods, inherently scoped). Updated the 3 affected unit tests + README/docs-site examples.
Cargo.toml was stuck at 0.8.0 while README/CHANGELOG/tags were at 1.6.x. This also corrupted every generated specforge-version.json stamp (it uses env!(CARGO_PKG_VERSION), inheriting the workspace version). Bumped [workspace.package].version + the 5 internal-crate dependency versions. Now Cargo.toml, cargo build, specforge --version, and the generated version stamp all agree on 1.6.1.
It skipped 'cargo build' when the binary already existed, so it silently regenerated the example SDKs with a STALE binary (this bit us: examples missing emit_readme changes). Now always rebuilds; incremental build is fast and guarantees examples reflect the current emitter.
- ir.rs: Model enum large_enum_variant — boxed ObjectModel would force deref at 70+ match sites for no real win (Model values aren't stack-hot); documented #[allow] with rationale instead - merge.rs: replace spec.clone() with std::slice::from_ref(&spec) in the test (merge_specs takes &[Value] by reference, so the clone was unnecessary) Clippy now: 0 warnings (was 120 at the start of this work).
The docs-site reference/changelog.md was stuck at 1.2.0 (missing 1.3.0, 1.4.0, 1.5.0, 1.6.0) and had drifted out of sync with the root CHANGELOG.md. Regenerated it from the root (the single source of truth), preserving the Docusaurus frontmatter. Now covers 0.1.0 -> 1.6.1.
The // Deprecated: Use {alt} and // Returns {desc} lines still inlined
spec text directly (alt from deprecation-alternative extraction, desc
from response descriptions). Both can contain embedded newlines or */
that break the generated Go. Added go_inline() (escape */ + collapse
newlines) and routed all 4 at-risk sites through it.
Property/param/body/returns/enum/object docs already used go_doc/
go_doc_label (fixed earlier); this closes the remaining inline gaps.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
specforge: OpenAPI spec diff (4 files)Breaking changes detected. This PR modifies OpenAPI specs in ways that may break existing clients.
|
PR Summary by QodoFix TS emitter collisions, expand TS regression gates, and align versions to 1.6.1
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Code Review by Qodo
1. TS rename breaks references
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| let name = crate::name::safe_model_name(&pascal(&o.name)); | ||
| let mut refs = BTreeSet::new(); |
There was a problem hiding this comment.
1. Ts rename breaks references 🐞 Bug ≡ Correctness
TypeScript model/enum names are now rewritten via safe_model_name() (e.g. ValidationError→ValidationErrorModel), but TypeScript reference rendering and import generation still use plain pascal(), so the generated SDK can import non-existent files/types and fail tsc for any spec that contains one of these reserved names.
Agent Prompt
## Issue description
The TS emitter now renames some schema model identifiers (and filenames/exports) using `safe_model_name()`, but other emitter components still assume the original `pascal()` name for schema references. This creates inconsistencies such as:
- file emitted as `ValidationErrorModel.ts` exporting `ValidationErrorModel`
- other generated files importing `../models/ValidationError` and using type `ValidationError`
This causes TypeScript compilation failures and/or wrong type bindings whenever a spec contains a schema whose PascalCase name matches a TS runtime builtin (e.g. `ValidationError`, `ApiClient`, `Logger`, etc.).
## Issue Context
`safe_model_name()` is intended to avoid collisions with hardcoded TS runtime exports, but it must be the single source of truth for *all* schema-reference naming (type rendering, imports, validator naming, schema descriptor refs, union guard arm names).
## Fix Focus Areas
- crates/specforge-ts/src/types.rs[55-63]
- crates/specforge-ts/src/models.rs[49-111]
- crates/specforge-ts/src/models.rs[115-177]
- crates/specforge-ts/src/models.rs[352-390]
- crates/specforge-ts/src/operations.rs[82-96]
- crates/specforge-ts/src/operations.rs[401-418]
- crates/specforge-ts/src/runtime.rs[765-812]
- crates/specforge-ts/src/runtime.rs[1158-1164]
### Implementation notes
- Introduce a helper like `safe_schema_type_name(name: &str) -> String` that does `safe_model_name(&pascal(name))`, and use it for every `Type::Reference` rendering/import/function naming.
- Update union guard generation so both `union_name` and `arm_ty` use safe names.
- Update model import generation (`build_imports`) and API imports (`operations.rs`) to import from the safe filename stem.
- Ensure `validate{Model}()` and `{Model}Schema` names in `validate.ts` are also based on the safe name so runtime validation wiring continues to compile.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Install devDependencies (typescript) quietly. | ||
| let install = std::process::Command::new("npm") | ||
| .args(["install", "--no-audit", "--no-fund", "--loglevel=error"]) | ||
| .current_dir(dir) | ||
| .output() | ||
| .map_err(|e| format!("spawn npm install: {e}"))?; | ||
| if !install.status.success() { | ||
| return Err(format!( | ||
| "npm install failed (exit {:?}):\n{}", | ||
| install.status.code(), | ||
| String::from_utf8_lossy(&install.stderr) | ||
| )); | ||
| } |
There was a problem hiding this comment.
2. Ts gate needs network 🐞 Bug ☼ Reliability
The new TS regression gate always runs npm install in a fresh temp directory, which introduces a network/package-registry dependency and can make CI slow or flaky even when the generator output is correct.
Agent Prompt
## Issue description
`assert_tsc()` unconditionally executes `npm install` in the generated temp package directory. This may require registry access and can fail due to transient network/registry issues, turning the regression suite into a flaky, externally-dependent test.
## Issue Context
This is a regression test, so failures should ideally reflect generator correctness rather than external package-manager availability.
## Fix Focus Areas
- crates/specforge-cli/tests/regression.rs[497-525]
### Possible approaches (pick one)
- Add `--prefer-offline`/`--no-progress` and configure an explicit npm cache directory to reduce network reliance.
- Make the TS gate opt-in via env var (e.g. `SPECFORGE_ENABLE_TSC_GATE=1`) so default CI remains hermetic.
- Vendor a minimal toolchain approach (e.g., run `tsc` from a repo-managed dependency/tooling) so the test doesn’t need to install from the network each time.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Err(format!( | ||
| "tsc --noEmit failed (exit {:?}):\n{}", | ||
| tsc.status.code(), | ||
| String::from_utf8_lossy(&tsc.stdout), | ||
| )) |
There was a problem hiding this comment.
3. Tsc stderr not reported 🐞 Bug ◔ Observability
On TypeScript typecheck failure, assert_tsc() only returns tsc.stdout and drops tsc.stderr, which can hide important diagnostics and makes failures harder to debug.
Agent Prompt
## Issue description
When `npm run typecheck` fails, the error string only includes stdout. If stderr contains useful output (npm wrapper errors, node runtime errors, or diagnostics), it will be lost.
## Issue Context
Other compile gates in this file include stderr in failure messages (e.g., cargo check); TS should match for debuggability.
## Fix Focus Areas
- crates/specforge-cli/tests/regression.rs[527-542]
### Suggested change
Include both stdout and stderr in the returned error message, similar to `assert_cargo_check()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Worked through the 10-item focus list from the prior review. Headline: 3 real TS emitter bugs fixed (found via new big-spec coverage), clippy at 0 warnings, version drift reconciled.
TS emitter correctness (#1–3)
index.tsre-exported 12 of 18 runtime modules, omittingstreaming,cache,concurrency,dedup,idempotency,middleware.import { streamSse } from "./index"failed. All 18 now re-exported.ValidationError-style guard to TS. A spec model named like a built-in (ApiClient,Logger, etc.) now gets aModelsuffix. Verified: GitHub TS (which has aValidationErrormodel) now generatesValidationErrorModel.tsand typechecks.github_generates_and_typechecks_ts+stripe_generates_and_typechecks_ts. This immediately caught a real bug: Stripe'sDeletedExternalAccountandDeletedPaymentSourceunions both containDeletedBankAccount/DeletedCardarms, so both emitted top-levelisDeletedBankAccountguards that collided underexport *(TS2308). Fixed by making TS guards union-scoped (is{Union}{Arm}); Rust unaffected (its guards are enum methods). Updated 3 unit tests + docs.Repo hygiene (#4–8)
Cargo.tomlwas0.8.0while docs/tags were1.6.x; this also corrupted every generatedspecforge-version.jsonstamp. Bumped to1.6.1everywhere.generate-examples.sh— always rebuilds the CLI now (was silently regenerating with a stale binary).website/index.html— a 220-insertion landing-page revamp was sitting uncommitted; preserved onfeat/landing-page-revamp-uncommitted(needs review before merge).#[allow]on theModelenum (boxing would touch 70+ sites for no win), replaced a needless clone withslice::from_ref.CHANGELOG.md.Go emitter (#9)
// Deprecated: Use {alt}and// Returns {desc}lines via a newgo_inline()helper (property/param/body/returns docs were already escaped).#10 — Deno
tsc --noEmitgate from docs: Deep README accuracy review #3 is the right coverage.Verification (all run, exit 0)
cargo build --workspace✓cargo clippy --workspace --all-targets→ 0 warningscargo check+ gogo buildclean