feat(lint): warn on 204-dropped outputs, with generated-lint and go-oracle exactness riders#525
Merged
Conversation
The generated go test client decoded JSON through float64, so integer comparisons lost exactness above 2 to the 53rd and adjacent ids could conflate. The client now decodes with UseNumber, equality and ordering take an exact int64 path when both sides are integral, and map-key lookups and set hashing canonicalize integral values through one string form so json.Number, int64, and integral float64 agree. Fractional values keep the float64 fallback, and arithmetic helpers stay float64 by design.
The admin reset endpoint's counter update and the enum Literal alias grew past 100 characters on ecommerce (three counters, seven variants), the only two ruff findings in any generated app. Both sites now wrap when the single-line form exceeds the limit and stay single-line otherwise, so the other specs' output is byte-identical.
An operation whose outputs go beyond a single Bool flag while its success status resolves to 204 No Content loses its payload: FastAPI refuses to register the route and the go and ts targets silently drop the body, which is exactly how ecommerce RemoveLineItem failed before its override. L07 resolves the effective status through the same convention functions the endpoint derivation uses and points at the http_status_success fix. A lone Bool output stays exempt as the designed 204-versus-404 selector, and the lint module now depends on convention for the classification.
📝 WalkthroughWalkthroughAdds a new L07 lint for outputs dropped by 204 responses, wires it into lint execution and docs, adds line-length-aware wrapping for generated Python output, and updates Go test-runtime numeric handling to preserve integer precision and canonical comparisons. ChangesDroppedOutputs (L07) Lint
Python codegen line-length wrapping
Go test runtime numeric handling
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/codegen/src/main/scala/specrest/codegen/python/EmitPython.scala (1)
476-483: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWrapped
Literal[...]alias can still exceed the 100-char limit for larger enums.Unlike
AdminRouter's scalar-reset wrapping (which places each seed on its own line), this only does a single-level wrap: allvaluesland on one indented line (" $values"). For an enum with many members or long member names, that line itself can exceed 100 chars, silently defeating the stated Ruff-compliance goal for enums not covered by current fixtures.♻️ Suggested per-value wrapping fallback
private def enumAliasDef(ir: ServiceIRFull, typeName: String): Option[String] = svcEnums(ir) .find(e => enumNameFull(e) == typeName) .map: e => - val values = enumValuesFull(e).map(v => s"\"$v\"").mkString(", ") - val single = s"${enumAliasName(typeName)} = Literal[$values]" - if single.length <= 100 then single - else s"${enumAliasName(typeName)} = Literal[\n $values\n]" + val quoted = enumValuesFull(e).map(v => s"\"$v\"") + val values = quoted.mkString(", ") + val single = s"${enumAliasName(typeName)} = Literal[$values]" + val oneWrap = s"${enumAliasName(typeName)} = Literal[\n $values\n]" + if single.length <= 100 then single + else if oneWrap.linesIterator.forall(_.length <= 100) then oneWrap + else + (s"${enumAliasName(typeName)} = Literal[" :: + quoted.map(v => s" $v,") ::: + List("]")).mkString("\n")🤖 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 `@modules/codegen/src/main/scala/specrest/codegen/python/EmitPython.scala` around lines 476 - 483, The wrapped enum alias in enumAliasDef can still violate the 100-character limit because all enum values are emitted on one indented line. Update enumAliasDef in EmitPython so that when the single-line Literal assignment is too long, it falls back to per-value wrapping across multiple lines rather than a single joined values line. Use the existing enumAliasName/typeName logic and keep the Literal[...] output Ruff-compliant for larger enums.modules/lint/src/main/scala/specrest/lint/DroppedOutputs.scala (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing
Path's resolution logic instead of duplicating it.Lines 20-28 inline the same
getConvention→resolveMethod/resolveStatussequence thatPathalready encapsulates in its privateresolveMethodandresolveStatusmethods. If the resolution logic inPathchanges, this lint won't automatically follow, creating a risk of divergence between the lint and actual endpoint derivation.Exposing a single public method on
Path(e.g.,successStatus) would eliminate the duplication and keep the comparison type-consistent (IntvsString).♻️ Proposed refactor
In
Path.scala(add a public wrapper around the existing private methods):def successStatus(c: operation_classification, conv: Option[conventions_decl]): Int = val method = resolveMethod(c, conv) resolveStatus(c, conv, method)Then in
DroppedOutputs.scala:val opName = classificationOperationName(classification) - val method = resolveMethod( - Path.getConvention(conv, opName, "http_method").flatMap(parseHttpMethod), - classificationMethod(classification) - ) - val status = resolveStatus( - Path.getConvention(conv, opName, "http_status_success"), - method, - classificationKind(classification) - ) - if status == "204" then + val status = Path.successStatus(classification, conv) + if status == 204 then🤖 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 `@modules/lint/src/main/scala/specrest/lint/DroppedOutputs.scala` around lines 20 - 28, Refactor DroppedOutputs to stop duplicating Path’s endpoint resolution logic by calling a single public helper on Path instead of inlining getConvention, parseHttpMethod, resolveMethod, and resolveStatus. Add a public wrapper on Path (for example, successStatus) that delegates to the existing private resolveMethod and resolveStatus methods, then update DroppedOutputs to use that helper so the lint stays aligned with Path’s behavior and keeps the comparison type-consistent.
🤖 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 `@docs/content/docs/research/spec_language_design/developer-experience.md`:
- Line 22: Clarify the L07 lint rule wording in the spec table to match the
simpler subject-predicate style used by the other rows. Update the description
in the developer experience docs so the entry for the rule currently described
as “an operation whose outputs a resolved `204 No Content` success status would
drop” reads more directly and scannably, while preserving the same meaning and
keeping the L07 row aligned with the surrounding lint descriptions.
---
Nitpick comments:
In `@modules/codegen/src/main/scala/specrest/codegen/python/EmitPython.scala`:
- Around line 476-483: The wrapped enum alias in enumAliasDef can still violate
the 100-character limit because all enum values are emitted on one indented
line. Update enumAliasDef in EmitPython so that when the single-line Literal
assignment is too long, it falls back to per-value wrapping across multiple
lines rather than a single joined values line. Use the existing
enumAliasName/typeName logic and keep the Literal[...] output Ruff-compliant for
larger enums.
In `@modules/lint/src/main/scala/specrest/lint/DroppedOutputs.scala`:
- Around line 20-28: Refactor DroppedOutputs to stop duplicating Path’s endpoint
resolution logic by calling a single public helper on Path instead of inlining
getConvention, parseHttpMethod, resolveMethod, and resolveStatus. Add a public
wrapper on Path (for example, successStatus) that delegates to the existing
private resolveMethod and resolveStatus methods, then update DroppedOutputs to
use that helper so the lint stays aligned with Path’s behavior and keeps the
comparison type-consistent.
🪄 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
Run ID: e7960822-7c9d-499a-bd48-311c7f78133d
📒 Files selected for processing (15)
build.sbtdocs/content/docs/design/architecture.mdxdocs/content/docs/research/spec_language_design/developer-experience.mddocs/content/docs/roadmap.mdxdocs/content/docs/spec-language.mdxmodules/arch/src/test/scala/specrest/arch/ArchitectureTest.scalamodules/codegen/src/main/scala/specrest/codegen/python/AdminRouter.scalamodules/codegen/src/main/scala/specrest/codegen/python/EmitPython.scalamodules/codegen/src/test/scala/specrest/codegen/EmitTest.scalamodules/lint/src/main/scala/specrest/lint/DroppedOutputs.scalamodules/lint/src/main/scala/specrest/lint/Lint.scalamodules/lint/src/test/scala/specrest/lint/DroppedOutputsTest.scalamodules/testgen/src/main/resources/testgen-templates/go/chi/tests/conf_client.gomodules/testgen/src/main/resources/testgen-templates/go/chi/tests/conf_runtime.gomodules/testgen/src/test/scala/specrest/testgen/AdminRouterTest.scala
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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.
Three hardening items left over from the last arcs, none changing behavior for the current fixture suite.
L07: declared outputs dropped by a 204
The RemoveLineItem seam in #524 showed the failure class: an operation declaring outputs while its success status resolves to 204 No Content loses the payload, fatally on python (FastAPI refuses to register a 204 route with a response body) and silently on go and ts. The new lint pass resolves the effective status through the same convention functions the endpoint derivation uses (classification, method override,
resolveStatus), warns naming the dropped outputs, and points at thehttp_status_successoverride. A lone Bool output stays exempt, since a success flag feeding 204-versus-404 semantics is the designed pattern. The lint module gains a dependency on convention for the classification, asserted in ArchitectureTest; all four fixture specs stay silent under the rule (RemoveLineItem now overrides to 200), and the docs' lint tables grow the L07 row.Generated python stays ruff-clean
Ecommerce's scale pushed two generated lines past the 100-character limit: the admin reset endpoint's three-counter
values(...)call and the seven-variant enumLiteralalias, the only ruff findings in any generated app. Both emitters now wrap when the single-line form exceeds the limit and stay single-line otherwise, so the other specs' generated output is byte-identical. Verified by before/after generation diffs across all four specs, ruff and mypy strict on the results, and the ecommerce smoke suite (124 passing) against the wrapped app.Go conformance oracle compares integers exactly
The deferred follow-up from #518's review: the generated go test client decoded JSON through float64, so integer comparisons lost exactness above 2^53 and adjacent ids could conflate. The client now decodes with
UseNumber, equality and ordering take an exact int64 path when both sides are integral, and map-key lookups and set hashing canonicalize integral values through one string form sojson.Number, int64, and integral float64 agree; fractional values keep the float64 fallback and the arithmetic helpers stay float64 by design. A standalone probe shows the old path conflating 9007199254740993 with its neighbor and the new helpers distinguishing them in equality, ordering, string form, and hashes. All four go conformance suites pass unchanged.Validation
Each slice was validated in isolation (lint 40/40 with the end-to-end CLI warning surfaced on a trap spec, python before/after diffs plus conformance, go suites on all four specs plus the exactness probe), and the assembled branch passes the full sbt suite (12 modules, zero failures).
Summary by cubic
Adds L07 lint to catch operations that resolve to 204 No Content and would drop outputs. Also keeps generated Python ruff-clean and makes Go conformance tests compare integers exactly to avoid precision loss.
New Features
Bug Fixes
values(...)and wide enumLiteral[...]aliases when lines exceed 100 chars; other specs remain byte-identical.Written for commit 1216daf. Summary will update on new commits.
Summary by CodeRabbit
204 No Contentwhile declaring outputs that can never be returned.