Give the four log-entry structs a shared LogEntry interface - #52107
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Great work on this refactor! 🎉 This PR cleanly introduces a shared LogEntry interface for the four independent log-entry structs, solving the code duplication problem identified in #52091. What stands out:
This looks ready for review once you remove draft status. Nice integration with the existing codebase! 🚀
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality 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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
There was a problem hiding this comment.
Pull request overview
Introduces a common interface for generic handling of the four parsed log-entry types.
Changes:
- Adds shared source and severity constants with accessor implementations.
- Adds generic timestamp normalization, message generation, and formatting.
- Adds table-driven tests across all log sources.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/log_entry.go |
Defines and implements the shared LogEntry API. |
pkg/cli/log_entry_test.go |
Tests accessors, formatting, and timestamp passthrough. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| func (g GatewayLogEntry) EntryLevel() string { | ||
| if g.Level != "" { | ||
| return g.Level | ||
| } | ||
| return levelFromAllowed(g.Error == "" && g.Status != LogLevelError) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.2 AIC · ⌖ 9.12 AIC · ⊞ 5.4K
| } | ||
| return levelFromAllowed(g.Error == "" && g.Status != LogLevelError) | ||
| } | ||
|
|
There was a problem hiding this comment.
Potential logic issue: Status != LogLevelError conflates two separate domains
When g.Level is empty, the fallback is:
levelFromAllowed(g.Error == "" && g.Status != LogLevelError)LogLevelError is "error" and Status uses the same string for a status outcome ("success", "error", "unknown"). A Status of "unknown" with no Error string returns LogLevelInfo, which is likely incorrect — unknown status should not be treated as a success.
Consider being explicit:
if g.Error != "" || g.Status == "error" || g.Status == "unknown" {
return LogLevelError
}
return LogLevelInfo@copilot please address this.
There was a problem hiding this comment.
Fixed in f22320f: EntryLevel now returns error whenever Status == "error", Error is non-empty, or Level == "error", matching processGatewayLogEntry's OR-based classification. This also removes the dead Status != LogLevelError comparison.
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52107 · auto · 21.3 AIC · ⌖ 4.97 AIC · ⊞ 6.8K
Comment /ponytail to run again
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
L1-168: yagni: entire file (LogEntry interface, 4 impls, FormatLogEntry, constants) has zero call sites anywhere in the repo. Delete until a real consumer needs generic log-entry handling.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (341 new lines in 📄 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on one correctness bug and two test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Logic bug in
GatewayLogEntry.EntryLevel: theg.Status != LogLevelErrorguard compares an HTTP-style status string against the severity constant"error"; the comparison is alwaystruefor HTTP statuses, making it dead code. - Missing test coverage: no case for an all-blank
GatewayLogEntrymessage, and the zero-epoch behaviour offormatEpochTimestampis undocumented.
Positive Highlights
- ✅ Clean interface design: minimal surface, four orthogonal accessors, no leaking of struct internals.
- ✅ Compile-time interface assertions (
var _ LogEntry = ...) are exactly right here. - ✅
joinEntryMessagefiltering out"-"placeholders is a nice defensive touch. - ✅ Table-driven tests with
t.Parallel()throughout — good discipline. - ✅ No serialization breakage: only methods added to existing structs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 47.7 AIC · ⌖ 6.79 AIC · ⊞ 7.1K
Comment /matt to run again
| // EntryLevel implements LogEntry. | ||
| func (g GatewayLogEntry) EntryLevel() string { | ||
| if g.Level != "" { | ||
| return g.Level |
There was a problem hiding this comment.
[/codebase-design] The fallback level logic compares g.Status (an HTTP-style string like "200") against the string constant LogLevelError ("error"). That comparison will always be true for any HTTP status, so the g.Status != LogLevelError guard is a no-op and the level is driven solely by g.Error == "".
💡 Suggested fix
Drop the dead Status check:
func (g GatewayLogEntry) EntryLevel() string {
if g.Level != "" {
return g.Level
}
return levelFromAllowed(g.Error == "")
}Also add a test case with a non-empty Status and no Level to pin the expected behaviour.
@copilot please address this.
There was a problem hiding this comment.
Fixed in f22320f — the dead Status comparison is gone and EntryLevel now compares Status against "error" directly, matching the real status vocabulary.
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
[/tdd] GatewayLogEntry.EntryMessage silently returns an empty string when all four candidate fields (Message, Error, Event, Type) are blank. The test suite has no case for this path, so the caller gets an unexpectedly empty message string with no indication anything is wrong.
💡 Suggested test
{
name: "gateway log entry with all-blank fields returns empty message",
entry: GatewayLogEntry{Timestamp: "2024-01-12T10:00:00Z", Level: LogLevelInfo},
expectedMessage: "",
},Consider whether an empty message should be surfaced as a structured warning.
@copilot please address this.
There was a problem hiding this comment.
Added a test case for all-blank message fields in f22320f.
| } | ||
|
|
||
| func TestFormatEpochTimestampKeepsNonEpochValues(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
[/tdd] The TestFormatEpochTimestampKeepsNonEpochValues test asserts assert.Empty(t, formatEpochTimestamp("")) — an empty input should return an empty string and the test is correct — but a zero-valued epoch string like "0" or "0.000" would be parsed as a valid epoch and formatted to "1970-01-01T00:00:00Z". Whether that is intentional isn't obvious; a test case would document the decision explicitly.
💡 Suggested addition
// Documents that a zero epoch is treated as a valid timestamp, not a placeholder.
assert.Equal(t, "1970-01-01T00:00:00Z", formatEpochTimestamp("0"))If zero is considered a placeholder (like "-"), add the guard in formatEpochTimestamp.
@copilot please address this.
There was a problem hiding this comment.
Added a test in f22320f documenting that "0" is treated as a valid epoch, not a placeholder.
🧪 Test Quality Sentinel Report🚨 Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
|
|
@copilot Please address the open review feedback below, refresh the branch after changes, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR Triage: #52107
Automated triage — see labels for machine-readable classification.
|
|
@copilot This PR looks ready for another finishing pass. Please refresh the branch, review any outstanding feedback, and run the Focus areas:
Run context: https://github.com/github/gh-aw/actions/runs/31545290203
|
Branch is already up to date with |
🤖 Triage: PR #52107
|
AccessLogEntry,FirewallLogEntry,AuditLogEntry, andGatewayLogEntryeach model a parsed log line but share no common shape, so any code wanting to handle "a log entry" generically must special-case all four. Their fields diverge enough (Timestampisstringin three,float64inAuditLogEntry; only gateway hasLevel) that an embedded base struct would force a wire-format change, so this uses an interface with accessors instead.pkg/cli/log_entry.go(new)LogEntryinterface:EntryTimestamp(),EntrySource(),EntryLevel(),EntryMessage().LogEntrySourceconstants (access,firewall,audit,gateway) and sharedLogLevelInfo/LogLevelError.isAllowedSquidStatus,isRequestAllowed,isEntryAllowed) rather than reimplementing them.float64) normalize to RFC3339 UTC; gateway timestamps are already RFC3339 and pass through. Unparseable values are returned unchanged.var _ LogEntry = ...assertions, plusFormatLogEntryas a first generic consumer.No struct fields, JSON tags, or parsers were touched, so serialization and existing call sites are unaffected.
pkg/cli/log_entry_test.go(new)Table-driven coverage of allowed/blocked cases per source, generic formatting over a mixed
[]LogEntry, and non-epoch timestamp passthrough.