feat: unified event timeline across MCP Gateway, AWF firewall, and agent logs#34782
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…aces Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… logs - Add TimelineSourceAgent + agent event kinds (agent_turn, agent_tool_start, agent_tool_done) to gateway_logs_timeline.go - Add parseEventsJSONL helper and collectAgentTimelineEvents in same file - Update BuildUnifiedTimeline to include agent events from events.jsonl - Add rendering primitives for agent events in gateway_logs_timeline_render.go (renderAgentTurnRow, renderAgentToolStartRow, renderAgentToolDoneRow) - Update renderUnifiedTimeline summary to show AG counts - Wire renderAuditUnifiedTimeline into audit.go (Go audit log generation surface) - Wire displayUnifiedTimeline into logs_orchestrator.go (Go logs CLI surface) - Create actions/setup/js/unified_timeline.cjs: JS parser + renderer for GITHUB_STEP_SUMMARY (all three sources, sorted merge, Markdown <details> table) - Create actions/setup/js/unified_timeline.test.cjs: 66 Vitest tests - Integrate generateUnifiedTimelineSummary into writeStepSummaryWithTokenUsage in parse_mcp_gateway_log.cjs so timeline is appended to every step summary - Create pkg/cli/gateway_logs_timeline_test.go: Go unit tests for all surfaces Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Guard timelineSourceLabel default case against short strings (Go + JS) - Deduplicate sorted-order check in displayUnifiedTimeline (calls sortUnifiedTimelineEvents once instead of inline-checking then calling) - Fix sortUnifiedTimelineEvents comment to reflect sort.SliceStable usage; inline the sort and remove now-unused sortEventsStable helper - Use truncate() helper for firewall detail in unified_timeline.cjs for consistent ellipsis handling with other collectors - Fix test assertion: replace buggy regex with plain string check for pipe escaping in buildUnifiedTimelineMarkdown Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a unified, chronologically sorted event timeline that merges JSONL streams from the MCP Gateway, the AWF firewall, and the agent (Copilot CLI) session. The merged timeline is rendered in two places: the Go CLI (audit and logs console output) and the JavaScript GITHUB_STEP_SUMMARY pipeline, both sharing the same source/kind taxonomy and per-event rendering primitives.
Changes:
- New Go module (
gateway_logs_timeline.go+gateway_logs_timeline_render.go) introducingUnifiedTimelineEvent, three per-source collectors (gateway/firewall/agent), a merge+sort builder, and per-kind table renderers; wired intoaudit.goandlogs_orchestrator.go. - New JS module (
unified_timeline.cjs) that mirrors the Go collectors/renderers and emits a collapsible<details>Markdown table; appended to step summary viaparse_mcp_gateway_log.cjs. - Comprehensive Go unit tests and 66 Vitest JS tests for the new collectors and rendering primitives.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/gateway_logs_timeline.go | New: unified timeline event types and per-source collectors (gateway/firewall/agent), plus BuildUnifiedTimeline. |
| pkg/cli/gateway_logs_timeline_render.go | New: per-kind row renderers, top-level renderUnifiedTimeline, and displayUnifiedTimeline console entry point. |
| pkg/cli/gateway_logs_timeline_test.go | New: Go unit tests for collectors, merge/sort, and rendering primitives. |
| pkg/cli/audit.go | Calls renderAuditUnifiedTimeline after gateway metrics in audit output. |
| pkg/cli/logs_orchestrator.go | Calls displayUnifiedTimeline in the console-render path. |
| actions/setup/js/unified_timeline.cjs | New: JS port of collectors + Markdown renderer. |
| actions/setup/js/unified_timeline.test.cjs | New: Vitest test suite for the JS module. |
| actions/setup/js/parse_mcp_gateway_log.cjs | Appends the JS-rendered timeline to step summary before write. |
| .github/workflows/smoke-antigravity.lock.yml | Adds --config awf-config.json invocation to awf (appears unrelated to PR purpose). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/unified_timeline.cjs:304
- The JS firewall classification logic diverges from the Go side. Go delegates to the canonical
isEntryAllowed(entry)(in firewall_policy.go), whereas this JS uses a regex on thedecisionfield plus a 4xx/5xx HTTP status heuristic. The two will disagree on entries where, for example, a successful CONNECT tunnel hasstatus=0but a non-matching decision string, or denied entries that don't contain "denied|blocked|reject" but use other Squid status tags (e.g.TCP_MISS_ABORTED,NONE). Since one of the PR's stated goals is rendering the same merged timeline in both Go and JS, the JS classification should mirror the Go logic field-by-field (status >= 400, plus the same set of denied/blocked Squid status prefixes used byisEntryAllowed).
// Decision field: "TCP_TUNNEL:HIER_DIRECT" or "TCP_DENIED:..." etc.
const decision = entry.decision ?? entry.squid_request_status ?? "";
const blocked = /denied|blocked|reject/i.test(decision) ||
(typeof status === "number" && status >= 400 && status < 600);
const detail = truncate([host, method].filter(Boolean).join(" "), 48);
const statusStr = status ? String(status) : (blocked ? "blocked" : "allowed");
events.push({
source: SOURCE_FIREWALL,
kind: blocked ? KIND_NET_BLOCKED : KIND_NET_ALLOWED,
time,
detail,
status: statusStr,
});
actions/setup/js/unified_timeline.cjs:304
- Column semantics diverge from the Go renderer. In Go's
renderFirewallNetworkAllowedRow/renderFirewallNetworkBlockedRow, the Detail column holds the host alone and the Status column is formatted asMETHOD HTTP <code>(e.g.CONNECT HTTP 200). Here, Detail is built ashost methodconcatenated and Status is the raw HTTP status number. Since the JS output is meant to mirror the Go output, please align the column layout: put host in Detail and a formatted "METHOD HTTP nnn" (or "blocked") string in Status.
const detail = truncate([host, method].filter(Boolean).join(" "), 48);
const statusStr = status ? String(status) : (blocked ? "blocked" : "allowed");
events.push({
source: SOURCE_FIREWALL,
kind: blocked ? KIND_NET_BLOCKED : KIND_NET_ALLOWED,
time,
detail,
status: statusStr,
});
- Files reviewed: 9/9 changed files
- Comments generated: 3
| for (const entry of parseJsonl(content)) { | ||
| // Firewall audit.jsonl uses Unix float64 `ts` field | ||
| const time = parseTimestamp(entry.ts); | ||
| if (!time) continue; | ||
|
|
||
| const host = entry.host ?? entry.domain ?? ""; | ||
| const method = entry.method ?? ""; | ||
| const status = entry.status ?? entry.http_status ?? ""; | ||
|
|
||
| // Decision field: "TCP_TUNNEL:HIER_DIRECT" or "TCP_DENIED:..." etc. | ||
| const decision = entry.decision ?? entry.squid_request_status ?? ""; | ||
| const blocked = /denied|blocked|reject/i.test(decision) || | ||
| (typeof status === "number" && status >= 400 && status < 600); | ||
|
|
||
| const detail = truncate([host, method].filter(Boolean).join(" "), 48); | ||
| const statusStr = status ? String(status) : (blocked ? "blocked" : "allowed"); | ||
|
|
||
| events.push({ | ||
| source: SOURCE_FIREWALL, | ||
| kind: blocked ? KIND_NET_BLOCKED : KIND_NET_ALLOWED, | ||
| time, | ||
| detail, | ||
| status: statusStr, | ||
| }); | ||
| } |
| // This file contains rendering primitives and the top-level render function for the | ||
| // unified MCP Gateway + AWF firewall timeline produced by BuildUnifiedTimeline. | ||
| // | ||
| // A dedicated rendering primitive exists for every TimelineEventKind so that each event | ||
| // type is displayed with appropriate context and formatting: | ||
| // | ||
| // TimelineKindToolCall – renderGatewayToolCallRow | ||
| // TimelineKindDIFCFiltered – renderGatewayDIFCFilteredRow | ||
| // TimelineKindGuardPolicyBlocked – renderGatewayGuardPolicyBlockedRow | ||
| // TimelineKindNetworkAllowed – renderFirewallNetworkAllowedRow | ||
| // TimelineKindNetworkBlocked – renderFirewallNetworkBlockedRow | ||
| // | ||
| // renderTimelineEventRow dispatches to the appropriate primitive and returns a | ||
| // []string suitable for inclusion in a console.TableConfig.Rows slice. |
| printf '%s\n' '{"$schema":"https://github.com/github/gh-aw-firewall/releases/download/v0.25.55/awf-config.schema.json","network":{"allowDomains":["*.githubusercontent.com","*.googleapis.com","*.grafana.net","*.sentry.io","api.snapcraft.io","archive.ubuntu.com","azure.archive.ubuntu.com","codeload.github.com","crl.geotrust.com","crl.globalsign.com","crl.identrust.com","crl.sectigo.com","crl.thawte.com","crl.usertrust.com","crl.verisign.com","crl3.digicert.com","crl4.digicert.com","crls.ssl.com","docs.github.com","generativelanguage.googleapis.com","github-cloud.githubusercontent.com","github-cloud.s3.amazonaws.com","github.blog","github.com","github.githubassets.com","host.docker.internal","json-schema.org","json.schemastore.org","keyserver.ubuntu.com","lfs.github.com","objects.githubusercontent.com","ocsp.digicert.com","ocsp.geotrust.com","ocsp.globalsign.com","ocsp.identrust.com","ocsp.sectigo.com","ocsp.ssl.com","ocsp.thawte.com","ocsp.usertrust.com","ocsp.verisign.com","packagecloud.io","packages.cloud.google.com","packages.microsoft.com","patch-diff.githubusercontent.com","ppa.launchpad.net","raw.githubusercontent.com","registry.npmjs.org","s.symcb.com","s.symcd.com","security.ubuntu.com","ts-crl.ws.symantec.com","ts-ocsp.ws.symantec.com","www.googleapis.com"]},"apiProxy":{"enabled":true,"enableTokenSteering":true,"maxRuns":500,"maxEffectiveTokens":25000000,"targets":{"antigravity":{"host":"generativelanguage.googleapis.com"}},"models":{"agent":["sonnet-6x","gpt-5.4","gpt-5.3","gemini-pro","any"],"antigravity":["copilot/antigravity*","google/antigravity*","gemini/antigravity*"],"any":["copilot/*","anthropic/*","openai/*","google/*","gemini/*"],"claude":["agent"],"codex":["agent"],"coding":["copilot/gpt-5*codex*","openai/gpt-5*codex*","gpt-5-codex"],"computer-use":["copilot/*computer-use*","google/*computer-use*","gemini/*computer-use*","openai/*computer-use*"],"copilot":["agent"],"deep-research":["copilot/deep-research*","copilot/o3-deep-research*","copilot/o4-mini-deep-research*","google/deep-research*","gemini/deep-research*","openai/o3-deep-research*","openai/o4-mini-deep-research*"],"gemini":["agent"],"gemini-3-flash":["copilot/gemini-3*flash*","google/gemini-3*flash*","gemini/gemini-3*flash*"],"gemini-3-pro":["copilot/gemini-3*pro*","google/gemini-3*pro*","gemini/gemini-3*pro*"],"gemini-3.1-flash":["copilot/gemini-3.1*flash*","google/gemini-3.1*flash*","gemini/gemini-3.1*flash*"],"gemini-3.1-pro":["copilot/gemini-3.1*pro*","google/gemini-3.1*pro*","gemini/gemini-3.1*pro*"],"gemini-3.5-flash":["copilot/gemini-3.5*flash*","google/gemini-3.5*flash*","gemini/gemini-3.5*flash*"],"gemini-flash":["copilot/gemini-*flash*","google/gemini-*flash*","gemini/gemini-*flash*"],"gemini-flash-lite":["copilot/gemini-*flash*lite*","google/gemini-*flash*lite*","gemini/gemini-*flash*lite*"],"gemini-pro":["copilot/gemini-*pro*","google/gemini-*pro*","gemini/gemini-*pro*"],"gemma":["copilot/gemma*","google/gemma*","gemini/gemma*"],"gpt-4.1":["copilot/gpt-4.1*","openai/gpt-4.1*"],"gpt-5":["copilot/gpt-5*","openai/gpt-5*"],"gpt-5-codex":["copilot/gpt-5*codex*","openai/gpt-5*codex*"],"gpt-5-mini":["copilot/gpt-5*mini*","openai/gpt-5*mini*"],"gpt-5-nano":["copilot/gpt-5*nano*","openai/gpt-5*nano*"],"gpt-5-pro":["copilot/gpt-5*pro*","openai/gpt-5*pro*"],"gpt-5.2":["copilot/gpt-5.2*","openai/gpt-5.2*"],"gpt-5.3":["copilot/gpt-5.3*","openai/gpt-5.3*"],"gpt-5.4":["copilot/gpt-5.4*","openai/gpt-5.4*"],"gpt-5.5":["copilot/gpt-5.5*","openai/gpt-5.5*"],"haiku":["copilot/*haiku*","anthropic/*haiku*"],"large":["sonnet","gpt-5-pro","gpt-5","gemini-pro"],"mini":["haiku","gpt-5-mini","gpt-5-nano","gemini-flash-lite"],"opus":["copilot/*opus*","anthropic/*opus*"],"opusplan":["opus?effort=high"],"reasoning":["copilot/o1*","copilot/o3*","copilot/o4*","openai/o1*","openai/o3*","openai/o4*"],"robotics":["copilot/*robotics*","google/*robotics*","gemini/*robotics*"],"small":["mini"],"sonnet":["copilot/*sonnet*","anthropic/*sonnet*"],"sonnet-6x":["copilot/*sonnet-4-5-*","anthropic/*sonnet-4-5-*","copilot/*sonnet-4-6*","anthropic/*sonnet-4-6*"],"summarization":["haiku","gpt-5-mini","gemini-flash-lite","mini"],"vision":["copilot/gemini-*image*","gemini/gemini-*image*","copilot/gemini-*flash*","gemini/gemini-*flash*"]}},"container":{"imageTag":"0.25.55"}}' > "${RUNNER_TEMP}/gh-aw/awf-config.json" | ||
| cp "${RUNNER_TEMP}/gh-aw/awf-config.json" /tmp/gh-aw/awf-config.json | ||
| GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS="" | ||
| if [[ "${DOCKER_HOST:-}" =~ ^tcp:// ]]; then | ||
| GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS="--docker-host-path-prefix /tmp/gh-aw" | ||
| fi | ||
| # shellcheck disable=SC1003 | ||
| sudo -E awf --container-workdir "${GITHUB_WORKSPACE}" --mount "${RUNNER_TEMP}/gh-aw:${RUNNER_TEMP}/gh-aw:ro" --mount "${RUNNER_TEMP}/gh-aw:/host${RUNNER_TEMP}/gh-aw:ro" ${GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS} --env-all --exclude-env ANTIGRAVITY_API_KEY --exclude-env GH_TOKEN --exclude-env GITHUB_MCP_SERVER_TOKEN --exclude-env MCP_GATEWAY_API_KEY --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --audit-dir /tmp/gh-aw/sandbox/firewall/audit --enable-host-access --allow-host-ports 80,443,8080 --skip-pull --difc-proxy-host host.docker.internal:18443 --difc-proxy-ca-cert /tmp/gh-aw/difc-proxy-tls/ca.crt \ | ||
| sudo -E awf --config "${RUNNER_TEMP}/gh-aw/awf-config.json" --container-workdir "${GITHUB_WORKSPACE}" --mount "${RUNNER_TEMP}/gh-aw:${RUNNER_TEMP}/gh-aw:ro" --mount "${RUNNER_TEMP}/gh-aw:/host${RUNNER_TEMP}/gh-aw:ro" ${GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS} --env-all --exclude-env ANTIGRAVITY_API_KEY --exclude-env GH_TOKEN --exclude-env GITHUB_MCP_SERVER_TOKEN --exclude-env MCP_GATEWAY_API_KEY --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --audit-dir /tmp/gh-aw/sandbox/firewall/audit --enable-host-access --allow-host-ports 80,443,8080 --skip-pull --difc-proxy-host host.docker.internal:18443 --difc-proxy-ca-cert /tmp/gh-aw/difc-proxy-tls/ca.crt \ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,543 new lines across business directories, ≫ 100-line threshold) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 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 References: §26421888908
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2.2M
| // section regardless of which gateway log format was detected above. | ||
| const timelineMd = generateUnifiedTimelineSummary(); | ||
| if (timelineMd) { | ||
| coreObj.info(`Appending unified event timeline to step summary`); |
There was a problem hiding this comment.
Timeline summary call is unguarded — a throw here will silently drop all previously buffered step-summary content.
💡 Suggested fix
generateUnifiedTimelineSummary() calls fs.readFileSync and JSON.parse under the hood. If either throws (e.g. a corrupt JSONL file, a permission error, or an unexpected log shape), the exception propagates and coreObj.summary.write() is never reached. Any token-usage stats or other sections already accumulated by the caller are silently discarded.
Wrap in try/catch:
try {
const timelineMd = generateUnifiedTimelineSummary();
if (timelineMd) {
coreObj.info('Appending unified event timeline to step summary');
coreObj.summary.addRaw(timelineMd);
}
} catch (err) {
coreObj.warning(`Failed to generate unified timeline: ${err?.message ?? err}`);
}
coreObj.summary.write();This matches how other optional enrichment sections are handled elsewhere in the file.
| } catch { | ||
| // ignore read errors | ||
| } | ||
| return null; |
There was a problem hiding this comment.
undefined success field is coerced to "error" status, misclassifying every tool completion from older agent versions.
💡 Suggested fix
case "tool.execution_complete": {
const server = entry.data?.mcpServerName ?? "";
const tool = entry.data?.toolName ?? "";
const success = entry.data?.success;
// ...
status: success ? "success" : "error", // ← undefined → "error"
}When the agent session events.jsonl format does not include a success field (field absent or emitting agent version predates it), entry.data?.success evaluates to undefined, which is falsy. Every tool completion is then classified as "error" — the opposite of the likely reality and a visually alarming misrepresentation in the step summary.
The Go counterpart uses a typed bool so the zero-value default is consistent; JS needs an explicit check:
const success = entry.data?.success;
const statusStr = success === true ? "success" : success === false ? "error" : "unknown";Or at minimum guard against the undefined case:
status: success == null ? "" : success ? "success" : "error",| const blocked = /denied|blocked|reject/i.test(decision) || | ||
| (typeof status === "number" && status >= 400 && status < 600); | ||
|
|
||
| const detail = truncate([host, method].filter(Boolean).join(" "), 48); |
There was a problem hiding this comment.
findEventsJsonlFile returns the first directory entry it finds — non-deterministic when multiple session directories exist.
💡 Suggested fix
readdirSync returns entries in filesystem order, which is platform-dependent (ext4 on Linux returns entries in hash-tree order, not creation order). When an agent retries or a prior run's state directory is left behind, the function picks an arbitrary session's events.jsonl — potentially the wrong run's data.
Sort by most-recently modified before returning:
const entries = fs.readdirSync(sessionStateDir, { withFileTypes: true });
const candidates = entries
.filter(e => e.isDirectory())
.map(e => {
const p = path.join(sessionStateDir, e.name, "events.jsonl");
try { return { p, mtime: fs.statSync(p).mtimeMs }; } catch { return null; }
})
.filter(Boolean);
candidates.sort((a, b) => b.mtime - a.mtime);
return candidates[0]?.p ?? null;The Go counterpart (findEventsJSONLFile) has the same issue and should be fixed in parallel.
| if (entry.type === "DIFC_FILTERED") { | ||
| const server = entry.server_id ?? entry.server_name ?? ""; | ||
| const tool = entry.tool_name ?? ""; | ||
| events.push({ |
There was a problem hiding this comment.
HTTP 4xx status codes are classified as firewall-blocked, conflating application errors with security denials.
💡 Suggested fix
const blocked = /denied|blocked|reject/i.test(decision) ||
(typeof status === "number" && status >= 400 && status < 600);A 404 Not Found, 401 Unauthorized, or 429 Too Many Requests from the upstream server is not a firewall block — the firewall permitted the connection and the server responded with an error. Treating these as net_blocked events inflates the blocked count and will confuse operators investigating security incidents.
Also, the regex matches any substring: "TCP_TUNNEL_UNBLOCKED" would match blocked, and a decision string like "not_rejected" would match reject. Use word-boundary anchors or explicit string comparison:
const blocked = /\bdenied\b|\bblocked\b|\brejected\b|TCP_DENIED/i.test(decision);
// HTTP status is not a reliable block signal — omit it
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics & Test Classification (73 tests analyzed)
Test Classification DetailsGo tests (23)
JavaScript tests (50 — sampled to 50)
Language SupportTests analyzed:
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 4.7M
| // Append the unified event timeline (gateway + firewall audit + agent events) | ||
| // to the step summary immediately before flushing, so it appears as the last | ||
| // section regardless of which gateway log format was detected above. | ||
| const timelineMd = generateUnifiedTimelineSummary(); |
There was a problem hiding this comment.
[/tdd] generateUnifiedTimelineSummary() is called without a try/catch guard. If it throws for any reason (unexpected JSONL shape, fs permission error, etc.), the coreObj.summary.write() call on the next line is never reached — silently dropping the entire step summary for every consumer of this function.
💡 Suggested fix
Wrap the new block in a try/catch so a timeline failure degrades gracefully:
try {
const timelineMd = generateUnifiedTimelineSummary();
if (timelineMd) {
coreObj.info('Appending unified event timeline to step summary');
coreObj.summary.addRaw(timelineMd);
}
} catch (err) {
coreObj.warning(`Failed to generate unified event timeline: ${err?.message ?? err}`);
}A test in unified_timeline.test.cjs that mocks fs.readFileSync to throw and asserts generateUnifiedTimelineSummary returns "" (rather than propagating) would catch this class of regression before it reaches production.
| return UnifiedTimelineEvent{}, false | ||
| } | ||
| evt.Kind = TimelineKindToolCall | ||
| evt.Status = "initiated" |
There was a problem hiding this comment.
[/grill-with-docs] Status: "initiated" is a third status value not documented anywhere in the codebase — every other tool call event uses "success" or "error". When rpc-messages.jsonl is the source (i.e., gateway.jsonl is absent), all tool-call rows will show "initiated" in the Status column instead of an outcome, making the timeline look incomplete or stalled.
💡 Options
Option A — Document the semantic gap: Add a comment explaining that rpc-messages entries capture the request only (no response), so "initiated" is intentionally different:
// "initiated" is used instead of "success"/"error" because rpc-messages.jsonl
// records only the outbound request; the response/outcome is not available.
evt.Status = "initiated"Option B — Use a different label ("→" or "sent") that visually distinguishes in-flight entries from completed ones, and add a test that verifies this label is preserved through renderGatewayToolCallRow.
Either way, TestRpcEntryToTimelineEvent_RequestStatus would be a useful test addition.
| gatewayLogsLog.Printf("collectAgentTimelineEvents error: %v", agErr) | ||
| } | ||
|
|
||
| events := make([]UnifiedTimelineEvent, 0, len(gatewayEvents)+len(firewallEvents)+len(agentEvents)) |
There was a problem hiding this comment.
[/tdd] BuildUnifiedTimeline returns (events, nil) even when one or more collectors fail — errors are only logged at debug level. A caller that receives an empty slice cannot distinguish "no events found" from "all three collectors errored", making silent regressions invisible.
💡 Suggestion
Consider returning a sentinel or a multi-error to surface partial failures. At minimum, the function could return the first non-nil error so callers can decide:
var firstErr error
gatewayEvents, gwErr := collectGatewayTimelineEvents(logDir, verbose)
if gwErr != nil {
gatewayLogsLog.Printf("collectGatewayTimelineEvents error: %v", gwErr)
firstErr = gwErr
}
// ... same for firewall/agent ...
return events, firstErrThis is especially important for renderAuditUnifiedTimeline in audit.go, which currently silences errors on both the BuildUnifiedTimeline call and inside the function itself.
| } | ||
| } | ||
|
|
||
| // ─── BuildUnifiedTimeline ──────────────────────────────────────────────────── |
There was a problem hiding this comment.
[/tdd] collectGatewayTimelineEvents and collectFirewallTimelineEvents have no direct unit tests — they are only exercised indirectly via BuildUnifiedTimeline. Missing coverage for the error paths in each collector (e.g., os.Open failure, malformed mid-stream JSONL that causes a scanner error) means a file-permission regression would surface only in production.
💡 Suggested additions
func TestCollectGatewayTimelineEvents_UnreadableFile(t *testing.T) {
dir := t.TempDir()
p := filepath.Join(dir, "gateway.jsonl")
require.NoError(t, os.WriteFile(p, []byte(`{"event":"tool_call","timestamp":"2024-01-15T10:00:00Z"}`+"\n"), 0000))
t.Cleanup(func() { _ = os.Chmod(p, 0600) })
_, err := collectGatewayTimelineEvents(dir, false)
assert.Error(t, err)
}
func TestCollectFirewallTimelineEvents_EmptyDir(t *testing.T) {
events, err := collectFirewallTimelineEvents(t.TempDir(), false)
assert.NoError(t, err)
assert.Empty(t, events)
}These two collectors handle the bulk of real-world data and deserve the same direct test coverage as collectAgentTimelineEvents.
| case "tool.execution_complete": { | ||
| const server = entry.data?.mcpServerName ?? ""; | ||
| const tool = entry.data?.toolName ?? ""; | ||
| const success = entry.data?.success; |
There was a problem hiding this comment.
[/tdd] entry.data?.success is undefined when the field is absent (rather than explicitly false). Since undefined is falsy, success ? "success" : "error" evaluates to "error" for any tool.execution_complete event that simply omits the success field — producing a false negative.
💡 Suggested fix
Check for false explicitly to distinguish "failed" from "field missing":
// Treat absent success field as success (default), explicit false as error.
const status = entry.data?.success === false ? "error" : "success";Add a test for the missing-field case:
it("treats missing success field as success", () => {
// entry.data has no "success" key at all
const events = collectAgentEvents({ eventsJsonlPath: pathToFileWith({ type: "tool.execution_complete", data: { toolName: "x" } }) });
expect(events[0].status).toBe("success");
});| // Path constants | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| const TMP_GH_AW = "/tmp/gh-aw"; |
There was a problem hiding this comment.
[/grill-with-docs] Path constants are duplicated between this file and the Go implementation (findEventsJSONLFile in copilot_events_jsonl.go, findGatewayJSONLPath in gateway_logs_timeline.go). A path change in Go (e.g., renaming copilot-session-state) would silently produce empty timelines in the JS step summary.
💡 Suggestion
Add a comment that cross-references the canonical Go sources, so future changes prompt the author to update both sides:
// These paths MUST stay in sync with their Go counterparts:
// pkg/cli/copilot_events_jsonl.go → findEventsJSONLFile
// pkg/cli/gateway_logs_timeline.go → findGatewayJSONLPath
// pkg/cli/firewall_policy.go → detectFirewallAuditArtifacts
const TMP_GH_AW = "/tmp/gh-aw";Longer-term, a constants.cjs that is referenced by both this file and gateway-log parsers would be the cleanest approach.
| // displayUnifiedTimeline collects all JSONL events from every processed run, merges them | ||
| // into a single chronologically ordered stream, and writes the rendered timeline to | ||
| // stderr. It is a no-op when no events can be collected from any run. | ||
| func displayUnifiedTimeline(processedRuns []ProcessedRun, verbose bool) { |
There was a problem hiding this comment.
[/zoom-out] When displayUnifiedTimeline merges events across multiple processed runs, the output interleaves events from different runs chronologically without any run boundary label. For a gh aw logs invocation covering 2+ runs, it is impossible to tell which run produced a given event — undermining the diagnostic value of the timeline.
💡 Suggestion
Either render a per-run timeline (one table per run) or add a "Run" column to distinguish events. A per-run approach integrates naturally with the existing ProcessedRun loop:
for _, pr := range processedRuns {
events, err := BuildUnifiedTimeline(pr.Run.LogsPath, verbose)
// ...
fmt.Fprintf(os.Stderr, "\nRun #%d\n", pr.Run.DatabaseID)
if output := renderUnifiedTimeline(events); output != "" {
fmt.Fprint(os.Stderr, output)
}
}The current merge-and-re-sort approach is reasonable for single-run output (which is the common case for gh aw audit), but worth reconsidering for the logs path where multiple runs are typical.
There was a problem hiding this comment.
Smoke bot reply. Me saw latest review comment.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt55 3.5M
|
@copilot Review all comments and pull-request review comments, address what’s needed, and resolve conversations based on your changes. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…k file revert Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three review threads:
|
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
|
|
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Smoke Test: Claude — Run §26422921399Core tests #1–13: ✅ all passed Overall: PASS Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · opus47 4M
| @@ -0,0 +1,540 @@ | |||
| // This file implements unified timeline merging for MCP Gateway, AWF firewall, and agent | |||
There was a problem hiding this comment.
Smoke test: consider adding a brief package-level comment describing the timeline parsing approach.
| @@ -0,0 +1,553 @@ | |||
| // @ts-check | |||
There was a problem hiding this comment.
Smoke test: a top-level JSDoc summarizing inputs/outputs would help readers navigate this 550+ line file.
|
Smoke Copilot 26422921402: FAIL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke review complete. Me leave non-blocking notes only.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt55 3.5M
| const GATEWAY_JSONL_PATH = `${TMP_GH_AW}/mcp-logs/gateway.jsonl`; | ||
| const RPC_MESSAGES_PATH = `${TMP_GH_AW}/mcp-logs/rpc-messages.jsonl`; | ||
| const FIREWALL_AUDIT_PATH = `${TMP_GH_AW}/sandbox/firewall/audit/audit.jsonl`; | ||
| /** Base directory to search recursively for events.jsonl */ |
There was a problem hiding this comment.
Smoke review note: these path constants look important to keep aligned with the Go log collectors.
| "time" | ||
| ) | ||
|
|
||
| // TimelineEventSource identifies which system produced a timeline event. |
There was a problem hiding this comment.
Smoke review note: unified source constants make the merged timeline easier to reason about.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
| displayAggregatedGatewayMetrics(processedRuns, opts.outputDir, opts.verbose) | ||
|
|
||
| // Display unified event timeline (gateway + firewall + agent events) | ||
| displayUnifiedTimeline(processedRuns, opts.verbose) |
There was a problem hiding this comment.
@copilot guard by --timeline flag in logs command
| * | ||
| * Path constants mirror the values in constants.cjs and the Go package. | ||
| */ | ||
|
|
Merges JSONL event streams from three systems — MCP Gateway (
gateway.jsonl/rpc-messages.jsonl), AWF Firewall (audit.jsonl), and agent session (events.jsonl) — into a single chronologically sorted timeline. Renders in two places: Go console output (audit + logs CLI) and JavaScript GITHUB_STEP_SUMMARY.Event sources and kinds
mcp-logs/gateway.jsonlorrpc-messages.jsonltool_call,difc_filtered,guard_blockedsandbox/firewall/audit/audit.jsonlnet_allowed,net_blockedsandbox/agent/logs/copilot-session-state/<uuid>/events.jsonlagent_turn,agent_tool_start,agent_tool_doneGo changes
gateway_logs_timeline.go— addsTimelineSourceAgent, three agent event kinds,parseEventsJSONLhelper,collectAgentTimelineEvents(viafindEventsJSONLFile); updatesBuildUnifiedTimelineto include agent eventsgateway_logs_timeline_render.go— addsrenderAgentTurnRow,renderAgentToolStartRow,renderAgentToolDoneRow; updatestimelineSourceLabel(AG),renderTimelineEventRowdispatcher, andrenderUnifiedTimelinesummaryaudit.go— addsrenderAuditUnifiedTimeline, called fromrenderAuditReportafterrenderAuditGatewayMetricslogs_orchestrator.go— callsdisplayUnifiedTimelineafterdisplayAggregatedGatewayMetricsin the console pathgateway_logs_timeline_test.go— unit tests for all three collectors, merge/sort, and rendering primitivesJavaScript changes
unified_timeline.cjs— standalone module:collectGatewayEvents,collectFirewallEvents,collectAgentEvents,collectUnifiedTimelineEvents,buildUnifiedTimelineMarkdown(renders a collapsible<details>Markdown table),generateUnifiedTimelineSummaryunified_timeline.test.cjs— 66 Vitest testsparse_mcp_gateway_log.cjs— importsunified_timeline.cjs;writeStepSummaryWithTokenUsageappends the timeline tocore.summarybefore everywrite()callTimeline output
Console (Go)
GITHUB_STEP_SUMMARY (JS) — renders as a collapsible
<details>block with the same columns.✨ PR Review Safe Output Test - Run 26422921399
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.