From d8a7b2a95f8b51762020794eb230aea2932ed16f Mon Sep 17 00:00:00 2001
From: Andrew Lee <andklee@amazon.com>
Date: Mon, 13 Jul 2026 16:45:39 +0000
Subject: [PATCH] =?UTF-8?q?fix(sync):=20address=20PR=20review=20=E2=80=94?=
 =?UTF-8?q?=20https=20enforcement,=20keychain=20test=20isolation,=20golden?=
 =?UTF-8?q?=20ID=20pins?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before-merge items:
1. HTTPS enforced on every remote endpoint (RFC 8252 §8.3): baseUrl,
   issuer, authorization/token/revocation endpoints, and the traces
   endpoint all reject non-https, with a loopback (127.0.0.1/::1/
   localhost) exception for offline tests and local dev. Enforcement is
   central (assertHttps) — the browser-open guard is no longer the only
   check whose failure was swallowed.
2. Credential store test isolation: CODEBURN_SYNC_TOKEN_STORE=file
   forces the file store (honors HOME) so the offline suite never
   touches the real macOS login keychain. Set in the e2e suite.
3. Golden pins for deriveSpanId/deriveTraceId/deriveDeviceId with fixed
   inputs and expected hex — the idempotency contract depends on these
   encodings being stable across releases; a green-tests encoding change
   would silently double-count history on span-ID-keyed backends.
   getDeviceId refactored over a pure deriveDeviceId(host, user).

Smaller review items:
- Callback server: ready promise resolves the actually-bound port from
  the listening event (kills the 100ms-sleep race after port fallback);
  Connection: close on all responses + closeAllConnections() on
  shutdown (pooled keep-alive sockets from a closed server could
  swallow requests aimed at a later server on the same port); error
  handler guarded so a post-bind error can never rebind to a different
  port than advertised; optional ports param ([0] = ephemeral) removes
  fixed-port contention between parallel test workers.
- fetchOidcConfig verifies the issuer claim matches the fetch origin
  (OIDC Discovery §4.3 mix-up defense).
- partialSuccess.rejectedSpans wrapped in Number() — proto3 int64 JSON
  mapping sends strings from strict protojson servers; += would
  concatenate.
- Ledger writes are atomic (temp + rename); corrupt-ledger recovery and
  no-tmp-left-behind tests added; XDG_CACHE_HOME honored (ledger is
  reconstructible state, not config).
- Mock IdP now implements /oauth2/authorize (registers PKCE challenge,
  302s to redirect_uri) and verifies S256 code_verifier + single-use
  codes at the token endpoint. The e2e drives the real redirect flow
  and asserts wrong-verifier and code-reuse are rejected — PKCE binding
  is now exercised end to end.
- sync reset calls clearLedger() instead of reimplementing the path.
- push sets exit code 1 on rate-limited/server-error outcomes so cron
  and script callers can detect incomplete pushes.

Deferred (noted for fast-follow): macOS 'security -i' stdin mode
(untestable on this Linux box), ai.cost_estimated as a real
ParsedApiCall flag (touches core parser types).

Sync suite: 81 passing (5x stable), 5 developer-only.

AI-Origin: human
---
 src/sync/auth.ts                | 79 +++++++++++++++++++++++------
 src/sync/cli.ts                 | 31 ++++++------
 src/sync/credentials.ts         |  7 +++
 src/sync/discovery.ts           | 26 ++++++++++
 src/sync/index.ts               |  2 +-
 src/sync/ledger.ts              | 20 ++++++--
 src/sync/otlp.ts                |  8 ++-
 src/sync/push.ts                |  9 +++-
 tests/fixtures/mock-idp.ts      | 45 +++++++++++++++++
 tests/sync-e2e.test.ts          | 39 +++++++++++----
 tests/sync-headless-e2e.test.ts |  6 +--
 tests/sync-ledger-otlp.test.ts  | 89 +++++++++++++++++++++++++++++++++
 tests/sync-push.test.ts         |  3 ++
 tests/sync.test.ts              | 32 ++++++------
 14 files changed, 327 insertions(+), 69 deletions(-)

diff --git a/src/sync/auth.ts b/src/sync/auth.ts
index 3ce7af2..fa91232 100644
--- a/src/sync/auth.ts
+++ b/src/sync/auth.ts
@@ -7,6 +7,7 @@
 import { createHash, randomBytes } from 'crypto'
 import { createServer, type Server } from 'http'
 import { URL } from 'url'
+import { assertHttps } from './discovery.js'
 
 export interface OidcConfig {
   authorization_endpoint: string
@@ -32,6 +33,7 @@ export class AuthError extends Error {
 // --- OIDC Discovery ---
 
 export async function fetchOidcConfig(issuer: string): Promise<OidcConfig> {
+  assertHttps(issuer, 'OIDC issuer')
   const url = `${issuer.replace(/\/$/, '')}/.well-known/openid-configuration`
 
   let response: Response
@@ -52,11 +54,26 @@ export async function fetchOidcConfig(issuer: string): Promise<OidcConfig> {
     throw new AuthError(`OIDC discovery returned invalid JSON: ${url}`)
   }
 
+  // Issuer mix-up defense (OIDC Discovery §4.3): the issuer claim in the
+  // metadata must match the issuer we fetched it from.
+  const issuerClaim = typeof body.issuer === 'string' ? body.issuer.replace(/\/$/, '') : ''
+  if (issuerClaim !== issuer.replace(/\/$/, '')) {
+    throw new AuthError(
+      `OIDC issuer mismatch: metadata claims "${body.issuer}" but was fetched from "${issuer}"`
+    )
+  }
+
   const authorization_endpoint = body.authorization_endpoint
   const token_endpoint = body.token_endpoint
   if (typeof authorization_endpoint !== 'string' || typeof token_endpoint !== 'string') {
     throw new AuthError('OIDC discovery missing authorization_endpoint or token_endpoint')
   }
+  // Tokens travel on these endpoints — enforce https (loopback exempt)
+  assertHttps(authorization_endpoint, 'authorization_endpoint')
+  assertHttps(token_endpoint, 'token_endpoint')
+  if (typeof body.revocation_endpoint === 'string') {
+    assertHttps(body.revocation_endpoint, 'revocation_endpoint')
+  }
 
   return {
     authorization_endpoint,
@@ -127,13 +144,29 @@ export interface CallbackResult {
 export function startCallbackServer(
   expectedState: string,
   timeoutMs: number = 300_000, // 5 minutes
-): { promise: Promise<CallbackResult>; port: number; server: Server } {
+  ports: readonly number[] = CALLBACK_PORTS, // tests pass [0] for an ephemeral port
+): { promise: Promise<CallbackResult>; ready: Promise<number>; server: Server } {
   let resolvedPort = 0
   let server: Server
+  let readyResolve!: (port: number) => void
+  let readyReject!: (err: Error) => void
+  const ready = new Promise<number>((resolve, reject) => {
+    readyResolve = resolve
+    readyReject = reject
+  })
 
   const promise = new Promise<CallbackResult>((resolve, reject) => {
+    // Fully shut down: stop listening AND destroy lingering keep-alive
+    // sockets. Without this, an HTTP client's pooled connection keeps the
+    // dead server alive and can swallow requests meant for a later server
+    // on the same port.
+    const shutdown = () => {
+      try { server.close() } catch { /* already closed */ }
+      try { server.closeAllConnections() } catch { /* Node < 18.2 */ }
+    }
+
     const timer = setTimeout(() => {
-      server?.close()
+      shutdown()
       reject(new AuthError('Login timed out after 5 minutes. Please try again.'))
     }, timeoutMs)
 
@@ -143,60 +176,74 @@ export function startCallbackServer(
       const state = url.searchParams.get('state')
       const error = url.searchParams.get('error')
 
+      // Connection: close on every response — the callback server is
+      // single-purpose and must never leave pooled keep-alive sockets behind.
       if (error) {
-        res.writeHead(400, { 'Content-Type': 'text/html' })
+        res.writeHead(400, { 'Content-Type': 'text/html', 'Connection': 'close' })
         res.end('<html><body><h1>Login failed</h1><p>You can close this tab.</p></body></html>')
         clearTimeout(timer)
-        server.close()
+        shutdown()
         reject(new AuthError(`IdP returned error: ${error}`))
         return
       }
 
       if (state !== expectedState) {
-        res.writeHead(400, { 'Content-Type': 'text/plain' })
+        res.writeHead(400, { 'Content-Type': 'text/plain', 'Connection': 'close' })
         res.end('Invalid state parameter')
         return // don't close — might be a stale request
       }
 
       if (!code) {
-        res.writeHead(400, { 'Content-Type': 'text/plain' })
+        res.writeHead(400, { 'Content-Type': 'text/plain', 'Connection': 'close' })
         res.end('Missing authorization code')
         return
       }
 
-      res.writeHead(200, { 'Content-Type': 'text/html' })
+      res.writeHead(200, { 'Content-Type': 'text/html', 'Connection': 'close' })
       res.end('<html><body><h1>✓ Login successful</h1><p>You can close this tab.</p></body></html>')
       clearTimeout(timer)
-      server.close()
+      shutdown()
       resolve({ code, port: resolvedPort })
     })
 
-    // Try ports in order
+    // Try ports in order. The error handler is guarded on `resolvedPort` so a
+    // late error event can never trigger a second listen() after a successful
+    // bind (which would silently move the server off the advertised port).
     const tryListen = (ports: readonly number[], idx: number) => {
       if (idx >= ports.length) {
         clearTimeout(timer)
-        reject(new AuthError(`All callback ports (${ports.join(', ')}) are in use. Close other codeburn instances and retry.`))
+        const err = new AuthError(`All callback ports (${ports.join(', ')}) are in use. Close other codeburn instances and retry.`)
+        readyReject(err)
+        reject(err)
         return
       }
       const port = ports[idx]!
       server.once('error', (err: NodeJS.ErrnoException) => {
+        if (resolvedPort !== 0) return // already bound — never rebind
         if (err.code === 'EADDRINUSE') {
           tryListen(ports, idx + 1)
         } else {
           clearTimeout(timer)
-          reject(new AuthError(`Callback server error: ${err.message}`))
+          const authErr = new AuthError(`Callback server error: ${err.message}`)
+          readyReject(authErr)
+          reject(authErr)
         }
       })
       server.listen(port, '127.0.0.1', () => {
-        resolvedPort = port
+        // port 0 = OS-assigned ephemeral port — read the real one back
+        const addr = server.address()
+        resolvedPort = typeof addr === 'object' && addr ? addr.port : port
+        readyResolve(resolvedPort)
       })
     }
 
-    tryListen(CALLBACK_PORTS, 0)
+    tryListen(ports, 0)
   })
 
-  // Return immediately so caller can read the port
-  return { promise, port: resolvedPort, server: server! }
+  // `ready` resolves with the actually-bound port once listening — callers
+  // must await it before building the redirect URI (port fallback means the
+  // first port in CALLBACK_PORTS is not guaranteed).
+  return { promise, ready, server: server! }
 }
 
 // --- Token Exchange ---
@@ -208,6 +255,7 @@ export async function exchangeCode(
   redirectUri: string,
   clientId: string,
 ): Promise<TokenResponse> {
+  assertHttps(tokenEndpoint, 'token_endpoint')
   const body = new URLSearchParams({
     grant_type: 'authorization_code',
     code,
@@ -246,6 +294,7 @@ export async function refreshToken(
   refreshTokenValue: string,
   clientId: string,
 ): Promise<TokenResponse> {
+  assertHttps(tokenEndpoint, 'token_endpoint')
   const body = new URLSearchParams({
     grant_type: 'refresh_token',
     refresh_token: refreshTokenValue,
diff --git a/src/sync/cli.ts b/src/sync/cli.ts
index 10bd941..9136dbf 100644
--- a/src/sync/cli.ts
+++ b/src/sync/cli.ts
@@ -52,13 +52,10 @@ export function registerSyncCommands(program: Command): void {
       const pkce = generatePkce()
       const state = randomBytes(16).toString('hex')
 
-      // 5. Start callback server
-      const { promise: callbackPromise, server } = startCallbackServer(state)
-
-      // Wait briefly for server to bind, then get the port
-      await new Promise(resolve => setTimeout(resolve, 100))
-      const addr = server.address()
-      const port = typeof addr === 'object' && addr ? addr.port : CALLBACK_PORTS[0]
+      // 5. Start callback server — await the actually-bound port (port
+      // fallback means it may not be the first in CALLBACK_PORTS)
+      const { promise: callbackPromise, ready } = startCallbackServer(state)
+      const port = await ready
       const redirectUri = `http://127.0.0.1:${port}/callback`
 
       // 6. Build auth URL and open browser
@@ -193,16 +190,12 @@ export function registerSyncCommands(program: Command): void {
         process.exit(1)
       }
 
-      const { join } = await import('path')
-      const { homedir } = await import('os')
-      const { unlinkSync, existsSync } = await import('fs')
-
-      const ledgerPath = join(homedir(), '.cache', 'codeburn', 'sync-ledger.json')
-      if (existsSync(ledgerPath)) {
-        unlinkSync(ledgerPath)
-        process.stderr.write('Ledger cleared. Next push will re-send all calls in window.\n')
+      const { clearLedger } = await import('./ledger.js')
+      const removed = clearLedger()
+      if (removed > 0) {
+        process.stderr.write(`Ledger cleared (${removed} entries). Next push will re-send all calls in window.\n`)
       } else {
-        process.stderr.write('No ledger file found (nothing to reset).\n')
+        process.stderr.write('No ledger entries found (nothing to reset).\n')
       }
     })
 
@@ -320,6 +313,12 @@ export function registerSyncCommands(program: Command): void {
         if (unsent.length > MAX_PER_PUSH) {
           process.stderr.write(`  ${unsent.length - MAX_PER_PUSH} calls remaining (safety limit). Run \`codeburn sync push\` again.\n`)
         }
+
+        // Non-zero exit when the push did not complete, so cron/scripts can
+        // detect it. Ledgered progress is kept; next push resumes.
+        if (result.outcome !== 'complete') {
+          process.exitCode = 1
+        }
       } catch (err) {
         process.stderr.write(`${(err as Error).message}\n`)
         process.exit(1)
diff --git a/src/sync/credentials.ts b/src/sync/credentials.ts
index a560581..9f80259 100644
--- a/src/sync/credentials.ts
+++ b/src/sync/credentials.ts
@@ -185,6 +185,13 @@ function isCommandAvailable(cmd: string): boolean {
 }
 
 export function createCredentialStore(): CredentialStore {
+  // Test/CI escape hatch: force the file store (respects $HOME, so tests
+  // can fully isolate with a temp HOME). Without this, darwin machines
+  // would hit the real login keychain during the offline test suite.
+  if (process.env.CODEBURN_SYNC_TOKEN_STORE === 'file') {
+    return new FileStore()
+  }
+
   if (process.platform === 'darwin') {
     return new KeychainStore()
   }
diff --git a/src/sync/discovery.ts b/src/sync/discovery.ts
index 60f5ffe..abd97dc 100644
--- a/src/sync/discovery.ts
+++ b/src/sync/discovery.ts
@@ -20,6 +20,28 @@ export class DiscoveryError extends Error {
   }
 }
 
+const LOOPBACK_HOSTS = new Set(['127.0.0.1', '[::1]', 'localhost'])
+
+/**
+ * Enforce https on remote endpoints (RFC 8252 §8.3). Refresh tokens and
+ * bearer tokens travel on these URLs — plaintext http is only permitted
+ * for loopback addresses (local development and offline tests).
+ */
+export function assertHttps(url: string, label: string): void {
+  let parsed: URL
+  try {
+    parsed = new URL(url)
+  } catch {
+    throw new DiscoveryError(`${label} is not a valid URL: ${url}`)
+  }
+  if (parsed.protocol === 'https:') return
+  if (parsed.protocol === 'http:' && LOOPBACK_HOSTS.has(parsed.hostname)) return
+  throw new DiscoveryError(
+    `${label} must use https (got ${parsed.protocol}//${parsed.host}). ` +
+    `Plain http is only allowed for loopback (127.0.0.1).`
+  )
+}
+
 const SUPPORTED_VERSION = 1
 
 export function parseDiscoveryDoc(raw: unknown): CodeburnDiscoveryDoc {
@@ -48,6 +70,9 @@ export function parseDiscoveryDoc(raw: unknown): CodeburnDiscoveryDoc {
     throw new DiscoveryError('Discovery doc missing required field: client_id')
   }
 
+  // Format validation after presence checks (clearer errors)
+  assertHttps(issuer, 'Issuer')
+
   // Optional with defaults
   const scopes = Array.isArray(doc.scopes)
     ? doc.scopes.filter((s): s is string => typeof s === 'string')
@@ -63,6 +88,7 @@ export function parseDiscoveryDoc(raw: unknown): CodeburnDiscoveryDoc {
 }
 
 export async function fetchDiscoveryDoc(baseUrl: string): Promise<CodeburnDiscoveryDoc> {
+  assertHttps(baseUrl, 'Base URL')
   const url = `${baseUrl.replace(/\/$/, '')}/.well-known/codeburn-export.json`
 
   let response: Response
diff --git a/src/sync/index.ts b/src/sync/index.ts
index 8aa19e4..347d03e 100644
--- a/src/sync/index.ts
+++ b/src/sync/index.ts
@@ -1,5 +1,5 @@
 export { registerSyncCommands } from './cli.js'
-export { fetchDiscoveryDoc, parseDiscoveryDoc, type CodeburnDiscoveryDoc } from './discovery.js'
+export { fetchDiscoveryDoc, parseDiscoveryDoc, assertHttps, type CodeburnDiscoveryDoc } from './discovery.js'
 export { fetchOidcConfig, generatePkce, buildAuthUrl, resolveScopes, exchangeCode, refreshToken } from './auth.js'
 export { createCredentialStore, type CredentialStore, type StorageMethod } from './credentials.js'
 export { readSyncConfig, writeSyncConfig, deleteSyncConfig, type SyncConfig } from './config.js'
diff --git a/src/sync/ledger.ts b/src/sync/ledger.ts
index eb4d7a3..99ef856 100644
--- a/src/sync/ledger.ts
+++ b/src/sync/ledger.ts
@@ -5,7 +5,7 @@
  * Push logic: window minus ledger = what to send.
  */
 
-import { existsSync, mkdirSync, readFileSync, writeFileSync, unlinkSync } from 'fs'
+import { existsSync, mkdirSync, readFileSync, writeFileSync, unlinkSync, renameSync } from 'fs'
 import { join } from 'path'
 import { homedir } from 'os'
 
@@ -16,8 +16,15 @@ export interface LedgerEntry {
 
 const SIX_MONTHS_MS = 180 * 24 * 60 * 60 * 1000
 
+function cacheDir(): string {
+  // Honor XDG_CACHE_HOME — the ledger is reconstructible state, not config
+  const xdg = process.env.XDG_CACHE_HOME
+  const base = xdg && xdg.trim() ? xdg : join(homedir(), '.cache')
+  return join(base, 'codeburn')
+}
+
 function ledgerPath(): string {
-  return join(homedir(), '.cache', 'codeburn', 'sync-ledger.json')
+  return join(cacheDir(), 'sync-ledger.json')
 }
 
 export function readLedger(): LedgerEntry[] {
@@ -36,9 +43,14 @@ export function readLedger(): LedgerEntry[] {
 }
 
 export function writeLedger(entries: LedgerEntry[]): void {
-  const dir = join(homedir(), '.cache', 'codeburn')
+  const dir = cacheDir()
   mkdirSync(dir, { recursive: true })
-  writeFileSync(ledgerPath(), JSON.stringify(entries))
+  // Atomic write: a crash mid-write must not corrupt the ledger — a corrupt
+  // ledger reads as empty and the next push re-sends the whole window.
+  const path = ledgerPath()
+  const tmp = `${path}.tmp`
+  writeFileSync(tmp, JSON.stringify(entries))
+  renameSync(tmp, path)
 }
 
 /** Append new entries after a successful push. Also prunes entries older than 6 months. */
diff --git a/src/sync/otlp.ts b/src/sync/otlp.ts
index b470431..8a12c7e 100644
--- a/src/sync/otlp.ts
+++ b/src/sync/otlp.ts
@@ -43,10 +43,14 @@ export interface OtlpPayload {
 
 let cachedDeviceId: string | null = null
 
+/** Pure derivation — exposed so the encoding can be golden-pinned in tests. */
+export function deriveDeviceId(host: string, username: string): string {
+  return createHash('sha256').update(`${host}:${username}`).digest('hex').slice(0, 16)
+}
+
 export function getDeviceId(): string {
   if (cachedDeviceId) return cachedDeviceId
-  const raw = `${hostname()}:${userInfo().username}`
-  cachedDeviceId = createHash('sha256').update(raw).digest('hex').slice(0, 16)
+  cachedDeviceId = deriveDeviceId(hostname(), userInfo().username)
   return cachedDeviceId
 }
 
diff --git a/src/sync/push.ts b/src/sync/push.ts
index ffa1102..0444c71 100644
--- a/src/sync/push.ts
+++ b/src/sync/push.ts
@@ -6,6 +6,7 @@
  */
 
 import type { ProjectSummary } from '../types.js'
+import { assertHttps } from './discovery.js'
 import { ledgerKeySet, appendToLedger, type LedgerEntry } from './ledger.js'
 import { buildOtlpPayload, batchCalls, type CallWithSession } from './otlp.js'
 
@@ -90,6 +91,7 @@ export function parseRetryAfterMs(value: string | null): number | null {
  * retry on the next push.
  */
 export async function sendBatches(opts: SendBatchesOptions): Promise<PushResult> {
+  assertHttps(opts.endpoint, 'Traces endpoint')
   const log = opts.log ?? (() => {})
   const sleep = opts.sleep ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)))
   const maxWaitMs = opts.maxWaitMs ?? 120_000
@@ -143,8 +145,11 @@ export async function sendBatches(opts: SendBatchesOptions): Promise<PushResult>
       // Check for partial success
       let rejected = 0
       try {
-        const body = await response.json() as { partialSuccess?: { rejectedSpans?: number } }
-        rejected = body?.partialSuccess?.rejectedSpans ?? 0
+        const body = await response.json() as { partialSuccess?: { rejectedSpans?: number | string } }
+        // proto3 int64 JSON mapping: strict protojson servers send int64 as a
+        // string — Number() both so `totalRejected +=` never concatenates.
+        rejected = Number(body?.partialSuccess?.rejectedSpans ?? 0)
+        if (!Number.isFinite(rejected) || rejected < 0) rejected = 0
       } catch { /* empty response = full success */ }
 
       if (rejected > 0) {
diff --git a/tests/fixtures/mock-idp.ts b/tests/fixtures/mock-idp.ts
index 8bd23aa..e873b6b 100644
--- a/tests/fixtures/mock-idp.ts
+++ b/tests/fixtures/mock-idp.ts
@@ -9,6 +9,7 @@
  * - /oauth2/revoke (token revocation)
  */
 
+import { createHash } from 'crypto'
 import { createServer, type Server, type IncomingMessage, type ServerResponse } from 'http'
 
 export interface MockIdpOptions {
@@ -40,6 +41,9 @@ export async function startMockIdp(opts: MockIdpOptions = {}): Promise<MockIdp>
   const accessToken = opts.accessToken ?? 'mock-access-token-xyz'
   let currentRefreshToken = refreshToken
   let rotationCounter = 0
+  let codeCounter = 0
+  // code -> S256 code_challenge registered at /oauth2/authorize
+  const pendingCodes = new Map<string, string>()
 
   const state: MockIdp = {
     port: 0,
@@ -84,6 +88,27 @@ export async function startMockIdp(opts: MockIdpOptions = {}): Promise<MockIdp>
       return
     }
 
+    // --- Authorize endpoint (records the PKCE challenge, issues a code,
+    //     redirects to the client's redirect_uri like a real IdP) ---
+    if (path === '/oauth2/authorize' && req.method === 'GET') {
+      const challenge = url.searchParams.get('code_challenge')
+      const method = url.searchParams.get('code_challenge_method')
+      const redirectUri = url.searchParams.get('redirect_uri')
+      const reqState = url.searchParams.get('state')
+      if (!challenge || method !== 'S256' || !redirectUri) {
+        res.writeHead(400, { 'Content-Type': 'application/json' })
+        res.end(JSON.stringify({ error: 'invalid_request', error_description: 'missing code_challenge/S256/redirect_uri' }))
+        return
+      }
+      codeCounter++
+      const code = `mock-code-${codeCounter}`
+      pendingCodes.set(code, challenge)
+      const location = `${redirectUri}?code=${encodeURIComponent(code)}&state=${encodeURIComponent(reqState ?? '')}`
+      res.writeHead(302, { Location: location })
+      res.end()
+      return
+    }
+
     // --- Token endpoint ---
     if (path === '/oauth2/token' && req.method === 'POST') {
       let body = ''
@@ -100,6 +125,26 @@ export async function startMockIdp(opts: MockIdpOptions = {}): Promise<MockIdp>
             return
           }
 
+          // PKCE S256 verification (RFC 7636 §4.6): the code must have been
+          // issued by /oauth2/authorize, and BASE64URL(SHA256(code_verifier))
+          // must equal the challenge registered with it.
+          const expectedChallenge = pendingCodes.get(code)
+          if (!expectedChallenge) {
+            res.writeHead(400, { 'Content-Type': 'application/json' })
+            res.end(JSON.stringify({ error: 'invalid_grant', error_description: 'unknown or reused code' }))
+            return
+          }
+          const verifier = params.get('code_verifier')
+          const computed = verifier
+            ? createHash('sha256').update(verifier).digest('base64url')
+            : ''
+          if (computed !== expectedChallenge) {
+            res.writeHead(400, { 'Content-Type': 'application/json' })
+            res.end(JSON.stringify({ error: 'invalid_grant', error_description: 'PKCE verification failed' }))
+            return
+          }
+          pendingCodes.delete(code) // single-use
+
           state.exchangedCodes.push(code)
           state.issuedTokens.access.push(accessToken)
           state.issuedTokens.refresh.push(currentRefreshToken)
diff --git a/tests/sync-e2e.test.ts b/tests/sync-e2e.test.ts
index 682d706..de1f5c4 100644
--- a/tests/sync-e2e.test.ts
+++ b/tests/sync-e2e.test.ts
@@ -29,13 +29,20 @@ import { writeSyncConfig, readSyncConfig, deleteSyncConfig } from '../src/sync/c
 let idp: MockIdp
 let tmpHome: string
 const originalHome = process.env.HOME
+const originalStore = process.env.CODEBURN_SYNC_TOKEN_STORE
 
 beforeAll(async () => {
   idp = await startMockIdp({ rotateTokens: false })
+  // Force the file store so this suite never touches the real OS keychain
+  // (on darwin, createCredentialStore() would otherwise ignore HOME and
+  // read/write the login keychain under the real service/account names).
+  process.env.CODEBURN_SYNC_TOKEN_STORE = 'file'
 })
 
 afterAll(async () => {
   await idp.close()
+  if (originalStore === undefined) delete process.env.CODEBURN_SYNC_TOKEN_STORE
+  else process.env.CODEBURN_SYNC_TOKEN_STORE = originalStore
 })
 
 beforeEach(async () => {
@@ -73,10 +80,8 @@ describe('sync e2e (mock IdP)', () => {
     const state = 'e2e-test-state'
 
     // 5. Start callback server
-    const { promise: callbackPromise, server } = startCallbackServer(state, 5000)
-    await new Promise(resolve => setTimeout(resolve, 100))
-    const addr = server.address()
-    const port = typeof addr === 'object' && addr ? addr.port : 19876
+    const { promise: callbackPromise, ready } = startCallbackServer(state, 5000, [0])
+    const port = await ready
     const redirectUri = `http://127.0.0.1:${port}/callback`
 
     // 6. Build auth URL (verify it's well-formed)
@@ -92,12 +97,17 @@ describe('sync e2e (mock IdP)', () => {
     expect(parsedUrl.searchParams.get('code_challenge_method')).toBe('S256')
     expect(parsedUrl.searchParams.get('client_id')).toBe('mock-client-id')
 
-    // 7. Simulate browser callback (as if user logged in and IdP redirected)
-    const authCode = 'e2e-auth-code-12345'
-    await fetch(`http://127.0.0.1:${port}/callback?code=${authCode}&state=${state}`)
+    // 7. Drive the real authorize flow: hit the IdP's authorize endpoint
+    // (registers the PKCE challenge, issues a code), then follow its
+    // redirect to our local callback server — like a browser would.
+    const authResp = await fetch(authUrl, { redirect: 'manual' })
+    expect(authResp.status).toBe(302)
+    const location = authResp.headers.get('location')!
+    expect(location).toContain(`http://127.0.0.1:${port}/callback`)
+    await fetch(location)
 
     const callbackResult = await callbackPromise
-    expect(callbackResult.code).toBe(authCode)
+    expect(callbackResult.code).toMatch(/^mock-code-/)
 
     // 8. Exchange code for tokens
     const tokens = await exchangeCode(
@@ -112,7 +122,18 @@ describe('sync e2e (mock IdP)', () => {
     expect(tokens.expires_in).toBe(3600)
 
     // Verify the mock IdP received the code
-    expect(idp.exchangedCodes).toContain(authCode)
+    expect(idp.exchangedCodes).toContain(callbackResult.code)
+
+    // 8b. PKCE negative checks: wrong verifier rejected; code is single-use
+    const authResp2 = await fetch(authUrl.replace(`state=${state}`, 'state=neg-test'), { redirect: 'manual' })
+    const loc2 = new URL(authResp2.headers.get('location')!)
+    const code2 = loc2.searchParams.get('code')!
+    await expect(
+      exchangeCode(oidc.token_endpoint, code2, 'wrong-verifier-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', redirectUri, discovery.client_id)
+    ).rejects.toThrow(/PKCE verification failed|Token exchange failed/)
+    await expect(
+      exchangeCode(oidc.token_endpoint, callbackResult.code, pkce.code_verifier, redirectUri, discovery.client_id)
+    ).rejects.toThrow(/unknown or reused code|Token exchange failed/)
 
     // 9. Store refresh token
     const store = createCredentialStore()
diff --git a/tests/sync-headless-e2e.test.ts b/tests/sync-headless-e2e.test.ts
index f6e6d96..2d40cdb 100644
--- a/tests/sync-headless-e2e.test.ts
+++ b/tests/sync-headless-e2e.test.ts
@@ -66,10 +66,8 @@ describe.skipIf(SKIP)('sync setup — headless browser PKCE flow', () => {
     const state = randomBytes(16).toString('hex')
 
     // 3. Start callback server
-    const { promise: callbackPromise, server } = startCallbackServer(state, 30000)
-    await new Promise(r => setTimeout(r, 200))
-    const addr = server.address()
-    const port = typeof addr === 'object' && addr ? addr.port : 19876
+    const { promise: callbackPromise, ready } = startCallbackServer(state, 30000)
+    const port = await ready
     const redirectUri = `http://127.0.0.1:${port}/callback`
 
     // 4. Build auth URL
diff --git a/tests/sync-ledger-otlp.test.ts b/tests/sync-ledger-otlp.test.ts
index 13e7c7a..0b32f80 100644
--- a/tests/sync-ledger-otlp.test.ts
+++ b/tests/sync-ledger-otlp.test.ts
@@ -13,6 +13,7 @@ import {
   buildOtlpPayload,
   batchCalls,
   getDeviceId,
+  deriveDeviceId,
   type CallWithSession,
 } from '../src/sync/otlp.js'
 
@@ -76,6 +77,15 @@ describe('deriveSpanId', () => {
     const b = deriveSpanId('key-2')
     expect(a).not.toBe(b)
   })
+
+  // GOLDEN PIN — do not update this value. The idempotency contract depends
+  // on span IDs being stable across releases: if the hash input or encoding
+  // ever changes, every re-sent span gets a new identity and backends that
+  // key on span ID double-count history. If this test fails, revert the
+  // encoding change (or design an explicit migration).
+  it('golden: SHA-256(deduplicationKey) first 8 bytes as hex', () => {
+    expect(deriveSpanId('golden-dedup-key')).toBe('ec3ca28cceacf381')
+  })
 })
 
 describe('deriveTraceId', () => {
@@ -89,6 +99,11 @@ describe('deriveTraceId', () => {
     const b = deriveTraceId('session-1')
     expect(a).toBe(b)
   })
+
+  // GOLDEN PIN — see deriveSpanId golden test for why this must not change.
+  it('golden: SHA-256(sessionId) first 16 bytes as hex', () => {
+    expect(deriveTraceId('golden-session-id')).toBe('ff1b1358ef64c52f80e50e7ae47ca176')
+  })
 })
 
 describe('getDeviceId', () => {
@@ -100,6 +115,12 @@ describe('getDeviceId', () => {
   it('is stable across calls', () => {
     expect(getDeviceId()).toBe(getDeviceId())
   })
+
+  // GOLDEN PIN — device ID must be stable across releases so a developer's
+  // machine keeps one identity in the backend.
+  it('golden: SHA-256(hostname:username) first 8 bytes as hex', () => {
+    expect(deriveDeviceId('host.example', 'alice')).toBe('004d1a2fc048f575')
+  })
 })
 
 // ── OTLP Payload Builder ──────────────────────────────────────────────
@@ -216,6 +237,9 @@ describe('ledger', () => {
   beforeEach(async () => {
     tmpDir = await mkdtemp(join(tmpdir(), 'codeburn-ledger-'))
     process.env.HOME = tmpDir
+    // env-isolation.ts redirects XDG_CACHE_HOME to a per-worker sandbox shared
+    // across tests — the ledger honors XDG, so point it at the per-test dir.
+    process.env.XDG_CACHE_HOME = join(tmpDir, '.cache')
   })
 
   afterEach(async () => {
@@ -283,4 +307,69 @@ describe('ledger', () => {
     const { clearLedger } = await import('../src/sync/ledger.js')
     expect(clearLedger()).toBe(0)
   })
+
+  it('corrupt ledger file reads as empty (crash-safe recovery)', async () => {
+    const { readLedger } = await import('../src/sync/ledger.js')
+    const { mkdirSync, writeFileSync } = await import('fs')
+    const { join } = await import('path')
+    const dir = join(process.env.HOME!, '.cache', 'codeburn')
+    mkdirSync(dir, { recursive: true })
+    writeFileSync(join(dir, 'sync-ledger.json'), '{"truncated mid-wri')
+    expect(readLedger()).toEqual([])
+  })
+
+  it('writes are atomic — no .tmp file left behind', async () => {
+    const { writeLedger } = await import('../src/sync/ledger.js')
+    const { existsSync } = await import('fs')
+    const { join } = await import('path')
+    writeLedger([{ key: 'a', ts: '2026-07-01T00:00:00Z' }])
+    const dir = join(process.env.HOME!, '.cache', 'codeburn')
+    expect(existsSync(join(dir, 'sync-ledger.json'))).toBe(true)
+    expect(existsSync(join(dir, 'sync-ledger.json.tmp'))).toBe(false)
+  })
+
+  it('honors XDG_CACHE_HOME when set', async () => {
+    const { writeLedger, readLedger } = await import('../src/sync/ledger.js')
+    const { existsSync } = await import('fs')
+    const { join } = await import('path')
+    const xdgDir = join(process.env.HOME!, 'xdg-cache')
+    const original = process.env.XDG_CACHE_HOME
+    process.env.XDG_CACHE_HOME = xdgDir
+    try {
+      writeLedger([{ key: 'xdg-entry', ts: '2026-07-01T00:00:00Z' }])
+      expect(existsSync(join(xdgDir, 'codeburn', 'sync-ledger.json'))).toBe(true)
+      expect(readLedger().map(e => e.key)).toEqual(['xdg-entry'])
+    } finally {
+      if (original === undefined) delete process.env.XDG_CACHE_HOME
+      else process.env.XDG_CACHE_HOME = original
+    }
+  })
+})
+
+// ── assertHttps (RFC 8252 §8.3) ───────────────────────────────────────
+
+describe('assertHttps', () => {
+  it('accepts https URLs', async () => {
+    const { assertHttps } = await import('../src/sync/discovery.js')
+    expect(() => assertHttps('https://telemetry.example.com', 'Base URL')).not.toThrow()
+  })
+
+  it('accepts http on loopback (offline tests, local dev)', async () => {
+    const { assertHttps } = await import('../src/sync/discovery.js')
+    expect(() => assertHttps('http://127.0.0.1:8080/x', 'Base URL')).not.toThrow()
+    expect(() => assertHttps('http://localhost:3000', 'Base URL')).not.toThrow()
+    expect(() => assertHttps('http://[::1]:9999', 'Base URL')).not.toThrow()
+  })
+
+  it('rejects plain http on non-loopback hosts', async () => {
+    const { assertHttps } = await import('../src/sync/discovery.js')
+    expect(() => assertHttps('http://telemetry.example.com', 'Base URL')).toThrow(/must use https/)
+    expect(() => assertHttps('http://192.168.1.10', 'Issuer')).toThrow(/must use https/)
+  })
+
+  it('rejects non-http(s) schemes and garbage', async () => {
+    const { assertHttps } = await import('../src/sync/discovery.js')
+    expect(() => assertHttps('ftp://example.com', 'Base URL')).toThrow(/must use https/)
+    expect(() => assertHttps('not a url', 'Base URL')).toThrow(/not a valid URL/)
+  })
 })
diff --git a/tests/sync-push.test.ts b/tests/sync-push.test.ts
index 9d9e804..0fe5c9b 100644
--- a/tests/sync-push.test.ts
+++ b/tests/sync-push.test.ts
@@ -89,6 +89,9 @@ const originalHome = process.env.HOME
 beforeEach(async () => {
   tmpDir = await mkdtemp(join(tmpdir(), 'codeburn-push-'))
   process.env.HOME = tmpDir
+  // env-isolation.ts redirects XDG_CACHE_HOME to a per-worker sandbox shared
+  // across tests — the ledger honors XDG, so point it at the per-test dir.
+  process.env.XDG_CACHE_HOME = join(tmpDir, '.cache')
 })
 
 afterEach(async () => {
diff --git a/tests/sync.test.ts b/tests/sync.test.ts
index 41e7a07..5c51257 100644
--- a/tests/sync.test.ts
+++ b/tests/sync.test.ts
@@ -34,7 +34,7 @@ describe('parseDiscoveryDoc', () => {
   })
 
   it('rejects version > 1', () => {
-    expect(() => parseDiscoveryDoc({ version: 2, issuer: 'x', client_id: 'y' }))
+    expect(() => parseDiscoveryDoc({ version: 2, issuer: 'https://idp.example', client_id: 'y' }))
       .toThrow('Please update codeburn')
   })
 
@@ -44,30 +44,35 @@ describe('parseDiscoveryDoc', () => {
   })
 
   it('rejects missing client_id', () => {
-    expect(() => parseDiscoveryDoc({ version: 1, issuer: 'x' }))
+    expect(() => parseDiscoveryDoc({ version: 1, issuer: 'https://idp.example' }))
       .toThrow('missing required field: client_id')
   })
 
   it('defaults traces_path to /v1/traces when absent', () => {
-    const doc = parseDiscoveryDoc({ issuer: 'x', client_id: 'y' })
+    const doc = parseDiscoveryDoc({ issuer: 'https://idp.example', client_id: 'y' })
     expect(doc.traces_path).toBe('/v1/traces')
   })
 
   it('defaults max_batch_size to 1000 when absent', () => {
-    const doc = parseDiscoveryDoc({ issuer: 'x', client_id: 'y' })
+    const doc = parseDiscoveryDoc({ issuer: 'https://idp.example', client_id: 'y' })
     expect(doc.max_batch_size).toBe(1000)
   })
 
   it('defaults scopes to ["openid"] when absent', () => {
-    const doc = parseDiscoveryDoc({ issuer: 'x', client_id: 'y' })
+    const doc = parseDiscoveryDoc({ issuer: 'https://idp.example', client_id: 'y' })
     expect(doc.scopes).toEqual(['openid'])
   })
 
   it('treats absent version as v1', () => {
-    const doc = parseDiscoveryDoc({ issuer: 'x', client_id: 'y' })
+    const doc = parseDiscoveryDoc({ issuer: 'https://idp.example', client_id: 'y' })
     expect(doc.version).toBe(1)
   })
 
+  it('rejects non-https issuer (RFC 8252 §8.3)', () => {
+    expect(() => parseDiscoveryDoc({ version: 1, issuer: 'http://idp.example', client_id: 'y' }))
+      .toThrow(/must use https/)
+  })
+
   it('rejects non-object input', () => {
     expect(() => parseDiscoveryDoc(null)).toThrow('must be a JSON object')
     expect(() => parseDiscoveryDoc('string')).toThrow('must be a JSON object')
@@ -164,12 +169,10 @@ describe('resolveScopes', () => {
 describe('startCallbackServer', () => {
   it('accepts valid callback with matching state', async () => {
     const state = 'test-state-123'
-    const { promise, server } = startCallbackServer(state, 5000)
+    const { promise, ready } = startCallbackServer(state, 5000, [0])
 
     // Wait for server to bind
-    await new Promise(resolve => setTimeout(resolve, 200))
-    const addr = server.address()
-    const port = typeof addr === 'object' && addr ? addr.port : CALLBACK_PORTS[0]
+    const port = await ready
 
     // Simulate IdP callback
     await fetch(`http://127.0.0.1:${port}/callback?code=auth-code-xyz&state=${state}`)
@@ -180,11 +183,8 @@ describe('startCallbackServer', () => {
 
   it('rejects callback with wrong state', async () => {
     const state = 'correct-state'
-    const { promise, server } = startCallbackServer(state, 5000)
-
-    await new Promise(resolve => setTimeout(resolve, 200))
-    const addr = server.address()
-    const port = typeof addr === 'object' && addr ? addr.port : CALLBACK_PORTS[0]
+    const { promise, ready } = startCallbackServer(state, 5000, [0])
+    const port = await ready
 
     // Send with wrong state — server stays running
     const resp = await fetch(`http://127.0.0.1:${port}/callback?code=xxx&state=wrong-state`)
@@ -197,7 +197,7 @@ describe('startCallbackServer', () => {
   }, 10000)
 
   it('times out after configured duration', async () => {
-    const { promise } = startCallbackServer('state', 300) // 300ms timeout
+    const { promise } = startCallbackServer('state', 300, [0]) // 300ms timeout
 
     await expect(promise).rejects.toThrow('timed out')
   }, 5000)