Repository navigation
Refresh the rubric audit against the SDK as it is now - #214
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1fba3b3fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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.
🟡 Changes recommended
The refreshed audit still contains factual inaccuracies that undermine its purpose as an accurate SDK assessment.
3 open issues
What changed in this PR
Refreshes the SDK rubric audit to reflect current implementation evidence, deliberately leaving Rubric Check red for failing criterion 1C.3.
Changes:
- Updates the audit date to October 17, 2026.
- Revises route-generation, body-limit, resilience, and release notes.
- Updates conformance counts to 134 cases across six runners.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| rubric-audit.json | Refreshes audit date, criterion results, and supporting evidence. |
Review effort: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdfbe92c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
Review threads: 15 resolved (15 fixed, 0 declined), over four rounds: 6, then 3, 3 and 3. Each round corrected a note, and every finding was true. |
d32efc9 to
8139cba
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8139cba91b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ions Sessions::MagicLinksController reads params[:code], not token, and reads the pending sign-in from the pending_authentication_token cookie CreateSession sets; CreateSession answers 201 and DestroySession 204. The model said token, 200 and 200, so nothing generated from it could complete a sign-in. RedeemMagicLinkInput now carries code, the two statuses are modelled, and the operations' docs say which cookie each side of the exchange carries. Every language regenerates; the conformance fixtures and runners send code and expect 201 and 204, and Rust's names.toml keeps the code redacted as the token was.
MagicLinkFlow posted {email} to /api/v1/sessions and {token} to
/api/v1/sessions/redeem, neither of which Fizzy routes, and dropped the
pending token the answer carries, so it could not sign anyone in. It
now sends through the generated SessionsService: CreateSession posts
{email_address} to /session.json and keeps the pending token from the
201; RedeemMagicLink posts {code} to /session/magic_link.json with the
token as the pending_authentication_token cookie, escaped as Rack
unescapes it, and answers the generated SessionAuthorization.
PendingToken and ResumeMagicLinkFlow let a sign-in finish in another
process, as Rust's flow does. Like NewClient, the flow now requires
HTTPS outside localhost.
This breaks the flow's API: CreateSession answers
*generated.PendingAuthentication, RedeemMagicLink takes the emailed
code and answers *generated.SessionAuthorization, Login answers that
session too, and the hand-written CreateSessionRequest/Response and
RedeemMagicLinkRequest/Response types, with their user_id, are gone.
fizzy-cli does not use this flow.
MagicLinkFlow posted {email} to /sessions and {token} to
/sessions/redeem, which Fizzy has never routed, and discarded the
pending token CreateSession answers. It now goes through the generated
sessions operations on a client from createFizzyClient: CreateSession
with {email_address}, keeping the 201's pending token; RedeemMagicLink
with {code} and the pending token as a percent-encoded
pending_authentication_token cookie, which is where Fizzy reads it; and
CompleteSignup with {full_name} under the session cookie.
This changes the API: requestLink(email) is createSession(emailAddress)
and answers the pending token; redeemToken(token) is redeem(code) and
answers requiresSignupCompletion beside the session token and auth;
resume(pendingToken) and pendingToken let a sign-in finish in another
process; completeSignup(fullName) is new. The options take the client
options that apply before there is a credential (baseUrl, userAgent,
requestTimeoutMs, hooks).
MagicLinkFlow posted {email} to /sessions and {token} to
/sessions/redeem, neither of which Fizzy routes, and never sent back
the pending_authentication_token cookie Fizzy reads the sign-in from,
so no sign-in could complete. It now goes through the generated
SessionsService: create_session(email_address:) posts /session.json,
expects the 201's pending token and keeps it; redeem(code:) posts
/session/magic_link.json with {code} and the token as a percent-encoded
Cookie, and answers session_token and requires_signup_completion.
pending_token hands the token out and pending_token: resumes a sign-in
another process started. request_magic_link(email:) and redeem(token:)
are gone; they could not have worked.
MagicLinkFlow posted {email} to /sessions.json and {token} to
/sessions/redeem.json, which Fizzy does not route, decoded a
pendingToken Fizzy never sends, and completed signup with {name} into a
body Fizzy does not answer. It now goes through the generated
SessionsService: start(emailAddress:) posts {email_address} to
/session.json and keeps the 201's pending token, redeem(code:) posts
{code} to /session/magic_link.json with the token as an explicit,
percent-encoded pending_authentication_token cookie and no cookie store
in play, and completeSignup(fullName:sessionToken:) posts {full_name}
and takes the empty 201. pendingToken and resume(pendingToken:) let a
flow finish in another process, as Rust's does.
This changes the flow's API: start(email:) is start(emailAddress:) and
answers PendingAuthentication, redeem(token:) is redeem(code:) and
answers SessionAuthorization, and completeSignup takes fullName and
answers nothing; PendingSession, RedeemResult and SignupResult are gone.
MagicLinkFlow posted to /api/sessions, /api/sessions/redeem and /api/sessions/signup, which Fizzy does not route, redeemed with the pending token in the body where Fizzy wants the emailed code and the token as a cookie, and named a new user with name rather than full_name. No sign-in could complete. The flow now runs on the generated Sessions operations through a FizzyClient, so the paths, bodies, statuses, error mapping, HTTPS check and body limits are the SDK's own: requestMagicLink posts /session.json with email_address and keeps the pending token from the 201; redeemMagicLink takes the code and sends it to /session/magic_link.json with the pending token as a percent-encoded pending_authentication_token cookie; completeSignup posts full_name to /signup/completion.json on the session cookie and takes the empty 201. A sign-in finishing in another process resumes from pendingToken, as a constructor argument or through resume; redeeming with none pending is a usage error. Breaking: redeemMagicLink takes the emailed code, not the pending token, and completeSignup's second argument is the full name.
…code
The flow sent a private {code, token} body so it worked against Fizzy
and against a server built from the model, which spelled the code
token. The model now says code, so the flow sends the generated
RedeemMagicLinkRequestContent, still with the pending cookie the model
documents but cannot send, and the module docs say that instead of
where the model was wrong.
…t's cache The client caches an AccountClient that holds the client, and the account caches the service that holds it, so a client built per request and reached through .sessions was never freed, nor the cookie it signed with. The flow now builds SessionsService(accountClient:) directly, and a test checks a released flow lets go of its transport.
Ruby, Kotlin and Swift kept the pending token after a successful redeem, so pendingToken reported a consumed sign-in and a second redeem sent it to Fizzy instead of failing locally. Go, TypeScript and Rust cleared it unconditionally, which erased a newer sign-in a create started while the redeem was in flight. Each flow now clears the token only if it is still the one it redeemed, and only after success, so a refused code can be retried. Tests assert the spent token and the local usage error on a second redeem.
9eff99c to
d5bb686
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bb686932
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
… state Kotlin's and Ruby's auth strategy read the flow's pending token when the request was signed, after hooks had run, so a sign-in started or resumed meanwhile could pair this code with another's cookie; Fizzy spends the code before refusing the mismatch. Each redeem now signs with a client bound to the token it read, and the pending state sits behind an atomic (Kotlin) or a mutex (Ruby). A Ruby test starts a second sign-in from a hook mid-redeem and checks the cookie and the surviving pending token.
d5bb686 to
e8c9b97
Compare
An older redeem finishing after a newer one overwrote the flow's stored session, so an implicit completeSignup could sign one person's sign-up under another's session. The flow now keeps no session: completeSignup takes the token redeem answered, as every other SDK's flow already does.
The audit was 38 days old, past audit-check's 30. Re-checking each criterion against main found notes that no longer, or never, matched the code: error-body and success-body limits differ by language and three languages have none, resilience is wired into the client only in Go and Rust, hooks never see headers, the fixtures now hold 134 cases naming 73 operations across six runners, and the hand-written magic-link flows in five languages build their own paths, off the spec's, rather than using the generated routes. The notes now say what the code does and the criteria it fails are marked failing, which includes the must-pass 1C.3.
Rust parses *_at fields as chrono DateTime (1B.5); Go's generated types use plain values with omitempty, not pointers (1B.4, now failing); Go's magic-link flow reads error bodies up to 50 MB (2A.6); only parsed server messages are bounded at 500 bytes, and Go's 404 quotes the full URL (3C.3). 2D.5, 3A.5 and 3A.6 are re-read against the rubric's names (resilience configurable, keyring integration, the no-keyring fallback): keyring storage and FIZZY_NO_KEYRING exist in Go only. 1A.6, 1B.2 and 3A.4 were rechecked and still hold, so no criterion is carried forward unread.
TypeScript's error fallback keeps statusText untruncated; 39 of 112 operations have no conformance case of their own; Kotlin's release treats any 409 as a finished publish, so a rerun after a partial upload leaves publications unpublished.
… notes The hand-written magic-link flows declare their own request and response shapes, the same root cause that fails 1C.1 and 1C.3. Go has one generated pointer (Column.Color), and the rate limiter is per client, not per operation.
Rust's flow sends generated types over generated routes through client.operation rather than the generated service; Go's magic-link flow no longer reads error bodies at the success limit; and Go's WithResilience installs gates no request path consults, so 2D.5 fails.
e8c9b97 to
e1515aa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1515aaf9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
Review threads: 19 resolved (18 fixed, 1 declined). |
Stacked on #215. #215 moves every SDK's magic-link flow onto Fizzy's real contract and the generated routes and types. With it underneath, 1B.2, 1C.1 and 1C.3 pass, and
make audit-checkis green: "fresh and must-pass criteria verified". Merge #215 first. GitHub then retargets this PR to main.The history below records how the audit found those failures.
The Rubric Check fails on main because
rubric-audit.jsonis dated 2026-09-09, whichmake audit-checkreports as 38 days old (the limit is 30). The repo has no generator for this file.audit-checkand #161, the last refresh, treat a refresh as re-checking each criterion against the code and updatingdate, and that is what this PR does.This PR does not turn Rubric Check green, and that is deliberate. The re-check found two must-pass criteria false, both since the Go SDK landed and both for the same reason: the hand-written magic-link flows. 1B.2 ("Body types generated") fails because Go, Swift and Kotlin declare their own magic-link request and response shapes. 1C.3 is quoted here:
Generated service paths hold in every language. The hand-written magic-link flows in five languages build their own paths, though, and those paths aren't in
openapi.json. The spec has/session.json,/session/magic_link.jsonand/signup/completion.json:/api/v1/sessions,/api/v1/sessions/redeemgo/pkg/fizzy/magic_link.go:49,98/sessions,/sessions/redeemtypescript/src/auth/magic-link.ts:72,97/sessions,/sessions/redeemruby/lib/fizzy/magic_link_flow.rb:34,43/sessions.json,/sessions/redeem.jsonswift/Sources/Fizzy/Auth/MagicLinkFlow.swift:72,111/api/sessions,/api/sessions/redeem,/api/sessions/signupkotlin/sdk/src/commonMain/kotlin/com/basecamp/fizzy/auth/MagicLinkFlow.kt:89,114,143Only Swift's signup path matches the spec. Rust uses
routes::CREATE_SESSIONandroutes::REDEEM_MAGIC_LINK. Turning the check green by re-dating the file with1C.3: pass: truewould certify a must-pass criterion that the code fails. So the audit records it as failing, and the check stays red on 1C.3 instead of on the date.Decision for @jeremy:
Other notes corrected. Every criterion was rechecked against main at 828edf3, so nothing is carried forward unread. Where the notes and the rubric's criterion names disagreed, I used the names from the sibling hey-sdk audit, which follows the same rubric. That re-read changed 2D.5, 3A.5 and 3A.6.
Now failing:
omitempty, not pointers, so an absent field reads as zero. The other five languages have real optionals.statusText, and Go's 404 message quotes the full URL.Still passing, with corrected notes:
*_atfields aschrono::DateTime<Utc>. The other languages keep ISO 8601 strings, andopenapi.jsonhas noformat: date-time.