feat: send + cache verified author identity on share#371
Merged
tomasz-tomczyk merged 1 commit intomainfrom Apr 27, 2026
Merged
Conversation
6 tasks
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (54.40%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #371 +/- ##
==========================================
- Coverage 67.10% 66.66% -0.45%
==========================================
Files 18 18
Lines 7823 7964 +141
==========================================
+ Hits 5250 5309 +59
- Misses 2181 2257 +76
- Partials 392 398 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds proper author attribution for the crit -> crit-web share flow. Each comment now carries: - author_display_name (cosmetic): falls back to cfg.Author when the local comment has no author set; resolves friendlier GitHub display names (cached gh api users/<login>) instead of bare logins. - user_id (verified identity): stamped at write time from cfg.AuthUserID and serialized in the share payload. The server is the source of truth on whether to persist it. Other changes: - ~/.crit.config.json gains auth_user_id alongside name/email; lazy- backfilled via /api/auth/whoami on first share if missing (5s timeout). Cleared atomically alongside the token via clearAuthIdentity on logout or 401. - 401 propagation: shareFilesToWeb / upsertShareToWeb / fetchWebComments return errShareUnauthorized; callers clear cached identity and exit. - Initial POST now sends external_id so subsequent PUT upserts can match existing rows server-side. - mergeWebComments preserves fetched user_id verbatim — never overwrites with the local sharer's id. - Server auth state is now mutex-guarded; config writes go through the existing atomicWriteFile (temp + fsync + rename) so a crash mid-write can't truncate the bearer token. Backwards-compatible: omitempty on UserID means old .crit.json files deserialise cleanly and old CLIs talking to a new server resolve to anon. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b9adba6 to
ba9b7fd
Compare
This was referenced Apr 29, 2026
tomasz-tomczyk
added a commit
that referenced
this pull request
May 5, 2026
* fix: atomically rewrite auth_token + identity on login Login persisted credentials in two separate saveGlobalConfig calls (saveAuthToken then saveAuthIdentity), leaving a window where a crash between writes could pair a fresh token with stale identity from the prior account — the failure mode that surfaced #371. Collapse them into one saveAuthSession closure that writes all four auth_* fields in a single atomic temp+rename. Test plan - Unit: TestSaveAuthSession_AtomicRewrite, _RemovesFieldsAbsentFromResponse - Integration (e2e-share): TestAuthSessionReLoginEndToEnd seeds two users on crit-web, persists user A then user B via saveAuthSession, runs `crit share` with HOME pointed at the test home (no CRIT_AUTH_TOKEN env override), and asserts the server stamps user B on the resulting comment. * refactor: drop dead saveAuthToken / saveAuthIdentity / removeAuthToken These three helpers no longer have any production callers — saveAuthSession is the sole login-persistence path, clearAuthIdentity owns logout/401, and lazyBackfillAuthUserID writes its own narrower closure. The remaining tests exercised them in isolation, which means they passed forever regardless of whether anything called them. Repoint the two TestSaveGlobalConfig_* cases at saveAuthSession (which still exercises saveGlobalConfig under the hood), and drop TestSaveAndRemoveAuthToken plus the TestSaveAuthIdentity_* pair — TestSaveAuthSession_AtomicRewrite and _RemovesFieldsAbsentFromResponse cover the same ground plus auth_token.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
author_display_name(cfg.Author / git / GitHub / "Anonymous" fallback) and a verifieduser_id(stamped from cachedcfg.AuthUserID). The server is the source of truth on whether to persistuser_id.~/.crit.config.jsongainsauth_user_idalongside name/email; lazy-backfilled via/api/auth/whoamion first share if missing (5s timeout). Cleared atomically alongside the token viaclearAuthIdentityon logout or 401.errShareUnauthorizedpropagates fromshareFilesToWeb/upsertShareToWeb/fetchWebComments; callers clear cached identity and exit.external_idso subsequent PUT upserts can match existing rows server-side.mergeWebCommentspreserves fetcheduser_idverbatim — never overwrites with the local sharer's id.atomicWriteFile(temp + fsync + rename) so a crash mid-write can't truncate the bearer token.gh api users/{login}lookups useexec.CommandContextwith a 10s timeout to avoid hangs.Backwards-compatible:
omitemptyonUserIDmeans old.crit.jsonfiles deserialize cleanly and old CLIs talking to a new server resolve to anonymous.Review
Test plan
go test -race -count=1 ./...— cleangofmt -l .— clean./scripts/e2e-share.sh) — all 24 attribution + share-sync tests passSee also: tomasz-tomczyk/crit-web#131 (server side)
🤖 Generated with Claude Code