Repository navigation
Codegen robustness + readability cleanup (4 failing regression tests now pass) - #1
Merged
Merged
Conversation
added 19 commits
July 30, 2026 07:59
Mechanical clippy suggestions (redundant closures, format! simplifications, collapsed if-lets, sort_by_key, char comparisons, matches! macro, etc.). No behavior change; lib tests (242) + TS (17) still pass.
- diff.rs: expand golfed fmt_md_msg into readable prefixed-if chain (fixes 13 clippy else-if warnings) - testgen.rs: Scalar::Binary was shadowed by a Boolean|Base64|Binary arm, so binary fields generated "true"/boolean asserts instead of Uint8Array. Split Binary into its own arm (3 sites). - spec.rs: collapse match to matches! macro - resolve.rs: take &T instead of &Box<T> - rust emitter: remove dead rust_method_doc_comment - apply remaining clippy autofixes across crates
- testgen.rs: remove now-unreachable _ catch-alls (all 9 Scalar variants are covered after the Binary split) and give Binary its own assertion - analyzer.rs, marketplace.rs: descending sorts via sort_by_key(Reverse(..))
- runtime.rs: collect() now builds the file list with vec![] instead of Vec::new() + 18 pushes (clearer intent) - schema31.rs: drop redundant borrow in to_value() in test - apply remaining clippy autofixes (collapsed if-lets)
- resolve.rs, models.rs, rust/lib.rs: collapse 3-deep if-let chains - cli/main.rs: sort_by_key(Reverse) for marketplace entry sort - e2e_smoke, e2e_advanced: ParsedRequest/LangLeg type aliases for readability of complex tuple signatures - marketplace.rs: drop unused mut in test Clippy warnings now: 4 (all deliberate non-fixes).
5 sites across rust/go/ts emitters checked the first char of a generated identifier with .unwrap(), relying on a separate is_empty() guard above. Replace with chars().next().is_some_and(|c| ...) so each check is self-safe and the guard dependency is explicit. No behavior change; all tests + generated-SDK compile test pass. Audit of other unwrap()/expect() in non-test source found them guarded by construction (len()==1, intersection membership, or infallible I/O).
Reformatted 9 crammed functions (diff_operations, diff_operation, diff_schemas, diff_schema, type_to_string, type_changed, format_text, format_colored, format_json, format_markdown, PropertyChange Display) into properly indented multi-line form. No logic change; all 242 core tests + 12 diff tests pass.
- diff.rs: format diff_detailed entry point - security.rs: expand match arms in scheme_info_from_raw - graph.rs: expand render_mermaid/render_dot node/edge loops Readability only; all tests pass.
- changelog.rs: build_operation_entries is only used by tests, so gate it with #[cfg(test)] (removes dead-code warning from the lib build) - plugin-example: make ReadmePlugin pub so the example is a usable, importable demonstration rather than an unused struct
Add # Panics sections to the 4 public functions flagged by missing_panics_doc (marketplace built_in x2, demo spec, deprecation migration guide). All document 'should never happen' panics on compile-time-baked data or resolve()-invariant assumptions.
- diff.rs: format DiffSeverity/SchemaDiffDetail Display impls - security.rs: expand analyze_common_issues/analyze_detailed_issues (multiple statements-per-line -> readable blocks), and use is_none() instead of !is_some() for OAuth2 flow checks
Reformat the text + markdown analysis report blocks in main.rs (each was a single line with a for-loop body inline). No logic change.
- ir.rs: format SchemaRegistry impl + HttpMethod as_str/upper matches - security.rs: expand scheme_info_from_ir, scheme_label, extract_scheme_names, op_key, build_op_security_map, and the OperationSecurity construction Readability only; all tests pass.
Real bug: schema/operation descriptions from real-world specs (GitHub, Stripe) contain embedded newlines, backticks, and '/*' '*/' sequences. The emitters emitted them as single '// ' or '/// ' comments, so: - continuation lines lost their comment prefix -> bare prose in source -> 'non-declaration statement' / parse errors - '*/' closed enclosing block comments -> 'unterminated block comment' - backticks -> 'looks like single quote' token errors Fix: - rust emitter: rust_doc now escapes /* */ and backticks; added rust_doc_indented for property docs (which were inline-format!ed without rust_doc at all) - go emitter: go_doc escapes */ and prefixes every line; property docs now route through go_doc; param/body/returns docs use new go_doc_label helper Result: github + stripe RUST regression tests now pass (were failing); stripe GO now passes. github GO still fails on a separate, deeper ValidationError name-collision bug (not doc-related).
Real bug: the GitHub spec defines its own 'ValidationError' model, which collided with the emitter's hardcoded ValidationError validation type, causing 'redeclared in this block' + missing-field compile errors. Fix: export_ident now suffixes any user model whose name matches a built-in SDK type (ValidationError, ServiceContainer, Client, Auth, Middleware, RateLimiter, etc.) with 'Model'. Result: github_generates_and_compiles_go now passes. Combined with the prior doc-escaping fix, ALL 11 regression tests now pass (4 were failing before this session: github+stripe x go+rust).
Same bug class as the Go ValidationError collision: a spec model named Error/Client/Semaphore/etc. would redefine an SDK built-in and break compilation. Add safe_model_name() that suffixes colliding names with 'Model', applied at both enum and object model emit sites. github/stripe/petstore rust regression tests still pass; this guards the latent case (none of these specs use the colliding names).
Reflects the doc-comment escaping + reserved-name fixes in the checked-in example output. All three compile/typecheck clean (cargo check, go build, tsc --noEmit).
…arget, TS tree Accuracy pass: - version badge 1.6.0 -> 1.6.1 - unit test count 227 -> 259 - CLI: remove phantom 'security'/'graph' subcommands (don't exist), add real 'profile'/'plugin', drop the now-wrong '22 commands' count, point readers to 'specforge help' as source of truth - WASM build target wasm32-wasi -> wasm32-wasip1 (wasi was renamed in recent Rust; the old target no longer installs) - TypeScript layout tree: add the 7 modules the emitter actually generates (cache, ratelimit, telemetry, logging, interceptors, validate, validation-middleware, service_container) - VS Code extension command count 23 -> 24 - compile-gates: note the per-emitter regenerate-and-compile tests + regression.rs covering real GitHub/Stripe specs
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
PR Summary by QodoHarden codegen: safe doc comments, reserved-name avoidance, and compile tests
AI Description
Diagram
High-Level Assessment
Files changed (54)
|
Code Review by Qodo
1. Rust model rename mismatch
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| fn emit_enum(e: &EnumModel) -> String { | ||
| let name = pascal(&e.name); | ||
| let name = safe_model_name(&pascal(&e.name)); |
There was a problem hiding this comment.
1. Rust model rename mismatch 🐞 Bug ≡ Correctness
The Rust emitter now suffixes colliding model/enum *declarations* with Model, but schema references (e.g., render_type and union arms) still use the unsuffixed pascal(name), so colliding schemas will either fail to compile or incorrectly bind to the SDK’s built-in types like Error/Client. This breaks generated SDK correctness for any spec that defines a schema with one of the built-in type names.
Agent Prompt
## Issue description
`safe_model_name()` is applied when emitting enum/object declarations, but type references still render as `pascal(name)` (and union arms use `pascal(arm)`), causing mismatches for colliding schema names (e.g. schema `Error` becomes `ErrorModel` when declared but is still referenced as `Error`).
## Issue Context
The PR’s intended behavior is “suffix colliding model names with `Model`”. That transformation must be applied consistently anywhere a schema name is converted into a Rust type identifier.
## Fix Focus Areas
- crates/specforge-rust/src/lib.rs[406-428]
- crates/specforge-rust/src/lib.rs[2814-2816]
- crates/specforge-rust/src/lib.rs[2910-2926]
## Suggested approach
1. Introduce a single helper for schema type identifiers, e.g. `fn rust_schema_type_ident(name: &str) -> String { safe_model_name(&pascal(name)) }`.
2. Use it in `render_type()` for `Type::Reference`.
3. Use it anywhere union/oneOf arms or other schema-derived type names are produced (e.g. `arm_ty`).
4. Add/extend a regression test where a schema named `Error` (or another built-in) is referenced from another model/operation, and ensure generated code compiles and types line up.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+214
to
+223
| let name = pascal(input); | ||
| // Avoid colliding with built-in SDK types emitted into the same package | ||
| // (e.g. a spec that defines its own `ValidationError` model would otherwise | ||
| // redeclare the emitter's validation type and break compilation). | ||
| if is_sdk_builtin_type(&name) { | ||
| format!("{name}Model") | ||
| } else { | ||
| name | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Export_ident renames methods 🐞 Bug ≡ Correctness
In the Go emitter, export_ident() now appends Model for any identifier matching the built-in SDK type list, but export_ident() is also used to derive operation method names from operation_id. This will rename methods like Client() to ClientModel() when an operationId collides, unexpectedly changing the generated SDK API surface.
Agent Prompt
## Issue description
`export_ident()` now performs built-in type collision avoidance by suffixing `Model`, but it is used for both schema/type identifiers and operation method identifiers. As a result, colliding operationIds (e.g. `Client`, `Auth`, `Logger`) will have their method names changed.
## Issue Context
The PR description states reserved-name collision handling is for *models*. Operation method names generally should not be subject to the same collision list.
## Fix Focus Areas
- crates/specforge-go/src/lib.rs[212-261]
- crates/specforge-go/src/lib.rs[2566-2571]
## Suggested approach
1. Revert `export_ident()` to only do Go export casing (PascalCase + leading-digit guard).
2. Add a separate helper for model/type names only, e.g. `export_model_ident()` (or `safe_model_name()`), that applies the `is_sdk_builtin_type` suffix.
3. Update call sites that emit schema/type declarations and schema/type references to use the model-specific helper, but keep operation method naming using plain `export_ident()`.
4. Add a regression test with an operationId equal to a built-in type name to ensure the method name remains stable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Scalar::Integer | Scalar::Integer64 => "1".to_string(), | ||
| Scalar::Float => "1.0".to_string(), | ||
| Scalar::Boolean | Scalar::Base64 | Scalar::Binary => "true".to_string(), | ||
| Scalar::Boolean | Scalar::Base64 => "true".to_string(), |
There was a problem hiding this comment.
3. Binary examples emit ts 🐞 Bug ≡ Correctness
In testgen.rs, the change that makes Scalar::Binary no longer match the boolean arm activates an existing Scalar::Binary branch that returns new Uint8Array([1,2,3]) for Go and Rust example JSON bodies. This produces invalid JSON in generated mock responses for Go/Rust testgen when Binary fields exist (Binary is expected to be represented as JSON string/object).
Agent Prompt
## Issue description
`generate_go_example()` and `generate_rust_example()` now reach the `Scalar::Binary` arm, which returns a TypeScript expression (`new Uint8Array([1,2,3])`). These functions are used to generate `application/json` response bodies, so Binary scalars must be JSON-compatible.
## Issue Context
The verifier treats `Scalar::Binary` as JSON string/object, and mock generation uses a JSON string like `"binary"`.
## Fix Focus Areas
- crates/specforge-core/src/testgen.rs[345-355]
- crates/specforge-core/src/testgen.rs[506-516]
## Suggested approach
1. Change the Go/Rust testgen Binary example to a JSON string (e.g. `"binary"` or a base64-like string), consistent with `mock.rs` and `verify.rs` expectations.
2. Keep the TS generator’s `Uint8Array` example if it is used in actual TS code contexts.
3. Add a small unit/regression test ensuring generated Go/Rust mock response bodies remain valid JSON when a response type includes `Scalar::Binary`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Jul 31, 2026
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
Code-quality pass across the emitters and core. Headline result: 4 regression tests that were failing now pass (
github/stripe×rust/go), driven by two real codegen bugs fixed along the way.Real bugs fixed
/*/*/sequences that broke generated code (parse errors, unterminated block comments, token errors). Emitters now escape these and prefix every line. → github+stripe × rust+go regression tests pass.Readability + cleanup
Docs
Verification (all run, not asserted)