-
Notifications
You must be signed in to change notification settings - Fork 141
Add --firewall and --no-firewall boolean flags to logs command and MCP server for filtering by firewall usage #3122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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>
|
…rewall-report workflow Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in d0d37a9:
The workflow now follows agentic workflow best practices by using MCP server tools rather than direct CLI commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new --firewall filter to the gh aw logs command, enabling users to filter workflow runs based on whether they have firewall enabled. The filter accepts "true" (only runs with firewall) or "false" (only runs without firewall).
Key changes:
- Added firewall filtering capability to the logs command, allowing users to filter runs based on firewall usage
- Updated the daily firewall report workflow to use the new
logstool filter instead of manually identifying firewall-enabled workflows - Added comprehensive test coverage for firewall field parsing and filtering logic
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs.go | Added firewall filter flag, validation logic, and filtering implementation based on steps.firewall field in aw_info.json |
| pkg/cli/logs_test.go | Updated test function calls to include the new firewall parameter |
| pkg/cli/logs_firewall_filter_test.go | Added comprehensive test suite for firewall field parsing, filtering logic, and JSON marshaling |
| pkg/cli/mcp_server.go | Added firewall parameter to the MCP server's logs tool schema and command argument construction |
| .github/workflows/daily-firewall-report.md | Updated workflow instructions to use the new logs tool firewall filter instead of manual workflow identification |
| .github/workflows/daily-firewall-report.lock.yml | Compiled version of the updated daily-firewall-report workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/cli/logs.go
Outdated
| // Validate firewall parameter | ||
| if firewallFilter != "" && firewallFilter != "true" && firewallFilter != "false" { | ||
| fmt.Fprintln(os.Stderr, console.FormatError(console.CompilerError{ | ||
| Type: "error", | ||
| Message: fmt.Sprintf("invalid firewall value '%s'. Must be 'true' or 'false'", firewallFilter), | ||
| })) | ||
| os.Exit(1) | ||
| } |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic allows empty strings but should also validate the value when passed to the MCP server. The MCP server's logsArgs struct has no validation, so invalid values could be passed through the MCP interface. Consider adding validation in the MCP server handler or documenting that the MCP tool relies on CLI validation.
pkg/cli/logs.go
Outdated
| if firewallFilter != "" { | ||
| // Check the firewall field in aw_info.json | ||
| awInfoPath := filepath.Join(result.LogsPath, "aw_info.json") | ||
| info, err := parseAwInfo(awInfoPath, verbose) | ||
| var hasFirewall bool | ||
| if err == nil && info != nil { | ||
| // Firewall is enabled if steps.firewall is non-empty (e.g., "squid") | ||
| hasFirewall = info.Steps.Firewall != "" | ||
| } |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parseAwInfo function is called multiple times for the same file within the same loop iteration (engine filter at line 617, staged filter at line 617, and now firewall filter at line 635). Consider parsing the aw_info.json file once per iteration and reusing the parsed result across all filters to avoid redundant file I/O and JSON parsing.
|
|
||
| **Using the logs tool:** | ||
| Call the `logs` tool with the following parameters: | ||
| - `firewall`: "true" (to filter only runs with firewall enabled) |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that the firewall parameter should be a string "true", but the code validates it as a string. While technically correct, it would be clearer to document that this is a string value, not a boolean. Consider adding quotes in the documentation or explicitly stating 'the string "true"' to avoid confusion for users who might try to pass a boolean value.
|
@copilot firewall cli value should be a Boolean, apply review comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Head branch was pushed to by a user without write access
Updated in 9a169be:
CLI usage: gh aw logs --firewall # Only runs with firewall
gh aw logs --no-firewall # Only runs without firewallMCP tool usage: {"firewall": true, "start_date": "-7d"} |
The
daily-firewall-report.mdworkflow required manually parsing frontmatter to identify firewall-enabled workflows. This adds--firewalland--no-firewallboolean flags to filter runs based on actual firewall usage from runtime metadata.Changes
Added
--firewalland--no-firewallboolean flags togh aw logscommand--firewallfilters to only runs with firewall enabled--no-firewallfilters to only runs without firewall enabledsteps.firewallfield inaw_info.json(e.g., "squid" when enabled)Added
firewallandno_firewallboolean parameters to MCP serverlogstool"firewall": trueor"no_firewall": true(boolean values)Optimized performance by reducing redundant file I/O
aw_info.jsononce per loop iteration instead of multiple timesUpdated
daily-firewall-report.mdworkflowlogstool withfirewall: trueboolean parameter to collect firewall-enabled runsaudittool to analyze firewall data from each runAdded unit tests for firewall field parsing and filtering logic (235 lines)
Usage
CLI Usage:
MCP Server Tool Usage:
{ "firewall": true, "start_date": "-7d", "count": 100 }Follows the same pattern as existing
--no-stagedboolean flag.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.