Meter Amp, Cline, Gemini CLI, OpenCode and Qwen Code from what each one keeps - #772

Review by Codex 0.155.0 · GPT-5.6-Sol · Low effort · via HarnessDesk

I found three correctness defects.

• [P1] packages/server/src/ledger/foreign-db.ts:29 — -shm absence does not establish that the owner is not running. SQLite may omit the shared-memory file while an active connection uses exclusive locking, and there is also a check/open race in which the owner can start after existsSync returns false. This branch then opens the live database with immutable=1, which deliberately skips locking and change detection; if the file changes, SQLite documents incorrect results or SQLITE_CORRUPT as possible outcomes. The resulting scan can silently replace the ledger with a stale/inconsistent snapshot. The test at packages/server/test/ledger-agents.test.ts:277 only closes the writer before opening the reader, so it proves “no side files are created for a quiescent database,” not the claimed owner-detection rule. This needs a read strategy whose safety does not depend on inferring process ownership from -shm, plus an active-writer/race test.

• [P1] packages/server/src/ledger/scan.ts:162 — aggregating a priced session with a session lacking vendorCost turns both into vendor-priced usage. Rows are keyed only by day/model/project. sumCost(null, 0.50) becomes 0.50, and then packages/server/src/ledger/index.ts:252 treats every request and every token in that row as vendor-metered and uses only $0.50. Thus one OpenCode/Cline session with tokens but no recorded cost, grouped with one that has a recorded cost, loses the former session's list-price fallback (or its unpriced coverage) and falsely reports complete vendorMetered provenance. The SQL upsert at packages/server/src/ledger/store.ts:150 has the same lossy representation, although whole-file replacement currently avoids that path for these two corpora. Please preserve vendor-priced and non-vendor-priced request/token totals separately, and add a same-day/same-model/same-project mixed-session test.

• [P1] packages/server/src/bootstrap.ts:520 — a moved Gemini home splits quota and spend across different accounts. The new corpus binding correctly resolves chat logs through the row's merged environment, but new GeminiMeter() still resolves oauth_creds.json from the process user's homedir() (packages/server/src/usage/gemini.ts:116). With GEMINI_CLI_HOME on the agent row, the card can therefore show quota/account A from the default home beside spend from account B in the moved home. Cline passes the row environment to both paths; Gemini needs the same treatment and a binding test that asserts both the meter watch path and corpus root move together.

Risks and missing coverage:

• Gemini's whole-file last-record-by-message-id scheme is internally consistent for the measured shape, and the test covers a repeated reply, later token counts, rewind, and a checkpoint. It does not cover a later checkpoint restating an older/incomplete copy of an already-final message; I cannot tell from this repository alone whether Gemini CLI can emit that ordering. A fixture captured from that transition, or a source-level invariant from its recorder, would settle it.
• Amp's five-minute minimum interval and narrow Signed in as / balance parsing are clear and tested. There is no fixture for multiple balance lines or a changed Markdown report, so an upstream format change will become silence; that is acceptable fail-closed behavior, but it should be an intentional monitoring risk.
• The database target includes WAL size and mtime, and whole-file scans replace prior rows, which is the right shape once a consistent snapshot is obtained. There is no concurrent WAL checkpoint/write test.

What I checked and found sound: Cline uses a valid access token without refreshing or writing another application's file; expiry preserves the last reading with its original age and account changes discard it. The billed-account selection (active organization, otherwise user), micro-dollar conversion, and usage rather than aggregateUsage avoid the stated subagent double count. OpenCode/Cline zero vendor cost remains a real priced zero. For homogeneous rows, vendorCost storage, replacement scans, ledger preference, and vendorMetered/mixed propagation agree. The Dashboard's headline, band hint, footnote, balance rail, and top-up wording follow provenance/credit semantics. Gemini/Qwen token arithmetic separates cached prompt tokens and adds thinking/tool tokens in the documented directions. Amp throttling, signed-out handling, and command-name binding are coherent.

I read the full 2,496-line diff and the touched implementation/tests at f0531fc2. Per the read-only instruction I did not run builds or tests; I independently confirmed the live head/base and that the three substantive CI checks are green.

Verdict: request changes

Round 1 of at most 3 — Codex GPT-5.6-Sol (low) requested changes on f0531fc2. All three P1s were real and are fixed in 6206125a; each fix has a test that fails without it (I mutated the code it guards and watched it fail, then restored it).

[P1] foreign-db.ts: no -shm does not mean the owner is not running — fixed.

• Agreed on both counts. Measured here: a Cline database has kept a -shm since a run that ended weeks ago, and an owner in exclusive locking mode never makes one.
• The reader no longer infers the owner. It picks from what is on disk: a rollback-journal database gets an ordinary read-only open; a WAL database with neither -wal nor -shm is opened immutable, and the read is checked afterwards (size and time of both files, and whether a side file appeared), so an owner that starts mid-read has its answer discarded; a WAL database with either file is copied, with its log, into a private folder and read there, trusted only if the source was the same before and after the copy, and retried up to three times.
• New tests: an owner with commits only in its log (its files are byte-for-byte and time-for-time unchanged, and the rows only the log holds are seen); an exclusive-locking owner; an owner starting during an immutable read; a commit during the copy (retried) and a writer that never stops (refused). Mutated to the old rule, to no check after the immutable read, and to no check after the copy: each fails its own test.
• One limit worth knowing: a database over 1 GiB that is in use is refused rather than copied. The real ones here are 1 MB and 172 KB.

[P1] scan.ts: a priced and an unpriced session folded into one row — fixed.

• Confirmed by reading add() and #price. sumCost(null, 0.5) is 0.5, and the ledger then treated every request in the row as the agent's.
• Priced and unpriced requests now never share a row: the scanner's key and the store's primary key both carry a vendored bit. An existing ledger is rebuilt in place with its rows (empty vendor cost stays unvendored), and reopening it changes nothing.
• New tests: one day, one model, one folder, one session with a recorded cost and one without, through the whole ledger (the total is $0.50 plus the other's list price, provenance mixed, both requests priced); the store keeps the two kinds apart and adds up within each; the migration test now also proves the rebuilt key. Without the fix, all three fail, and mutating the scanner's key and the store's key separately each fails a test.

[P1] bootstrap.ts: a moved Gemini home split quota from spend — fixed. GeminiMeter now takes the row's environment and reads its sign-in from GEMINI_CLI_HOME when set, the same place corpusRoot reads the chat logs. A binding test asserts the meter's watch path and the corpus root move together; it fails without the change.

Risks and missing coverage

• Gemini checkpoint restating an older copy of a final message: no code change. Measured on 166 real chat logs here (104 message ids with counts, 242 checkpoints): no later record has lower counts than an earlier one for the same id, so last-record-wins and largest-wins never differ.
• Amp's report format: now stated in usage/amp.ts as an intentional fail-closed choice (a reworded line reads as "not metered", not a guessed figure; the first balance line is the one shown). The no-balance-line report is already tested.
• Concurrent WAL checkpoint or write: covered by the new reader tests above.

node script/verify.mjs is green on 6206125a. Round 2 will be asked to review the repair, since that is where the next defect is most likely.

Round 2 of at most 3 — Codex GPT-5.6-Sol (low) approved on 9bc375a1, with no remaining correctness defect: "The three round-1 P1 repairs are sound."

Three residual risks, all in code this PR touches but none an actionable defect:

• A same-size/same-mtime rewrite would slip past the copy's consistency check. Agreed this is a real gap in the check, not in the data: readForeignDatabase compares size and modification time, not bytes. Neither OpenCode's nor Cline's writer does this — every write through SQLite's WAL path changes at least the WAL file's size — so I've stated the tradeoff explicitly in foreign-db.ts and docs/usage-dashboard.md rather than adding a byte- or version-level check that would cost a read of the file on every scan to guard a pattern neither writer uses.
• A store already carrying vendorCost but not vendored wasn't tested. This is the shape between the two migrations. Added: a store built with that exact intermediate schema, migrated and read back correctly. New, it fails without the migration's carried detection — mutated to always take the "no vendorCost" branch and it fails.
• No boundary test at the 1 GiB snapshot limit. Also now stated as deliberate in foreign-db.ts: a database that size and possibly open is refused rather than copied, so a very large store keeps its last good rows rather than paying for a whole-file copy on every scan.

node script/verify.mjs is green on 05795ce4.

Both rounds are done: round 1 found and fixed three P1s, round 2 found no defect and closed the one real coverage gap it raised. Merging.

Review by Codex 0.155.0 · GPT-5.6-Sol · Low effort · via HarnessDesk

I found one remaining correctness defect.

• [P1] packages/server/src/bootstrap.ts:528 — Cline's meter and ledger ignore the row's supported --data-dir override. The runtime starts Cline with agent.args, and this codebase explicitly records in packages/server/src/installs/known-agents.ts:271 that Cline moves its state with --data-dir <dir> and has no environment variable for that purpose. The existing identity reader already resolves that flag at packages/server/src/installs/identity.ts:192. The new usage binding instead constructs ClineMeter from only env and resolves the corpus through corpusRoot('cline', env); packages/server/src/usage/cline.ts:94 and packages/server/src/ledger/scan.ts:891 therefore fall back to the process user's ~/.cline/data (or to undocumented CLINE_*_DIR variables) even when the row actually runs cline --data-dir /other/account --acp. On such a valid custom row, the Dashboard can show the default Cline account's balance and spend beside a runtime using the other account, or show “not metered” despite records in the configured directory. The binding needs to derive both the credentials path and sessions.db from the same --data-dir argument Cline runs with, including Cline's relative-path resolution against the row's cwd. A regression test should bind a row with args: ['--acp', '--data-dir', moved] and assert both meter.watchPaths() and root point under moved; the current tests cover only invented environment overrides and therefore miss the supported path.

Risks and missing coverage:

• The same binding layer does not exercise relative --data-dir values with a custom cwd. That is part of the defect above rather than a separate finding; the existing identity implementation documents the required resolution rule.
• Amp's Markdown parser intentionally fails closed on a changed report and selects the first balance line. The no-balance case is tested, but there is still no multiple-balance fixture; I agree this is a documented upstream-format risk, not a correctness defect in the measured shape.
• The foreign-database consistency check remains stamp-based, and a possibly-live database above 1 GiB remains unread. Both limits are now explicit in foreign-db.ts and docs/usage-dashboard.md, match the measured SQLite writers, and preserve the last good ledger rows on refusal. I do not consider either an actionable defect here.

What I checked and found sound: the round-1 immutable/snapshot reader repair covers quiescent WAL, active WAL, exclusive locking, start-during-read, commit-during-copy, retry exhaustion, cleanup, and read-only behavior. The round-1 priced/unpriced split remains correct in both the scanner key and store primary key; zero vendor cost stays priced, and the query produces honest vendorMetered or mixed provenance. The round-2 intermediate-schema migration test now covers a store with vendorCost but without vendored, and the migration preserves both null and recorded costs. Gemini's moved-home fix keeps its credential and transcript roots together for GEMINI_CLI_HOME; Gemini/Qwen token arithmetic, whole-file versus incremental scanning, OpenCode/Cline session-total treatment, and Cline's usage rather than aggregateUsage rule are coherent. Amp throttling/error handling, Cline token non-refresh, active-organization balance selection, micro-dollar conversion, stale-reading behavior, and account-change invalidation are sound. The Dashboard's balance headline/rail/top-up copy and list-price/vendor/mixed explanations match the ledger provenance.

I read the complete 2,909-line diff and the touched implementation/tests at 180a7a3b, plus the repository's agent registration and identity contracts that govern these paths. Per the read-only instruction I did not edit files, build, or run tests. I confirmed the exact head and current checks: build/typecheck/test and both UI-system jobs are green; the unsigned package job is still not reported successful.

Verdict: request changes

Round 3 of at most 3 — Codex GPT-5.6-Sol (low) requested changes on 180a7a3b. This was the last round under the project's review limit.

[P1] bootstrap.ts: Cline's usage binding ignored the row's --data-dir, its real (and only) way to move accounts — fixed in 776a6995.

• Agreed. known-agents.ts already documents that Cline has no environment variable for --data-dir, and identity.ts already resolves it for the account chip; the usage binding read only CLINE_DATA_DIR/CLINE_DIR, which such a row never sets. A row running cline --data-dir /other/account --acp could show the default account's balance and spend beside a runtime signed in as someone else.
• identity.ts now exposes clineDataDirOverride, the unambiguous half of the existing resolver (just --data-dir, resolved relative to the row's cwd), and bootstrap.ts builds both ClineMeter and the ledger's corpus root from it when a row carries the flag.
• New test: a row with --data-dir /work/second-cline binds both to that folder, a relative --data-dir resolves against the row's cwd, and a plain row keeps the environment-based default. It fails without the fix (mutated the binding back to the env-only path and watched it fail).
• node script/verify.mjs is green on 776a6995.

Per the project's 3-round limit, I am not running a fourth round or merging. I've reported the full state — every round, this fix, and what's still open — to the owner directly; they'll decide how to proceed.

Round 4 (one round beyond the project's normal 3-round limit, approved by the owner given round 3's fix was small) — Codex GPT-5.6-Sol (low) requested changes on 086058a6, reviewing round 3's fix and the full diff.

[P1] bootstrap.ts: the round-3 Cline fix dropped CLINE_DB_DATA_DIR's own precedence — fixed in db5ed07b.

• Agreed. corpusRoot's Cline branch already gives CLINE_DB_DATA_DIR precedence over the data folder when there is no override; the round-3 branch hard-coded <override>/db/sessions.db instead, so a row using --data-dir for the account and CLINE_DB_DATA_DIR for the database bound the right account but scanned the wrong (usually stale or absent) database.
• The database path now still checks the environment variable first even under an override; only the settings path is unconditionally the override's.
• New test: a row with both --data-dir and CLINE_DB_DATA_DIR set proves settings follow the override and the database follows the environment variable. Fails without the fix.

[P1] bootstrap.ts/amp.ts: Amp was bound with no row environment — fixed in db5ed07b.

• Agreed. localUsageFor had already built the row's merged environment for every other case, but AmpMeter was constructed bare, so it asked the host process's own PATH/HOME and would report the host's account, not a row-specific one.
• AmpMeter now takes the row's environment and uses it both to locate amp and to run it.
• New test: a row with a fixture amp on a row-only PATH proves the row's environment reaches both. Fails without the fix.

[P1] usage.ts: the plural exhausted headline called every group "quota" — fixed in db5ed07b.

• Agreed. Two spent balance-only accounts (Amp, Cline) have no quota at all; the plural line said "quota" regardless.
• It now says "credits" when every exhausted account is balance-only, "quota" when none is, and "credits or quota" for a mix — the distinction the singular line already drew.
• New tests: all-credits, all-quota, and mixed cases. The all-credits and mixed cases fail without the fix.

node script/verify.mjs is green on db5ed07b. Per the process, an issue found in this extra round stops the loop the same way it would in a normal one — I'm not running a further round on my own authority. Reporting the full state to the owner.

Review by Codex 0.155.0 · GPT-5.6-Sol · Low effort · via HarnessDesk

I found one remaining row-binding correctness defect.

• [P1] packages/server/src/bootstrap.ts:512 — a row-specific HOME is discarded by the new local-usage paths. localUsageFor correctly merges agent.env over process.env, but records() calls corpusRoot(corpus, env) without its home argument, so packages/server/src/ledger/scan.ts:874 defaults that argument to the host process's homedir(). The same split exists in GeminiMeter (packages/server/src/usage/gemini.ts:122) and ClineMeter (packages/server/src/usage/cline.ts:188), and Cline's --data-dir ~/... resolver also expands against the host home (packages/server/src/installs/identity.ts:202). This is not how the row runs: ACP spawn overlays the row environment at packages/transport-acp/src/index.ts:937-940, and Node in that child resolves os.homedir() from the overlaid HOME (I confirmed with a focused HOME=/tmp/hd-row-home node -e ... probe). Therefore a Gemini or Cline row isolated with env: { HOME: '/accounts/two' } can run as account two while its quota, credentials, identity, and spend are read from the host account; Qwen and OpenCode spend have the same wrong fallback when their dedicated home variables are absent. Amp's round-4 fix does not have this gap: both its installer fallback and subprocess receive the merged row environment. Pass the effective row home into every fallback (including identity and ~ expansion), and add binding tests for HOME-only Gemini, Cline (plain and --data-dir ~/...), Qwen, and OpenCode rows.

Risks and missing coverage:

• The new binding tests cover dedicated variables (GEMINI_CLI_HOME, CLINE_DB_DATA_DIR), absolute/relative Cline --data-dir, and Amp's row-only PATH, but none covers the ordinary HOME inherited by each CLI. That is why the shared fallback escaped the earlier rounds.
• Cline's --data-dir parser is exercised for separate arguments but not an equals form. I did not find code or repository evidence that Cline accepts --data-dir=<dir>, so I am not reporting that as a defect; a real CLI invocation or upstream parser source would be needed to establish it.
• Per the read-only instruction I did not build or run the test suites. GitHub reports Build/typecheck/test and both UI-system integration jobs green on exact head 9d9b826b; the unsigned package job is skipped.

What I checked and found sound: all three round-4 repairs are internally complete for their covered inputs. Cline keeps CLINE_DB_DATA_DIR ahead of the database derived from --data-dir while its settings remain under the override; Amp uses the merged row environment for both amp discovery and execution; and the plural exhausted headline selects credits, quota, or the mixed phrase from the group. I also re-read the full 3,097-line diff and every touched implementation, test, and documentation file. The foreign-SQLite immutable/snapshot consistency paths, vendored/unvendored row separation and schema migration, Gemini/Qwen token arithmetic and deduplication, OpenCode/Cline whole-file replacement, Cline's non-refreshing account API flow and usage rather than aggregateUsage, ledger provenance propagation, and the Dashboard's balance/provenance copy remain coherent with their tests. The two merges from main do not conflict with the usage work; the host.ts overlap is the separate update-advisory change.

Verdict: request changes

Round 5 (second round beyond the project's normal 3-round limit, approved by the owner) — Codex GPT-5.6-Sol (low) requested changes on 9d9b826b, reviewing the round-4 fixes and the full diff, specifically asked to look for any other row-specific binding still falling back to the host process.

[P1] A row isolated with a bare HOME was read from the desk's own account everywhere — fixed in fa24ac81.

• Agreed, and confirmed at every one of the four call sites named: records()'s call to corpusRoot dropped its home argument (defaulting to the desk's homedir()); GeminiMeter and Cline's internal clineDataDir each had the same bare fallback; and --data-dir ~/... expanded ~ against the desk's home. HOME is the ordinary way to isolate an agent — ahead of any of the bespoke variables rounds 1, 3 and 4 already fixed — so a row using only it had its Gemini/Cline sign-in and its Qwen/OpenCode spend read from whichever account the desk itself runs as.
• localUsageFor now computes the row's effective home once and threads it through all four: corpusRoot's home parameter, a new home option on GeminiMeter, Cline's fallback, and clineDataDirOverride.
• New tests: one HOME-only row proves Gemini, Cline, Qwen and OpenCode all move together, plus a --data-dir ~/... case under that same HOME; a GeminiMeter built directly with only env (the path localUsageFor never exercises, since it always supplies home explicitly) is pinned on its own, since that internal fallback would otherwise never be covered.
• All five checks fail under their own mutation and pass restored.

node script/verify.mjs is green on fa24ac81. This is the second round in a row to find a real issue in the same area (row-specific binding), so per the process I'm not running a sixth round on my own authority — reporting the state to the owner, same as after round 4.

Merging without a round 6, on the owner's explicit decision.

Five rounds, four of them (1, 3, 4, 5) found a real defect and one (2) approved outright. Every finding was fixed on the head that follows it, each with a test proven to fail under its own mutation:

Rounds 3 through 5 all found the same class of gap — a row-specific override the binding forgot to thread through — each time in the fix the previous round produced or in a sibling path the reviewer went looking for after. node script/verify.mjs is green on fa24ac81, and CI is green on that exact commit.

Merging now on the strength of that evidence rather than a sixth round.