fix(log): route Writer through the structured encoder instead of raw passthrough - #797
Merged
Merged
Conversation
…passthrough
log.Writer() is used as a subprocess's Stdout/Stderr at ~65 call sites
across the codebase (docker build, buildkit, SSH, IDE installers, git
clone, etc). Its levelWriter.Write wrote raw bytes directly to the
stderr sink, bypassing zap's encoder entirely -- contradicting its own
doc comment ("writes each line as a log entry") and producing
unencoded lines in what's supposed to be a uniform JSON/text/logfmt
stream in --log-output json mode.
levelWriter now line-buffers and calls Info/Debug/Error per complete
line, so subprocess output goes through the same encoder as every
other log call. Close flushes any trailing line left without a
newline (previously silently dropped).
✅ Deploy Preview for images-devsy-sh canceled.
|
📝 WalkthroughWalkthrough
ChangesLog writer behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Writer
participant levelWriter
participant PackageLogger
participant Sink
Writer->>levelWriter: Write byte stream
levelWriter->>levelWriter: Buffer and split complete lines
levelWriter->>PackageLogger: Log line at configured level
PackageLogger->>Sink: Emit JSON record
Writer->>levelWriter: Close
levelWriter->>PackageLogger: Flush trailing partial line
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for devsydev canceled.
|
Reorder levelWriter's exported Close before the unexported logLine helper, and hoist the repeated "json" format literal in writer_test.go into a constant.
skevetter
marked this pull request as ready for review
July 29, 2026 03:55
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/log/writer_test.go (1)
37-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover fragmented writes and emitted message values.
This test only checks two records from one
Writeand their JSON shape. Add a case such asWrite("line one\nline ")followed byWrite("two\n"), then decode and assert the messages are exactlyline oneandline two.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/writer_test.go` around lines 37 - 60, Extend TestWriter_SplitsMultipleLinesInOneWrite to perform fragmented writes, first writing “line one\nline ” and then “two\n”. Decode each emitted JSON record and assert their message values are exactly “line one” and “line two”, while retaining the existing record-count validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/log/writer.go`:
- Around line 44-51: The Write handling around w.buf.ReadString must avoid
resetting and copying the entire incomplete fragment on every partial write;
process complete lines incrementally while retaining only the unconsumed suffix.
Add a bounded pending-line policy for unterminated output, such as flushing or
truncating once the configured maximum is reached, so fragmented progress output
cannot grow without limit.
- Around line 68-70: The logLine method currently discards newline-only records
by returning when line is empty. Remove the empty-line early return in
levelWriter.logLine so blank output lines are preserved, relying on Close’s
existing buf.Len() guard to avoid phantom records.
---
Nitpick comments:
In `@pkg/log/writer_test.go`:
- Around line 37-60: Extend TestWriter_SplitsMultipleLinesInOneWrite to perform
fragmented writes, first writing “line one\nline ” and then “two\n”. Decode each
emitted JSON record and assert their message values are exactly “line one” and
“line two”, while retaining the existing record-count validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0efadac0-d1f8-4a08-8831-55a13193947a
📒 Files selected for processing (2)
pkg/log/writer.gopkg/log/writer_test.go
skevetter
marked this pull request as draft
July 29, 2026 04:02
- levelWriter.Write now scans only the bytes passed to that call for '\n' (via bytes.IndexByte) instead of resetting and re-copying the entire accumulated pending line on every partial write, which made writing one large unterminated line in many small chunks effectively O(n^2). Previously-seen bytes are never rescanned. - Bound the pending line to 64KiB: a subprocess that never emits a newline no longer grows levelWriter's buffer without limit. - logLine no longer discards empty lines. That guard was only ever needed to stop Close from emitting a phantom record when nothing was pending, which Close already handles itself via its buf.Len() check -- so it was silently dropping genuine blank lines from subprocess output. - Extended the multi-line test to cover a line split across two Write calls (as os/exec delivers chunks arbitrarily) and decode each emitted record to assert its exact msg, and added a test asserting blank lines are preserved as empty-message records.
skevetter
marked this pull request as ready for review
July 29, 2026 05:12
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/log/writer_test.go (1)
90-112: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the 64 KiB pending-line boundary.
The suite covers split writes and
Closeflushing, but not the writer’s documented oversized unterminated-line behavior. Add a payload larger thanmaxPendingLine, verify it is emitted beforeClose, and confirmClosehandles only any remaining tail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/log/writer_test.go` around lines 90 - 112, Add regression coverage alongside TestWriter_FlushesTrailingPartialLineOnClose for a payload exceeding maxPendingLine; verify the oversized unterminated line is emitted before calling Writer.Close, then confirm Close flushes only any remaining tail without duplicating the already-emitted payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/log/writer_test.go`:
- Around line 72-86: Update assertJSONLineMsg to distinguish a missing msg field
from an explicit empty value by making the decoded Msg field nullable or
validating the raw JSON field, and reject missing fields. Replace the separate
substring assertion for the blank record with assertJSONLineMsg(t, 1, lines[1],
""), including the corresponding assertion path around the later blank-record
check.
---
Nitpick comments:
In `@pkg/log/writer_test.go`:
- Around line 90-112: Add regression coverage alongside
TestWriter_FlushesTrailingPartialLineOnClose for a payload exceeding
maxPendingLine; verify the oversized unterminated line is emitted before calling
Writer.Close, then confirm Close flushes only any remaining tail without
duplicating the already-emitted payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93b66d81-be19-41c0-b6b2-6e16d8325f91
📒 Files selected for processing (2)
pkg/log/writer.gopkg/log/writer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/log/writer.go
Comment on lines
+72
to
+86
| func assertJSONLineMsg(t *testing.T, i int, line, wantMsg string) { | ||
| t.Helper() | ||
| if !strings.HasPrefix(line, "{") || !strings.HasSuffix(line, "}") { | ||
| t.Errorf("line %d is not valid single-object JSON: %q", i, line) | ||
| return | ||
| } | ||
| var rec struct { | ||
| Msg string `json:"msg"` | ||
| } | ||
| if err := json.Unmarshal([]byte(line), &rec); err != nil { | ||
| t.Errorf("line %d: json.Unmarshal: %v", i, err) | ||
| return | ||
| } | ||
| if rec.Msg != wantMsg { | ||
| t.Errorf("line %d msg = %q, want %q", i, rec.Msg, wantMsg) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the blank record through the JSON helper.
The substring check can accept malformed output, while Msg string cannot distinguish a missing field from {"msg":""}. Make Msg a pointer (or validate the raw field), then call assertJSONLineMsg(t, 1, lines[1], "").
Proposed test hardening
var rec struct {
- Msg string `json:"msg"`
+ Msg *string `json:"msg"`
}
if err := json.Unmarshal([]byte(line), &rec); err != nil {
t.Errorf("line %d: json.Unmarshal: %v", i, err)
return
}
- if rec.Msg != wantMsg {
- t.Errorf("line %d msg = %q, want %q", i, rec.Msg, wantMsg)
+ if rec.Msg == nil {
+ t.Errorf("line %d is missing msg", i)
+ return
+ }
+ if *rec.Msg != wantMsg {
+ t.Errorf("line %d msg = %q, want %q", i, *rec.Msg, wantMsg)
}
- if !strings.Contains(lines[1], `"msg":""`) {
- t.Errorf("line 1 = %q, want an empty msg field", lines[1])
- }
+ assertJSONLineMsg(t, 1, lines[1], "")Also applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/log/writer_test.go` around lines 72 - 86, Update assertJSONLineMsg to
distinguish a missing msg field from an explicit empty value by making the
decoded Msg field nullable or validating the raw JSON field, and reject missing
fields. Replace the separate substring assertion for the blank record with
assertJSONLineMsg(t, 1, lines[1], ""), including the corresponding assertion
path around the later blank-record check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
log.Writer() is used as a subprocess's Stdout/Stderr at ~65 call sites (docker build, buildkit, SSH, IDE installers, git clone, etc). Its levelWriter.Write previously wrote raw bytes directly to the sink, bypassing zap's encoder and producing unencoded lines in what should be a uniform JSON/text/logfmt stream in
--log-output jsonmode.levelWriter now line-buffers and calls Info/Debug/Error per complete line, so subprocess output goes through the same encoder as every other log call. Close flushes any trailing partial line (previously silently dropped).
Summary by CodeRabbit