[code-simplifier] Simplify AI credits audit log parsing: extract shared helper, eliminate double file read#37731
Conversation
…it log read ai_credits_context.cjs: - Extract iterateAuditEntries() generic JSONL scaffolding helper (path resolution, existsSync, readFileSync, line-split loop, JSON.parse with catch, outer try/catch) — eliminates ~40 lines of near-identical boilerplate duplicated across parseMaxAICreditsFromAuditLog and parseAICreditsErrorInfoFromAuditLog - Add parseAuditLogCombined() single-pass helper that accumulates all three fields (aiCredits, maxAICredits, rateLimitError) in one file read - resolveAICreditsFailureState now calls parseAuditLogCombined() once instead of calling two separate log-parse functions (was: 2x existsSync + 2x readFileSync + 2x full line iteration on every failure-state resolution) - Use inner loop over ['log.jsonl', 'audit.jsonl'] in resolveFirewallAuditLogPath to replace two copy-pasted probe blocks (simplification D) - Remove unreachable candidateBases[0] || fallback in resolveFirewallAuditLogPath — two unconditional pushes guarantee the array is never empty (simplification C) - Document intentional asymmetry: parseAICreditsErrorInfoFromAuditLog has no content-guard fast-path because AI_CREDITS_RATE_LIMIT_TEXT_FIELDS contains ubiquitous field names; a pre-scan would almost never skip iteration check_daily_aic_workflow_guardrail.cjs: - Collapse shouldSkipDailyAICGuardrail into a single boolean expression - Remove unreachable Math.round(value || 0) dead code in formatInteger — Number.isFinite already excludes non-finite values; 0/-0 are unaffected - Move let runs = [] inside the while-loop body as const runs — was declared before the loop but unconditionally reassigned on every iteration - Use ...summaryMeta spread in logDailyGuardrail call to avoid re-listing the same two property names already in the summaryMeta object All public-API signatures and behavior preserved. No logic changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the AI Credits audit-log parsing and daily AIC guardrail logic to reduce duplication and avoid redundant file reads, while preserving the existing public API surface and behavior.
Changes:
- Extracts a shared JSONL iteration helper in
ai_credits_context.cjsand introduces a single-pass combined parser to avoid double-reading the firewall audit log. - Simplifies several small logic patterns in the daily AIC workflow guardrail (boolean return, dead fallback removal, tighter scoping, and object spread).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/ai_credits_context.cjs | Extracts shared JSONL parsing helper and consolidates audit-log parsing into a single pass for failure-state resolution. |
| actions/setup/js/check_daily_aic_workflow_guardrail.cjs | Simplifies guardrail conditionals and minor dead-code patterns; improves local variable scoping and logging call shape. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #37731 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (well under the 100-line threshold). No custom config present. |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #37731 only changed actions/setup/js/ai_credits_context.cjs (+81/-42) and actions/setup/js/check_daily_aic_workflow_guardrail.cjs (+8/-10) — neither is a test file. Test Quality Sentinel skipped. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /improve-codebase-architecture — approving with minor observations.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
iterateAuditEntriesis the new core abstraction butai_credits_context.cjshas no test file (see inline comment). - Undocumented logging change:
...summaryMetaspread addstruncatedByRateLimitto thelogDailyGuardrailcall; this is correct context but is a behavioural diff from the old explicit listing. - Missing rationale for
nullcontentGuard inparseAuditLogCombined: the design choice is sound (one full pass < two guarded passes) but the JSDoc does not explain it.
Positive Highlights
- ✅ Dead-code removals are all correct: the
|| "/tmp/gh-aw/sandbox/firewall/audit"fallback is provably unreachable, and|| 0inside aNumber.isFiniteguard is provably a no-op. - ✅
const runsscoping fix is textbook; theletdeclaration before the loop was a false signal of shared state. - ✅
iterateAuditEntriesaccumulator callbacks faithfully preserve last-wins semantics foraiCredits/maxAICreditsand monotonic-true forrateLimitError. - ✅
parseAuditLogCombinedcorrectly eliminates the 2× file read inresolveAICreditsFailureStatewith no behavioural change to the returned values. - ✅ The
contentGuardasymmetry betweenparseMaxAICreditsFromAuditLogandparseAICreditsErrorInfoFromAuditLogis well-documented and intentional.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 236.6 AIC · ⌖ 25.6 AIC
| * @returns {T} | ||
| */ | ||
| function parseMaxAICreditsFromAuditLog(auditJsonlPathOverride) { | ||
| function iterateAuditEntries(auditJsonlPathOverride, defaultValue, contentGuard, accumulate) { |
There was a problem hiding this comment.
[/tdd] iterateAuditEntries is a non-trivial pure function with several independently-testable edge cases — but no test file exists for this module (unlike peer modules such as parse_mcp_gateway_log.test.cjs, error_codes.test.cjs, etc.).
💡 Suggested edge cases to cover
// 1. Missing file → returns defaultValue
// 2. Empty file → returns defaultValue
// 3. contentGuard returns false → returns defaultValue without parsing
// 4. All lines are malformed JSON → returns defaultValue unchanged
// 5. Accumulate is called per-entry in order
// 6. Combined: only error-info entries do not bleed into maxAICredits fieldThe parseAuditLogCombined single-pass semantics (both parsers run on every line) are particularly worth a test to guard against future regressions when either entry-parser changes.
| logDailyGuardrail("Completed AIC inspection window", { | ||
| candidateRunsCount: summaryMeta.candidateRunsCount, | ||
| inspectedRunsCount: summaryMeta.inspectedRunsCount, | ||
| ...summaryMeta, |
There was a problem hiding this comment.
[/zoom-out] The spread silently adds a third field to the log that was not there before: truncatedByRateLimit is now emitted alongside candidateRunsCount and inspectedRunsCount. This is likely desirable (it is relevant context), but it is a behavioural change — log consumers or audit tooling may not expect it.
💡 Options
If the intent is truly to spread everything, add a short comment:
// Spreads candidateRunsCount, inspectedRunsCount, truncatedByRateLimit
...summaryMeta,If only the two original fields were intended, keep them explicit:
candidateRunsCount: summaryMeta.candidateRunsCount,
inspectedRunsCount: summaryMeta.inspectedRunsCount,The JSDoc @type above summaryMeta pins its shape, so future field additions would still require a deliberate edit — the spread is safe today, just undocumented as a change.
| * @returns {{ aiCredits: string, maxAICredits: string, rateLimitError: boolean }} | ||
| */ | ||
| function parseAuditLogCombined(auditJsonlPathOverride) { | ||
| return iterateAuditEntries( |
There was a problem hiding this comment.
[/improve-codebase-architecture] parseAuditLogCombined passes null for contentGuard, which drops the fast-path that parseMaxAICreditsFromAuditLog uses (the /(?:max_ai_credits|maxAiCredits)/ content scan). The trade-off is correct — you must iterate for the rate-limit signal anyway, so one full pass beats two guarded passes — but the JSDoc does not explain this reasoning.
💡 Suggested JSDoc addition
/**
* Single-pass combined read of the audit log...
*
* No contentGuard is applied: because the rate-limit signal must be scanned
* regardless, a full single pass is cheaper than two guarded passes even when
* max_ai_credits entries are absent.
*/A future maintainer seeing null for contentGuard alongside an accumulator that also handles maxAICredits might re-add the guard unnecessarily without this context.
There was a problem hiding this comment.
Non-blocking observations on iterateAuditEntries
The core refactor is correct — single-pass combined read eliminates the double file I/O, accumulate semantics match the original last-value-wins behavior for all three fields, and the check_daily_aic_workflow_guardrail.cjs cleanups are safe. Two advisory concerns about the new generic abstraction are noted inline.
🔎 Code quality review by PR Code Quality Reviewer · 20.7 AIC · ⌖ 13.4 AIC
| let parsedMaxAICredits = ""; | ||
| if (!content.trim()) return defaultValue; | ||
| if (contentGuard && !contentGuard(content)) return defaultValue; | ||
| let result = defaultValue; |
There was a problem hiding this comment.
defaultValue aliased into result — mutation-safe only by convention, not contract. Any accumulate callback that mutates acc in-place instead of returning a fresh object will silently corrupt the defaultValue reference passed by the caller.
💡 Details and suggested fix
Currently all callers pass inline object literals ({ aiCredits: "", ... }), so each call site creates a fresh object and corruption is limited to a single invocation. However the function is a new generic abstraction that future contributors will re-use. Nothing in the signature or JSDoc prevents passing a module-level constant as defaultValue, which would be permanently corrupted on the first mutating accumulate.
Simplest fix: note the invariant in the JSDoc and optionally shallow-clone the seed:
// Option A: document the contract
/**
* `@param` {(acc: T, entry: unknown) => T} accumulate - MUST return a new value/object; must not mutate acc.
*/
// Option B: defensive clone (only needed if T is an object)
let result = (defaultValue !== null && typeof defaultValue === "object")
? { ...defaultValue }
: defaultValue;Without this, the first caller that accidentally writes acc.field = value; return acc; will introduce a silent state-pollution bug that is very hard to diagnose.
| const entry = JSON.parse(trimmed); | ||
| const value = parseMaxAICreditsFromAuditEntry(entry); | ||
| if (value) parsedMaxAICredits = value; | ||
| result = accumulate(result, JSON.parse(trimmed)); |
There was a problem hiding this comment.
accumulate returning undefined silently breaks the defaultValue fallback contract. If any accumulate callback forgets a return statement, result becomes undefined and iterateAuditEntries returns undefined — not defaultValue — because returning undefined does not throw and the outer catch never fires.
💡 Why this matters and how to fix it
The outer try/catch at line 165 only rescues thrown exceptions; a return undefined from accumulate silently escapes it. The propagation chain:
accumulate(result, entry)returnsundefined→result = undefined- Next iteration:
accumulate(undefined, entry)tries to readacc.aiCredits→ throwsTypeError - Inner catch ignores it;
resultstaysundefined iterateAuditEntriesreturnsundefinedinstead ofT- Caller destructures:
const { aiCredits } = parseAuditLogCombined()→TypeError: Cannot destructure property ... of undefined
This crashes resolveAICreditsFailureState with an unhandled exception — not a silent default.
Fix options:
// Option A: guard the return value
result = accumulate(result, JSON.parse(trimmed)) ?? result;
// Option B: add an assertion comment in JSDoc
// `@param` {(acc: T, entry: unknown) => T} accumulate - must always return a defined value; returning undefined is a bug.Option A is more defensive and maintains the invariant automatically.
|
@copilot review all comments and the reviews, create a commit, then apply the copilot review skill. |
|
CI is failing on js-typecheck and lint-js; please address those failures and push an update.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed all in-scope comments/reviews, addressed the CI failures ( |
pelikhan
deleted the
simplify/ai-credits-audit-log-parsing-88a1fa719fce87c4
branch
What changed
Two production files simplified after analysis from scope-filter and simplification-scout agents. All public-API signatures and runtime behavior are preserved.
actions/setup/js/ai_credits_context.cjsA — Extract
iterateAuditEntries()scaffolding helperparseMaxAICreditsFromAuditLogandparseAICreditsErrorInfoFromAuditLogshared ~40 lines of near-identical JSONL boilerplate (path resolution,existsSync,readFileSync, line-split loop,JSON.parsewith catch, outer try/catch). Extracted into a genericiterateAuditEntries(pathOverride, defaultValue, contentGuard, accumulate)helper. Each caller now supplies only its per-entry accumulation logic.B — Eliminate double audit log read in
resolveAICreditsFailureStatePreviously called both parse functions independently (2×
existsSync+ 2×readFileSync+ 2× full line iteration for the same file). AddedparseAuditLogCombined()— a single-pass reader that accumulates{ aiCredits, maxAICredits, rateLimitError }in one sweep.resolveAICreditsFailureStatenow calls this once.C — Remove dead
||fallback inresolveFirewallAuditLogPathcandidateBases[0] || "/tmp/gh-aw/sandbox/firewall/audit"was unreachable: two unconditional pushes always populate the array. Removed the dead fallback arm.D — Inner loop for filename probing
Replaced two copy-pasted
log.jsonl/audit.jsonlprobe blocks withfor (const filename of ["log.jsonl", "audit.jsonl"]), making the try-each-filename intent explicit.E — Follow-up robustness/typecheck fixes
tsc --noEmit.iterateAuditEntries()now defensively ignoresundefinedaccumulator returns, preserving prior accumulated state.parseAuditLogCombined()intentionally does not use acontentGuard(single full pass is still cheaper because rate-limit signal detection must scan all entries).actions/setup/js/check_daily_aic_workflow_guardrail.cjsshouldSkipDailyAICGuardrailinto a single boolean outcome with clearer named booleans|| 0informatInteger, while keeping explicit numeric narrowing for type safetylet runs = []inside loop body asconst runs— was declared before the while-loop but unconditionally reassigned on every iteration and never read after the loopcandidateRunsCountandinspectedRunsCount(without broad spreading ofsummaryMeta)Validation
npm run lint:cjspassesnpm run typecheckpassesnpm run test:jspassescheck_daily_aic_workflow_guardrail.test.cjspassesSource references
439a16f— Fix failing pin test and normalize wasm golden parity (Stabilize pinning tests and unify wasm golden normalization #37711)References: §27116645729
> Generated by 🔧 Code Simplifier · 1.5K AIC · ⌖ 27.5 AIC · ⊞ 20.9K · ◷
> - [x] expires on Jun 8, 2026, 9:05 PM UTC-08:00