Repository navigation
Emit AWF agentTimeout for literal timeouts and show delivered steering notices in audit/logs - #67597
SivaKesava1 with Copilot wants to merge 4 commits into
Conversation
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1071 new lines across 📄 Draft ADR committed:
🔍 Decision inferred from the diff
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
🟡 Changes recommended
The unreleased AWF gate is bypassed by latest, and fractional thresholds can be misreported as valid notices.
3 open findings
What changed in this PR
Adds AWF runtime steering deadlines and preserves delivered steering notices through unified sessions, audit, and logs.
Changes:
- Emits version-gated
container.agentTimeout. - Projects steering notices into unified-session and CLI reports.
- Adds schemas, documentation, and comprehensive fixtures/tests.
| File | Description |
|---|---|
schemas/logs.schema.json |
Adds steering notice fields. |
schemas/logs-jsonl.schema.json |
Updates JSONL report schema. |
schemas/audit.schema.json |
Updates audit schema. |
pkg/workflow/awf_feature_flags.go |
Adds the AWF capability gate. |
pkg/workflow/awf_config_build.go |
Resolves container agent timeouts. |
pkg/workflow/awf_agent_timeout_test.go |
Tests timeout compilation. |
pkg/constants/version_constants.go |
Defines the minimum AWF version. |
pkg/cli/token_usage_types.go |
Adds steering data types. |
pkg/cli/token_usage_subagent_session.go |
Collects notices from sessions. |
pkg/cli/token_usage_steering.go |
Processes delivered and gateway notices. |
pkg/cli/token_usage_steering_notices_test.go |
Tests reporting and rendering. |
pkg/cli/token_usage_analyze.go |
Applies notice collection. |
pkg/cli/testdata/model_routing_golden/README.md |
Documents the synthetic fixture. |
pkg/cli/testdata/model_routing_golden/logs_expected.json |
Updates aggregate expectations. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/usage/aw_session.jsonl |
Adds unified-session fixture data. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/usage/agent_usage.json |
Adds usage fixture data. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/sandbox/firewall/logs/api-proxy-logs/token-usage.jsonl |
Adds raw steering records. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/sandbox/firewall/logs/api-proxy-logs/model-routing.jsonl |
Adds routing fixture data. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/expected.legacy.json |
Adds legacy expectations. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/expected.json |
Adds unified expectations. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/aw_info.json |
Adds workflow metadata fixture. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/agent/awf-routing-outcome.json |
Adds routing outcome fixture. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/agent/aw_info.json |
Adds agent metadata fixture. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/agent-session.jsonl |
Adds agent session fixture. |
pkg/cli/testdata/model_routing_golden/claude-awf-steering-notices/agent_usage.json |
Adds legacy usage fixture. |
pkg/cli/model_routing_golden_test.go |
Registers and verifies the fixture. |
pkg/cli/logs_report.go |
Exposes notices in logs output. |
pkg/cli/logs_models.go |
Extends report models. |
pkg/cli/gateway_logs_timeline.go |
Displays AI-credit thresholds. |
pkg/cli/audit_report.go |
Exposes notices in audits. |
pkg/cli/audit_report_render.go |
Renders notices in the console. |
docs/src/content/docs/specs/unified-agent-session-specification.md |
Defines the steering contract. |
docs/src/content/docs/reference/sandbox.md |
Documents timeout steering. |
docs/src/content/docs/reference/artifacts.md |
Documents report fields. |
docs/public/schemas/unified-session.schema.json |
Extends the session schema. |
actions/setup/js/unified_session.test.cjs |
Tests end-to-end projection. |
actions/setup/js/unified_session.cjs |
Maps AI-credit events. |
actions/setup/js/unified_session_render.cjs |
Renders steering thresholds. |
actions/setup/js/unified_session_payload.test.cjs |
Tests notice validation. |
actions/setup/js/unified_session_payload.cjs |
Projects validated notices. |
actions/setup/js/types/unified_session.d.ts |
Adds steering session types. |
🧠 Review effort: Balanced
| var value float64 | ||
| if err := json.Unmarshal(data, &value); err != nil || value <= 0 || value > 100 { | ||
| *t = 0 | ||
| return nil | ||
| } | ||
| *t = steeringThreshold(math.Round(value)) | ||
| return nil |
| func awfSupportsAgentTimeoutSteering(firewallConfig *FirewallConfig) bool { | ||
| return awfVersionAtLeast(firewallConfig, constants.AWFAgentTimeoutSteeringMinVersion) |
| With steering enabled, AWF adds a notice to the next model request when the run reaches 80, 90, 95 or 99% of its AI-credit budget, its effective-token budget or its runtime deadline. Runtime notices need an agent deadline: with AWF v0.28.51 or newer, gh-aw sends `container.agentTimeout` on every runtime when `timeout-minutes` is a literal number or omitted at compile time (default 20, or the `GH_AW_DEFAULT_TIMEOUT_MINUTES` compile-time override). When `timeout-minutes` is a GitHub Actions expression, including the default emitted into compiled workflows that read `vars.GH_AW_DEFAULT_TIMEOUT_MINUTES`, the deadline is omitted and AWF sends no runtime notices. Older AWF versions receive the deadline only on the Cloud Hypervisor and NVX runtimes, as before. The threat-detection run uses its own job timeout. | ||
|
|
||
| AWF also stops the agent at this deadline (exit code 124). AWF counts it from agent start, after the step has started, and gh-aw never sends a deadline shorter than the agent step timeout, so the GitHub Actions step timeout normally fires first and runtime notices arrive slightly after the step's own deadline. Delivered notices are listed in `gh aw audit` and `gh aw logs --json` (see [Artifacts](/gh-aw/reference/artifacts/)). |
|
Checked
Blocking: time warnings stay off for most workflows. Workflows without a literal Follow-up (no change needed now): the new golden case |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review verdict
Non-blocking overall, but I found two correctness holes in the new steering-notice path.
Details
- The Go-side threshold decoder rounds arbitrary numeric values into valid warning buckets, which can turn malformed telemetry into a fake 80/90/95/99 steering notice.
applySteeringNoticessilently drops unified-session read failures, so a parse error is reported the same way as “no notices were recorded”.
Those are both worth tightening before people start depending on steering_notices for audit/logs accuracy.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 116 AIC · ⌖ 5.9 AIC · ⊞ 21.2K
Comment /review to run again
| *t = 0 | ||
| return nil | ||
| } | ||
| *t = steeringThreshold(math.Round(value)) |
There was a problem hiding this comment.
Rounding arbitrary threshold values into one of the allowed 80/90/95/99 buckets manufactures steering notices that the unified-session schema is supposed to reject, so malformed telemetry can be reported as a real budget warning.
💡 Why this matters and how to fix it
94.6 currently becomes 95, which then passes valid() and shows up in steering_notices / gateway_steering_events even though the JS normalizer drops anything except the exact enum values. That gives Go and JS two different interpretations of the same corrupted record.
Prefer an exact integer decode instead of math.Round, so malformed thresholds stay invalid instead of being silently rewritten into a supported warning.
| return | ||
| } | ||
| _, notices, err := readUnifiedTokenUsage(runDir) | ||
| if err != nil { |
There was a problem hiding this comment.
Dropping read errors here makes a broken aw_session.jsonl look exactly like "no steering notices were recorded", which gives the new steering_notices field a false negative meaning.
💡 Why this matters and how to fix it
The review output now treats a missing steering_notices list as a real signal, but this branch silently erases the list for any parse failure (oversized line, malformed unrelated event, unreadable file, etc.). That means gh aw audit / gh aw logs can report the absence of notices when we actually failed to read them.
Please surface this as a warning/error on the summary instead of returning quietly, or plumb the notices out of the earlier parse path so the only outcomes are "present", "empty", or a visible read failure.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design (pr-triage: new_feature). This is a well-scoped, thoroughly tested change: compiled-config, unified-session JS, Go reader, audit/logs rendering, and spec/schema are all updated together with matching golden fixtures.
📋 Verification performed
go build ./...— passesgo test ./pkg/workflow/... -run AgentTimeout -v— all new compiler tests pass (literal/omitted/expression/unsupported-version cases, step-timeout floor, detection job timeout)go test ./pkg/cli/... -run 'Steering|ModelRoutingGolden' -v— all new steering-notice, audit/logs projection, and golden fixture tests pass (including the newclaude-awf-steering-noticescase)go vet ./pkg/workflow/... ./pkg/cli/...— clean- JS vitest suite could not be run in this sandbox (npm registry blocked by self-signed cert / no
node_modules), sounified_session.test.cjs/unified_session_payload.test.cjschanges were reviewed by inspection only — they look correct and symmetric with the Go-side handling (lenient decode, strict allow-list oftype/threshold).
📋 Key Themes & Highlights
Positive Highlights
- ✅
resolveAWFAgentTimeoutMinutescleanly reuses the existingliteralStepTimeoutMinutes/resolveStepTimeoutValuehelpers rather than duplicating timeout-string parsing logic — consistent with the existing codebase pattern (codebase-design). - ✅ Steering decoding is defensively lenient end-to-end:
TokenUsageSteering.UnmarshalJSON,steeringThreshold.UnmarshalJSON, and the JSSTEERING_TYPES/STEERING_THRESHOLDSallow-lists all independently guard against malformed/future values without failing the surrounding record — good belt-and-suspenders symmetry between the Go and JS consumers. - ✅ New behavior is properly version-gated (
AWFAgentTimeoutSteeringMinVersion) following the existingawfSupports*feature-flag convention, and the "unsupported version" / "default version" paths are both covered by tests. - ✅ Test coverage is excellent and written specification-first:
awf_agent_timeout_test.gocovers presence/absence across literal, expression, and runtime-variable-default timeouts, plus an end-to-end compiled-lockfile assertion (TestCompileWorkflow_AgentTimeoutDockerRuntime) that few PRs bother to add. - ✅ Detection-phase steering notices are collected without letting a malformed detection event fail agent token-usage parsing (
token_usage_subagent_session.go), and this exact behavior is asserted inTestAnalyzeTokenUsageSteeringNotices. - ✅ Spec/schema/doc updates (unified-session spec v1.8.0,
T-UAS-071,schemas/audit.schema.json,sandbox.md,artifacts.md) are all kept in lock-step with the code change — exactly the kind of documentation-model consistency/grill-with-docslooks for.
Minor Observations (non-blocking)
resolveAWFContainerAgentTimeoutMinutes's fallback path silently returns the default timeout whenrawTimeoutparses to a non-positive integer (e.g."0"or"-5") without logging, unlike the non-numeric branch which logs its reasoning. Low impact since such values shouldn't reach this point after frontmatter validation, but worth a one-line debug log for symmetry if this function is touched again.- The PR description itself flags that
parallel_validationhasn't been run and the full pre-commit check/tests weren't re-run before this session ended — worth confirming CI is green given the scope of schema regeneration involved (hand-verifying generated JSON Schema from.d.tsis error-prone).
No blocking issues found; approving.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 159.6 AIC · ⌖ 14.8 AIC · ⊞ 10.3K
Comment /matt to run again
Co-authored-by: SivaKesava1 <11771739+SivaKesava1@users.noreply.github.com>
AWF (gh-aw-firewall#9790) can warn the agent at 80/90/95/99% of its AI-credit budget, token budget or runtime deadline. In gh-aw, the runtime warnings never fire on the default Docker runtime, because only Cloud Hypervisor and NVX get
container.agentTimeout. Also, the per-requeststeeringrecord AWF writes is dropped before it reaches the unified session, sogh aw auditandgh aw logscan't show which warnings were delivered.Note
These changes were not committed or pushed when the session ended. The last refactor (helpers split out of the
pkg/clisession reader) builds, but its tests and the full pre-commit check have not been re-run.parallel_validationhas not been run.Important
The new behaviour is gated on AWF v0.28.51, which is not published yet. It is the first version after v0.28.50, as the issue suggested. The default AWF version is still v0.28.50, so recompiling this repo's workflows produces no lock-file changes.
Compiler:
container.agentTimeoutAWFAgentTimeoutSteeringMinVersionconstant andawfSupportsAgentTimeoutSteeringcheck, following the existing AWF feature-flag pattern.resolveAWFAgentTimeoutMinutes:timeout-minutesvalue. It is omitted (with a debug log) when the step timeout isn't a literal, and raised to the step timeout if it would be shorter. AWF counts its deadline from agent start, so the GitHub step timeout fires first and AWF's exit 124 can't cut off the agent's final output.jobs.detection.timeout-minutes).timeout-minutesget an expression (${{ fromJSON(vars.GH_AW_DEFAULT_TIMEOUT_MINUTES || '20') }}), so they get noagentTimeout. In practice, runtime warnings need a literaltimeout-minutes. This is documented insandbox.md.Unified session
firewall.token_usage(and itsusage.reportalias) now keepssteering, projected to{type, threshold}. Notices with an unknown type, an unknown threshold or a malformed value are dropped.ai_credit_steeringevent-log lines now map tofirewall.steeringinstead of the genericfirewall.event.firewall.steeringkeepsthreshold.types/unified_session.d.ts. NewSteeringNoticetype (enumerated type and threshold) andFirewallSteeringDatatype.Audit and logs
firewall.token_usageevents. A malformed detection event is skipped, not treated as fatal. It does not readtoken-usage.jsonldirectly.steering_noticeslist ingh aw audit(JSON and console output) and ingh aw logs --json. The run-level copy survives compact output.gateway_steering_eventsnow includes AI-credit warnings and keepsthresholdandrequest_id. In the timeline these show as e.g.credit 90%.schemas/audit.schema.jsonandschemas/logs.schema.jsonare updated to match. Runs withoutsteeringserialize exactly as before.Example audit and logs JSON output (
phaseisagentordetection):Tests and fixtures
claude-awf-steering-notices(copy ofclaude-awf-selected-messageswith steering fields added by hand). Existingexpected*.jsonfiles are unchanged;logs_expected.jsonchanges only because it now includes the new case.Known gaps