Skip to content

Harden the CLI for 0.2.0 - #148

Draft
ohong wants to merge 5 commits into
mainfrom
codex/cli-0.2.0-hardening
Draft

Harden the CLI for 0.2.0#148
ohong wants to merge 5 commits into
mainfrom
codex/cli-0.2.0-hardening

Conversation

@ohong

@ohong ohong commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Why

The July 23 audit found correctness and reliability gaps across collection, local state, transport, ingestion, identity reconciliation, and release packaging. Retries could advance state incorrectly, concurrent submissions could race, collector fallback pricing could silently produce inaccurate totals, and scheduled runs lacked bounded failure behavior.

This release makes each date a durable unit of work: validate before submission, persist before network I/O, commit atomically on the server, and advance local state only for contiguous proven outcomes. It also removes Straude's exact ccusage patch pin so new models and sources can flow through as soon as a stable upstream collector supports them.

What changed

Correct ingestion and identity

  • Added a shared v2 request/response validator with strict token, cost, date, agent, model, and aggregate invariants.
  • Added transaction-scoped submit_usage_day_v2 ingestion with per-agent storage, idempotent request/date outcomes, explicit partial responses, and legacy-request adaptation.
  • Made durable installation identity user-scoped, added proof-based device reconciliation commands, and added ledgered historical repair with exact rollback.
  • Aligned trusted source-reconciliation corrections with stable ccusage versions >=20.0.18, including later compatible majors, while rejecting old, prerelease, and malformed version claims.

Bounded and recoverable CLI

  • Added atomic 0600 config/outbox writes, corruption preservation, crash recovery, process-aware single-flight leases, queued overlap handling, and contiguous watermarks.
  • Added strict arguments, timezone-aware calendar arithmetic, the 3/7/30-day sync contract, bounded authentication and HTTP retries, classified exit codes, and noninteractive authentication behavior.
  • Retained independent per-agent accounting with --by-agent, strict live-pricing retries, and fail-closed handling for zero-priced Claude/Codex usage, including reasoning-only and partially priced mixed-model rows.
  • Hardened scheduler and hook installation, log rotation, dry-run output, and post-sync dashboard degradation behavior.

Upstream ccusage compatibility

  • Raised the verified floor to ccusage 20.0.18, which includes the issue #934 pricing fix, Claude advisor accounting, Kimi/Moonshot coverage, and the latest native performance work.
  • Published an open stable dependency floor (>=20.0.18) while keeping bun.lock fixed at 20.0.18 for reproducible repository CI.
  • Kept model and source IDs opaque, so Opus 5, future Fable releases, GPT-6, and newly supported agents do not require a Straude allowlist change.
  • Added packaged-install checks against the collector version npm actually resolves and a weekly/manual workflow that installs ccusage@latest in isolation, runs the production GPT-5.6 fixture/parser, enforces a 60-second budget, and proves unknown paid usage fails closed.
  • Kept runtime collection offline from npm. Existing installations retain their installed dependency until Straude is reinstalled or upgraded; fresh installs resolve the newest stable matching collector.

Release and operations

  • Added redacted structured diagnostics with request/date outcomes and stage timings.
  • Added exact-tarball package testing and a tag release workflow with provenance, digest attachment, and Node 20/22 coverage across Linux, macOS, and Windows.
  • Added CLI and collector benchmarks, concurrency/fault/migration tests, an operations runbook, and updated CLI/API/security documentation.

CI reliability cleanup

  • Traced every failed run to the packaged Windows test: direct npm execution failed first, then direct npm.cmd execution failed with spawn EINVAL. The script now invokes npm's JavaScript CLI through Node without a shell, and the packaged Windows test remains as the regression gate.
  • Upgraded GitHub and Supabase actions to their current Node-24-based majors, removing the obsolete-action warning annotation previously emitted by every job.
  • Scoped web typecheck/build to web and shared packages, removed per-PR benchmark and source-map artifacts that did not gate correctness, and reduced the PR package matrix to four non-overlapping cells. The tag release retains the full six-cell OS/Node matrix.
  • Corrected the weekly ccusage canary so the upstream binary runs under setup-node's executable instead of Bun's process.execPath.

Impact

  • Retries and crashes no longer lose or double-count committed dates.
  • Concurrent devices and account switches are isolated without overwriting usage.
  • Background jobs terminate within documented deadlines and return actionable exit codes.
  • Incomplete, embedded, or zero-priced paid-model results are never submitted.
  • Fresh installs can adopt later stable ccusage releases without a Straude release when the upstream output still satisfies the production schema and accounting contract.

Validation

  • bun install --frozen-lockfile
  • bun run lint
  • bun run typecheck
  • bun run test (635 web tests and 270 CLI tests)
  • bun run build
  • bun --cwd packages/cli test:packaged against installed ccusage 20.0.18
  • bun --cwd packages/cli canary:ccusage against the production GPT-5.6 fixture
  • Collector benchmarks for 1, 3, 7, and 30 days
  • The latest GitHub CI run passed web/shared typecheck and build, web tests, real-Supabase integration, browser E2E, package creation, and packaged installs on Ubuntu Node 20/22, macOS Node 22, and Windows Node 22 with zero warning annotations.
  • The tag release workflow retains packaged verification on Linux, macOS, and Windows with Node 20 and 22.
  • Independent pre-ship review found a reasoning-only zero-cost gap; the guard and two regressions were added. Its warning about the intentionally open executable dependency range is the explicit product trade-off requested here and is mitigated by the frozen release graph, packed-install matrix, strict parser, and upstream canary. A later review incorrectly claimed the newly published action majors did not exist; each literal moving-major tag was verified directly through GitHub before the clean CI run.

Local Docker/OrbStack was unavailable, so the real-Supabase regression was proven in the clean GitHub CI environment instead.

Rollout

This PR does not deploy or publish. After merge, follow docs/CLI_OPERATIONS.md: deploy the backward-compatible server path through the 5% / 25% / 100% gates, publish only after production accepts v2, run historical repair after the healthy window, and enforce the v1 cutoff on schedule.

@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
straude Ready Ready Preview, Comment Jul 23, 2026 8:30pm

Request Review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c53dd678-39a4-485e-836e-9ab7e0d07cbb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/cli-0.2.0-hardening

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ohong

ohong commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review. I focused on the surfaces where a bug is expensive and hard to walk back — the two migrations, the submit path, and the telemetry change — rather than trying to cover 82 files evenly. The identity/lease/watermark machinery is well built: advisory locks are taken in a consistent order (user+date first, then device ids sorted), RLS is on with nothing granted to anon/authenticated, every function carries SET search_path = '', and the outcome table makes retries genuinely idempotent. Three things I'd want fixed before this leaves draft, one of them a data-correctness bug.

I haven't pushed commits — the SQL fix needs your design intent, and pushing into an active draft of this size would just create conflicts.


1. A day can silently freeze at stale numbers (correctness)

submit_usage_day_v2 writes an agent row only if one of these holds (m1 L502–516): the row is new, the submission is authoritative, it's the legacy-codex correction, or all seven metrics are >= the stored ones — cost, input, output, reasoning output, cache creation, cache read, total.

Two defects compose here.

The guard is a conjunction over seven metrics, so one decrease rejects everything. A resubmission where cost went up but cache_read_tokens went down is dropped whole — including the higher cost. That combination isn't exotic: it's exactly what a collector-side pricing recalculation produces, and this PR's own justification for moving to 20.0.18 is upstream's issue #934 pricing fix. Re-parsing after Claude Code log rotation does the same thing to cache counters.

The skip is invisible, and the CLI treats invisible as done. Nothing in the function reports it — I grepped the whole migration for a skip/stale/noop signal and there is none. The outcome comes back status: 'committed', so push.ts L405 counts the date as settled and advances the watermark; resolvePushDateRange then starts the next incremental sync at lastPushDate + 1 (L204). The date is never visited again. The stored value is stale, the CLI believes it committed, and no telemetry fires.

The escape hatch doesn't cover this. v_authoritative needs authoritative_correction: true and migration_id = 'ccusage-by-agent-v2', and createUsageEntry sets both only when migration is true (push.ts L254–258) — the one-time backfill. Routine syncs are monotonic-only by construction: a legitimate downward correction can never land.

Note the interaction with the dependency change below. Opening the ccusage floor is what makes a recomputed-downward cost likely, and this guard's response to that is to discard it without telling anyone.

What I'd do: keep monotonicity as the default for a same-content retry, but decide on one defensible key rather than a seven-way conjunction, and make a skip a first-class outcome (status: 'stale_rejected' per agent, or at minimum a skipped_agents array in the result) so the CLI can hold the watermark and retry instead of sealing the date.

2. Error telemetry goes dark (observability)

Three changes land together: reportUsagePushFailed drops the message and sends only error_fingerprint; reportCliException stops calling posthog.captureException and sends a plain cli_exception event; and posthog.ts flips enableExceptionAutocapture to false.

The net effect is that a production CLI failure now records an event name, error_name — which is "Error" for nearly everything we throw — and a 24-hex hash. Not the message, not a stack, and nothing in PostHog's Error Tracking product, since dropping captureException and autocapture together removes the only two paths into it. When a user reports "push stopped working," there's nothing to look at.

The fingerprint itself is good work and the path normalization is careful. But it hashes name + frames 1–5, so two genuinely different failures thrown from the same call site — a network timeout and a malformed-JSON parse out of the same await — collapse into one hash. With the message gone, they're no longer distinguishable by any field.

The privacy concern behind this is real; messages do carry home directories, hostnames, and collector stderr. Deleting the field is a heavier remedy than the risk needs. Send a redacted message alongside the fingerprint: strip $HOME and absolute paths (the normalizer in errorFingerprint already does most of this), drop query strings, and truncate. That keeps failures diagnosable and still never ships a raw path.

3. ccusage: ">=20.0.18" is unbounded on a financial path (supply chain)

Every fresh install resolves whatever ccusage is newest at install time, across any future major. Whatever that version emits flows through the parser into daily_usage.cost_usd — the project's north-star metric.

The weekly canary is a good control but it's the wrong shape for this: installs are continuous and the canary is weekly, so a breaking or compromised release reaches new users up to seven days before CI notices. And bun.lock stays at 20.0.18, so repo CI never exercises what users actually run. The v2 validator's invariants catch malformed output; they can't catch output that's well-formed and differently priced.

The stated goal — new models and sources flowing through without a Straude release — is fully served by ^20.0.18, since new model and source IDs ship in minors. The unbounded range only buys automatic cross-major adoption, which is precisely the case that should involve a person. I'd narrow it to ^20.0.18 and keep the canary as the signal for when to widen deliberately.

4. Merge coordination with #146

shutdownTelemetryWithTimeout here wraps the race in an inner try/catch, and #146 puts .catch(() => {}) on the _shutdown promise. Both are correct and both will conflict textually.

They're not redundant, though: index.ts L221 in this branch is still .finally(() => shutdownTelemetryWithTimeout().then(() => process.exit(exitCode))). Your inner catch means the function can't reject today, so this doesn't bite yet — but the exit remains conditional on that promise fulfilling, so anything that ever throws in there hangs the CLI on the event loop instead of returning its exit code. #146 also adds .catch(() => {}) before the .then on the caller. Whichever merges second should keep both halves and both tests rather than resolving the conflict to one side.

Smaller notes

  • Legacy backfill attributes unexplained tokens to reasoning (m1 L145–154). Where total_tokens exceeded the sum of the itemized fields, the remainder is added to reasoning_output_tokens to satisfy the new CHECK. That keeps totals honest at the cost of inflating a specific user-visible stat. Worth a comment in the migration recording that these are unattributed, not measured — otherwise the next person reads them as real.
  • content_hash on backfilled rows is md5() || md5() (m1 L167–169). It satisfies ^[a-f0-9]{64}$ but it isn't a sha256, so it can never equal a CLI-computed hash. That fails safe — it forces a rewrite rather than a false match — but it's surprising enough to deserve a line saying so.
  • is_verified is sticky-true on daily_usage (m1 L784) while the total it labels sums across devices, including an unverified web-import device. A day can therefore read "verified" over a mixed total. Pre-existing behavior as far as I can tell, not introduced here, but this PR is the moment the mixing becomes routine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant