Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"schema": 1,
"task": "Ensure analytics report always populates periodStart/periodEnd fields via fallback derivation from session min/max timestamps, fix --last to also set filter.toDate, add ConfigLoader fallback for userEmail in BaseAgentAdapter, and guard against NaN/Infinity timestamps.",
"generated": "2026-07-27T00:00:00.000Z",
"dimensions": {
"component_scope": { "score": 3, "label": "M" },
"requirements_clarity": { "score": 1, "label": "XS" },
"technical_risk": { "score": 2, "label": "S" },
"file_change_estimate": { "score": 6, "label": "XXL" },
"dependencies": { "score": 1, "label": "XS" },
"affected_layers": { "score": 2, "label": "S" }
},
"total": 15,
"size": "M",
"routing": "brainstorming",
"key_reasoning": [
{
"dimension": "file_change_estimate",
"reason": "Diffstat reports 19 files changed; per the scoring table (16+ = XXL), this maps to score 6. Twelve of the nineteen are SDLC planning artifacts under docs/; actual production files are 7 (payload-builder.ts, index.ts, BaseAgentAdapter.ts, types.ts, plus 3 test files), but the guide mandates mapping N from diffstat directly."
},
{
"dimension": "component_scope",
"reason": "Three subsystems touched: analytics CLI command (index.ts), analytics payload builder (payload-builder.ts), and the core agent adapter base class (BaseAgentAdapter.ts). Bumped from S to M because BaseAgentAdapter is the shared base class inherited by all agent adapters."
},
{
"dimension": "affected_layers",
"reason": "Two distinct architectural layers: Service/CLI layer (analytics/index.ts + report/payload-builder.ts) and Agent-Tool layer (BaseAgentAdapter.ts). Coordination is straightforward over established interfaces."
}
],
"red_flags_applied": [
"Component Scope bumped from S (2) to M (3): BaseAgentAdapter.ts is a core shared base class used by all agent adapters — falls under 'Touches core shared utilities'"
],
"split_recommendation": null
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
diff --git a/src/cli/commands/analytics/report/__tests__/payload-builder.test.ts b/src/cli/commands/analytics/report/__tests__/payload-builder.test.ts
index c5d35f4..1cd3911 100644
--- a/src/cli/commands/analytics/report/__tests__/payload-builder.test.ts
+++ b/src/cli/commands/analytics/report/__tests__/payload-builder.test.ts
@@ -366,6 +366,31 @@ describe('buildPayload', () => {
expect(payload.meta.periodEnd).toBeUndefined();
});

+ it('skips records with non-finite startTime or duration and does not throw', () => {
+ // Guard against RangeError from new Date(NaN|Infinity).toISOString() — a single
+ // malformed session must not crash report generation for every other session.
+ const nanDur = session({ sessionId: 's-nan-dur', startTime: 1_700_000_000_000, duration: Number.NaN });
+ const infStart = session({ sessionId: 's-inf-start', startTime: Number.POSITIVE_INFINITY, duration: 60_000 });
+ const good = session({ sessionId: 's-good', startTime: 1_700_000_300_000, duration: 60_000 });
+ const rootMixed = {
+ ...root,
+ projects: [
+ { projectPath: '/repo/app', branches: [{ branchName: 'main', sessions: [nanDur, infStart, good] }] },
+ ],
+ } as unknown as RootAnalytics;
+ const costIndexMixed: SessionCostIndex = new Map([
+ ['s-nan-dur', { sessionId: 's-nan-dur', tokens: { input: 1, output: 1, cacheRead: 0, cacheCreation: 0, total: 2 }, costUSD: 0, perModel: [], priced: true, hadLog: true }],
+ ['s-inf-start', { sessionId: 's-inf-start', tokens: { input: 1, output: 1, cacheRead: 0, cacheCreation: 0, total: 2 }, costUSD: 0, perModel: [], priced: true, hadLog: true }],
+ ['s-good', { sessionId: 's-good', tokens: { input: 1, output: 1, cacheRead: 0, cacheCreation: 0, total: 2 }, costUSD: 0, perModel: [], priced: true, hadLog: true }],
+ ]);
+ const payload = buildPayload(rootMixed, costIndexMixed, summary, ctxAll);
+ // nanDur contributes its finite startTime (NaN duration collapses to 0 → endMs=startTime).
+ // infStart is skipped entirely (non-finite startTime fails the guard).
+ // good extends maxEndMs past nanDur's startTime.
+ expect(payload.meta.periodStart).toBe(new Date(1_700_000_000_000).toISOString());
+ expect(payload.meta.periodEnd).toBe(new Date(1_700_000_300_000 + 60_000).toISOString());
+ });
+
it('skips records with startTime <= 0 when computing min/max', () => {
const zero = session({ sessionId: 's-zero', startTime: 0, duration: 60_000 });
const valid = session({ sessionId: 's-valid', startTime: 1_700_000_000_000, duration: 60_000 });
diff --git a/src/cli/commands/analytics/report/payload-builder.ts b/src/cli/commands/analytics/report/payload-builder.ts
index ba6eea8..18ccd35 100644
--- a/src/cli/commands/analytics/report/payload-builder.ts
+++ b/src/cli/commands/analytics/report/payload-builder.ts
@@ -46,11 +46,16 @@ export function buildPayload(
continue;
}
seen.add(s.sessionId);
- if (s.startTime > 0) {
+ // Guard both fields with Number.isFinite so a single malformed session
+ // (NaN/Infinity from a corrupted timestamp event or unreliable native log)
+ // does not propagate into new Date(...).toISOString() and throw RangeError,
+ // which would crash report generation for every user.
+ if (Number.isFinite(s.startTime) && s.startTime > 0) {
if (minStartMs === undefined || s.startTime < minStartMs) {
minStartMs = s.startTime;
}
- const endMs = s.startTime + Math.max(s.duration ?? 0, 0);
+ const dur = Number.isFinite(s.duration) ? Math.max(s.duration, 0) : 0;
+ const endMs = s.startTime + dur;
if (maxEndMs === undefined || endMs > maxEndMs) {
maxEndMs = endMs;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"decision": "approve",
"rationale": "CR-001 is resolved. Fix-up diff (payload-builder.ts +7/-2, payload-builder.test.ts +25) adds Number.isFinite guards on both s.startTime (composed with the existing > 0 check) and s.duration (coerced to 0 when non-finite) exactly as CR-001 recommended, plus a new Vitest case with mixed NaN duration + Infinity startTime + a good session that asserts derived period is stable and no exception is thrown. The Infinity startTime session is correctly skipped by the composite guard; the NaN duration session contributes its finite startTime with duration collapsed to 0. Full test suite now green (20/20 payload-builder cases). No new high-risk issues introduced by the fix-up: the change makes the code more defensive, adds no I/O, no dependencies, no schema changes, and no new attack surface. Standards + security carried forward from the final round unchanged.",
"confidence": "high",
"risk_flags": [],
"business_review": [
{"kind": "spec", "item": "buildPayload tracks minStartMs and maxEndMs in the flatten loop", "status": "pass", "notes": "unchanged from final round; guards strengthened"},
{"kind": "spec", "item": "meta.periodStart precedence: ctx → derived → omit", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "meta.periodEnd precedence: ctx → derived → omit", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "No generatedAt fallback — fields omitted when no session data", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "buildPayload remains pure (no I/O, no config reads, no env access)", "status": "pass", "notes": "fix-up adds Number.isFinite only — no I/O"},
{"kind": "spec", "item": "parseFilterOptions sets filter.toDate = new Date() for --last", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "BaseAgentAdapter maybeWriteSessionReport falls back to ConfigLoader for userEmail", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "ReportMeta fields remain TypeScript-optional", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "rangeLabel semantics unchanged", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "session-report.ts unchanged", "status": "pass", "notes": "unchanged"},
{"kind": "spec", "item": "CLI surface unchanged (help, exit codes)", "status": "pass", "notes": "unchanged"},
{"kind": "story-ac", "item": "All four failing modes produce populated meta.userEmail/periodStart/periodEnd", "status": "pass", "notes": "unchanged"},
{"kind": "story-ac", "item": "--from/--to behavior unchanged (regression-guarded)", "status": "pass", "notes": "unchanged"},
{"kind": "story-ac", "item": "npm run lint, typecheck, build all clean", "status": "pass", "notes": "re-verified after fix-up commit"},
{"kind": "story-ac", "item": "No changes to CLI help text or exit codes", "status": "pass", "notes": "unchanged"}
],
"standards_review": [
{"kind": "commit-format", "status": "pass", "notes": "Conventional Commits: fix(analytics): guard buildPayload period derivation against NaN/Infinity (CR-001)"},
{"kind": "code-quality", "status": "pass", "notes": "Number.isFinite is the standard idiomatic guard; project already uses it in aggregator.ts:400"},
{"kind": "security", "status": "pass", "notes": "no new attack surface; guards make code stricter"}
],
"findings": [],
"finding_status": [
{"id": "CR-001", "status": "resolved", "notes": "Number.isFinite(s.startTime) && s.startTime > 0 gates the min/max block; s.duration coerced to 0 via Number.isFinite(...) ? Math.max(...,0) : 0. New Vitest case asserts NaN duration + Infinity startTime do not throw and derived period is stable. Fix-up commit 021a0cd."}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"decision": "request-changes",
"rationale": "All three lenses ran cleanly. One real robustness defect surfaced: the new period-derivation math in buildPayload has no NaN/Infinity guard, so a single malformed session (NaN or Infinity in startTime or duration) makes maxEndMs/minStartMs NaN, then new Date(NaN).toISOString() throws RangeError and crashes report generation — a strictly worse failure mode than the pre-fix behavior of silently omitting the fields. Acceptance lens's 'durationMs vs duration' finding is a false positive (spec described the output ReportSessionRecord.durationMs field, while the code correctly reads input SessionAnalytics.duration and maps it via the pre-existing durationMs: s.duration pattern on line 82). Blind's --last + --to clobber concern is real but extends a pre-existing pattern (--last has always overwritten --from too), not a regression introduced by this diff — defer to a separate ticket. Other blind concerns (silent catch, dynamic import on each call, all-startTime-zero edge case, semantic contract change) are consistent with documented project patterns or are edge cases outside realistic input. 1 patch, 4 dismiss, 2 defer.",
"confidence": "high",
"risk_flags": [],
"business_review": [
{"kind": "spec", "item": "buildPayload tracks minStartMs and maxEndMs in the flatten loop", "status": "pass", "notes": "payload-builder.ts declares trackers before loop, updates with startTime > 0 guard"},
{"kind": "spec", "item": "meta.periodStart precedence: ctx → derived → omit", "status": "pass", "notes": "payload-builder.ts:155-166 ternary matches"},
{"kind": "spec", "item": "meta.periodEnd precedence: ctx → derived → omit", "status": "pass", "notes": "same block"},
{"kind": "spec", "item": "No generatedAt fallback — fields omitted when no session data", "status": "pass", "notes": "ternary falls through to omit branch"},
{"kind": "spec", "item": "buildPayload remains pure (no I/O, no config reads, no env access)", "status": "pass", "notes": "only in-memory declarations and arithmetic added"},
{"kind": "spec", "item": "parseFilterOptions sets filter.toDate = new Date() for --last", "status": "pass", "notes": "analytics/index.ts:278"},
{"kind": "spec", "item": "BaseAgentAdapter maybeWriteSessionReport falls back to ConfigLoader for userEmail", "status": "pass", "notes": "BaseAgentAdapter.ts:79-89 with existing non-fatal try/catch"},
{"kind": "spec", "item": "ReportMeta fields remain TypeScript-optional", "status": "pass", "notes": "types.ts delta is JSDoc-only"},
{"kind": "spec", "item": "rangeLabel semantics unchanged", "status": "pass", "notes": "no diff touches rangeLabel"},
{"kind": "spec", "item": "session-report.ts unchanged", "status": "pass", "notes": "file absent from diff"},
{"kind": "spec", "item": "CLI surface unchanged (help, exit codes)", "status": "pass", "notes": "no Commander option or process.exit added"},
{"kind": "story-ac", "item": "All four failing modes produce populated meta.userEmail/periodStart/periodEnd", "status": "pass", "notes": "E2E verified in Stage 5 Task 5 across bare, --last, --session, --project/--branch"},
{"kind": "story-ac", "item": "--from/--to behavior unchanged (regression-guarded)", "status": "pass", "notes": "E2E verified; ctx precedence preserves user-supplied dates"},
{"kind": "story-ac", "item": "npm run lint, typecheck, build all clean", "status": "pass", "notes": "run after each task in Stage 5; all zero-issue"},
{"kind": "story-ac", "item": "No changes to CLI help text or exit codes", "status": "pass", "notes": "confirmed absent from diff"}
],
"standards_review": [
{"kind": "commit-format", "status": "pass", "notes": "Conventional Commits: fix(analytics): / fix(agents): with body + Ref EPMCDME-13643"},
{"kind": "code-quality", "status": "pass", "notes": "ES modules with .js imports, no any, interface shapes, non-fatal try/catch pattern preserved, dynamic import pattern matches existing"},
{"kind": "security", "status": "pass", "notes": "no new credentials, no injection surface, ConfigLoader read is inside existing try/catch; no path traversal"}
],
"findings": [
{
"id": "CR-001",
"severity": "major",
"triage": "patch",
"file": "src/cli/commands/analytics/report/payload-builder.ts",
"line": 49,
"title": "Missing NaN/Infinity guard in period derivation crashes report on malformed session data",
"problem": "The new min/max block guards only on s.startTime > 0. If s.startTime or s.duration is NaN or Infinity (e.g. from a session with a malformed timestamp event where endTime-startTime arithmetic yielded NaN, or from a corrupted native agent log), the arithmetic propagates: Math.max(NaN, 0) === NaN in JavaScript, so endMs = startTime + NaN = NaN, and new Date(NaN).toISOString() throws 'RangeError: Invalid time value'. That RangeError bubbles up through buildPayload and crashes the entire report generation. Same failure mode if s.startTime itself is Infinity: minStartMs = Infinity, then new Date(Infinity).toISOString() also throws RangeError. The pre-fix code silently omitted the fields on any data anomaly; the new code makes an anomaly fatal — a strictly worse contract for the caller.",
"impact": "One bad session in the dataset kills report generation for every user — bare, --last, --session, --project/--branch. Silent NaN was already a latent issue in the headline totals (which also arithmetically absorb it), but the throw path is new to this diff.",
"recommendation": "Guard both fields with Number.isFinite before use. Concretely, change the min/max block to: if (Number.isFinite(s.startTime) && s.startTime > 0) { ... const dur = Number.isFinite(s.duration) ? Math.max(s.duration, 0) : 0; const endMs = s.startTime + dur; ... }. Add a regression test in payload-builder.test.ts asserting that a session with NaN duration does not throw and that the field is either omitted or derived from a finite peer session."
}
]
}
Loading
Loading