The README claimed "no accounts" and "no passwords" but the codebase has POST /api/v1/register and POST /api/v1/login endpoints with bcrypt password hashing. This PR updates the README to accurately describe the dual authentication model.
Changes
Identity & Sessions section
Renamed from "No Accounts" to "Dual Authentication Model"
Documented anonymous sessions (POST /api/v1/session) as the instant-access path
Documented optional account registration (POST /api/v1/register) with password requirements
Documented login (POST /api/v1/login) for returning to registered accounts
Updated rationale to explain why both paths exist
API Reference
Added POST /api/v1/register endpoint documentation: request/response format, field constraints (min 8 char password), error codes, curl example
Added POST /api/v1/login endpoint documentation: request/response format, channel state initialization behavior, error codes, curl example
Security Model → Authentication
Added password hashing details (bcrypt at default cost)
Documented that anonymous sessions have empty password_hash and cannot use /login
Distinguished between anonymous and registered auth paths
Design Principles
Changed principle #2 from "No accounts" to "Accounts optional" with updated description
Schema section
Updated from outdated users table to actual sessions table (with password_hash, signing_key, away_message, uuid columns)
Updated MVP note to document that POST /api/v1/login is the working multi-client mechanism
Client Development Guide
Added register and login curl examples alongside anonymous session creation
Updated error handling and reconnection guidance for registered accounts
Data Lifecycle
Documented that registered sessions persist across logouts (unlike anonymous)
Added client lifecycle documentation
Other
Fixed token storage description (SHA-256 hash, not raw)
Updated "What didn't change" section to reflect optional accounts
closes #36
The README claimed "no accounts" and "no passwords" but the codebase has `POST /api/v1/register` and `POST /api/v1/login` endpoints with bcrypt password hashing. This PR updates the README to accurately describe the dual authentication model.
## Changes
### Identity & Sessions section
- Renamed from "No Accounts" to "Dual Authentication Model"
- Documented anonymous sessions (`POST /api/v1/session`) as the instant-access path
- Documented optional account registration (`POST /api/v1/register`) with password requirements
- Documented login (`POST /api/v1/login`) for returning to registered accounts
- Updated rationale to explain why both paths exist
### API Reference
- Added `POST /api/v1/register` endpoint documentation: request/response format, field constraints (min 8 char password), error codes, curl example
- Added `POST /api/v1/login` endpoint documentation: request/response format, channel state initialization behavior, error codes, curl example
### Security Model → Authentication
- Added password hashing details (bcrypt at default cost)
- Documented that anonymous sessions have empty `password_hash` and cannot use `/login`
- Distinguished between anonymous and registered auth paths
### Design Principles
- Changed principle #2 from "No accounts" to "Accounts optional" with updated description
### Schema section
- Updated from outdated `users` table to actual `sessions` table (with `password_hash`, `signing_key`, `away_message`, `uuid` columns)
- Added `clients` table documentation (session_id FK, token, uuid)
### Session Lifecycle
- Added "Registered Account" flow diagram showing register → use → login-from-new-device
### Multi-Client Model
- Updated MVP note to document that `POST /api/v1/login` is the working multi-client mechanism
### Client Development Guide
- Added register and login curl examples alongside anonymous session creation
- Updated error handling and reconnection guidance for registered accounts
### Data Lifecycle
- Documented that registered sessions persist across logouts (unlike anonymous)
- Added client lifecycle documentation
### Other
- Fixed token storage description (SHA-256 hash, not raw)
- Updated "What didn't change" section to reflect optional accounts
Token generation: 32 random bytes → hex → 64 chars
✅
tokenBytes = 32 in internal/db/queries.go:19, hex.EncodeToString at :32.
Token storage: SHA-256 hash
✅
hashToken at queries.go:37-40 uses sha256.Sum256.
bcrypt at default cost
✅
bcryptCost = bcrypt.DefaultCost in internal/db/auth.go:14.
Anonymous sessions: empty password_hash
✅
CreateSession at queries.go:103 INSERTs without password_hash, default is ''.
Login rejects empty password_hash
✅
if passwordHash == "" check at auth.go:123 returns errNoPassword.
Session persistence for registered accounts
❌WRONG
See critical finding below.
3. Critical Finding: Session Persistence Claim Is Incorrect
The README states in the Data Lifecycle section (line 1980-1985):
Sessions: Anonymous sessions are deleted on QUIT or when all clients have logged out. Registered sessions persist across logouts — the session remains so the user can log in again later.
This is factually incorrect. The code treats registered and anonymous sessions identically on cleanup:
Also unconditional. No distinction between registered and anonymous sessions.
Impact: A registered user who QUITs or whose last client logs out will have their session deleted, making it impossible to log in again. The registration is lost. This contradicts the README's claim that registered sessions "persist across logouts."
This false claim also propagates to:
The Registered Accounts subsection, which implies login can reclaim a session after all clients are gone
The Clients subsection in Data Lifecycle: "A session can have multiple clients; removing one doesn't affect others" — true, but removing the last one deletes the session, which isn't noted
The documentation must describe the actual behavior: both session types are deleted when the last client is removed.
4. Build Result
✅docker build . passes. All tests green. Build completes successfully.
5. Verdict: ❌ FAIL
The PR accurately documents the register/login endpoints, schema, error codes, and security model — all verified line-by-line against source code. However, the session persistence claim for registered accounts is demonstrably false based on the actual cleanup code paths. Since the entire purpose of this PR is to make the README accurately reflect the code (closing a BLOCKER about documentation lies), introducing a new false claim about session behavior is a failing condition.
Required fix: Update the Data Lifecycle section and any related text to state that both anonymous and registered sessions are deleted when the last client is removed (via QUIT or logout). If the intent is for registered sessions to persist, that's a code change for a separate issue — the README should document what the code actually does today.
## Review: [PR #77](https://git.eeqj.de/sneak/chat/pulls/77) — docs: document register/login and dual authentication model
Closing [issue #36](https://git.eeqj.de/sneak/chat/issues/36).
---
### 1. Policy Divergences
| Policy | Status | Notes |
|--------|--------|-------|
| Prettier (proseWrap:always, 80 cols) | ⚠️ Pre-existing | The entire README is not prettier-formatted. This PR follows the same wrap style as existing content. Not introduced here — not blocking. |
| REPO_POLICIES — schema path | ✅ | README correctly references `internal/db/schema/` (matches actual location). |
| Docker build | ✅ | `docker build .` passes — all tests green, build succeeds. |
---
### 2. Accuracy Checklist — Verified Against Source Code
| Section | Status | Details |
|---------|--------|---------|
| API endpoint paths (`/register`, `/login`) | ✅ | Match `internal/server/routes.go` lines 79-86. |
| Register request fields (`nick`, `password`) | ✅ | Match `registerRequest` struct in `internal/handlers/auth.go`. |
| Register response (201, `{id, nick, token}`) | ✅ | Match `respondJSON` call at auth.go:87-90 with `http.StatusCreated`. |
| Register errors (400 invalid nick, 400 short password, 409 nick taken) | ✅ | Exact error strings match auth.go:53, 62, 100. |
| Login request fields (`nick`, `password`) | ✅ | Match `loginRequest` struct in auth.go. |
| Login response (200, `{id, nick, token}`) | ✅ | Match auth.go:191-194 with `http.StatusOK`. |
| Login errors (400 nick/password required, 401 invalid credentials) | ✅ | Match auth.go:162, 174. |
| Login channel state init (MOTD + JOIN/TOPIC/NAMES) | ✅ | `deliverMOTD` + `initChannelState` called in auth.go:183-189. `initChannelState` at api.go:525 enqueues synthetic JOIN + TOPIC/NAMES per channel. |
| Schema: `sessions` table columns | ✅ | Matches `internal/db/schema/001_initial.sql` exactly (id, uuid, nick, password_hash, signing_key, away_message, created_at, last_seen). |
| Schema: `clients` table columns | ✅ | Matches schema (id, uuid, session_id FK, token, created_at, last_seen). |
| Token generation: 32 random bytes → hex → 64 chars | ✅ | `tokenBytes = 32` in `internal/db/queries.go:19`, `hex.EncodeToString` at :32. |
| Token storage: SHA-256 hash | ✅ | `hashToken` at queries.go:37-40 uses `sha256.Sum256`. |
| bcrypt at default cost | ✅ | `bcryptCost = bcrypt.DefaultCost` in `internal/db/auth.go:14`. |
| Anonymous sessions: empty password_hash | ✅ | `CreateSession` at queries.go:103 INSERTs without password_hash, default is `''`. |
| Login rejects empty password_hash | ✅ | `if passwordHash == ""` check at auth.go:123 returns `errNoPassword`. |
| **Session persistence for registered accounts** | ❌ **WRONG** | **See critical finding below.** |
---
### 3. Critical Finding: Session Persistence Claim Is Incorrect
The README states in the **Data Lifecycle** section (line 1980-1985):
> **Sessions**: Anonymous sessions are deleted on `QUIT` or when all clients have logged out. **Registered sessions persist across logouts** — the session remains so the user can log in again later.
This is **factually incorrect**. The code treats registered and anonymous sessions identically on cleanup:
**`handleQuit`** (`internal/handlers/api.go:1796`):
```go
_ = hdlr.params.Database.DeleteSession(
request.Context(), sessionID,
)
```
Unconditional delete. No check for `password_hash`.
**`HandleLogout`** (`internal/handlers/api.go:2411-2415`):
```go
if remaining == 0 {
hdlr.cleanupUser(
ctx, sessionID, nick,
)
}
```
**`cleanupUser`** (`internal/handlers/api.go:2467`):
```go
_ = hdlr.params.Database.DeleteSession(ctx, sessionID)
```
Also unconditional. No distinction between registered and anonymous sessions.
**Impact:** A registered user who QUITs or whose last client logs out will have their session **deleted**, making it impossible to log in again. The registration is lost. This contradicts the README's claim that registered sessions "persist across logouts."
This false claim also propagates to:
- The **Registered Accounts** subsection, which implies login can reclaim a session after all clients are gone
- The **Clients** subsection in Data Lifecycle: "A session can have multiple clients; removing one doesn't affect others" — true, but removing the *last* one deletes the session, which isn't noted
The documentation must describe the **actual** behavior: both session types are deleted when the last client is removed.
---
### 4. Build Result
✅ `docker build .` passes. All tests green. Build completes successfully.
---
### 5. Verdict: ❌ FAIL
The PR accurately documents the register/login endpoints, schema, error codes, and security model — all verified line-by-line against source code. However, the session persistence claim for registered accounts is demonstrably false based on the actual cleanup code paths. Since the entire purpose of this PR is to make the README accurately reflect the code (closing a BLOCKER about documentation lies), introducing a new false claim about session behavior is a failing condition.
**Required fix:** Update the Data Lifecycle section and any related text to state that both anonymous and registered sessions are deleted when the last client is removed (via QUIT or logout). If the *intent* is for registered sessions to persist, that's a code change for a separate issue — the README should document what the code actually does today.
The README incorrectly claimed that registered sessions persist across
logouts. The actual code (handleQuit and cleanupUser in api.go) deletes
ALL sessions unconditionally when the last client disconnects — no check
for password_hash.
Updated Data Lifecycle, Registered Accounts, Identity & Sessions rationale,
flow diagram, and Design Principles to accurately state that both anonymous
and registered sessions are deleted on QUIT or last-client-logout.
Registration enables multi-client access (login from another device while
session is active), not session persistence across all-client removal.
The reviewer correctly identified that the README falsely claimed "Registered sessions persist across logouts" while the actual code (handleQuit at api.go:1796 and cleanupUser at api.go:2467) deletes ALL sessions unconditionally — no check for password_hash.
Changes made
Data Lifecycle → Sessions: Replaced the false claim with accurate text stating both anonymous and registered sessions are deleted on QUIT or when the last client logs out. Explicitly notes that handleQuit and cleanupUser call DeleteSession unconditionally.
Data Lifecycle → Clients: Added note that removing the last client triggers full session deletion (parted from channels, QUIT broadcast, nick released).
Registered Accounts subsection: Changed "persistent identity across sessions" to "multi-client access" and added explicit note that login only works while the session still exists.
Identity & Sessions rationale: Replaced "keep their nick across sessions" framing with "access the same session from multiple devices" and added note that registration does not make sessions survive all-client removal.
Registered Account flow diagram: Changed "Later, from a new device or after token expiry" to "From another device, while session is still active" and added note about session non-existence after all-client logout.
Design Principles: Changed "persistent identity" to "multi-client access".
Curl examples: Changed comment from "persistent identity" to "multi-client support".
All changes describe what the code actually does today. If session persistence for registered accounts is desired, that's a code change for a separate issue.
## Rework: Fixed false session persistence claim
The reviewer correctly identified that the README falsely claimed "Registered sessions persist across logouts" while the actual code (`handleQuit` at api.go:1796 and `cleanupUser` at api.go:2467) deletes ALL sessions unconditionally — no check for `password_hash`.
### Changes made
**Data Lifecycle → Sessions**: Replaced the false claim with accurate text stating both anonymous and registered sessions are deleted on QUIT or when the last client logs out. Explicitly notes that `handleQuit` and `cleanupUser` call `DeleteSession` unconditionally.
**Data Lifecycle → Clients**: Added note that removing the last client triggers full session deletion (parted from channels, QUIT broadcast, nick released).
**Registered Accounts subsection**: Changed "persistent identity across sessions" to "multi-client access" and added explicit note that login only works while the session still exists.
**Identity & Sessions rationale**: Replaced "keep their nick across sessions" framing with "access the same session from multiple devices" and added note that registration does not make sessions survive all-client removal.
**Registered Account flow diagram**: Changed "Later, from a new device or after token expiry" to "From another device, while session is still active" and added note about session non-existence after all-client logout.
**Design Principles**: Changed "persistent identity" to "multi-client access".
**Curl examples**: Changed comment from "persistent identity" to "multi-client support".
All changes describe what the code actually does today. If session persistence for registered accounts is desired, that's a code change for a separate issue.
The first review FAILED because the README falsely claimed "Registered sessions persist across logouts" while the code unconditionally deletes all sessions. The rework addressed this in six places:
Section
Fix Applied
Verified
Data Lifecycle → Sessions
Replaced false persistence claim with accurate "both types deleted unconditionally" language referencing handleQuit and cleanupUser
The /register endpoint intro (line 1100-1101) states:
The nick is claimed and can be reclaimed later via POST /api/v1/login.
"Reclaimed later" implies the nick can be recovered after the session is gone — which is false. If all clients disconnect or the user sends QUIT, DeleteSession is called unconditionally (api.go:1796, api.go:2467), the nick is released, and LoginUser will fail with "get session for login" because the sessions row no longer exists.
The word "reclaimed" implies getting something back that was lost. But login doesn't reclaim a nick — it adds a client to an existing session that already holds the nick. The nick was never lost. And if the session IS gone, reclamation is impossible.
This is the same class of misleading statement the first review caught. The fix is a one-sentence rewrite, e.g.:
The password enables login from additional clients via POST /api/v1/login while the session remains active.
Line 205 (120 chars): logout); registration does not make a session survive all-client removal. Identity verification at the message layer via — should wrap after "removal."
Line 2592 (115 chars): with password is available — but never required. Identity verification at the message layer uses cryptographic — should wrap after "required."
Both appear to be wrapping accidents where two sentences ended up on one line. The previous review noted 80-column compliance is a pre-existing README-wide issue, but these two lines were introduced by this PR and are easily fixable. Not blocking on its own, but should be fixed while the register description is being corrected.
5. Build Result
✅docker build . passes. All tests green.
6. Verdict: ❌ FAIL
The rework was thorough and correctly fixed the major false persistence claim in six locations. All endpoint documentation, schema references, error codes, and security model details are verified accurate against source code.
However, the /register endpoint description still contains "The nick is claimed and can be reclaimed later via POST /api/v1/login" — which implies post-disconnect nick recovery that doesn't work. This is the same class of misleading statement that caused the first review failure. Since the entire purpose of this PR is to make the README accurately describe the code, a remaining misleading statement about session lifecycle is a failing condition.
Required fix:
Reword the register endpoint intro to remove the "reclaimed later" implication. Suggested: "The password enables login from additional clients via POST /api/v1/login while the session remains active."
Fix the two line-length overruns at lines 205 and 2592 (wrap after the period).
## Review: [PR #77](https://git.eeqj.de/sneak/chat/pulls/77) — docs: document register/login and dual authentication model (post-rework)
Closing [issue #36](https://git.eeqj.de/sneak/chat/issues/36).
---
### 1. Rework Verification — Session Persistence Fix
The first review [FAILED](https://git.eeqj.de/sneak/chat/pulls/77#issuecomment-11753) because the README falsely claimed "Registered sessions persist across logouts" while the code unconditionally deletes all sessions. The rework addressed this in **six places**:
| Section | Fix Applied | Verified |
|---------|-------------|----------|
| Data Lifecycle → Sessions | Replaced false persistence claim with accurate "both types deleted unconditionally" language referencing `handleQuit` and `cleanupUser` | ✅ Matches [api.go:1796](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L1796) and [api.go:2467](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L2467) |
| Data Lifecycle → Clients | Added note that last-client removal triggers full session deletion | ✅ Matches [api.go:2410](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L2410) (`remaining == 0` → `cleanupUser`) |
| Identity & Sessions → Registered Accounts | Changed "persistent identity" to "multi-client access", added explicit "login only works while session still exists" note | ✅ Accurate |
| Identity & Sessions → Rationale | Replaced "keep their nick across sessions" with "access the same session from multiple devices", added non-persistence caveat | ✅ Accurate |
| Registered Account flow diagram | Changed to "while session is still active" with note about session non-existence | ✅ Accurate |
| Design Principles | Changed "persistent identity" to "multi-client access" | ✅ Accurate |
The rework substantively corrected the critical finding. These six sections now accurately describe the code's behavior.
---
### 2. Remaining Accuracy Issue — Register Endpoint Description
The `/register` endpoint intro (line 1100-1101) states:
> The nick is claimed and can be reclaimed later via `POST /api/v1/login`.
"Reclaimed later" implies the nick can be recovered after the session is gone — which is false. If all clients disconnect or the user sends QUIT, `DeleteSession` is called unconditionally ([api.go:1796](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L1796), [api.go:2467](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L2467)), the nick is released, and `LoginUser` will fail with "get session for login" because the `sessions` row no longer exists.
The word "reclaimed" implies getting something back that was lost. But login doesn't reclaim a nick — it adds a client to an **existing** session that already holds the nick. The nick was never lost. And if the session IS gone, reclamation is impossible.
This is the same class of misleading statement the first review caught. The fix is a one-sentence rewrite, e.g.:
> The password enables login from additional clients via `POST /api/v1/login` while the session remains active.
---
### 3. Full Accuracy Checklist (All Other Claims)
| Claim | Status | Source |
|-------|--------|--------|
| API paths: `/register`, `/login` | ✅ | [routes.go:79-86](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/server/routes.go#L79) |
| Register request: `{nick, password}` | ✅ | [auth.go `registerRequest` struct](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L36) |
| Register response: 201, `{id, nick, token}` | ✅ | [auth.go:87-90](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L87) |
| Register errors (400 invalid nick, 400 short password, 409 nick taken) | ✅ | [auth.go:53, 62, 100](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L53) |
| Login request: `{nick, password}` | ✅ | [auth.go `loginRequest` struct](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L130) |
| Login response: 200, `{id, nick, token}` | ✅ | [auth.go:191-194](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L191) |
| Login errors (400 nick/password required, 401 invalid credentials) | ✅ | [auth.go:162, 174](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L162) |
| Login channel state init (MOTD + JOIN/TOPIC/NAMES) | ✅ | [auth.go:183-189](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/auth.go#L183), [api.go:525 `initChannelState`](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L525) |
| Schema: `sessions` table columns | ✅ | [001_initial.sql](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/schema/001_initial.sql#L5) |
| Schema: `clients` table columns | ✅ | [001_initial.sql](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/schema/001_initial.sql#L17) |
| Token: 32 random bytes → hex → 64 chars | ✅ | [queries.go:19, 32](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/queries.go#L19) |
| Token storage: SHA-256 hash | ✅ | [queries.go:37-40](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/queries.go#L37) |
| bcrypt at default cost | ✅ | [auth.go:13](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/auth.go#L13) |
| Anonymous sessions: empty `password_hash` | ✅ | [queries.go:103](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/queries.go#L103) (INSERT without `password_hash`, DEFAULT `''`) |
| Login rejects empty `password_hash` | ✅ | [auth.go:123](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/db/auth.go#L123) |
| Idle timeout: default 30 days | ✅ | [handlers.go:35](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/handlers.go#L35) |
| Session deletion unconditional (both types) | ✅ | Confirmed in rework verification above |
| Nick regex: 1-32 chars | ✅ | [api.go:18-20](https://git.eeqj.de/sneak/chat/src/branch/docs/update-readme-auth-model/internal/handlers/api.go#L18) (`{0,31}` after first char = max 32) |
---
### 4. Line Length (Minor)
Two new prose lines exceed 80 columns:
- **Line 205** (120 chars): `logout); registration does not make a session survive all-client removal. Identity verification at the message layer via` — should wrap after "removal."
- **Line 2592** (115 chars): `with password is available — but never required. Identity verification at the message layer uses cryptographic` — should wrap after "required."
Both appear to be wrapping accidents where two sentences ended up on one line. The previous review noted 80-column compliance is a pre-existing README-wide issue, but these two lines were **introduced by this PR** and are easily fixable. Not blocking on its own, but should be fixed while the register description is being corrected.
---
### 5. Build Result
✅ `docker build .` passes. All tests green.
---
### 6. Verdict: ❌ FAIL
The rework was thorough and correctly fixed the major false persistence claim in six locations. All endpoint documentation, schema references, error codes, and security model details are verified accurate against source code.
However, the `/register` endpoint description still contains "The nick is claimed and can be reclaimed later via `POST /api/v1/login`" — which implies post-disconnect nick recovery that doesn't work. This is the same class of misleading statement that caused the first review failure. Since the entire purpose of this PR is to make the README accurately describe the code, a remaining misleading statement about session lifecycle is a failing condition.
**Required fix:**
1. Reword the register endpoint intro to remove the "reclaimed later" implication. Suggested: *"The password enables login from additional clients via `POST /api/v1/login` while the session remains active."*
2. Fix the two line-length overruns at lines 205 and 2592 (wrap after the period).
- Register endpoint: replace 'nick is claimed and can be reclaimed
later' with accurate description that login adds clients while
session remains active
- Wrap line 205 (120 chars) after 'all-client removal.'
- Wrap line 2592 (115 chars) after 'never required.'
1. Register endpoint description (line ~1100): Replaced "The nick is claimed and can be reclaimed later via POST /api/v1/login" with accurate text: "The password enables login from additional clients via POST /api/v1/login while the session remains active." This removes the false implication that a nick can be recovered after all clients disconnect.
2. Line 205 wrap (was 120 chars): Wrapped after "all-client removal." — now 4 lines within 80 columns.
3. Line ~2592 wrap (was 115 chars): Wrapped after "never required." — now 3 lines within 80 columns.
Full README grep for remaining misleading claims
Searched for persist, reclaim, survive, recover, permanent, durable across the entire README:
Line 7: "persistent sessions" — refers to HTTP vs TCP architecture, accurate
Lines 89-90: "sessions persist independently of connections" — HTTP polling model description, accurate
Line 167: "no way to reclaim it" — about anonymous sessions, accurate
Line 205: "does not make a session survive" — our fix, explicitly denies persistence
Line 325: "persistent channel registration" — about channels, not sessions
No remaining false claims about session persistence or reclamation.
Build
✅docker build . passes — all tests green, build succeeds.
## Rework (2nd): Fixed false "reclaimed later" claim and line-length issues
Addressed both findings from the [second review](https://git.eeqj.de/sneak/chat/pulls/77#issuecomment-11757).
### Changes
**1. Register endpoint description** (line ~1100): Replaced "The nick is claimed and can be reclaimed later via `POST /api/v1/login`" with accurate text: "The password enables login from additional clients via `POST /api/v1/login` while the session remains active." This removes the false implication that a nick can be recovered after all clients disconnect.
**2. Line 205 wrap** (was 120 chars): Wrapped after "all-client removal." — now 4 lines within 80 columns.
**3. Line ~2592 wrap** (was 115 chars): Wrapped after "never required." — now 3 lines within 80 columns.
### Full README grep for remaining misleading claims
Searched for `persist`, `reclaim`, `survive`, `recover`, `permanent`, `durable` across the entire README:
- Line 7: "persistent sessions" — refers to HTTP vs TCP architecture, accurate
- Lines 89-90: "sessions persist independently of connections" — HTTP polling model description, accurate
- Line 167: "no way to reclaim it" — about anonymous sessions, accurate
- Line 205: "does not make a session survive" — our fix, explicitly denies persistence
- Line 325: "persistent channel registration" — about channels, not sessions
No remaining false claims about session persistence or reclamation.
### Build
✅ `docker build .` passes — all tests green, build succeeds.
handleQuit (api.go:1796) and cleanupUser (api.go:2467) — no password_hash check
Last-client removal triggers session deletion
✅
remaining == 0 → cleanupUser (api.go:2409)
Registered account flow diagram says "while session is still active"
✅
Accurate
Reconnection guidance distinguishes anonymous vs registered
✅
Accurate
5. Build Result
✅docker build . passes. Lint, fmt-check, tests, and build all succeed.
6. Verdict: ✅ PASS
All three findings from the previous two reviews are resolved:
The false "registered sessions persist across logouts" claim — removed and replaced with accurate text in 6+ locations across the first rework
The false "reclaimed later" implication in /register — rewritten to "while the session remains active"
The two line-length overruns — both wrapped well within 80 columns
The full-README grep for persistence/reclaim keywords returns zero false claims. Every documentation statement has been verified against the actual source code. The PR accurately documents the register/login endpoints, dual authentication model, schema, error codes, security model, and session lifecycle — including the critical fact that both session types are deleted unconditionally on cleanup.
## Review: [PR #77](https://git.eeqj.de/sneak/chat/pulls/77) — docs: document register/login and dual authentication model (2nd rework)
Closing [issue #36](https://git.eeqj.de/sneak/chat/issues/36).
---
### 1. Critical Check: Session Persistence/Reclaim Claims
Grepped the entire README for `persist`, `reclaim`, `survive`, `recover`, `later`, `permanent`, `durable`. Every match verified:
| Line | Text | Context | Accurate? |
|------|------|---------|-----------|
| 7 | "persistent sessions" | HTTP transport description (sessions live in DB, not tied to TCP) | ✅ |
| 73/75 | "persistent TCP" | Describing traditional IRC's TCP requirement | ✅ |
| 89-90 | "sessions persist independently of connections... survive disconnects" | HTTP vs TCP transport model — sessions survive *network interruptions*, not *all-client logout* | ✅ |
| 167 | "no way to reclaim it" | Anonymous sessions after QUIT — accurately states nick is lost | ✅ |
| 205 | "registration does not make a session survive all-client removal" | Explicitly denies false persistence | ✅ |
| 326 | "persistent channel registration" | About channels, not sessions | ✅ |
| 2606 | "persistent connections" | About TCP connections | ✅ |
**No remaining false claims about session persistence or nick reclamation.** The recurring failure class from reviews 1 and 2 is fully resolved.
---
### 2. Rework Verification — "reclaimed later" Fix
The `/register` endpoint intro (line ~1101) now reads:
> The password enables login from additional clients via `POST /api/v1/login` while the session remains active.
The false "reclaimed later" implication is gone. ✅
---
### 3. Rework Verification — Line Length Fixes
| Line | Previous | Now | Status |
|------|----------|-----|--------|
| 205 | 120 chars | 64 chars | ✅ Fixed |
| ~2594 | 115 chars | 62 chars | ✅ Fixed |
All new prose lines introduced by this PR are within 80 columns. Lines over 80 in new content are exclusively in markdown tables (unavoidable). ✅
---
### 4. Full Accuracy Checklist — Verified Against Source Code
| Claim | Status | Source |
|-------|--------|--------|
| API paths: `/register`, `/login` | ✅ | `internal/server/routes.go` |
| Register request: `{nick, password}` | ✅ | `registerRequest` struct in `auth.go` |
| Register response: 201, `{id, nick, token}` | ✅ | `auth.go` `respondJSON` with `http.StatusCreated` |
| Register errors (400 invalid nick, 400 short password, 409 nick taken) | ✅ | `auth.go` error paths |
| Login request: `{nick, password}` | ✅ | `loginRequest` struct in `auth.go` |
| Login response: 200, `{id, nick, token}` | ✅ | `auth.go` `respondJSON` with `http.StatusOK` |
| Login errors (400 nick/password required, 401 invalid credentials) | ✅ | `auth.go` error paths |
| Login calls `deliverMOTD` + `initChannelState` | ✅ | `auth.go` login handler |
| Schema: `sessions` table columns | ✅ | `001_initial.sql` (id, uuid, nick, password_hash, signing_key, away_message, created_at, last_seen) |
| Schema: `clients` table (session_id FK, cascade delete) | ✅ | `001_initial.sql` |
| Token: 32 random bytes → hex → 64 chars | ✅ | `queries.go` |
| Token storage: SHA-256 hash | ✅ | `queries.go` `hashToken` |
| bcrypt at default cost | ✅ | `internal/db/auth.go` |
| Anonymous sessions: empty `password_hash` | ✅ | `queries.go` INSERT default |
| Min password length: 8 chars | ✅ | `const minPasswordLength = 8` in `auth.go` |
| Session deletion unconditional (both types) | ✅ | `handleQuit` (api.go:1796) and `cleanupUser` (api.go:2467) — no `password_hash` check |
| Last-client removal triggers session deletion | ✅ | `remaining == 0` → `cleanupUser` (api.go:2409) |
| Registered account flow diagram says "while session is still active" | ✅ | Accurate |
| Reconnection guidance distinguishes anonymous vs registered | ✅ | Accurate |
---
### 5. Build Result
✅ `docker build .` passes. Lint, fmt-check, tests, and build all succeed.
---
### 6. Verdict: ✅ PASS
All three findings from the previous two reviews are resolved:
1. The false "registered sessions persist across logouts" claim — removed and replaced with accurate text in 6+ locations across the first rework
2. The false "reclaimed later" implication in `/register` — rewritten to "while the session remains active"
3. The two line-length overruns — both wrapped well within 80 columns
The full-README grep for persistence/reclaim keywords returns zero false claims. Every documentation statement has been verified against the actual source code. The PR accurately documents the register/login endpoints, dual authentication model, schema, error codes, security model, and session lifecycle — including the critical fact that both session types are deleted unconditionally on cleanup.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
closes #36
The README claimed "no accounts" and "no passwords" but the codebase has
POST /api/v1/registerandPOST /api/v1/loginendpoints with bcrypt password hashing. This PR updates the README to accurately describe the dual authentication model.Changes
Identity & Sessions section
POST /api/v1/session) as the instant-access pathPOST /api/v1/register) with password requirementsPOST /api/v1/login) for returning to registered accountsAPI Reference
POST /api/v1/registerendpoint documentation: request/response format, field constraints (min 8 char password), error codes, curl examplePOST /api/v1/loginendpoint documentation: request/response format, channel state initialization behavior, error codes, curl exampleSecurity Model → Authentication
password_hashand cannot use/loginDesign Principles
Schema section
userstable to actualsessionstable (withpassword_hash,signing_key,away_message,uuidcolumns)clientstable documentation (session_id FK, token, uuid)Session Lifecycle
Multi-Client Model
POST /api/v1/loginis the working multi-client mechanismClient Development Guide
Data Lifecycle
Other
Review: PR #77 — docs: document register/login and dual authentication model
Closing issue #36.
1. Policy Divergences
internal/db/schema/(matches actual location).docker build .passes — all tests green, build succeeds.2. Accuracy Checklist — Verified Against Source Code
/register,/login)internal/server/routes.golines 79-86.nick,password)registerRequeststruct ininternal/handlers/auth.go.{id, nick, token})respondJSONcall at auth.go:87-90 withhttp.StatusCreated.nick,password)loginRequeststruct in auth.go.{id, nick, token})http.StatusOK.deliverMOTD+initChannelStatecalled in auth.go:183-189.initChannelStateat api.go:525 enqueues synthetic JOIN + TOPIC/NAMES per channel.sessionstable columnsinternal/db/schema/001_initial.sqlexactly (id, uuid, nick, password_hash, signing_key, away_message, created_at, last_seen).clientstable columnstokenBytes = 32ininternal/db/queries.go:19,hex.EncodeToStringat :32.hashTokenat queries.go:37-40 usessha256.Sum256.bcryptCost = bcrypt.DefaultCostininternal/db/auth.go:14.CreateSessionat queries.go:103 INSERTs without password_hash, default is''.if passwordHash == ""check at auth.go:123 returnserrNoPassword.3. Critical Finding: Session Persistence Claim Is Incorrect
The README states in the Data Lifecycle section (line 1980-1985):
This is factually incorrect. The code treats registered and anonymous sessions identically on cleanup:
handleQuit(internal/handlers/api.go:1796):Unconditional delete. No check for
password_hash.HandleLogout(internal/handlers/api.go:2411-2415):cleanupUser(internal/handlers/api.go:2467):Also unconditional. No distinction between registered and anonymous sessions.
Impact: A registered user who QUITs or whose last client logs out will have their session deleted, making it impossible to log in again. The registration is lost. This contradicts the README's claim that registered sessions "persist across logouts."
This false claim also propagates to:
The documentation must describe the actual behavior: both session types are deleted when the last client is removed.
4. Build Result
✅
docker build .passes. All tests green. Build completes successfully.5. Verdict: ❌ FAIL
The PR accurately documents the register/login endpoints, schema, error codes, and security model — all verified line-by-line against source code. However, the session persistence claim for registered accounts is demonstrably false based on the actual cleanup code paths. Since the entire purpose of this PR is to make the README accurately reflect the code (closing a BLOCKER about documentation lies), introducing a new false claim about session behavior is a failing condition.
Required fix: Update the Data Lifecycle section and any related text to state that both anonymous and registered sessions are deleted when the last client is removed (via QUIT or logout). If the intent is for registered sessions to persist, that's a code change for a separate issue — the README should document what the code actually does today.
Rework: Fixed false session persistence claim
The reviewer correctly identified that the README falsely claimed "Registered sessions persist across logouts" while the actual code (
handleQuitat api.go:1796 andcleanupUserat api.go:2467) deletes ALL sessions unconditionally — no check forpassword_hash.Changes made
Data Lifecycle → Sessions: Replaced the false claim with accurate text stating both anonymous and registered sessions are deleted on QUIT or when the last client logs out. Explicitly notes that
handleQuitandcleanupUsercallDeleteSessionunconditionally.Data Lifecycle → Clients: Added note that removing the last client triggers full session deletion (parted from channels, QUIT broadcast, nick released).
Registered Accounts subsection: Changed "persistent identity across sessions" to "multi-client access" and added explicit note that login only works while the session still exists.
Identity & Sessions rationale: Replaced "keep their nick across sessions" framing with "access the same session from multiple devices" and added note that registration does not make sessions survive all-client removal.
Registered Account flow diagram: Changed "Later, from a new device or after token expiry" to "From another device, while session is still active" and added note about session non-existence after all-client logout.
Design Principles: Changed "persistent identity" to "multi-client access".
Curl examples: Changed comment from "persistent identity" to "multi-client support".
All changes describe what the code actually does today. If session persistence for registered accounts is desired, that's a code change for a separate issue.
Review: PR #77 — docs: document register/login and dual authentication model (post-rework)
Closing issue #36.
1. Rework Verification — Session Persistence Fix
The first review FAILED because the README falsely claimed "Registered sessions persist across logouts" while the code unconditionally deletes all sessions. The rework addressed this in six places:
handleQuitandcleanupUserremaining == 0→cleanupUser)The rework substantively corrected the critical finding. These six sections now accurately describe the code's behavior.
2. Remaining Accuracy Issue — Register Endpoint Description
The
/registerendpoint intro (line 1100-1101) states:"Reclaimed later" implies the nick can be recovered after the session is gone — which is false. If all clients disconnect or the user sends QUIT,
DeleteSessionis called unconditionally (api.go:1796, api.go:2467), the nick is released, andLoginUserwill fail with "get session for login" because thesessionsrow no longer exists.The word "reclaimed" implies getting something back that was lost. But login doesn't reclaim a nick — it adds a client to an existing session that already holds the nick. The nick was never lost. And if the session IS gone, reclamation is impossible.
This is the same class of misleading statement the first review caught. The fix is a one-sentence rewrite, e.g.:
3. Full Accuracy Checklist (All Other Claims)
/register,/login{nick, password}registerRequeststruct{id, nick, token}{nick, password}loginRequeststruct{id, nick, token}initChannelStatesessionstable columnsclientstable columnspassword_hashpassword_hash, DEFAULT'')password_hash{0,31}after first char = max 32)4. Line Length (Minor)
Two new prose lines exceed 80 columns:
logout); registration does not make a session survive all-client removal. Identity verification at the message layer via— should wrap after "removal."with password is available — but never required. Identity verification at the message layer uses cryptographic— should wrap after "required."Both appear to be wrapping accidents where two sentences ended up on one line. The previous review noted 80-column compliance is a pre-existing README-wide issue, but these two lines were introduced by this PR and are easily fixable. Not blocking on its own, but should be fixed while the register description is being corrected.
5. Build Result
✅
docker build .passes. All tests green.6. Verdict: ❌ FAIL
The rework was thorough and correctly fixed the major false persistence claim in six locations. All endpoint documentation, schema references, error codes, and security model details are verified accurate against source code.
However, the
/registerendpoint description still contains "The nick is claimed and can be reclaimed later viaPOST /api/v1/login" — which implies post-disconnect nick recovery that doesn't work. This is the same class of misleading statement that caused the first review failure. Since the entire purpose of this PR is to make the README accurately describe the code, a remaining misleading statement about session lifecycle is a failing condition.Required fix:
POST /api/v1/loginwhile the session remains active."Rework (2nd): Fixed false "reclaimed later" claim and line-length issues
Addressed both findings from the second review.
Changes
1. Register endpoint description (line ~1100): Replaced "The nick is claimed and can be reclaimed later via
POST /api/v1/login" with accurate text: "The password enables login from additional clients viaPOST /api/v1/loginwhile the session remains active." This removes the false implication that a nick can be recovered after all clients disconnect.2. Line 205 wrap (was 120 chars): Wrapped after "all-client removal." — now 4 lines within 80 columns.
3. Line ~2592 wrap (was 115 chars): Wrapped after "never required." — now 3 lines within 80 columns.
Full README grep for remaining misleading claims
Searched for
persist,reclaim,survive,recover,permanent,durableacross the entire README:No remaining false claims about session persistence or reclamation.
Build
✅
docker build .passes — all tests green, build succeeds.Review: PR #77 — docs: document register/login and dual authentication model (2nd rework)
Closing issue #36.
1. Critical Check: Session Persistence/Reclaim Claims
Grepped the entire README for
persist,reclaim,survive,recover,later,permanent,durable. Every match verified:No remaining false claims about session persistence or nick reclamation. The recurring failure class from reviews 1 and 2 is fully resolved.
2. Rework Verification — "reclaimed later" Fix
The
/registerendpoint intro (line ~1101) now reads:The false "reclaimed later" implication is gone. ✅
3. Rework Verification — Line Length Fixes
All new prose lines introduced by this PR are within 80 columns. Lines over 80 in new content are exclusively in markdown tables (unavoidable). ✅
4. Full Accuracy Checklist — Verified Against Source Code
/register,/logininternal/server/routes.go{nick, password}registerRequeststruct inauth.go{id, nick, token}auth.gorespondJSONwithhttp.StatusCreatedauth.goerror paths{nick, password}loginRequeststruct inauth.go{id, nick, token}auth.gorespondJSONwithhttp.StatusOKauth.goerror pathsdeliverMOTD+initChannelStateauth.gologin handlersessionstable columns001_initial.sql(id, uuid, nick, password_hash, signing_key, away_message, created_at, last_seen)clientstable (session_id FK, cascade delete)001_initial.sqlqueries.goqueries.gohashTokeninternal/db/auth.gopassword_hashqueries.goINSERT defaultconst minPasswordLength = 8inauth.gohandleQuit(api.go:1796) andcleanupUser(api.go:2467) — nopassword_hashcheckremaining == 0→cleanupUser(api.go:2409)5. Build Result
✅
docker build .passes. Lint, fmt-check, tests, and build all succeed.6. Verdict: ✅ PASS
All three findings from the previous two reviews are resolved:
/register— rewritten to "while the session remains active"The full-README grep for persistence/reclaim keywords returns zero false claims. Every documentation statement has been verified against the actual source code. The PR accurately documents the register/login endpoints, dual authentication model, schema, error codes, security model, and session lifecycle — including the critical fact that both session types are deleted unconditionally on cleanup.