firewall: add request logger disable flag

[ellemouton]

This introduces a new firewall.request-logger.disable config option to completely disable request logging. When disabled, the request logger interceptor is not instantiated, avoiding all logging overhead including database writes and request processing.

This is implemented as a separate disable flag rather than adding a new log level because the goal is to bypass the logging system entirely for performance reasons, not just filter events. A log level would still process and filter each request through the interceptor.

The change also adds validation to ensure autopilot remains enabled only when request logging is active, since autopilot relies on action logs for rule enforcement and auditing.

[ellemouton]

@claude review this

[gemini-code-assist]

Summary of Changes

Hello @ellemouton, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new configuration option to completely disable the firewall's request logger. This is primarily aimed at improving performance by allowing the system to bypass all logging overhead, including interceptor processing and database writes, when logging is not required. It also includes a crucial validation to prevent a misconfiguration where the Autopilot feature, which depends on these logs, might be active while logging is disabled.

Highlights

• New Request Logger Disable Flag: Introduced a Disable boolean flag within the RequestLoggerConfig to allow complete disabling of request logging, bypassing the interceptor and database writes for performance.
• Conditional Request Logger Instantiation: Implemented logic to conditionally instantiate the RequestLogger based on the new Disable flag, preventing its creation and associated overhead when disabled.
• Autopilot Dependency Validation: Added a validation check to ensure that the Autopilot feature cannot be enabled if request logging is disabled, as Autopilot relies on action logs for rule enforcement and auditing.
• Refactored Error Marking Function: Refactored the markActionErrored function to be conditionally defined, providing a no-op version when logging is disabled and the actual logging function when enabled, ensuring correct behavior regardless of logging status.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

Claude finished @ellemouton's task in 54s —— View job

---

Code Review

I've reviewed the changes in this PR. Overall, the implementation looks solid and correctly addresses the goal of providing a way to completely disable request logging. Here are my findings:

Positive Observations

1. Well-documented design choice: The comment in firewall/config.go:14-19 clearly explains why this is a separate disable flag rather than a log level - this is excellent documentation that helps future maintainers understand the rationale.

2. Proper validation logic: The check at terminal.go:1165-1167 correctly validates that autopilot cannot be enabled when request logging is disabled. This is critical since autopilot relies on action logs for rule enforcement.

3. Clean refactoring: The code now conditionally instantiates the request logger only when needed, avoiding unnecessary overhead when disabled. The markActionErrored function approach is elegant.

4. Null-safe design: When request logging is disabled, markActionErrored is set to a no-op function rather than leaving it nil, which prevents potential nil pointer panics.

Issues and Suggestions

1. Misleading error message (terminal.go:1182)

The error message "error creating new request logger" doesn't include the actual error details from err. This makes debugging difficult.

return fmt.Errorf("error creating new request logger")

Suggestion: Include the error context:

return fmt.Errorf("error creating new request logger: %w", err)

2. Typo in existing code (terminal.go:1553)

While not introduced by this PR, there's a typo in the error message: "Error stoppint firewall DB" should be "Error stopping firewall DB".

This could be fixed in a follow-up, but since you're already touching firewall-related code, it might be worth including.

3. Missing test coverage

The PR doesn't appear to include tests for the new validation logic. Consider adding tests for:

• Attempting to enable autopilot with request logging disabled (should fail)
• Successfully disabling request logging with autopilot also disabled
• Verifying that the request logger is not instantiated when disabled

4. Configuration default clarity

The DefaultConfig() in firewall/config.go:25-30 doesn't explicitly set Disable: false. While this is the zero value and therefore implicit, explicitly setting it would improve code clarity and make the default behavior more obvious to readers.

Minor Observations

• The conditional logic in terminal.go:1177-1194 is well-structured and readable
• The use of a named return in the no-op markActionErrored function is appropriate
• Variable initialization is clean and follows Go conventions

Summary

This is a well-implemented feature with good design decisions. The main issue is the missing error wrapping in the error message, which should be fixed. The validation logic correctly prevents misconfiguration, and the performance optimization approach is sound.

Recommendation: Fix the error message wrapping, consider adding tests, and this will be ready to merge.

---

[gemini-code-assist]

Code Review

This pull request introduces a new configuration flag to disable the firewall's request logger, which is a useful performance optimization. The implementation correctly handles the conditional instantiation of the logger and its associated components. The added validation to ensure autopilot is disabled when the logger is off is a crucial safeguard. The code is well-structured and the changes are clearly explained. I have one minor suggestion to improve error wrapping, which will enhance debuggability.

[gemini-code-assist]

For better error diagnostics, it's a good practice to wrap the original error. This provides more context when debugging issues related to the request logger creation.

Suggested change

	return fmt.Errorf("error creating new request logger")
	return fmt.Errorf("error creating new request logger: %w", err)

[ViktorT-11]

Generally looks great! Just adding a few notes below.

Also, I think we should add itest coverage for this new behaviour as well. I've prepared a test locally (which depends on the config validation fix mentioned below).
I can push to this PR if you'd like :)?

[ellemouton]

thanks @ViktorT-11 🙏 yes, feel free to push your diffs from your branch!

[ViktorT-11]

@ellemouton, I force pushed some changes to this PR. Let me know if you think they look good :)

[ellemouton]

looks good 🙏 nice itest!
for future - good practice to keep og commiter as author of the original commits they added (even if you amend to them) and then add new things as new commiter :)

[ViktorT-11]

Thanks 🎉!

for future - good practice to keep og commiter as author of the original commits they added (even if you amend to them) and then add new things as new commiter :)

Yes sorry about that 😞. As I needed to remove stuff from the first commit to split out the release docs from the commit, I noticed now that splitting that commit removed you as the author. I then fixed up the actual release docs commit, with the "second" split commit from the first commit, which resulted in me becoming the author for the second commit as well. I can try to fix that if you'd like?