Repository navigation
Surface AWF steering counters in compact audit usage data - #63664
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Event-file selection and typed payload decoding can silently omit valid steering events.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds per-type AWF steering counters to compact usage artifacts and exposes them through audit token-usage reporting.
Changes:
- Aggregates steering events across supported filenames and schemas.
- Backfills usage-only audit results with steering totals and counters.
- Extends schemas, documentation, and regression coverage.
| File | Description |
|---|---|
.changeset/minor-audit-gateway-steering.md |
Updates the release note. |
actions/setup/js/generate_usage_activity_summary.cjs |
Adds steering aggregation to usage summaries. |
actions/setup/js/generate_usage_activity_summary.test.cjs |
Tests summary steering aggregation. |
actions/setup/js/steering_helpers.cjs |
Adds per-type event counting. |
actions/setup/js/steering_helpers.test.cjs |
Tests schema variants and counters. |
docs/src/content/docs/reference/artifacts.md |
Documents compact steering data. |
pkg/cli/audit_analysis_fanout.go |
Backfills usage-only audit results. |
pkg/cli/logs_usage_activity.go |
Parses and applies compact steering summaries. |
pkg/cli/logs_usage_activity_test.go |
Tests backfill and preservation behavior. |
pkg/cli/token_usage_analyze.go |
Applies steering summaries during analysis. |
pkg/cli/token_usage_find.go |
Adds alternate event-log discovery. |
pkg/cli/token_usage_steering.go |
Aggregates normalized steering event types. |
pkg/cli/token_usage_test.go |
Tests discovery and aggregation. |
pkg/cli/token_usage_types.go |
Extends token usage and event structures. |
schemas/audit.schema.json |
Adds steering counters to the audit contract. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| for _, relativePath := range relativePaths { | ||
| candidate := filepath.Join(root, relativePath) | ||
| if fileutil.FileExists(candidate) { | ||
| return candidate |
There was a problem hiding this comment.
Addressed in d66cd7e: event-log discovery skips empty candidates and continues through supported roots, including legacy layouts, until it finds steering data. Regression tests cover the empty preferred file and legacy fallback.
| Payload *struct { | ||
| Event string `json:"event"` | ||
| Type string `json:"type"` | ||
| } `json:"payload,omitempty"` |
There was a problem hiding this comment.
Addressed in 4235a13: payload decoding is flexible, and nested event/type fields are read only from object payloads. Regression cases cover non-object string and array payloads.
ADR requiredThis PR introduces an architectural decision that is not currently recorded in Evidence reviewed
Next actionPlease review and refine the draft ADR, then keep it in the PR as the explicit record of this design decision.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
REQUEST_CHANGES
The new steering-counter paths short-circuit on the first candidate proxy-event file. In mixed-version or partially populated artifacts, that drops real steering data instead of falling through to the later events.jsonl/audit fallback that actually contains it.
Blocking themes
generate_usage_activity_summary.cjsnow returnsnullas soon as the first readable candidate has zero steering entries, so compact usage artifacts can silently omit counters.findAPIProxyEventsFilestill resolves by file existence alone, which lets an empty or steering-free preferred filename mask a later populated fallback and undercountfirewall_token_usage.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 49.5 AIC · ⌖ 9.14 AIC · ⊞ 20.2K
Comment /review to run again
| } | ||
| const eventCounts = countSteeringEventsByTypeInApiProxyJsonl(fs.readFileSync(eventLogPath, "utf-8")); | ||
| const totalEvents = Object.values(eventCounts).reduce((total, count) => total + count, 0); | ||
| return totalEvents > 0 ? { total_events: totalEvents, event_counts: eventCounts } : null; |
There was a problem hiding this comment.
This short-circuits on the first readable candidate file, so a present event-logs.jsonl with no steering entries prevents fallback to events.jsonl and silently drops real steering counters from usage/activity/summary.json.
💡 Why this blocks the change
The rest of the codebase treats event-logs.jsonl and events.jsonl as fallbacks for the same signal source, but this helper returns null as soon as the first non-empty file parses to zero steering events. That means mixed-version artifacts can lose steering metrics even though a later candidate file has them.
Please keep iterating when totalEvents === 0 and only return once you find a candidate with steering events, or after every candidate has been checked.
There was a problem hiding this comment.
Addressed in 4235a13: the compact collector continues to later event-log candidates unless it finds steering events. The regression test covers empty and steering-free candidates before the populated fallback.
| } { | ||
| for _, relativePath := range relativePaths { | ||
| candidate := filepath.Join(root, relativePath) | ||
| if fileutil.FileExists(candidate) { |
There was a problem hiding this comment.
This returns the first existing proxy log path, so a steering-free event-logs.jsonl in sandbox/firewall/logs will mask a populated events.jsonl or audit fallback and under-report firewall_token_usage.
💡 Why this blocks the change
The new logic added a second filename variant, but the resolver still exits on existence alone. If both variants are present during a transition, analyzeTokenUsage() will parse whichever file happens to exist first, even when that file contains no steering events and a later candidate does.
Please mirror the fallback behavior used elsewhere for proxy-event reads: keep searching until you find a candidate that actually carries steering data, or make the downstream parser try the ordered candidate list instead of a single path.
There was a problem hiding this comment.
Addressed in d66cd7e: Go event-log discovery checks the ordered candidates for steering data before falling back, so an existing steering-free file no longer masks a populated logs, audit, or legacy candidate. Regression coverage exercises fallback across layouts.
There was a problem hiding this comment.
Impeccable Skills Review — PR #63664
This PR is a backend/data-plumbing change (steering-event counters surfaced in usage/audit JSON), not a UI change, so the UI-focused Impeccable modes (critique, harden, distill, extract, clarify) don't apply directly. I reviewed it as mixed_unclear using correctness/reliability/maintainability criteria instead.
Findings: none blocking.
- Go (
token_usage_steering.go,logs_usage_activity.go,audit_analysis_fanout.go) and JS (steering_helpers.cjs,generate_usage_activity_summary.cjs) implementations are consistent: both normalize event names to lowercase, match"steering"or*_steeringsuffix, and aggregate per-type counts the same way. applyUsageActivitySteeringSummarycorrectly guards against overwriting aTotalSteeringEventsthat's already populated from detailed firewall analysis (verified byTestApplyUsageActivitySteeringSummaryPreservesDetailedAnalysis).- New multi-path file discovery (
event-logs.jsonl/events.jsonl,logs/auditroots, legacyfirewall-audit-logs) is covered by table-driven tests. - Schema (
audit.schema.json) and docs (artifacts.md) were updated alongside the code. go build,go vet, and the targeted Go test suite (TestApplyUsageActivity*,TestLoadUsageActivitySummary*,TestFindAPIProxyEventsFile,TestAnalyzeTokenUsage*) all pass locally.- JS vitest suite could not be executed in this sandbox (network-restricted
npm installfailure unrelated to the PR), but the added JS tests mirror the passing Go equivalents and the logic is straightforward.
No actionable changes requested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 106 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
@copilot run pr-finisher skill |
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>
Ran the PR finisher. Fixed steering-log fallback and non-object payload handling, added regression tests, and committed generated schemas in 7a9e4fc, 4235a13, and d66cd7e. Local build, lint, and tests pass. CI must be re-triggered by a maintainer after the agent push; review-thread resolution and human approval remain outstanding. |
|
|
|
🎉 This pull request is included in a new release. Release: |

Audit required full firewall artifacts to inspect AWF steering behavior. Compact usage artifacts now expose aggregate counters for each steering event type.
Usage payload
usage/activity/summary.json.Audit reporting
firewall_token_usagefor usage-only audits.Contract
{ "total_steering_events": 3, "steering_event_counts": { "token_steering": 2, "timeout_steering": 1 } }