Give every generated Swift model a public init, and build a consumer that would have caught it - #749
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes #735 by making every generated Swift struct externally constructible and adding public-API regression coverage.
Changes:
- Always emits public entity-model initializers.
- Regenerates 35 all-optional models.
- Adds a plain-import consumer target and generator coverage tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
swift/Package.swift |
Registers the consumer target. |
swift/Sources/BasecampGenerator/ModelEmitter.swift |
Emits public initializers unconditionally. |
swift/Sources/BasecampPublicAPIConsumer/PublicAPIConsumer.swift |
Exercises the external public API. |
swift/Tests/BasecampTests/PublicInitCoverageTests.swift |
Adds emitter and packaging guards. |
swift/Sources/Basecamp/Generated/Models/AccountLimits.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/AccountLogo.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/AccountSettings.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/AccountSubscription.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/CampfireLineAttachment.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/ClientApprovalResponse.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/ClientSide.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/CreateAttachmentResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/DoorService.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/EventDetails.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/EverythingFile.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/GaugeNeedleUpdatePayload.swift |
Makes the request payload constructible. |
swift/Sources/Basecamp/Generated/Models/GetAssignedTodosResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/GetMyAssignmentsResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/GetOverdueTodosResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/GetPersonProgressResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/OutOfOffice.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/PauseQuestionResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/Preferences.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/PreferencesPayload.swift |
Makes the request payload constructible. |
swift/Sources/Basecamp/Generated/Models/PreviewableAttachment.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/ProjectAccessResult.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/QuestionReminder.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/QuestionSchedule.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/ResumeQuestionResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/ScheduleAttributes.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/TimelineAttachment.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/TimelineEvent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/UpdateQuestionNotificationSettingsResponseContent.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookCopy.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookCopyBucket.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookDelivery.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookDeliveryRequest.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookDeliveryResponse.swift |
Adds a public initializer. |
swift/Sources/Basecamp/Generated/Models/WebhookEvent.swift |
Adds a public initializer. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
swift/Sources/BasecampGenerator/ModelEmitter.swift:220
- This still makes the same inaccurate claim removed from the new test and consumer headers: some tests import only
BasecampGenerator; the relevant invariant is that every test importingBasecampuses@testable. Please state that narrower invariant here as well.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (2)
swift/Tests/BasecampTests/PublicInitCoverageTests.swift:1
- The plain-import detection is overly strict:
trimmed == "import Basecamp"will fail if someone adds a trailing comment (e.g.,import Basecamp // ...) or extra spaces, even though it’s still a plain import. Consider matching with a prefix check (while still excluding@testable) and/or stripping inline comments before comparing so the guard doesn’t create false negatives.
swift/Tests/BasecampTests/PublicInitCoverageTests.swift:1 - The hardcoded lower bounds (
files.count > 200,structs > 180) can turn legitimate spec/model churn into unrelated test failures. If the goal is to ensure the scan is pointing at the right directory and not silently scanning nothing, a more stable check is to assert the directory exists andfilesis non-empty (or validate a small set of known sentinel filenames), while keeping the core assertion focused onmissingbeing empty.
|
Picked up from the suppressed comment on the second Copilot review ( The emitter comment still said "every test uses
31 vs 0 in the current tree. Comment-only, and it lives in the generator rather than in anything the generator emits, so @codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
5eee995 to
6754314
Compare
Answering three suppressed Copilot findingsThese were filed inside Branch is rebased onto 1.
|
| mutation | result |
|---|---|
| revert to exact equality | exit 1 — XCTAssertTrue failed - a trailing comment does not stop it being a plain import |
| the suggested prefix check | exit 1 — XCTAssertFalse failed - a prefix check would wrongly accept this |
The second row is the reason for the tokenizer rather than the one-line fix.
3. Hardcoded bounds — partly taken. Fixed in 6754314c6.
You are right that files.count > 200 / structs > 180 sat close enough to the real numbers (220) that ordinary spec churn would have failed this test for an unrelated reason. I did not take "assert non-empty", because it gives up something real: a scan that finds three files is exactly as vacuous as one that finds none, and looks just as green.
So the bounds stay as an extraction floor set an order of magnitude below the true count (> 20), which only trips when the scan has lost ~90% of the roster — never legitimate churn. Demonstrated rather than asserted: with a deliberately broken file filter, the test is red with the floor (exit 1) and vacuously green without it (exit 0).
The reasoning lives in a comment on the assertion itself, including what the floor is explicitly not for — it is not a tripwire for models being added or removed, since the consumer target's roster already fails to compile on a rename and missing carries the actual contract.
Verification
LC_ALL=C, real exit codes read back from logs, all after the rebase: swift-generate 0 (zero-line diff under Generated/), swift-check 0 (424 tests, 0 failures), swift-check-drift 0, doc-constants-check 0.
@codex review
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
6754314 to
aa1fe5a
Compare
|
Rebased onto Re-ran under
Working tree clean afterwards. @codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 39 changed files in this pull request and generated no new comments.
Suppressed comments (1)
swift/Tests/BasecampTests/PublicInitCoverageTests.swift:166
- This scan also accepts Codable's
public init(from decoder:)as the initializer it is trying to protect. Models with required-nullable fields (for example,SearchType.swift) contain both forms, so removing their memberwise initializer would leave this check green even though consumers could no longer construct them. Exclude the decoding initializer when recognizing a constructible model.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…that would have caught it
Swift's implicit memberwise initializer is `internal`. The Swift model
emitter compensated with an explicit `public init`, but only for structs
with at least one required member (`if !requiredFields.isEmpty`). An
all-optional model got none and was unconstructible outside the module.
Two of the 35 affected models are request payloads, which made their
operations uncallable: `UpdateGaugeNeedleRequest(gaugeNeedle:)` could
only be handed `nil` from outside, and an empty `{}` PUT is what bc3
rejects with a 400. `UpdateMyPreferences` had the same shape via
`PreferencesPayload`. The other two init-less models are an enum and a
typealias, neither of which takes one.
The two emitters already disagreed: `emitRequestModel` emits its init
unconditionally. That disagreement was the bug, so `emitEntityModel` now
matches it. The required-member shape is unchanged — required parameters
still take no default, optional ones still default to nil.
The second half is why this shipped at all. All 30 files in
Tests/BasecampTests use `@testable import Basecamp`, which raises
internal to visible; zero use a plain `import`. GaugesServiceTests
already constructs `GaugeNeedleUpdatePayload()` and passes — against a
surface no consumer has. No target in the package built against the
public API, so no CI job modelled a customer, and fixing only the emitter
would leave that test green for the wrong reason and let the next
all-optional model repeat this silently.
So `Sources/BasecampPublicAPIConsumer` is now a plain-`import` target
that constructs all 35 payloads and calls both affected operations the
way an external app would. It is a non-test target on purpose: `swift
build` compiles it, so `make swift-build`, `make swift-check`, the Swift
CI job, the release workflow and the CodeQL Swift build all cover it —
broader than `swift test` alone. It stays out of `products` so no
dependent package builds it.
PublicInitCoverageTests holds what the consumer cannot: it unit-tests the
emitter directly, scans every generated model for a `public init` (so a
model added after the consumer's roster was written is still covered),
and asserts the consumer never gains `@testable` and stays a non-test
target — the two edits that would silently retire it.
Verified by reverting the emitter and regenerating: the consumer target
fails to compile with 38 errors, all of the form "missing argument for
parameter 'from'" — the only public initializer left being Codable's.
GaugesServiceTests still passes 24/24 against that same reverted tree.
Closes #735
Three review points from the bot, all correct.
The drive-by that made the assignment loop skip a property whose schema
is not a dictionary had no test. It is unreachable through today's
`openapi.json` — every property there is a dictionary — but a branch with
no test is how the two loops drifted apart in the first place, so assert
it directly: a mixed schema emits the valid property's declaration,
parameter and assignment, and nothing at all for the non-schema one.
Removing the guard turns that test red.
The header comments stated file counts ("all 30 files in
Tests/BasecampTests"), which were already off and would drift with every
test added. The claim that matters is an invariant, not a census: every
test source that imports the SDK imports it as `@testable import
Basecamp`, and none plain-imports it. Both comments now say that.
Also names the one way this target is not a customer: it lives in the
same SwiftPM package, so `package`-level declarations are visible to it
and not to an external consumer. Nothing here touches one. Closing that
would take a separate nested package, which would drop out of `swift
build` and need its own CI step, and the failure class this target exists
for is internal-vs-public, which it does observe.
The blanket "the identifier appears nowhere in the output" assertion already forbade a declaration, a parameter and an assignment — absence is strictly stronger than three presence checks — but it could not say which loop leaked. Split it: one negative per emission shape, plus the blanket check behind them, so a regression names the loop instead of reporting "it leaked somewhere". Positive controls for the valid property, one per loop, sit alongside. Worth recording why only one of the three loops can regress this way: the declaration and parameter loops bind `propSchema` in their own `guard let`, so deleting that guard does not compile (`cannot find 'propSchema' in scope`, verified by deleting each). The assignment loop took no value from the property, which is exactly why its guard could go missing unnoticed — and it is the one the mutation check exercises, now failing on the loop-specific assertion.
The same inaccurate claim the other two headers dropped survived here: "every test uses `@testable import`" is false for any test that does not import the SDK at all — this repo has several, including the new coverage test, which imports only BasecampGenerator. Say the invariant that actually carries the argument instead: every test source that imports the SDK imports it as `@testable import Basecamp`, and none plain-imports it (31 vs 0 in the current tree). Comment-only, inside the generator rather than in anything it emits: regenerating produces a zero-line diff under Generated/.
Two suppressed review findings, both correct. They never became threads, so they were only visible inside the review body. The plain-import check was exact string equality, so `import Basecamp // note` or a doubled space read as "no plain import at all". That is a false negative in the guard whose entire job is to notice when the consumer target stops importing the SDK the way a customer does — an unrecognized import and a missing one look identical to it. Same brittleness class as the `@testable` blind spot, one level up. The obvious repair is a prefix check, and it is wrong the other way: `import BasecampGenerator` starts with `import Basecamp`. So tokenize — drop a `//` comment, ignore surrounding whitespace and a trailing semicolon, require exactly the two tokens. `isPlainBasecampImport` is now a named function with a table test over the spellings that must and must not count, because the disk scan only ever sees the one line the consumer happens to contain today. Both mutations are red: exact equality fails the trailing-comment case, a prefix check fails `import BasecampGenerator`. The roster scan's bounds were `files.count > 200` / `structs > 180`, close enough to the real numbers that ordinary spec churn would have failed this test for an unrelated reason. But dropping them for "non-empty" gives up something real: a scan that finds three files is as vacuous as one that finds none, and looks just as green. Kept as an extraction floor an order of magnitude below the true count (220), which only trips when the scan has lost ~90% of the roster. Demonstrated rather than asserted: with a deliberately broken file filter the test is red with the floor and vacuously green without it. The reasoning is in the test, not just here.
The scan recognized a constructible model by any line starting ` public init(`, which Codable's `init(from decoder:) throws` also matches. Nine generated models carry both that and the memberwise initializer, so deleting the memberwise one from any of them left the scan green while consumers lost the ability to build the model — the test-passes-for-the-wrong-reason failure this file exists to catch. Recognize the memberwise form by what it is rather than blocklisting the one spelling of what it is not: an initializer counts when every parameter it takes names one of the struct's own properties and it takes every property the struct requires at construction. `from` names no property, so the decoding initializer no longer qualifies, and any future initializer is judged by the same rule instead of needing its own exception. A struct that has properties must also be built by an initializer that takes at least one, so a broken label parser cannot make the subset clause hold vacuously. Red-proved on `Draft.swift` — memberwise initializer deleted, decoding one left in place. The old predicate passes that tree, the new one fails naming `Draft.swift`, and `swift build` succeeds throughout: nothing else in the repo observes the loss. Eight of the nine dual-initializer models are constructed nowhere in the suite, so the compiler is not a backstop here.
aa1fe5a to
753b598
Compare
|
Answering the suppressed findings, which carry no thread of their own. Latest review — Both halves check out. The fix recognizes the memberwise form by what it is rather than blocklisting Red proof, on
Nothing else in the repo observes the loss — the compiler is not a backstop here — which is what makes the vacuity worth closing. Restored by Table-driven unit tests cover the matcher directly, since the disk scan can only ever see today's models and today none of them is missing an initializer: both forms present, decoding-only, the wrapped over-three-parameter form, a property-less struct, an initializer omitting a required member, and the The two older suppressed findings stay answered. The strict plain-import comparison became |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #735.
Two halves, and the second is the one that closes the class
The defect. Swift's implicit memberwise initializer is
internal.ModelEmitter.swiftcompensated with an explicitpublic init— but onlyif !requiredFields.isEmpty. An all-optional model got none, so nothing outside the module could construct it. The two emitters already disagreed about this:emitRequestModelemits its init unconditionally. That disagreement was the bug, soemitEntityModelnow matches it.Two of the affected models are request payloads, which made their operations uncallable:
UpdateGaugeNeedleRequest(gaugeNeedle:)could only ever be handednilfrom outside — an empty{}PUT, which bc3 answers with a 400.UpdateMyPreferenceshad the same shape viaPreferencesPayload.The blind spot — this is what the PR is really closing. Every test source in
Tests/BasecampTeststhat imports the SDK imports it as@testable import Basecamp, which raisesinternalto visible; none plain-imports it.GaugesServiceTests.swift:537already constructsGaugeNeedleUpdatePayload()and passes today, against a surface no consumer has. No target inswift/Package.swiftbuilt against the public API, so no CI job modelled a customer — which is the mechanism, not an adversary, that let this ship. Fixing only the emitter would leave that test green for a different reason and let the next all-optional model repeat the bug silently.So this PR adds
Sources/BasecampPublicAPIConsumer: a plain-import Basecamptarget that constructs all 35 payloads and calls both affected operations the way an external app would.Counts
public initThe two remaining are
FirstWeekDay(apublic enum— its cases are already public constructors) andWebhookHeadersMap(apublic typealiasto[String: String]— no initializer of its own). 35 models gained an init.Why a non-test target
swift buildcompiles it, somake swift-build,make swift-check, thetest-swiftCI job,release-swift.yml, and the CodeQL Swift build (working-directory: swift,run: swift build) all cover it — strictly broader thanswift test. (swift testbuilds it too, verified.) It stays out ofproducts, so no package that depends on this one ever builds it.swift buildcompiling a target with no product membership was verified empirically, not assumed.One way it is not a customer, stated in its header rather than left for a reader to discover: it lives in the same SwiftPM package, so
package-level declarations (e.g.BasecampClient.httpClient) are visible to it and not to an external consumer. Nothing in it touches one. Closing that last gap means a separate nested package, which would drop out ofswift buildand need its own CI step — not worth it for a failure class that is internal-vs-public, which this target does observe.Guards, and what each one can actually see
PublicInitCoverageTestsholds what the consumer target cannot:public structmust declare a memberwisepublic init, so a model added after the consumer's hand-written roster still gets covered. "Memberwise" is load-bearing: Codable'spublic init(from decoder:) throwsalso beginspublic init(, and nine models carry both, so the first cut of this scan would have stayed green after the memberwise one was deleted from any of them. It now recognizes an initializer by whether its parameters name the struct's own properties and cover every member it requires at construction. Red-proved onDraft.swift, whereswift buildstill succeeds — nothing else in the repo observes the loss;@testableguard — the consumer only proves anything while it imports the way a customer does, and adding@testableis the obvious way to make a compile error there go away. Verified: with@testable, the consumer still compiles, so nothing else would have caught it;.testTargetwould narrow its CI coverage with no visible signal.The consumer target itself lists all 35 models rather than a sample, so reverting the fix produces 35 independent errors rather than one arguable case.
Red proof
Emitter reverted by
cp(nevergit checkout --), regenerated, mutation asserted to have reached the output (GaugeNeedleUpdatePayload.swifthas nopublic init):swift build→ exit 1, 38 errors inPublicAPIConsumer.swift, nearly allmissing argument for parameter 'from' in call— the only public initializer left being Codable'sinit(from:). That is the customer's experience, verbatim.PublicInitCoverageTests→ the emitter tests and the roster scan fail.GaugesServiceTests→ 24/24 still pass on that same reverted tree. The blind spot, demonstrated rather than asserted.Per-case mutation matrix for the cases the emitter revert does not exercise:
testConsumerTargetImportsBasecampWithoutTestable@testabletestConsumerIsDeclaredAsANonTestTarget.target(→.testTarget(testRequiredMemberModelKeepsItsInitShape= niltestNonDictionaryPropertyIsSkippedByAllThreeLoopsAll mutations restored by
cp+diff -q, verified on disk, and the generated tree re-diffed byte-for-byte against the pre-mutation snapshot.Drive-by, in the block being rewritten
The property-declaration and init-parameter loops both skip a property whose schema is not a dictionary; the assignment loop did not, so it would emit
self.x = xfor a parameter that was never declared. Latent (every property inopenapi.jsonis a dictionary) and pre-existing, but the three loops have to agree, and making the init unconditional widens where it could bite. Regenerated output is byte-identical with and without it, and it now has its own test (last row above).Verification
LC_ALL=Cthroughout, real exit codes read back from log files:make swift-generate→ 0make swift-check-drift→ 0make swift-check→ 0 (423 tests, 0 failures)make doc-constants-check→ 0swift/Sources—check-deprecation-parity,check-idempotency-parity,check-retry-metadata-parity,check-readme-env-vars,check-write-semantics-parity→ 0Rebase check immediately before each push:
origin/mainhad not moved off49f18e463, so the branch is already on it.Untouched on purpose:
swift/Sources/Basecamp/Services/BaseService.swift, which a concurrent workstream owns.