fix(claude-usage): review findings of the #52/#51/#53 stack #56

Merged

maximus merged 4 commits from issue-54-claude-usage-review-findings into main 2026-10-08 00:54:24 +00:00

Conversation 2 Commits 4 Files changed 11 +435 -31

maximus commented 2026-10-08 00:30:35 +00:00

Owner

Copy link

Refs #54

The six non-blocking findings from the reviews of the claude-usage stack (#52, #51, #53), with the decisions recorded on the issue.

Server (e2a24d7)

• S2: readClaudeUsage() and readHostSnapshot() now require a full ISO timestamp with an offset for the stored receivedAt, through isIsoTimestamp() exported by claude-usage.js (built on the sanitizer's own check, no duplicated regex). "1", a date alone and a time without an offset now degrade to no_data / neverSeen (6 new cases, red before the fix).
• S1: two new cases make the rename fail after the temp file is written (the target is a non-empty directory) and require that no .tmp is left, for /claude-usage and /hosts/<id>. The existing tests failed at mkdir, so the cleanup was never exercised.

Agent (123b3c3)

• A1: frozen days are dropped after deduplication. A request straddling midnight whose best line sat on a day frozen since was counted again on the open day. On equal output the earliest line now wins (before, the first line read won), so the choice no longer depends on which files a beat reads. Known limit, documented in the header comment: the window start is still filtered per line.
• A2: aggregate() returns readErrors (unlistable directory, failed stat including ENOENT, stream or opener error; a missing root is still an empty scan). A beat with readErrors > 0 freezes no new day (nextState(..., { freeze: false })), still pushes, and logs claude-usage: warning: read_errors=N, no day frozen this beat, with no path.
• A3: readBody() reads the response chunk by chunk, counts bytes and cancels the stream past 256 KiB. It used to read everything, then compare a character count.
• A4: new test for the chmodSync of a temp file left at mode 644.

Docs (ca33f10)

CLAUDE.md (case counts, gotcha on freezing), agent/README.md (new section "Unreadable transcripts").

Verification

• npm test: 634/634 (611 + 23). bash test-curl.sh: 45/45. sh scripts/boot-from-copy.sh: PASS.
• Mutations, one per fix, each restored from a backup copy:

Mutation	Red
remove unlinkSync(tmpPath) in writeHostSnapshot	1
remove unlinkSync(tmpPath) in writeClaudeUsage	1
filter frozen days before dedup again	1
remove the earliest-line tie-break	1
readLines resolves true on a stream error	2
readdir error not counted	1
stat error not counted	1
root ENOENT counted	1
opener throw not counted	1
beat always freezes	1
no read_errors log line	1
nextState ignores freeze	1
remove chmodSync	1
remove reader.cancel()	1
count characters instead of bytes	1
ENOENT ignored on every directory, not only the root (from the review, fixed in f694677)	1

S2 was proven by test-first: 6 red before the fix.

Post-merge checklist (hence Refs, not a closing keyword)

1. Redeploy Coolify (no auto-deploy), then check that GET /claude-usage and GET /hosts still return content.
2. Copy agent/claude-usage/* to ~/.local/share/maximus-claude-usage/ on the ThinkPad (the cp from agent/README.md), then check journalctl -t claude-usage on the next beat.
3. Once 1 and 2 are done, #54 is closed by hand.

🤖 Generated with Claude Code

Refs #54 The six non-blocking findings from the reviews of the claude-usage stack (#52, #51, #53), with the decisions recorded on the issue. ## Server (`e2a24d7`) - **S2**: `readClaudeUsage()` and `readHostSnapshot()` now require a full ISO timestamp with an offset for the stored `receivedAt`, through `isIsoTimestamp()` exported by `claude-usage.js` (built on the sanitizer's own check, no duplicated regex). `"1"`, a date alone and a time without an offset now degrade to `no_data` / `neverSeen` (6 new cases, red before the fix). - **S1**: two new cases make the `rename` fail after the temp file is written (the target is a non-empty directory) and require that no `.tmp` is left, for `/claude-usage` and `/hosts/<id>`. The existing tests failed at `mkdir`, so the cleanup was never exercised. ## Agent (`123b3c3`) - **A1**: frozen days are dropped **after** deduplication. A request straddling midnight whose best line sat on a day frozen since was counted again on the open day. On equal `output` the earliest line now wins (before, the first line read won), so the choice no longer depends on which files a beat reads. Known limit, documented in the header comment: the window start is still filtered per line. - **A2**: `aggregate()` returns `readErrors` (unlistable directory, failed `stat` including `ENOENT`, stream or opener error; a missing root is still an empty scan). A beat with `readErrors > 0` freezes no new day (`nextState(..., { freeze: false })`), still pushes, and logs `claude-usage: warning: read_errors=N, no day frozen this beat`, with no path. - **A3**: `readBody()` reads the response chunk by chunk, counts bytes and cancels the stream past 256 KiB. It used to read everything, then compare a character count. - **A4**: new test for the `chmodSync` of a temp file left at mode 644. ## Docs (`ca33f10`) `CLAUDE.md` (case counts, gotcha on freezing), `agent/README.md` (new section "Unreadable transcripts"). ## Verification - `npm test`: 634/634 (611 + 23). `bash test-curl.sh`: 45/45. `sh scripts/boot-from-copy.sh`: PASS. - Mutations, one per fix, each restored from a backup copy: | Mutation | Red | |---|---| | remove `unlinkSync(tmpPath)` in `writeHostSnapshot` | 1 | | remove `unlinkSync(tmpPath)` in `writeClaudeUsage` | 1 | | filter frozen days before dedup again | 1 | | remove the earliest-line tie-break | 1 | | `readLines` resolves true on a stream error | 2 | | readdir error not counted | 1 | | stat error not counted | 1 | | root `ENOENT` counted | 1 | | opener throw not counted | 1 | | beat always freezes | 1 | | no `read_errors` log line | 1 | | `nextState` ignores `freeze` | 1 | | remove `chmodSync` | 1 | | remove `reader.cancel()` | 1 | | count characters instead of bytes | 1 | | `ENOENT` ignored on every directory, not only the root (from the review, fixed in `f694677`) | 1 | S2 was proven by test-first: 6 red before the fix. ## Post-merge checklist (hence `Refs`, not a closing keyword) 1. Redeploy Coolify (no auto-deploy), then check that `GET /claude-usage` and `GET /hosts` still return content. 2. Copy `agent/claude-usage/*` to `~/.local/share/maximus-claude-usage/` on the ThinkPad (the `cp` from `agent/README.md`), then check `journalctl -t claude-usage` on the next beat. 3. Once 1 and 2 are done, #54 is closed by hand. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

maximus added 3 commits 2026-10-08 00:30:35 +00:00

fix(server): require a full ISO receivedAt on read, cover the temp-file cleanup ... e2a24d768b

readClaudeUsage() and readHostSnapshot() validated the stored receivedAt
with new Date(), which accepts "1" or a bare date. Both now go through
isIsoTimestamp(), exported by claude-usage.js and built on the sanitizer's
own check, so no regex is duplicated.

The existing 500 tests failed at mkdir, before the temp file existed, so
removing the unlinkSync() cleanup of either writer left the suite green. Two
new cases make the rename fail after the temp write (the target is a
non-empty directory) and require that no temp file is left.

Refs #54

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(agent): no double count across midnight, no freeze on a read error, bounded limits read ... 123b3c3c8c

- aggregate: frozen days are dropped after deduplication, not before. A
  request straddling midnight whose best line sat on a day frozen since
  was counted again on the open day. On equal output the earliest line now
  wins (it was the first one read), so the choice no longer depends on
  which files a beat reads.
- aggregate: unreadable directories, failed stats (ENOENT included) and
  stream errors are counted in `readErrors`; a missing root is still an
  empty scan. The beat then freezes no new day (nextState freeze: false),
  still pushes, and logs `read_errors=N` with no path.
- limits: the response is read chunk by chunk and counted in bytes; past
  256 KiB the stream is cancelled instead of being read in full and
  measured in characters.
- state: a test now covers the chmodSync of a leftover temp file.

Each fix was checked by mutation (13 mutations, 13 red).

Refs #54

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs: document the read-error freeze rule and count the #54 cases ... ca33f10b85

Refs #54

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

maximus commented 2026-10-08 00:39:33 +00:00

Author

Owner

Copy link

Revue adversariale — PR #56

Verdict : APPROVE

Les six findings de #54 sont traites comme annonce, chacun avec un test qui rougit sous la mutation du defaut. Suite rejouee et mutations du relecteur executees sur une copie hors depot de la tete ca33f10.

Ce qui a ete execute

• npx vitest run sur la tete : 633/633. CI de la tete (CI / verification (pull_request)) : success.
• Mutations du relecteur (distinctes de celles du corps de la PR) :

Mutation	Rouges
tie-break inverse (entry.ts > prev.ts, la ligne la plus tardive gagne)	2
stat en ENOENT non compte (catch (err) { if (err.code !== "ENOENT") onError(); })	1
plafond de readBody() en >= au lieu de >	1
readHostSnapshot() revenu a new Date(receivedAt)	3
stream.on("error", finish) au lieu de fail	2
ENOENT ignore sur tout dossier, pas seulement la racine	0
rl.on("error", finish) au lieu de fail	0 (attendu : l'ecouteur du flux pose failed avant)

Points verifies

• S2 : isIsoTimestamp() reutilise cleanTimestamp() (regex + Date.parse), pas de regex dupliquee ; "1", date seule et heure sans decalage degradent en no_data / neverSeen.
• S1 : les deux nouveaux cas echouent bien au renameSync (cible = dossier non vide, echec meme en root en CI) et exigent l'absence de .tmp.
• A1 : ts est un nombre (Date.parse), donc la comparaison du tie-break est numerique et non lexicographique. Le filtre des jours figes apres deduplication est correct.
• A2 : la racine absente reste un scan vide ; nextState(..., { freeze: false }) conserve les jours deja figes et ne fige rien de neuf. Le cas du fichier durablement illisible est documente dans agent/README.md (rescan complet a chaque battement, ligne de log seule alerte). La ligne de log ne porte qu'un compte, aucun chemin.
• A3 : lecture par morceaux, octets comptes, reader.cancel() au-dela de 256 Kio ; le timeout du fetch couvre aussi la lecture du corps.
• Fermeture : timeline de #54 sans closes (ref_action: none), coherent avec le Refs #54 et la fermeture manuelle annoncee.

Suggestions (non bloquantes)

1. agent/claude-usage/aggregate.js (listTranscripts) : la distinction « ENOENT sur la racine = scan vide, sur un sous-dossier = erreur » n'est pas epinglee. Ma mutation qui ignore ENOENT sur tout dossier laisse les 633 cas verts. Un cas ou un sous-dossier disparait entre le readdir du parent et le sien (opener ou double de readdir) fermerait le trou. Le code est correct : epingle manquante, pas defaut.
2. Limite connue, adjacente a celle documentee : le seuil de scan par mtime suppose que toutes les lignes d'une requete vivent dans un fichier ecrit au plus tot le jour de sa derniere ligne. Une requete dupliquee dans un transcript de sous-agent dont le dernier ecrit precede le seuil pourrait encore etre vue sans sa ligne figee. Cas tres improbable ; une phrase dans l'en-tete suffirait si on veut l'acter.
3. Transition : les jours figes par l'ancienne version de l'agent ont ete tranches par « premiere ligne lue » en cas d'egalite d'output. En pratique c'est la ligne la plus ancienne du meme fichier, donc le meme choix ; rien a faire.

Actions faites

• Copie hors depot (scratchpad/pr56) supprimee en fin de revue ; arbre du depot inchange (?? reports/ seul, preexistant).
• Label de #54 : status:review -> status:approved.

## Revue adversariale — PR #56 **Verdict : APPROVE** Les six findings de #54 sont traites comme annonce, chacun avec un test qui rougit sous la mutation du defaut. Suite rejouee et mutations du relecteur executees sur une copie hors depot de la tete `ca33f10`. ### Ce qui a ete execute - `npx vitest run` sur la tete : **633/633**. CI de la tete (`CI / verification (pull_request)`) : success. - Mutations du relecteur (distinctes de celles du corps de la PR) : | Mutation | Rouges | |---|---| | tie-break inverse (`entry.ts > prev.ts`, la ligne la plus tardive gagne) | 2 | | `stat` en `ENOENT` non compte (`catch (err) { if (err.code !== "ENOENT") onError(); }`) | 1 | | plafond de `readBody()` en `>=` au lieu de `>` | 1 | | `readHostSnapshot()` revenu a `new Date(receivedAt)` | 3 | | `stream.on("error", finish)` au lieu de `fail` | 2 | | `ENOENT` ignore sur **tout** dossier, pas seulement la racine | **0** | | `rl.on("error", finish)` au lieu de `fail` | 0 (attendu : l'ecouteur du flux pose `failed` avant) | ### Points verifies - **S2** : `isIsoTimestamp()` reutilise `cleanTimestamp()` (regex + `Date.parse`), pas de regex dupliquee ; `"1"`, date seule et heure sans decalage degradent en `no_data` / `neverSeen`. - **S1** : les deux nouveaux cas echouent bien au `renameSync` (cible = dossier non vide, echec meme en root en CI) et exigent l'absence de `.tmp`. - **A1** : `ts` est un nombre (`Date.parse`), donc la comparaison du tie-break est numerique et non lexicographique. Le filtre des jours figes apres deduplication est correct. - **A2** : la racine absente reste un scan vide ; `nextState(..., { freeze: false })` conserve les jours deja figes et ne fige rien de neuf. Le cas du fichier durablement illisible est documente dans `agent/README.md` (rescan complet a chaque battement, ligne de log seule alerte). La ligne de log ne porte qu'un compte, aucun chemin. - **A3** : lecture par morceaux, octets comptes, `reader.cancel()` au-dela de 256 Kio ; le timeout du `fetch` couvre aussi la lecture du corps. - Fermeture : timeline de #54 sans `closes` (`ref_action: none`), coherent avec le `Refs #54` et la fermeture manuelle annoncee. ### Suggestions (non bloquantes) 1. `agent/claude-usage/aggregate.js` (`listTranscripts`) : la distinction « `ENOENT` sur la racine = scan vide, sur un sous-dossier = erreur » n'est pas epinglee. Ma mutation qui ignore `ENOENT` sur tout dossier laisse les 633 cas verts. Un cas ou un sous-dossier disparait entre le `readdir` du parent et le sien (opener ou double de `readdir`) fermerait le trou. Le code est correct : epingle manquante, pas defaut. 2. Limite connue, adjacente a celle documentee : le seuil de scan par `mtime` suppose que toutes les lignes d'une requete vivent dans un fichier ecrit au plus tot le jour de sa derniere ligne. Une requete dupliquee dans un transcript de sous-agent dont le dernier ecrit precede le seuil pourrait encore etre vue sans sa ligne figee. Cas tres improbable ; une phrase dans l'en-tete suffirait si on veut l'acter. 3. Transition : les jours figes par l'ancienne version de l'agent ont ete tranches par « premiere ligne lue » en cas d'egalite d'`output`. En pratique c'est la ligne la plus ancienne du meme fichier, donc le meme choix ; rien a faire. ### Actions faites - Copie hors depot (`scratchpad/pr56`) supprimee en fin de revue ; arbre du depot inchange (`?? reports/` seul, preexistant). - Label de #54 : `status:review` -> `status:approved`.

maximus referenced this pull request from a commit 2026-10-08 00:53:30 +00:00

test(agent): pin ENOENT as a read error below the root, note the subagent-copy limit

maximus added 1 commit 2026-10-08 00:53:31 +00:00

test(agent): pin ENOENT as a read error below the root, note the subagent-copy limit ... f694677a1a

Review of #56: making ENOENT an empty scan on every directory, not only the
root, left the suite green. A vanished subdirectory is now a pinned read
error. The header also names the other known limit of the midnight fix:
the mtime threshold may skip a subagent transcript holding the only copy
of a straddling request's earlier line.

Refs #54

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

maximus merged commit f694677a1a into main 2026-10-08 00:54:24 +00:00

maximus deleted branch issue-54-claude-usage-review-findings 2026-10-08 00:54:24 +00:00

maximus commented 2026-10-08 00:54:39 +00:00

Author

Owner

Copy link

Merged fast-forward at f694677, CI green on that head.

f694677 was added after the APPROVE and has not been reviewed: it applies suggestions 1 and 2 of the review (a test pinning ENOENT as a read error below the root, which turns the reviewer's zero-red mutation red; one sentence in the aggregate.js header on the subagent-copy limit). 634/634, test-curl.sh 45/45, boot-from-copy.sh PASS.

Suggestion 3 needs no action. #54 stays open until the post-merge checklist is done.

Merged fast-forward at `f694677`, CI green on that head. `f694677` was added after the APPROVE and has not been reviewed: it applies suggestions 1 and 2 of the review (a test pinning ENOENT as a read error below the root, which turns the reviewer's zero-red mutation red; one sentence in the `aggregate.js` header on the subagent-copy limit). 634/634, `test-curl.sh` 45/45, `boot-from-copy.sh` PASS. Suggestion 3 needs no action. #54 stays open until the post-merge checklist is done.

maximus referenced this pull request 2026-10-08 00:54:39 +00:00

claude-usage : constats non bloquants des revues de la pile #52/#51/#53 #54

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

autopilot:pending-human

PR produite par /autopilot, attend review humaine (jamais mergee par le run)

  

source:analyste

Créée par le Super-Analyste

  

source:defenseur

Créée par l Escouade Défenseur

  

source:human

Créée par un humain

  

source:medic

Créée par le Medic (auto-détection)

  

status:approved

PR approuvée, prête à merger

  

status:blocked

Bloquée (question, dépendance, clarification)

  

status:in-progress

Un agent (Medic/Dev) travaille dessus

  

status:needs-clarification

Issue bloquee par une ambiguite LOW pendant un run autonome, Max doit trancher

  

status:needs-fix

Reviewer demande des corrections

  

status:ready

Analysée et documentée, prête à prendre

  

status:review

PR créée, en attente du Reviewer

  

status:triage

Nouvelle issue, pas encore analysée

  

type:bug

Correction de bug → Medic

  

type:feature

Nouvelle fonctionnalité → Dev

  

type:infra

Config, CI/CD, déploiement → Dev

  

type:refactor

Refactoring / amélioration technique → Dev

  

type:schema

Modèle de données / migrations → Dev

  

type:security

Issue de sécurité → Medic ou Dev

No labels

autopilot:pending-human

source:analyste

source:defenseur

source:human

source:medic

status:approved

status:blocked

status:in-progress

status:needs-clarification

status:needs-fix

status:ready

status:review

status:triage

type:bug

type:feature

type:infra

type:refactor

type:schema

type:security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/vps-health-api#56

No description provided.