Decisions
Choices that shaped this SDK, with what was decided and why, in the words of whoever decided it where that was recorded.
The code shows the outcome. It cannot show the alternative that was rejected, or the reason. Anyone asking "why is it like this?" months from now — including the people who built it — has only this file to answer from.
Newest first. A reversal records both positions and what changed between them; those are the entries most worth keeping and the easiest to quietly overwrite.
1. Client-side failover: built, then removed — REVERSAL
Decided: 2026-09-06. Reverses: the 0.4.0 decision to build it, taken 2026-09-01.
The original position
Failover was built at the owner's request after asking whether the SDK could fall
back to a working server if the main one went down. It shipped in 0.4.0 as
UseFailover, rotating to another endpoint on a refused connection,
502/503/504, or an attempt past a per-attempt timeout.
It was greenlit by Thomas at TCGdex, who asked at the time that it be easy to remove once the API handled this server-side. That condition was written into the design and is why the removal was a clean deletion rather than an unpicking.
The owner's reason for expecting part of it to outlive a server-side implementation, recorded then and still correct as far as it goes:
a server-side implementation can only rotate among nodes TCGdex runs, so an unofficial mirror or a self-hosted server is reachable only this way
What changed
TCGdex deployed new infrastructure in early September 2026. Two things followed, both confirmed by Thomas on 2026-09-06:
i belive those routes are now no longer open as the new infra got deployed on na
the main route should now route if any of the nodes are down
Avior described the backend topology that makes the second statement true:
na1 and na2 are now both behind a single load balancer that automatically handle disconnection from the other servers that should be better for you
The owner's reading of that, and the right one — the assistant had first taken
it to mean the na1/na2 names now sat behind a load balancer:
i think he meant it as the servers that ran for NA had a load balancer now and the NA enpoints were not there
So the NA machines were pooled behind one balancer, and the NA endpoints
were retired — not kept and re-pointed. That matches what was measured after the
balancer went in: api.na1 still answered 404, api.na2 still failed TLS,
and api.tcgdex.net served normally. The hardware is healthy and now fails over
by itself; the prefixed names are simply gone, which is the fact the enum
depended on.
The timing was expected rather than a surprise. From the same conversation:
i was ready for this, you told me you guys were going to do the fallback last week, i just didnt expect it to be so fast lol
Ryan's reply — "Needed doing quickly really" — and the owner's note that the maintainers are volunteers he was not going to rush are worth keeping for the next time this SDK covers a gap upstream intends to close: the gap closed a week after it was announced, not a quarter later.
Measured the same day, before that confirmation arrived: api.na1.tcgdex.net
resolved and its certificate covered the name, but the new front end had no route
for it and answered 404 page not found as text/plain.
api.na2.tcgdex.net presented CN=TRAEFIK DEFAULT CERT and failed TLS outright.
api.tcgdex.net served correctly from the same IP — the difference was the
Host header, not the server.
So the condition Thomas set in 0.4.0 had been met, and two of the six mirror enum members could no longer work at all.
The decision
Remove client-side failover entirely, and the mirror enum with it. Keep the ability to point the SDK at a custom server.
All six enum members went, though only two were provably dead. Measured at
the time of removal, api.eu1/eu2/eu3 and api.as1 still served; only the
NA endpoints had been retired, which matches Thomas naming NA specifically and
Ryan's "Needed doing quickly really". The owner's expectation — an expectation
from the shape of the rollout, not something upstream stated — was that the same
treatment reaches the other regions:
probably did that for every area, and theres probably a server on demand thing going if he's really smart about it and can afford it
The three EU names already resolved to a single address, which is consistent
with that region being pooled behind one balancer too. Whether anything scales
on demand is not observable from a client and is not claimed here. Either way the
decision does not rest on it: upstream recommends api.tcgdex.net for everyone,
so removing only the two confirmed-dead members would mean doing this again when
the rest follow.
The owner, on why the dead enum members were not simply left in place or marked obsolete:
i know it technically can be left in but itll never fire, and id rather not tempt people to use something they shouldnt use anymore
And on why no deprecation period was needed:
no one is using the sdk yet im pretty sure
What survived, and why
BaseAddress and GraphQlEndpoint stay. The owner's requirement:
i want to keep the ability to point the sdk to a custom server though
Thomas named the same exception unprompted, which is as close to an upstream endorsement of this split as it gets:
yeah you shouldnt need to manually do it now. unless you wanna define like local apis or somthing
And the shape was stated to the maintainers before it was built, so the SDK's behaviour and what upstream was told match:
im removing the prefix enums completely, and just leaving the api.tcgdex.net endpoint, while letting the host be overridden for custom endpoints
This is the durable half of the original reasoning. A server-side implementation can only route among nodes TCGdex runs, so an unofficial mirror or a self-hosted instance is reachable this way and no other. What changed is that rotation between endpoints stopped being the SDK's job — not that choosing an endpoint did.
What it cost, and what it bought
An intermediate design was built and abandoned the same day: falling back to the
official host automatically when a mirror stopped working, with the official host
held as a reserved final attempt. It was complete and passing when the upstream
confirmation arrived and made it pointless. It is preserved unmerged on
feat/fall-back-to-official-host rather than deleted.
Removing failover also removed the 404-discrimination fix that had been merged
days earlier — a real defect, fixed correctly, in code that no longer exists. The
reasoning is kept in learnings.md because it is about reading
status codes rather than about failover.
One thing the removal exposed and improved: the trailing-slash guard existed only
on failover endpoints. BaseAddress had the identical hazard, had never been
checked, and became the only way to reach a custom server. That validation was
added in the same change.
The general lesson, which is why this entry exists rather than just a changelog line: a feature built to cover a gap in someone else's system has a built-in expiry date, and the time to agree how it comes out is while it is going in. Thomas asking for it to be easy to remove is the only reason this was a deletion instead of an excavation.
2. dotnet test on the multi-target Tests project must pin --framework
Added a CI job (verify-test-coverage) that asserts the union of test names across all lanes
equals a fixed count. It caught a pre-existing flake: build-and-test and macos-test ran
dotnet test TcgDex.CSharpSdk.Tests.csproj without --framework, against a project multi-targeting
net10.0;net8.0;netstandard2.0. Which TFM's build dotnet test picks when none is specified is not
guaranteed stable — two macOS runs on the identical commit produced 518 and 519 distinct test names,
differing by exactly ThePublicSurface_MatchesTheApprovedBaseline, a test gated by TFM.
Both jobs now pin --framework net10.0 explicitly. framework-test already pinned net472 for the
same reason — this closes the gap on the other two.
With both pinned, the two lanes report 519 each and agree run to run. The expected union is 532:
519 unit (net10.0, same names in unit-tests.trx and macos-tests.trx) + 11 offline integration
- 2 that exist only under
net472inframework-tests.trx. Re-derive it that way if a lane changes; do not adjust the constant to whatever CI last printed.
The general lesson: an exact-count coverage assertion is only as trustworthy as the determinism of what it counts. It surfaced this bug rather than causing it — the flake predates the coverage job and had been running unnoticed since nothing compared lane counts before.
3. Unit tests are hermetic — never reach the network
A unit test can be hermetic only while the code is correct. Create_DisposesItsOwnHttpClient
tried to verify disposal by awaiting a real request and expecting ObjectDisposedException. Under
Stryker, every mutant that defeated the disposal dialled the live API. On a night the API was
down, each mutant hit the 30-second timeout: the run ballooned from ~20 minutes to 2h38m and
silently starved a neighbouring job on the shared measurement box.
Assert observable state directly, never behaviour that depends on external infrastructure being
reachable. See learnings.md for the full measurement.
4. Query builder never calls Expression.Compile()
Runtime code generation is not AOT-safe and breaks Unity and Native AOT consumers. Expression trees are walked and translated to query parameters, never compiled to delegates.
5. Collection initializers require coalescing backing fields
The System.Text.Json source generator discards collection initializers. A property declared as
= [] deserializes to null when the field is omitted in the JSON. Use a coalescing backing
field to guarantee non-null.
6. Public API surface is pinned by PublicApi.approved.cs
TcgDex.CSharpSdk.Tests/PublicApi.approved.cs (PublicApiGenerator) pins the public surface.
An intentional change requires regenerating that baseline. Regenerate with LF line endings
only — the CRLF variant fails the comparison on the line endings, not on the surface itself.
7. Strict analyzers are scoped to the library only
AnalysisMode=All + SonarAnalyzer on test code is near-total noise: S2699 on every CsCheck
property, CA2000 on every undisposed test HttpClient. The signal-to-noise ratio is unusable.
Analyzers are enabled only on TcgDex.CSharpSdk, not on test or benchmark projects.
8. Native AOT publish needs VS Installer directory on PATH
The native link step fails with a misleading error if the C++ toolchain is not reachable. VS
Installer puts those tools in a subdirectory that is not on PATH by default. Add it before
running Native AOT publish, or the build will fail with a cryptic message about ml64.exe or
similar.
9. main is protected
Direct pushes to main are disabled. All work goes through a pull request. Auto-merge is off, so
a merge is an explicit step taken after CI is green. (Auto-merge superseded 2026-10-15 by #16:
now on, and still gated on every required check.) Releases are git-tagged and published by
workflow, not by hand.
This arrangement ensures every commit to the main branch has been reviewed and CI-validated.
10. Roslynator.Analyzers and Meziantou.Analyzer added alongside SonarAnalyzer
Requested by the owner directly ("i use it in PBJ, and want it here too"), and traced to
PinKushin/ANALYZER-COVERAGE-LOG.md
(entry 2026-09-08), which surveyed the house's C# repos and identified Meziantou as a genuine
fourth analyzer rather than a duplicate of Sonar/Roslynator — its standout categories are async
correctness (ConfigureAwait, and using on a Task<IDisposable> disposing the Task itself
rather than the awaited result) and culture-sensitive string comparison, which neither existing
tool focuses on as heavily. This SDK is an async HTTP client throughout, exactly the shape those
rules target.
Adding both surfaced 49 unique findings across four rule IDs, resolved as follows rather than suppressed wholesale:
- MA0048 ("file name must match type name"), 33 sites — disabled repo-wide via
.editorconfig. This codebase deliberately groups small, tightly related types in one file (Resources.csholds six resource classes,GraphQlMessages.csaJsonContextplus the message types it serializes) so a reader following one concept finds every type for it in one place. Splitting them would be a purely mechanical, purely cosmetic change against how the codebase is organized on purpose — not a defect the rule caught. - MA0015 ("is not a valid parameter name"), 6 sites, all in
TcgDexOptions.Validate()— the identical pattern Sonar'sS3928was already suppressed for at the same call sites:Validate()is parameterless by design and reports the offending property name vianameoffor an actionableArgumentException.ParamName, which neither analyzer's parameter- matching heuristic recognises as valid. Folded into the existing suppressed span rather than a second pragma repeating the same reasoning. - MA0051 ("method is too long"), 7 sites, longest 107 lines — threshold raised from 60 to
130 via
.editorconfig, not disabled. This codebase documents why alongside what, so a method that exceeds 60 lines usually does so because of the comments explaining a non-obvious decision, not because the control flow does too much. The 107-line outlier isBoundedContent.ReadAsBytesAsync— the decompression-bomb guard — where splitting it for a line count would fragment security-relevant logic across methods with no independent correctness meaning, in a codebase where the "verify by manipulation" testing discipline demands re-verifying anything touched. 130 leaves headroom without disabling the rule outright. - RCS1139 ("add summary element"), 3 sites — fixed for real rather than suppressed: three
methods had a
<remarks>block with no<summary>, which is a genuine documentation gap (<summary>is what an IDE tooltip shows;<remarks>is secondary) rather than a false positive.
Full local gate green after: build (0 warnings across all three TFMs), unit tests (519/518/514, unchanged — no test source touched), coverage (99.82% line / 96.29% branch, both above gate), docs (0 warnings).
11. thirdParty modelled against the real live shape, not the pre-deployment guess
Waited deliberately, per thirdparty-pending-upstream in memory: TCGdex merged
cards-database#2184 on 2026-08-27,
removing the deepOmit that stripped thirdParty from card responses, but it had not
redeployed to api.tcgdex.net yet. Modelling against a merged-but-undeployed PR is exactly the
mistake the SDK rewrite existed to undo — the discarded SDK shipped ~10 fields the API never
actually served. The daily live-api.yml fixture-drift check was left as the trigger rather than
guessing a redeploy date, and it went red on 2026-09-07.
The real shape differed from what the memory anticipated in two ways, both found by fetching live cards rather than assumed from the PR description:
- A third marketplace. The memory named
tcgplayer/cardmarket; the live payload also carriescardtrader, observed only intermittently — present on one fetch of a card and gone on the next fetch of the same card. Modelled asint?alongside the other two rather than assumed absent because a given fixture snapshot happened not to catch it. - Two independent placements, not one.
thirdPartysits at the card root or inside eachvariants_detailed[]entry depending on the card —swsh3-136(two distinctly-priced printings) carries it per-variant with no root-level field;swsh1-1(one undifferentiated"generated"printing) carries it at the root instead, with its single variant carrying none. This exactly mirrors howPricingwas already placed at both levels, which is what made the right shape recognisable rather than another guess:ThirdPartyfollows the same pattern as the type it sits beside, not a new one invented for this field.
Landed alongside an unrelated drift in the same fixture-refresh: Serie.LastSet (typed
SetBrief, which already had Logo/Symbol) had a recorded fixture where the referenced set
predated those fields being populated for it — a data-completeness gap in the recording, not a
schema gap, so no model change, only scripts/Update-Fixtures.ps1.
New tests written first (red on a compile pass against fixtures with no thirdParty yet, since
the model didn't exist), confirmed red for the right reason, then the model added and fixtures
refreshed per Update-Fixtures.ps1's own instruction to update the SDK before refreshing — a
refresh first would have made the drift check pass while hiding the change it was reporting.
Addendum, 2026-09-11 afternoon — the deploy itself may not have been intentional. Asked in the TCGdex Discord whether the 2026-09-07 change was announced anywhere. Ryan (FalconChipp) and Thomas, the maintainers, had no record of it — Thomas: "that would be impressive if it was, i dont even know that". After the owner described the shape, Thomas: "well this might have been an unintentional release when the infra switched over."
Doesn't change the model or the decision to wait for the live shape — that reasoning holds regardless of whether the deploy was deliberate.
"Unintentional" and "unstable" are separate claims, though, and only the first is established here. Owner's read, and the more relevant one going forward: "i dont think it will be reverted, the fact it rolled out and worked for 5 days without complaints, or even some people noticing, means it should be ok to run publicly." Five days of live operation with no incident, no rollback and no visible complaint is real evidence toward the field staying, not just an absence of evidence against it. The practical risk is low; "unplanned at deploy time" describes how it arrived, not how long it is expected to last.
Sharper still, from the owner: "this was a planned update, and pretty much done that im aware, so it just needed rolled out, it doing it accidently during the server migration is just a happy accident i guess." "Unintentional" describes the TRIGGER, not the FEATURE — the field was already finished, planned work on TCGdex's side; what was accidental is only that the infra migration flipped it on as a side effect instead of a formal announcement doing it. That is a stronger stability signal than an experimental field slipping out early: this is the real, intended release, just reached production by the side door rather than the front one.
No SDK change is warranted either way — every ThirdParty property is nullable by design, so if
it is ever pulled back the field just deserializes to null, a state already handled. Recorded so
a future "why did thirdParty disappear" investigation starts from this conversation rather than
re-diagnosing it as a bug, whichever way it turns out.
12. C3's per-lane floors get raised whenever tests are added — REVERSAL
Reverses the "no need to bump a floor when tests are added" framing decision #10's neighbouring PR (#56, C3's union-to-per-lane-floors change) shipped with.
The original position
scripts/Assert-TestCount.ps1's header and ci.yml's comment both said a floor's advantage over
an exact count is that it "does not need an edit every time a test is added" — motivated by this
repo's old union-total check having been hand-bumped twice in one week (9499f9e, c879724) for
exactly that kind of churn. When three new tests (this PR, thirdParty) pushed the real count from
519/519/514 to 522/522/517, the floors were left at 519/519/514 on the reasoning that 522 ≥ 519
already passes, so nothing needed to change.
The correction
Owner: "no floors get bumped when you add tests too or they are worthlessx."
A floor set once and never raised drifts further from the true count with every addition, and the gap it leaves IS the blind spot the whole check exists to close. If the true count grows to 600 while the floor stays at 519, up to 81 tests could silently stop running and the check would still pass — silently, which is exactly the failure mode C3 exists to make loud. "Does not need an edit" was true in the narrow sense that CI would not turn red, and wrong in the sense that mattered: the check's actual sensitivity had quietly degraded.
What actually follows
The floor's real exemption from exact-count maintenance is narrower than the original framing
claimed: it need not be bumped for a transient dip that resolves on its own (a flaky skip, a
timing-dependent count). Anything that changes the suite on purpose — tests added or removed
— changes the floor in the same commit. Raised here: 519/11/519/514 → 522/11/522/517, in the
same PR as the tests that moved the count. Assert-TestCount.ps1's header and the ci.yml
comment both corrected to say this outright, and PinKushin/C3-COVERAGE-LOG.md carries the same
correction for whichever repo copies this pattern next.
13. A User-Agent header, requested by TCGdex directly
Before this, the SDK sent none at all — HttpClient adds no default the way a browser or curl
does, so every request from every consumer of this library was, on the wire, indistinguishable
from any other .NET client hitting the API.
Raised in TCGdex's Discord while debugging an unrelated latency report. Avior: "Might be better to set a standard one with the version of the SDK them[selves]." Asked why it mattered beyond courtesy, since the traffic is already read-only and low-risk: "i store logs for stats and debugging purposes and to respect GDPR i don't store ip addresses, so the only way i can 'identify' [a client] is by using the user agent." With no header at all, this SDK's traffic was unidentifiable by design — not a minor gap, the only signal TCGdex's own privacy stance leaves available.
Format agreed in the same conversation and matched exactly: TcgDex.CSharpSdk/{version}.
Applied per HttpRequestMessage, not via HttpClient.DefaultRequestHeaders. The SDK is used
two ways — owning its own HttpClient (TcgDexClient.Create, AddTcgDex) or given one the
caller built and shares with the rest of their application. Setting a header on the client object
itself would reach into a caller's shared instance, the same reasoning RequestBudget already
applies to the request timeout. Setting it per-request works identically either way and touches
nothing the SDK doesn't own.
Version read from AssemblyName.Version, not AssemblyInformationalVersionAttribute. Both
report "0.6.0" today — this repository sets no build-metadata suffix — but AssemblyName.Version
is assembly identity the runtime always preserves, while a custom attribute is metadata a trimmer
can remove when nothing else references it. Chosen so the header needs no Native AOT verification
of its own beyond what TcgDex.CSharpSdk.AotSmokeTest already provides by publishing and running
a real binary.
Verified live as well as in the test suite: sent one request with User-Agent: pinkushin-debug/1.0
directly against api.tcgdex.net so Avior could confirm the header actually reaches his logs
before relying on it.
14. CI stays on ubuntu-latest through the Ubuntu 26 migration — not pinned
GitHub annotated every Linux job on 2026-09-21: ubuntu-latest moves to Ubuntu 26 from
2026-10-19. Pinning ubuntu-24.04 was offered and declined. The owner, 2026-09-23: "let it
switch, i want my shit to stay updated and keep everything it uses updated, thats what makes a good
program imo."
So the runner image follows GitHub's default, the same way Dependabot keeps packages and actions current. If the switch breaks a job, fix the job for Ubuntu 26. Do not pin back.
15. Mutation testing moved from the Oracle box to GitHub Actions
By 2026-10-15 mutation-box had been unreachable for some time. The owner, 2026-10-15: "the oracle
box is down, been down", "we are using github now", and "using github counts for all the
projects, the oracle box got shutdown, and not because it wasnt being used, but because some stupid
trial ended, and their customer service sucks so they care not getting my CC, trying to get money
back if they fuck up and charge me for soemthing will be impossible."
So the move is permanent and covers every project, not only this one. Oracle ended a trial, and the owner will not hand Oracle a card to keep the box, because a wrong charge would be unrecoverable. Measurement work now runs on GitHub Actions.
.github/workflows/mutation.yml runs Stryker daily and on dispatch. It publishes the HTML report as
an artifact and the score in the job summary. A score below stryker-config.json's break
threshold fails the run. build/run-measurements.sh still holds the box's stryker mode, and it is
dead code for as long as the box is down.
16. Repository auto-merge enabled; main stays protected
Asked whether to turn on the repository's Allow auto-merge setting, which was off. The owner, 2026-10-15: "i mean sure as long as main is still protected."
Enabled. Branch protection is unchanged: 6 required checks, branch must be up to date, no force pushes, no deletions. An auto-merge PR merges only after every required check passes.
Dependabot PRs: reviewed by risk — REVERSAL, same day.
- First position: review every Dependabot PR before merge. The owner: "i want to review dependabot wwith you, before a merge, it can fuck up sometimes i think."
- What changed it: the owner, refining the worry: "if they dont break anything, it should be fine, i just worry if we have a weak test it will bring a bug in." Only packages that ship inside the SDK can bring a bug into it. Test tooling can quietly weaken tests, and the test count plus the daily mutation score catch that. Analyzers and CI actions never ship. The owner: "yea thats a decision, go with that."
The rule now:
| The bump touches | Who merges |
|---|---|
A runtime dependency of the shipped library (System.Text.Json, Microsoft.Extensions.*, Microsoft.Bcl.*, …) |
Reviewed with the owner before merge |
| Test tooling (NUnit, the test adapter, coverlet, CsCheck, Test.Sdk) | Merged without the owner, once CI is green and the test totals per TFM have not dropped |
Analyzers (PrivateAssets=all) and GitHub Actions |
Merged without the owner, once CI is green with no new annotations |
Classify by package, not by the Dependabot group name. The runtime group has carried analyzers.
Auto-merge is never enabled on Dependabot PRs.
17. Dependencies track the latest patch of each target's line — not a minimum-version policy — CORRECTION
Directory.Packages.props and dependabot.yml used to say runtime dependencies were pinned to the
lowest version that satisfies each target. That was never the owner's intent. The owner,
2026-10-15: "I never meant for anything to be minimum version, I wanted true usage and backwards
compatibility for Unity and people who might not be on windows 10 or 11, but i never wanted the
modern .net to get behind in updates, because those updates are security and features and
performance." And: "latest patches, because those are the best performing and secure normally,
although if the lates is slower or less secure then of course we dont use it."
The rule now:
- Each target framework stays on its own release line. net10.0 uses Microsoft.Extensions 10.x. net8.0 and netstandard2.0 use 8.0.x, because 10.x there would drag ASP.NET Core 8, .NET Framework and Unity consumers onto the 10.x graph. Moving a line stays a human decision.
- Within its line, every dependency tracks the newest patch. Not doing so is a finding.
- Exception: a release measured slower, or carrying a known vulnerability or regression, is skipped. Say why next to the pin.
- netstandard2.0 exists for compatibility, not to hold versions back.
The same review found a test gap. The 10.x condition was != net8.0 and != netstandard2.0, which
also matched the net472 test leg. So the netstandard2.0 asset was only ever tested against 10.x
dependencies, never the 8.0.x ones it ships with. The condition is now == net10.0.
DependencyLineTests asserts the resolved major version on every leg, and it was seen failing on
net472 before the fix.
18. TcgDexCachingHandler throws TcgDexApiException, not HttpRequestException
TcgDexCachingHandler is a public DelegatingHandler. When a body is too large, or its connection
fails mid-read, it throws TcgDexApiException from inside the HttpClient pipeline. A review in
0.6.2 asked whether HttpRequestException would be more conventional for a handler.
The owner, 2026-10-15, asked which of the two was more useful, and answered: "yea keep it, thats a decision". The reasoning came from the assistant, and the owner accepted it:
- Retry policies such as
AddStandardResilienceHandlerretryHttpRequestException. A body that is too large will be too large again, so retrying only downloads it again, up to the limit, on every attempt. - Callers going through
TcgDexClientseeTcgDexApiExceptioneither way, because the transport wrapsHttpRequestExceptionitself. Only a caller using the handler directly can tell the difference.
The behaviour is documented in the handler's XML remarks.
19. A soft bar for 1.0
The first cross-model review (Fable over Opus-written code, 2026-10-15) found one real bug and several leaks in the exception contract, in the three files with the lowest mutation scores. On 1.0 those fixes would have pushed toward a major version. On 0.x they are a patch. The owner: "im actually glad this wasnt in a 1.0 now", then "add that 1.0 bar to the decisions doc, at least a soft bar, we do need to raise those scores if they can be."
Before tagging 1.0 (soft: a missed item needs a stated reason, not a silent skip):
- A full cross-model review comes back clean. The whole shipped library must be reviewed by a model other than the one that wrote it. A review of a single diff does not count.
- The weakest files' mutation scores are raised toward the rest of the library, as far as they
can be. At 2026-09-23 those were
Caching/TcgDexCachingHandler.cs67%,Http/BoundedContent.cs65% andTcgDexTransport.cs77%, against 88% overall. "If they can be": a surviving mutant that is equivalent, or untestable for a stated reason, is acceptable once written down. - CI and the local gate measure the same thing. Met on 2026-10-15: CI now gates coverage across all three target frameworks.
20. TcgDexJsonContext becomes internal before 1.0
The Fable full-library review (2026-10-17) flagged that the source-generated TcgDexJsonContext is
public. That makes every generated JsonTypeInfo<T> property part of the pinned public API, so after
1.0 renaming or removing a model would break twice. Offered: make it internal, or keep it public on
purpose. The owner, 2026-10-17: "internal". No further reasoning given.
Done in 0.7.0. PublicApi.approved.cs shrinks accordingly, and tests reach the context through
InternalsVisibleTo if they need it.
21. DeserializePricing = false also skips per-variant pricing — CORRECTION
DeserializePricing was documented as skipping Card.Pricing, and the code did exactly that. But the
pricing inside each variants_detailed[] entry was still parsed, so an application that turned
pricing off to save the work still paid for it on every printing. The second mutation round
(2026-10-17) pinned the old behaviour with a test and asked which was intended. The owner:
"fix that deserialized bug too". So it is a bug: the option means no pricing is parsed anywhere on
a card.
Done in 0.7.0. The old behaviour had not reached main as a pinning test, so the test written is
WhenTurnedOff_PerVariantPricingIsNullToo. The option's docs and CHANGELOG say so.
22. .NET conventions everywhere; ReleaseDate becomes DateTime?
The owner, 2026-10-17, approving ReleaseDate changing from string? to a date type: "i want .net
conventions everywhere, thats the point int he sdk really". The SDK exists to give .NET developers
idiomatic types, so where the JSON's shape and .NET convention differ, convention wins. Before 1.0
that applies even when the change is breaking.
ReleaseDate is DateTime? (date only, kind Unspecified) on every target. The modern convention
for a calendar date is DateOnly, but netstandard2.0 (Unity, .NET Framework) lacks it, and public
types that differ by target framework are ruled out. A string had been kept on the assumption of
partial dates. That assumption was wrong: on 2026-10-17 all 606 sets (en 220, fr 202, ja 184) had a
full YYYY-MM-DD. An absent or malformed value reads as null rather than failing the set.
23. 1.0 review notes, decided
The Fable full-library review (2026-10-17) raised notes that would be breaking or awkward to change after 1.0. Each was put to the owner, who said "do 8, and 9, fix 12 too that seems like a bug, and support 11, 1-6 are obviously fixed". All are done in 0.7.0:
- #8, failure kind:
TcgDexApiExceptiongainsKind(aTcgDexErrorKind) andRequestUri, so callers branch on the failure instead of parsing the message. A language 404 staysHttpStatuswithIsLanguageErrorset, so that fact lives in one place.GraphQlErrorexists because GraphQL errors arrive as200. - #9, options: options are read once, when the client is created, on REST and GraphQL alike.
- #11, value expressions:
Whereevaluates indexers, array elements, parameterless instance methods and casts on the value side by interpretation, neverExpression.Compile()(#4). - #12, handler rotation: a bug, as the owner said. The caching handler's coalescing and counters
live in a singleton (
TcgDexCacheStatistics) and surviveIHttpClientFactoryrebuilding handlers. - Also decided in the same pass: a second
AddTcgDexthrows rather than silently keeping the first options.#13, a server page-size cap, was measured, not assumed: there is none (5000 returned 5000).
24. Breaking changes on 0.x bump the minor version
The release prepared as 0.6.4 contained breaking changes (ReleaseDate became DateTime?,
TcgDexJsonContext became internal, invalid options moved from ArgumentException at registration
to OptionsValidationException at start). It was renamed to 0.7.0 before it shipped. The owner,
2026-10-17: "yep sounds good", agreeing with the proposal that a release with breaking changes
ships as 0.7.0 rather than 0.6.4, following semver practice for 0.x.
From now on, any breaking change on 0.x bumps the minor version. A patch release contains fixes only.
25. The 1.0 naming pass: .NET conventions over the API's names, before 1.0
The owner, approving the 0.8.0 scope taken from the Fable .NET-convention audit: "i want .net conventions everywhere, thats the point in the sdk" — the same principle as #22, applied to names. Every public name follows the .NET Framework Design Guidelines, and the change ships before 1.0 because after it each rename is a major version. Under #24 it ships as 0.8.0.
What that meant in practice:
- No non-words or abbreviations.
Serie→Series,Hp→HitPoints(matchingGetHitPointsAsync),Avg7→Average7Days,IdProduct→ProductId,FirstEd→FirstEdition,Lang→Language. - No type named after a keyword or a general concept.
Set→CardSet. The rest of the family moved with it (CardSetBrief,CardSetAbbreviation), so a brief still pairs with its full type the wayCardBriefpairs withCard. - One casing per brand.
TcgPlayereverywhere, matching theTcgPlayerPricingtype;CardTrader, the marketplace's own casing. - Methods are verbs. Resource methods without one gained
Get(GetRaritiesAsync). - Nothing public that no caller needs.
QueryOperatorbecame internal; the status-onlyTcgDexApiExceptionconstructor, which producedKind = Unspecified, was removed; the caching handler's counters collapsed into the oneTcgDexCacheStatistics.
JSON keys did not change. Every renamed property carries [JsonPropertyName] with the API's
key, and tests pin the written keys, since case-insensitive reading means a round trip cannot catch
a missing mapping. The query builder now takes a property's field name from that attribute: before,
it camel-cased the .NET name, and c.HitPoints > 100 would have become the unknown hitPoints.
Names the audit raised but that were left alone, because the case is not clear-cut, are listed in the 0.8.0 report rather than changed on a guess.
26. CachedResponse has reference equality, not record equality
Decided while doing the naming pass (#25). The owner asked for the decision to be made and recorded rather than giving one; the reasoning below is the implementer's, not the owner's.
CachedResponse was a record, so it had value equality, but record equality compares its
byte[] Body by reference. Two entries holding the same bytes therefore compared equal or not
depending on whether they happened to share an array, which is equality nobody can rely on. The
alternatives were:
- Compare the bytes. Correct, but every comparison walks the body (22 KB for a set), and the hash code would change if anyone wrote to the array, which breaks it as a dictionary key.
- Plain class, reference equality. Honest about what an entry is: a stored blob with a timestamp, not a value. Nothing in the SDK compares entries, and a custom backend (Redis, disk) builds a fresh one on every read anyway.
The second was taken. CachedResponse is a sealed class; it loses with and ==, which the
SDK's own revalidation path used once and now replaces with an initializer.
27. Full image URLs are Uri?; the API's base URLs stay string?
Decided while doing the naming pass (#25). As with #26, the owner asked for the decision to be made and recorded; the reasoning is the implementer's.
The .NET guideline (CA1055/CA1056) is that a URL is a System.Uri, not a string. The helpers that
build a fetchable address (ImageUrl.Build, ImageUrl.BuildAsset, GetImageUrl, GetLogoUrl,
GetSymbolUrl) now return Uri?, which a caller hands straight to HttpClient or an image control.
The model properties Card.Image, Logo and Symbol stay string?. They are not addresses: the
API sends a base without a file extension, and requesting it as-is returns 404. Typing them as
Uri would present something unfetchable as a URL, which is the mistake the helpers exist to
prevent. The helpers' baseUrl parameter is a string for the same reason.
A base that does not make an absolute http(s) URL (relative, or not a URL at all) produces null,
following the tolerance policy of ReleaseDate and the timestamps: a value from the API that
cannot be read becomes null rather than an exception. The scheme check also stops /en/swsh/1
parsing as a file:// URI off Windows. An unknown ImageQuality or ImageFormat still throws,
because that is a caller's mistake, not the API's.
28. JSON converters stay internal
Decided while doing the naming pass (#25); the owner's scope was "internal unless a consumer genuinely needs one", and the reasoning is the implementer's.
The SDK's converters (FlexibleStringConverter, ReleaseDateConverter, TimestampConverter,
TcgPlayerPricingConverter) are all internal, and stay so. They are attached to the models with
[JsonConverter], so reflection-based JsonSerializer applies them without a consumer naming one.
Public, each would be pinned API with nothing to call. They do appear in PublicApi.approved.cs,
but only as the type argument of those attributes, which is metadata rather than callable surface.
The cost, already noted in the 0.7.0 CHANGELOG: a consumer's own source-generated
JsonSerializerContext cannot include the models, because the generated code would have to
construct an internal converter. Serializing models under Native AOT is therefore not supported.
That is the consequence of #20 as much as of this entry, and no consumer has asked for it. Two other
cases would justify a public converter: that one, or a consumer modelling their own type with the
same quirk (a string-or-number field, a tolerant date). Making a type public later is not breaking,
whereas making it internal after 1.0 would be, so internal is the default until someone asks.
29. The doubtful renames: the owner's picks
The naming pass (#25) left a list of names whose case was not clear-cut. On 2026-10-18 the owner chose from that list in a multiple-choice question. No further reasoning was given for any pick or rejection, and none is supplied here.
Renamed (each keeps its JSON key through [JsonPropertyName]):
Card.Retreat/CardFilter.Retreat→RetreatCostCard.VariantsDetailed→DetailedVariantsCard.EvolveFrom/CardFilter.EvolveFrom→EvolvesFromISetResource→ICardSetResourceGraphQl→GraphQLin every public name (TcgDexOptions.GraphQLEndpoint,TcgDexErrorKind.GraphQLError), with internal names followingCard.Legal/CardSet.Legal→Legality
Left as they are: the type of TcgDexProblem.Status, WPromo, Attack.Cost, Holo and
CardQuery.Page.
Kept from the first round: the three renames the implementer added beyond the audit —
CardSetBrief, CardTrader, and the plural DexIds / Stamps. CardFilter.DexId stays singular
because it filters by one id; the 0.8.0 migration table says so.
30. 0.8.0 ships as a normal release, not a 1.0 release candidate
The naming pass (#25) was the last item on the 1.0 bar (#19), so it could have shipped as
1.0.0-rc.1. The owner chose 0.8.0: "it wont be served on nuget natively if we do a rc tag, so
everyone would be on old stuff, so im leaning towads the 0.8.0 for a little". NuGet hides
prerelease versions unless a consumer opts in, so an RC would have left every default install on
0.7.0.
1.0.0 follows on the same code once 0.8.0 has had some use. A published NuGet version cannot be
renamed, so that is a small version-bump PR (csproj <Version> plus a [1.0.0] CHANGELOG
section) and a new v1.0.0 tag, not a retag of 0.8.0.
Same release: the GitHub Release now carries the published .nupkg and .snupkg, and its notes
and the Discord post link the NuGet page and give the install command. The owner had been linking
the GitHub release on Discord, which until now pointed only at source zips: "people havent
actually seen a real release lol i think".
31. Mutation runs skip the live-API tests
Every Mutation run, including the daily 05:17 UTC one, discovered 894 tests: the 723 unit tests
plus all 171 in TcgDex.CSharpSdk.IntegrationTests, because Stryker takes the solution
(stryker-config.json names TcgDex.CSharpSdk.Tests as the only test project, but the solution
brings the other one in). The live tests therefore called the real TCGdex API for every mutant
they covered, and were credited with kills: 180 in run 39065526941, which inflated that run's
per-file scores. The number of requests was not measured.
"test-case-filter": "TestCategory!=Integration" excludes exactly the live tests, which all carry
that category through LiveApiFixture, and keeps the hermetic JsonShape tests. A mutant only a
live test used to kill now survives, and shows up as a gap in the unit suite. That is the point:
per-push CI does not run the live tests either.
The owner asked for this one-line change to be split out of the mutation-score work and merged the same day.
32. No file's mutation score stays below its best without being examined
After 0.8.0 the total was 92.45% against 92.57%, and I called that flat, within the ±2.7 noise. The owner overruled it: "if things are lower than they were anywhere, it needs looked at". The noise bound applies to the total, not to one file.
Per file, against each file's best score in any past run counting only unit-test kills (#31), six
files were lower: TcgDexServiceCollectionExtensions (89.1% from 100%), ExpressionTranslator
(84% from 92%), TcgDexClient (81.2% from 87.5%), CardImage (87.1% from 93.1%),
MemoryTcgDexResponseCache (87.1% from 91.3%) and TcgDexTransport (88.2% from 89.3%). Comparing
the survivors by mutator, replacement and source line, not by count, showed which were new code and
which had lost a test. Of the 33 survivors in those files, 21 now have a killing test, each checked
by applying the mutant by hand, and 12 are written off in docs/coverage.md with a reason. One
code comment that called a mutant equivalent was wrong: removing that early return makes a miss
read the clock and Remove a key that was never stored.
The first run after those tests found five more survivors in TcgDexTransport (four constructor
statements) and TcgDexJsonContracts (one guard). A run had "killed" them only through
PublicApiTests.ThePublicSurface_MatchesTheApprovedBaseline, which cannot test a constructor guard,
so those were accidental kills. Each now has a test that fails against exactly its mutant.
MemoryTcgDexResponseCache still reads 90.3% against a best of 91.3%, but only because it has fewer
mutants than it did (31, from 46): its three survivors are the written-off equivalents.
The survivors in files that were not below their best were read as well, because the owner asked
for everything that "might be able to be killed": fourteen more got a killing test and four are
written off in docs/coverage.md (fourth round). Among them, one comment that said a survivor "only
shows up as a lower hit rate, invisible to an assertion about a value" was wrong: a tagless entry in a
full DeserializedResponseCache evicts one that can be served, which a test can see.
The rule from here: a file below its best-ever score is examined before a release or merge, whatever the total does.
33. A string indexer is accepted as a filter value; shipped as 0.8.1
The translator evaluates the value side of a filter by interpretation (#4, #11). It accepted
names[0] on an array, a list and an IReadOnlyList, but rejected text[0], because the string
indexer is String.get_Chars and the check looked for get_Item only. Nothing pinned either
behaviour. The question put to the owner was whether to accept both or reject both, with a
recommendation to accept both because the same kind of expression should behave the same way. The
owner: "ok accept both". The check now accepts any indexer getter (get_ plus arguments).
Shipped as a patch release, 0.8.1, not a minor one: under #24 only a breaking change on 0.x bumps the minor version, and this makes an input that threw work without changing anything that worked. The owner confirmed the release: "yes, fix everything we have tallked about and do the release".
34. The packed package is installed and run, in CI and before every publish
Before 1.0 the owner asked what would show the API is stable, and the first thing put on the list
was installing the built package into a fresh project. His reply: "install smoke tests first, those
whould really already be there". They were not. CI ran dotnet pack and uploaded the result, and the
AOT smoke test reaches the library through a ProjectReference; nothing installed the package, so a
missing target framework, README or icon, or README links that only work on GitHub, would have passed
every check.
scripts/Test-Package.ps1 reads the .nupkg and .snupkg (the lib assets and XML docs for
netstandard2.0, net8.0 and net10.0, README, icon, nuspec licence, icon, readme and commit, one
dependency group per target, no .pdb in the .nupkg, no repo-relative link in the packaged README),
restores TcgDex.CSharpSdk.PackageSmokeTest from it and runs it. That project is a consumer: it is
outside the solution, inherits nothing from the repository's build props, and uses the public API
only. It runs on net472, which loads the netstandard2.0 asset, as well as net8.0 and net10.0.
Two things make it able to fail rather than just run:
- nuget.org already has the version in the csproj, so a restore that is not pinned would install
the published package and pass. The restore uses a
nuget.configwith package source mapping that sendsTcgDex.CSharpSdkto the local feed alone, into an empty packages folder, and the program fails unless the assembly it loaded carries the version and the git commit that were just packed. Mapping the id to nuget.org by hand made it fail on all three frameworks, naming the published commit. - The frameworks are listed in the script, not read from the project, so one that silently drops out fails the run. Dropping net8.0 from the library failed it five ways.
CI runs it on Ubuntu and Windows after the package is built and uploads the symbols package with it;
the release workflow runs it on ./artifacts before the NuGet login, on Linux, so the netstandard2.0
asset is covered at release time by CI's Windows lane. It was first added as an ordinary job, since
required checks are branch protection and the owner's to change; he then asked for it: "make it
required". Both matrix jobs, Package install smoke test (ubuntu-latest) and (windows-latest),
joined the six required checks on 2026-10-19, making eight. A flaky restore from nuget.org therefore
blocks a merge until the job is re-run, which is the price of the check being worth having.
35. The dependency-injection entry points are in the pinned public API
Found while building the package smoke test (#34): PublicApi.approved.cs had no AddTcgDex or
AddTcgDexWithCaching. PublicApiGenerator treats every Microsoft.* and System.* namespace as not
part of the public API unless told otherwise, and the extension methods sit in
Microsoft.Extensions.DependencyInjection, the convention for them. So the way most consumers
register the SDK could change without the test that exists for exactly that noticing. I said it
sounded like a bug; the owner: "fix that it soundss like a bug".
AllowNamespacePrefixes now names that one namespace, and the baseline gains the two methods (8 lines,
nothing else). ThePublicSurface_IncludesTheDependencyInjectionEntryPoints fails if they ever leave it,
so the allow-list cannot be dropped quietly. Changing the signature of AddTcgDex now fails the baseline
test at the line of the method; before, it passed.
36. The public interfaces stay interfaces; paging and the catalog are reshaped before 1.0
The interface review the soak plan called for (#30), run on 2026-10-24 at the owner's request ("yep, do the interface review"). Run in-session, not on the credits queue: the owner, "not on the api, do it here, we have basically no api creds left". Two independent reviews, then a comparison: mine first, then a Fable review ("do a cross review with fable, give it zero, none, of your findings"). Fable's prompt held the question and file pointers and none of my verdicts.
Both reviews agreed, so this is the decision:
ITcgDexClient,ICardResource,ICardSetResource,ISeriesResource,IRandomResourceandITcgDexResponseCachestay interfaces. A sealed class cannot be stubbed, which is the reason a consumer wants these types, andITcgDexResponseCacheis implemented by consumers on purpose (Redis, disk). A non-sealed class withvirtualmembers buys nothing: nobody subclasses a resource.- The real risk is growth after 1.0. An interface cannot gain a member without breaking a hand-written
implementer, and netstandard2.0 has no default interface methods. So growth goes into parameters of
sealed types (
CardFilter,CardQuery), which can gaininitproperties freely, not into new interface members. SearchDetailedAsync(CardFilter, int? page, int? itemsPerPage, ...)was the worst signature: loose optionals on the method, while REST paging lives onCardQuery.Page. Sort would have been a fourth optional or an overload.
Where the reviews differed, and which was right: I said keep ICatalogResource as it was. Fable said
change it: upstream grows by adding enumeration endpoints (13 members, one per endpoint), every new list
would be a new member and a major bump, and nobody stubs 13 near-identical methods. Fable's reasoning
held; I had weighed mockability and not growth. It also found StreamAsync's pageSize had the same
two-sources-of-truth problem as SearchDetailedAsync, which I had missed.
What changed (the owner: "Yes: branch, record the decision in docs/DECISIONS.md (both reviews), then do the ICatalogResource and paging changes in one PR, TDD, with PublicApi.approved.cs updated"):
ICatalogResourcekeeps its 13 named methods and gainsGetValuesAsync(name)(text) andGetNumberValuesAsync(name)(numbers), so an endpoint the SDK does not model yet is reachable without a breaking release. The name is escaped as a single path segment, like an id.CardFiltergainsPageandItemsPerPage;SearchDetailedAsyncbecomes(CardFilter, CancellationToken).StreamAsync(CardQuery, CancellationToken): the page size is the query'sPage(_, itemsPerPage), 100 when unset; its page number is still ignored.
Review of the change itself (2026-10-25): three reviewers, run independently of each other and of me: a
sonnet diff review, a sonnet test-validity review, and Fable. All were given the branch and the intent, no
findings. They agreed on two defects, both fixed in the same PR: a null or blank catalog name threw naming
id instead of name (it went through the id escaper), and . or .. survived Uri.EscapeDataString
and were collapsed by the URI layer, so the "single segment" claim was false. The second also affected ids,
so the fix is in the shared segment escaper and ids are rejected too. Also fixed: the CHANGELOG said the
wire requests were unchanged, which was false for a query that set a page size; the test gaps (each rule
exercised through both catalog methods); and the page-size exceptions now name Page and ItemsPerPage.
Fable asked whether CardFilter.Page (an init property) against CardQuery.Page(page, size) (a method)
is intended. It is: each follows its own type's idiom, a settings record against an immutable fluent
builder, and unifying them would be a bigger change than this PR.
Not done: merging the two ListAsync overloads into ListAsync(CardQuery? query = null, ...), which
Fable listed. It would make ListAsync(cancellationToken) stop compiling for every caller, to head off a
third overload that no interface has asked for. Recorded as a known trade-off, not a regret.
37. The pre-1.0 checklist: Fable's cold read, every row now checked
Asked on 2026-10-25, after 0.9.0 (#36): "what else should be done before a real 1.0 release, have fable answer that question too, without the context of our 'plan' for release right noww". Fable ran with no release plan, no soak timeline and none of my conclusions, and was told to form its own list from the code, tests, CI and package metadata before reading this file. The owner then agreed to record it here ("sure").
Each row began as Fable's claim, not a measurement. All 19 were checked on 2026-10-25 (rows 1 to 4 first, the
rest when the owner said "verify those rows"). A status says how: measured means a probe test ran and was
deleted, read means I read the code or file, queried means a gh api call. Evidence paths are Fable's;
some line numbers have drifted.
| # | Sev | Item | Evidence | Status |
|---|---|---|---|---|
| 1 | blocker | SECURITY.md says nothing is published to NuGet and lists only main as supported |
SECURITY.md:30-36 |
confirmed 2026-10-25: read the lines; 0.9.0 is live on NuGet. Fixed in #39 |
| 2 | blocker | No written compatibility contract: SemVer scope, the interface-growth rule (#36), TFM lifecycle, [Obsolete] lead time, how long an old major gets fixes. .NET 8 reaches end of life 2026-11-10, so decide if net8.0 stays |
CHANGELOG.md:5-6, SECURITY.md:28-36 |
confirmed 2026-10-25: no SemVer, deprecation or support-window text in the README, SECURITY.md, CONTRIBUTING.md or docs/*.md (only the CHANGELOG header names SemVer). docs/learnings.md:127 notes the net8.0 end-of-life date but nothing decides it. Fixed in #39 (docs/versioning.md) |
| 3 | blocker | Models are records holding IReadOnlyList<T>, so two cards from identical JSON may not compare equal; record to class later is breaking (#26 did it for CachedResponse) |
Models/Card.cs:21,27-34, PublicApi.approved.cs:177 |
confirmed by measurement 2026-10-25: two Cards deserialized from the same JSON (one types entry) gave Equals false, == false and different hash codes. Cards whose collections are all empty compare equal, because [] is a shared empty array, so the bug shows only on real data. No model overrides Equals. Fixed in #40 |
| 4 | blocker | No EnablePackageValidation with a baseline; the API baseline test is net10.0-only and does not check binary compatibility |
TcgDex.CSharpSdk.csproj:56-80 |
confirmed 2026-10-25: no EnablePackageValidation, PackageValidationBaselineVersion or ApiCompat setting in any csproj, props, targets or workflow. Fixed in #41 |
| 5 | should | release.yml does not require green CI on the tagged commit; unit tests on Ubuntu only, no net472, coverage gate or AOT smoke; a comment claims net472 runs |
release.yml:97-123 |
confirmed (read): the release job's Test step runs on ubuntu-latest, where the test project multi-targets only net8.0;net10.0 (net472 is Windows-only, TcgDex.CSharpSdk.Tests.csproj), yet the comment above it says "netstandard2.0 is executed through net472"; no needs: or CI-status condition ties a tag push to green CI (the required checks gate the merge to main, and the tag goes on that merge commit); no coverage gate or AOT smoke in the job. The package smoke test (Test-Package.ps1) does run in it. Fixed in #42 |
| 6 | should | Publish job's actions pinned by tag, not SHA | release.yml:47,49,136,153 |
confirmed (read): release.yml uses checkout@v7, setup-dotnet@v6, NuGet/login@v1 (the step that receives the OIDC token) and upload-artifact@v7 by tag. SHA pins are rare: only ci.yml's package-smoke-test job (checkout, setup-dotnet, download-artifact) and mutation.yml (checkout, setup-dotnet, upload-artifact) use them, with a version comment. Every other job in ci.yml, and release.yml, codeql.yml, docs.yml, fuzz.yml and live-api.yml, use tags (codeql-action, upload-pages-artifact, deploy-pages and cache among them). dependabot.yml exists (corrected after review). Fixed in #42 |
| 7 | should | No ContinuousIntegrationBuild, no global.json |
Directory.Build.props:45 |
confirmed (read): Deterministic is true (Directory.Build.props:45); ContinuousIntegrationBuild appears in no props, csproj or workflow; there is no global.json, so the SDK version floats with the runner. Fixed in #42, except that the SDK still floats forward on purpose |
| 8 | should | No 429 handling; TcgDexApiException exposes no Retry-After. Additive later, but 1.0 docs should say what a rate-limited caller sees |
TcgDexApiException.cs:46-73 |
confirmed (read): no 429, Retry-After or TooManyRequests anywhere in the library source; TcgDexApiException carries StatusCode, Problem and RequestUri but no response headers, so a 429 would surface as Kind = HttpStatus with StatusCode 429 and the wait time is lost (inferred from the generic status handling; not run against a 429). docs/getting-started.md:268-282 says the SDK ships no retry policy on purpose and shows AddStandardResilienceHandler, but does not say what a rate-limited caller sees |
| 9 | should | StreamAsync ignores the query's page number. Decided in #36; Fable still flags it |
Resources/Resources.cs |
decided, re-open only if the owner wants |
| 10 | should | ITcgDexClient has no escape hatch for a new resource group |
TcgDexClient.cs:17-33 |
confirmed (read), and the consequence is decided: it has exactly five get-only properties and no generic accessor. #36 and docs/versioning.md say no interface gains a member within a major, so a new resource group waits for the next major. An escape hatch (as ICatalogResource got) was not added for the client |
| 11 | should | required on Id, Name, LocalId, Category, Set: one upstream card missing name fails the whole ListAsync |
Models/CardBrief.cs:18,31,34 |
confirmed by measurement: a two-card list response whose second card has no name makes Cards.ListAsync throw TcgDexApiException with Kind = InvalidResponse; the first, valid card is lost with it. Attack.Name is already nullable for the known upstream nameless attack, but the card and brief fields are not. Correction to Fable's evidence: CardBrief has required on Id, LocalId and Name only; Category and Set are required on Card, not on the brief. No tolerance decision has been made |
| 12 | should | docs/publishing.md stale (version, secret name, dates) |
docs/publishing.md:205,328-330,364 |
confirmed (read): line 205 says the version is "currently 0.1.0, and the only judgement left" although line 3 says 0.1.0 was published on 2026-08-08 and 0.9.0 is out; it describes a NUGET_USER repository secret while release.yml writes user: PinKushin literally; the manual-push examples still push 0.1.0; a "Checked 2026-08-07, still no C# SDK listed" note is dated. Fixed 2026-10-26: the page now has a "How a release happens now" section (tag the merge commit; ci-gate, publish, announce), no version number that can go stale, no NUGET_USER, the 0.1.0 checklist labelled as history, and the listing note re-checked against tcgdex.dev/sdks that day |
| 13 | nice | Three TcgDexApiException constructors give Kind = Unspecified |
TcgDexApiException.cs:20-37 |
confirmed (read), and documented: the parameterless, message and message-plus-inner constructors leave Kind at Unspecified (0), and the enum's own doc says only an exception built without a kind, "such as one a caller builds in a test", carries it. Nothing in the library throws one. Left as is |
| 14 | nice | TcgDexClient.Create(configureCache: ...) validates differently from the DI path |
TcgDexClient.cs:272-275 |
confirmed by measurement: MaxEntries = 0 throws ArgumentOutOfRangeException from TcgDexClient.Create and OptionsValidationException on resolving the client from AddTcgDexWithCaching. Both fail loudly and early; the types differ |
| 15 | question | AddTcgDex throws on a second call; the convention is idempotent |
TcgDexServiceCollectionExtensions.cs:57-66 |
decided in #23 (read: #23 records that a second AddTcgDex throws rather than silently keeping the first call's options) |
| 16 | nice | ReleaseDate is DateTime?, Updated is DateTimeOffset? |
ReleaseDateConverter.cs:5-9 |
decided in #22 for ReleaseDate (read: DateOnly is unavailable on netstandard2.0, so DateTime?). The mix with DateTimeOffset? for Updated is not separately argued there |
| 17 | nice | Docs: index.md omits netstandard2.0; README says .NET Framework 4.6.1+ but only net472 runs; PublicApiTests remark about IAsyncEnumerable is wrong; thread-safety and the "TcgDex" log category are not documented publicly |
docs/index.md:11, README.md:26,218, PublicApiTests.cs:33-38 |
confirmed (read), five sub-claims: (a) docs/index.md:11 says "Targets .NET 8 and .NET 10" and omits netstandard2.0, true; (b) README says ".NET Framework 4.6.1+" at lines 26 and 218 while only net472 is ever run, so 4.6.1 is untested, true; (c) the PublicApiTests remark that netstandard2.0 "cannot expose IAsyncEnumerable" is false: the pack with strict-in-package validation (#41) passes, so the three assets expose the same API apart from the three enums whose ISpanFormattable difference is suppressed (Microsoft.Bcl.AsyncInterfaces supplies IAsyncEnumerable on netstandard2.0); (d) thread safety: the client's remarks say to hold one instance for the application's life but nowhere state that it is thread-safe, partly true; (e) the TcgDex log category is documented in the README and docs/observability.md:38 but is not a public constant, a nit. Not yet fixed |
| 18 | nice | No signing, no CODEOWNERS or templates; tag protection for v* not visible in the repo; CONTRIBUTING.md bans --no-build while the workflows use it |
release.yml:107,115, CONTRIBUTING.md:67 |
confirmed (read, queried): no CODEOWNERS, issue or pull-request templates, or CODE_OF_CONDUCT.md exist; no signing step in the release workflow; gh api repos/.../rulesets (unfiltered) returned an empty list and .../tags/protection returned 404, so there is no repository ruleset at all and no classic tag-protection rule; I could not see an organisation-level ruleset, so v* tags are unprotected as far as this repository shows (anyone with write access can push a tag and trigger a publish, which the trusted-publishing policy still limits to this workflow); CONTRIBUTING.md:67 bans --no-build while ci.yml (4 places) and release.yml (2) use it |
| 19 | nice | The netstandard2.0 asset is never run on a modern Core runtime, only net472 | TcgDexClient.cs:237-263 |
confirmed (read): the test project multi-targets net472;net8.0;net10.0, and each target loads the library asset for its own framework, so netstandard2.0 is executed only on net472. A net6.0 or net7.0 consumer resolves that asset (docs/getting-started.md) and runs on a Core runtime, a combination no test or smoke test exercises (the package smoke test runs net8.0 and net10.0) |
Fable's three first steps: write the compatibility contract (rows 1, 2, 12), settle model equality (row 3), and turn on package validation with SHA-pinned actions and a CI gate on release (rows 4 to 7). It found nothing against the interface decisions in #36.
38. The Discord announcement is called from the release workflow
The owner, 2026-10-25, after 0.9.0 published: "i dont think the sdk sent the update notification to the
discord, i see nothing." He was right. discord-release.yml (PR #95, secret set 2026-10-21) had no runs at
all. It listens for release: published, but release.yml creates the GitHub Release with the built-in
GITHUB_TOKEN, and GitHub does not start workflows from events that token causes (workflow_dispatch and
repository_dispatch are the exceptions). So the trigger it was built on could never fire for a tag-driven
release. Nothing had exercised it first: the manual workflow_dispatch test message was never run either.
The fix: discord-release.yml also takes workflow_call with a tag, and a new announce job in
release.yml (after publish, tag pushes only) calls it. The workflow reads the release back with
gh release view, so the fields have one source and a tag that does not exist fails loudly instead of
posting an empty message. A manual workflow_dispatch with a tag announces an existing release, which is
how 0.9.0 is announced late. The release trigger stays for a release a person publishes by hand, which does
fire it; the two cannot both fire for one release. The announce is its own job so a Discord failure cannot fail
or re-run the NuGet publish, and the publish workflow's file name and job are untouched (the trusted
publishing policy names release.yml).
Verified with a dry run from the branch before merging (no secret needed); the first real post is the late 0.9.0 announcement, which is the owner's call because it is outward-facing.
Amended 2026-10-25, after a Fable review (it had not been run before the merge):
- "The two cannot both fire for one release" was false. A release created by hand in the GitHub UI with a new
tag fires
release: publishedas a person, and the tag push then reachesrelease.yml, whoseannouncejob posts a second time. The first post would also land before NuGet has the package. Therelease:trigger is removed; a release made another way is announced by dispatching with its tag. secrets: inheritgave the called workflow every repository secret. It now passesDISCORD_WEBHOOK_URLby name.- A dispatch
tagmust look likev<digit>...(a leading-would be read byghas a flag; it failed rather than injected, so this is tidiness). The job comment says that re-running all jobs re-runspublishharmlessly.
39. The versioning contract: docs/versioning.md
Blockers 1 and 2 of #37 (SECURITY.md still said nothing was published; no written compatibility
promise). The owner, 2026-10-25: "Fix the four confirmed blockers: SECURITY.md, the compatibility contract
(versioning doc, I will decide net8.0 with you)". On net8.0 he first chose, from a recommendation of mine,
keep net8.0; drop it in a minor release once its runtime is out of support (no reasoning given). That
was reversed the same day, see the amendment below: a target framework is dropped only in a major
version.
docs/versioning.md states what each of patch, minor and major may contain, what is public API (the pinned
PublicApi.approved.cs), the interface rule, the target-framework rule above, and what is not promised.
SECURITY.md now says only the latest release is supported and points there.
Proposed by me, confirmed by the owner on 2026-10-25 (see "Resolved" below): these four rules were not decided in conversation at first; I wrote them as the least surprising defaults:
A minor release may add a member to the consumer-facing interfaces.Reversed, see the amendment below.[Obsolete]comes in a minor release and removal is no earlier than the next major, with no minimum number of minors between.- Only the latest release of the current major gets fixes (one maintainer).
- The wire requests, log wording and exception messages are not API; the exception types,
Kind, and log categories are.
Amended 2026-10-25, after three independent reviews of the merged page (a sonnet diff review of the docs and the code reviews of #40). What changed and why:
- The interface rule was backwards. I had written that a minor release may add a member to a consumer-facing
interface. The reviewer pointed out that this contradicts the CHANGELOG for 0.9.0 and #36, whose whole reason for
moving paging was that an interface cannot gain a member or parameter without breaking every hand-written
implementer, which is a major change under semver. The page now says what #36 says: no interface gains a
member within a major, and growth goes onto sealed types and the
GetValuesAsyncescape hatch. This is still my proposal, now the stricter one; it is the owner's to confirm. - Adding an optional parameter is a signature change, not a minor addition, because it breaks compiled callers; it ships as an overload.
- Added: a fix is a patch even if someone depended on the defect; new model members must be optional; enums may
gain values;
CachedResponseandTcgDexCacheOptionsare extension points too. - The
net8.0drop was described too kindly. I said only "framework-specific tuning" is lost. Checked againstdocs/getting-started.mdand the csproj: anet8.0consumer falling back tonetstandard2.0loses prompt cancellation mid-body,PooledConnectionLifetime(so stale DNS on a long-lived client), and the trimming and AOT declaration, and gains five package dependencies. His first choice, "drop in a minor after end of life", rested on the premise that nothing is lost but tuning, so I put the premise to him again with the costs. Reversed by the owner, 2026-10-25: "No. Dropping a target framework should only happen in a major version." He gave no further reasoning; the costs above are the reason I had put in front of him. Both positions, so the reversal can be defended later: drop in a minor after end of life (a smaller matrix once a runtime is out of support) against only in a major (an upgrade within a major never costs anyone those behaviours).net8.0therefore ships for as long as1.xdoes, whatever its runtime's support status, and its first chance to go is the next major after its end of life.docs/versioning.mdsays so and lists dropping a TFM under Major. - Not changed:
FlexibleStringConverteris internal on purpose (#28); the page's "not in that file is not public API" is right, and the attribute that names it only shows up in the generated file.
Checked against established practice (an Opus review, 2026-10-25), after the owner asked "i think your proposals
are good, are they standard? have them reviewed too ... they seem pretty much best practises arent they?".
The reviewer fetched one source (Microsoft Learn's library breaking-changes guidance) and judged the rest from
memory, which it said. Its verdicts: standard are no new interface members within a major, [Obsolete] then
removal in a major, enums may gain values, new model members optional, messages not API. Stricter than usual
is dropping a TFM only in a major. Looser than usual are latest-release-only fixes (normal for Polly, Serilog
and Dapper; Microsoft.Extensions and Azure keep older lines) and the "fix is a patch" rule. Changes made because of
it: the fix rule is narrowed to behaviour that contradicts the documentation (an undocumented behaviour change is a
minor with a changelog entry); the page now says binary compatibility is firm and source compatibility is best
effort (a new overload can make a null argument ambiguous), that a dependency major bump is not a patch, that a
serialized model is not a format promise, and how the contract is enforced.
Resolved with the owner, 2026-10-25. I explained the four open items in plain terms with a recommendation each; his reply: "that all seems good" and then, interrupting himself, "wait actually i probably wont pin though" (the assembly version, below). So "all seems good" covered every recommendation except pinning. That settles:
- The four proposed rules above are confirmed (no interface gains a member within a major;
[Obsolete]first and removal no earlier than the next major; only the latest release of the current major gets fixes; wire requests, log wording and exception messages are not API). This is after his earlier lean ("i think your proposals are good") and after the amended text and both reviews. - No preview tier. Nothing is waiting for one and adding it later is additive. Revisit only if a feature needs it.
- A language minimum is stated: C# 11 or later, because the public types have
requiredmembers; raising it is a major-version change. The page says so. It applies to constructing a model, not reading one, and holds on thenetstandard2.0asset too:Compatibility/CompilerFeatureShims.csdefinesRequiredMemberAttributeandCompilerFeatureRequiredAttributethere. Not tested with an older compiler.
Assembly version: settled, not pinned. It tracks the package version. I recommended pinning AssemblyVersion
to the major (so a .NET Framework consumer on the netstandard2.0 asset needs no new binding redirect for a minor
or patch). The owner first said "wait actually i probably wont pin though, that seems objectively bad", which I
treated as a lean and pushed back on (both choices are defensible; Newtonsoft.Json pins, Microsoft's own packages
mostly do not). He then decided, with his reason: "i dont care if it costs .netstandard a little bit, people
running that should know enough to be able to work around, i want the right iinformatiion in traces", and chose
to leave it unpinned. So the DLL's version identifies the exact release in a stack trace or a crash dump, at the
price that a .NET Framework consumer may need a binding redirect on each upgrade, patches included. That cost is
accepted knowingly and the versioning page states it. Pinning can be revisited only as a deliberate change; it
would also be a visible change for those consumers.
Release timing (the owner, 2026-10-25): "i actualy asked tcgdex if they wanted me to stay pre 1.0 until they took a look, no reply yet". So 1.0.0 is waiting on TCGdex's answer as well as the soak (#30); nothing here sets a date.
40. Models compare by value, through two internal wrapper collections
Blocker 3 of #37, confirmed by measurement: two Cards from the same JSON were not equal, because a record's
synthesised equality compares an IReadOnlyList<T> by reference. The owner asked for it fixed ("Fix the four
confirmed blockers ... model equality"); he gave no view on how.
Chosen: ValueList<T> and ValueDictionary<TKey, TValue> (internal), stored by each collection property's
setter. The property types stay IReadOnlyList<T> and IReadOnlyDictionary<K, V>, so PublicApi.approved.cs is
unchanged (the baseline test passes untouched). The records keep their synthesised equality, which now does the
right thing for every collection, including any property added later.
Rejected:
- Dropping
recordforclass, which Fable offered. It removeswith,==andIEquatable<T>from every model, a much larger break than the bug. - A hand-written
EqualsandGetHashCodeon each record.Cardalone has about thirty properties; a property added later and forgotten would silently drop out of the comparison.
Semantics: lists compare in order, item by item with the item's own equality; dictionaries compare by entries
in any order. Null, absent and [] are all the same empty list. A list wrapper holds the list it is given rather
than copying it, so it costs one small object per non-empty collection and nothing else changes. A model's text
(ToString) now shows [Fire, Water] instead of the list's type name.
Amended 2026-10-25, after a Fable review (run after the merge): two statements above were wrong or incomplete.
- Dictionaries copy. A key comparer is not part of the entries, so wrapping a caller's dictionary and honouring
its comparer made
Equalsasymmetric (one side finds"Normal"under"normal", the other does not) and gave equal dictionaries different hashes.ValueDictionarynow copies into the default comparer (ordinal for strings), which also stops later edits to the caller's dictionary changing the hash. A source whose comparer is finer than the default can hold two keys the default treats as one; the last wins, it does not throw. Tests failed first. - Lists still alias. A
List<T>given to a model is held, not copied, exactly as before, so editing it afterwards changes the model and now its equality and hash. A copy would remove that at the price of a second array per collection on every deserialization; not taken, and documented in the CHANGELOG and on the wrapper. - Also recorded:
Equals,GetHashCodeandToStringcost time proportional to the card's contents (key a cache onCard.Id), andToStringon a 300-card set prints all 300.
Found while testing: Shouldly's ShouldBe compares two lists, and two dictionaries, entry by entry in
enumeration order, and never calls Equals. An assertion written that way cannot test a wrapper's equality, and
the dictionary test failed for exactly that reason before it was changed to call Equals. The record-level tests are
unaffected: a record is not an enumerable, so Shouldly uses its Equals.
Not measured: the extra allocation on a large deserialization. By size alone it is one 24-byte object (header
plus one reference) per non-empty collection, so at most eight per Card; no benchmark was run. The SDK's stated
position is lighter on allocations than the other C# SDK (docs/comparison.md), and this has not been
re-checked against it.
41. dotnet pack validates the package against the last release
Blocker 4 of #37: nothing stopped a release from breaking an assembly someone had already compiled against.
PublicApi.approved.cs pins the source-level surface, but on net10.0 only, and it does not look at binary
compatibility or at whether the three target frameworks still expose the same API. The owner asked for it ("Fix
the four confirmed blockers ... package validation"); he gave no view on the settings.
EnablePackageValidation with PackageValidationBaselineVersion 0.9.0 in the library csproj. Measured on
2026-10-25 with dotnet pack TcgDex.CSharpSdk/TcgDex.CSharpSdk.csproj -c Release on one change: renaming the
public MemoryTcgDexResponseCache.Clear() packed without a word before, and fails the pack after with
CP0002: Member 'void ...Clear()' exists on the baseline, once for each of net10.0, net8.0 and
netstandard2.0. (Not yet in VERIFICATION.md; the command above reproduces it.) The baseline is the published
0.9.0 from nuget.org and the build under test is compared with it whatever its <Version> says, so the check bites
from the first commit.
Corrected after a Fable review: the first settings did not enforce "same API on all three". I had set
EnableStrictModeForCompatibleTfms and written that it makes an API one framework has and another lacks an
error. It does not: that setting is runtime compatibility and is already the default. The across-frameworks check is
EnableStrictModeForCompatibleFrameworksInPackage, off by default. Measured with a probe member under
#if NET10_0: with the first settings the pack succeeded without a word; with the second setting it fails with
CP0002: Member ... exists on lib/net10.0 but not on lib/net8.0. Both are now set.
What turning it on cost: on unmodified code the second setting also fails the pack, with CP0008 on three
enums (ImageFormat, ImageQuality, TcgDexErrorKind): they implement ISpanFormattable on net8.0 and later and
not on netstandard2.0, which is the runtime's doing, not the library's. They are recorded in
TcgDex.CSharpSdk/CompatibilitySuppressions.xml, exactly three entries, generated with
-p:ApiCompatGenerateSuppressionFile=true; unmodified code packs clean again and the probe is still caught. A
suppression file can hide a break, so it is read, not trusted: add an entry only for a difference of that kind,
or a deliberate break before 1.0.0, never to quiet a failure.
The baseline has to move. It is the release just published, so it is bumped in the PR that prepares the next
one, and it is 0.9.0 until 1.0.0 ships. Forgetting to move it is not dangerous (every break against an older release is still
caught); it only makes the check looser about what the newer release added, and after 1.0.0 an unmoved 0.9.0
baseline would let 1.0.x break 1.0.0 unnoticed wherever 0.9.0 allows it. Nothing gates this: release.yml
checks <Version> and the CHANGELOG against the tag but not the baseline. A release-time check that the
baseline is the previous published tag is the obvious addition, and was not built.
Cost, corrected after review. I first wrote that pack restores the baseline. It does not: the baseline is
a downloadDependencies entry in project.assets.json, so it comes down at restore, in every job that
restores the library (macOS, Windows, net472, Stryker, AOT, fuzz, benchmarks), and a missing or unlisted 0.9.0
fails them all with NU1102. Those jobs already restore from nuget.org, so the added exposure is one package
that exists and cannot be unlisted without the owner doing it, but it is repo-wide, not pack-only. Checked by
reading obj/project.assets.json after a plain dotnet restore.
release.yml packs with -p:Version=...; that only changes the version being validated, not the baseline, so it
is unaffected, and a prerelease tag compares fine.
A future API that exists on net10.0 but not on net8.0 or netstandard2.0 now fails the pack and needs either a suppression or a change, which is the "same API on all three" promise doing its job.
42. The release pipeline: a CI gate, pinned actions, a CI-build flag, a global.json floor
Rows 5, 6 and 7 of #37. The owner: "Fix the release pipeline rows: rows 5, 6 and 7 (CI gate and net472 comment in release.yml, SHA-pin the actions, ContinuousIntegrationBuild and global.json). Branch, TDD where it applies, Fable review before auto-merge." He gave no view on the design; the choices below are mine.
CI gate (row 5). A tag can be pushed to any commit, and the merge's own CI run can still be going when it is.
New first job ci-gate in release.yml runs scripts/Assert-CiPassed.ps1 on github.sha and publish now
needs it. The script takes the newest ci.yml run per triggering event for the commit (so a re-run that passed
supersedes the failure it retried), exits 0 only if all concluded success, exits 1 on any other conclusion, waits
while one is running (the job gives it 15 minutes; CI takes about 3), and exits 1 if no run exists after two
minutes. It is a separate job so publish keeps its name and file, which the trusted-publishing policy names.
The false comment on the Test step (it claimed net472 ran there) now says what runs on that Linux runner and that
the gate is what guarantees net472, coverage and AOT passed.
Tested, against real commits: a green main commit exits 0; a commit whose CI failed (a PR branch) exits 1 naming
both runs; a commit with no CI run exits 1 after the grace period. The third case first exited 1 for the wrong
reason: Count on an empty result crashed under strict mode. Fixed with @(...) and re-measured, so the failure
message is now the intended one.
Pinned actions (row 6). Every uses: in every workflow is a full commit SHA with a version comment: 31 lines
by the pin-actions-to-sha script plus the two codeql-action subpath actions by hand (v4.39.1; the script cannot
resolve subpath actions). The one that matters most is NuGet/login, which receives the OIDC token. Dependabot's
github-actions entry should keep them current, since it is meant to update a SHA pin together with its comment;
the first Dependabot action PR after this is the check that it does.
CI-build flag (row 7). -p:ContinuousIntegrationBuild=true is passed to the Build and Pack steps of
release.yml, not set repository-wide: set globally it would also rewrite the source paths CI's coverage tooling
maps back to files. Measured: the symbols of a pack with the flag carry the normalised /_/ path and no trace of
the checkout directory; without it they embed the local path.
global.json (row 7): a floor, not a pin. { "version": "10.0.100", "rollForward": "latestFeature" }. It stops a
build with an SDK older than the one net10.0 needs, and documents it. It does not pin the exact SDK: the owner's
standing rule is to stay current and fix forward (#14 keeps CI on ubuntu-latest unpinned), and Fable's complaint
("the SDK floats with the runner") would be met only by an exact pin that goes stale and has to be bumped by hand.
I chose the floor, and a Fable review then asked the owner to confirm it rather than leave it as my call. Confirmed
by the owner, 2026-10-25: "Yes, keep global.json as a floor (10.0.100, latestFeature)." He gave no further
reasoning; the reasons above are the ones I put in front of him. The alternative he declined was an exact SDK pin
with setup-dotnet's global-json-file, which would also be what makes a rebuild byte-identical. Revisit only as a
deliberate change.
Amended after the Fable review of this change (0 blockers, 6 risks):
- The gate aborted on the first
gherror. Reproduced (a bad repository made it die in 1 second with a stack trace), then fixed: an error fromghis "unknown" and is retried at the next poll until the deadline (13 retries over 67 seconds when measured against a repository that does not exist). One 5xx can no longer kill a release. Write-Errorfollowed byexitwas dead code under$ErrorActionPreference = 'Stop', becauseWrite-Errorthrows. Failures now print one::error::line andexit 1; re-measured on the green, red and no-CI commits.- A commit that was both pushed and opened as a pull request has a run for each event, and both must be green; that is intended and now documented. A newer run of the same event supersedes an older one (the script's comment named the wrong mechanism before).
- The "measured healthy" comment on
ci-gatecited a plain CI run as if it were a gate run. It now says the ~3 minutes is CI's duration and that the gate itself has not run. - The recovery for a slow or unreachable CI is "Re-run all jobs", which is safe; documented on the job.
- A byte-identical rebuild needs the same SDK, which the
global.jsonfloor does not guarantee; the comment on the build step now claims only that the checkout path stays out of the binary and symbols. dependabot.ymlstill said actions were pinned by major tag; reworded.- Left alone: the pin comments are mixed (
# v7against# v7.0.1), cosmetic, and Dependabot rewrites each to the exact tag on its next bump. - The reviewer asked the owner to confirm the
global.jsonfloor rather than an exact pin; he did (above).
Not verified: release.yml has not been run end to end with these changes; that happens at the next tag, and a
manual dispatch would attempt a real publish. The workflow files were checked for valid YAML and for job
dependencies (ci-gate -> publish -> announce), and the release build and pack flags were run locally with a
new version number; nothing was published.