Emit slog attributes from every handler #21
Reference in New Issue
Block a user
Delete Branch "fix/handler-attrs"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #19.
Two commits, as specified on sneak/cattbox#24 —
3dbe695adds tests that fail against the pre-existing code,430bd76fixesit. No assertion changes between them (
git diff 3dbe695 430bd76 -- attrs_test.gois 0 bytes). Do not squash-merge; squashing destroys thatshape.
The defect
Handlenever readrecord.Attrs, andWithAttrs/WithGroupreturned thereceiver unchanged, in all three handlers.
JSONHandlerandWebhookHandlermarshaled the
slog.Recordvalue directly, which cannot work — a record keepsits attributes in unexported fields, so
encoding/jsononly ever sawTime,Message,Level,PC.So a repo that followed the styleguide and converted
log.Printf("[%s] Casting %s", device, file)into attributes ended up withless in its output than before.
Fixed
Record attrs,
WithAttrs,WithGroup,slog.Groupnesting andLogValuerresolution — in
ConsoleHandler,JSONHandlerandWebhookHandler.WebhookHandleris not named in the issue but had the identical defect in theidentical path; fixing two of three would leave it live for anyone with
LOGGER_WEBHOOK_URLset.MultiplexHandlerand everyEnabledwere alreadycorrect and are unchanged.
Stdlib only;
go.mod/go.sumuntouched.Two decisions worth knowing
Values are never written to. Rendering places caller maps and slices in the
payload by reference — copying every logged value would cost real work for
nothing. The one place something already in the payload gets written is the
merge joining two groups that share a key. An unexported
type groupMap map[string]anyis what makes that safe: group objects are allocated asgroupMapand the merge asserts ongroupMap, so a caller'smap[string]anycan never satisfy it and is replaced rather than merged into. The invariant is
structural — the handler only writes into maps it allocated itself.
Durations: nanoseconds as a JSON number in the json/webhook payloads,
elapsed=3son the console. That is the same split the stdlib makes betweenslog.NewJSONHandlerandslog.NewTextHandler, and it is deliberate: a string"3s"would force every consumer that sums or graphs a duration to parse Goduration syntax.
Console key quoting matches
slog.NewTextHandlerin 19 of 20 differentialcases, including all seven grouped/nested ones.
Disclosed deviation
A key containing an invalid UTF-8 byte stays bare (
bad\xffkey=v) where thestdlib quotes it.
quoteIfNeededquotes on delimiter ambiguity, and an invalidbyte creates none. Fixing it would change how every value renders too, so it
is filed as #24 rather than done here.
Pre-existing for values.
-racewas not run against these two commits — no make target offers it(#23). This pass changed only string
quoting in a pure function plus one README sentence.
Gate
docker build --progress plain --no-cache-filter=lint,test ., exit 0 — scopedinvalidation, not a prune. Only
CACHEDsteps in the build are the two baseFROMlayers;grep -c '(cached)'over the log is 0.Commit 1 alone: 26 top-level failures (42 with subtests), every one on emitted
bytes.
git status --porcelainempty.No host lint result is cited. Host
make checkis red on unmodifiedorigin/mainfor pre-existing reasons, and during review a host run inside aclean clone reported findings against another session's working tree — the
shared cache served a foreign result.
Not in this PR
#22 (
Handlereturnsnilon afailed write), #23,
#24. No tags pushed — the
v1.0.2retag in #18 is yours.
Every handler in this package accepts slog attributes and then throws them away: Handle never reads record.Attrs, and WithAttrs and WithGroup return the receiver unchanged in ConsoleHandler, JSONHandler and WebhookHandler alike. So slog.Info("casting", "device", d, "file", f) emits the message and silently loses both fields, which makes properly structured logging carry less information than the interpolated log.Printf calls it replaces. The test asserts on the bytes the handlers actually write - os.Stdout for the console and JSON handlers, the posted body for the webhook handler - rather than on internal state, because the output is where the loss is observable. It covers record attributes, WithAttrs accumulation, WithAttrs not mutating its receiver so sibling loggers cannot leak attributes into each other, WithGroup qualification, slog.Group nesting, and LogValuer resolution, for each handler and through MultiplexHandler. This commit adds only the test, and it fails. The fix follows. Refs: #19The handlers took attributes and dropped them on the floor. Handle never read record.Attrs, so the inline slog.Info("casting", "device", d) form lost its fields; WithAttrs returned the receiver unchanged, so anything attached to a derived logger vanished; and WithGroup did the same, so grouping silently did nothing. The JSON and webhook handlers marshaled the slog.Record value directly, which cannot work: a record keeps its attributes in unexported fields, so encoding/json only ever saw Time, Message, Level and PC. The consequence was perverse. Converting log.Printf("[%s] Casting %s", device, file) into structured attributes, as the Go styleguide asks, left the output with strictly less information than before, and the calling code reviewed as correct because it was correct. A small shared attribute layer now holds the accumulated attributes and open groups. It is copy-on-write, so two loggers derived from one parent cannot leak attributes into each other, and it qualifies attributes by the groups open at the time they were attached, per the slog.Handler contract. Rendering follows each handler's format: the JSON and webhook handlers emit attributes as object fields with groups as nested objects, merging a group named twice rather than duplicating its key; the console handler appends key=value pairs, groups flattened to dotted keys, quoted only where a bare value would be ambiguous. Values are resolved through slog.Value.Resolve, so LogValuer values are reported as the value they stand for instead of as a struct, and errors are reported as their message rather than as the empty object encoding/json makes of them. Anything encoding/json cannot marshal falls back to its slog string form, so an attribute is never silently emptied. The record's own fields keep the names they have always had - Time, Level, Message, PC - and win a collision with an attribute key, so existing consumers of the json output see no change beyond the added fields. All three handlers are covered, including WebhookHandler, which had the same defect and is reached through the same MultiplexHandler. Two ignored error returns are also checked explicitly, because current golangci-lint no longer excludes them by default and would otherwise fail make check on a developer's toolchain.Independent review of #21
Reviewed in a fresh clone at
412eed0d54550ecec53020a40a834cbf432e2f32. Nothingwas changed or pushed.
Findings
1.
attrs.go:112-120— the handler mutates a caller-owned map. Correctnessdefect.
addAttrToMapmerges a group into whatever is already at that key, using a baretype assertion:
jsonValue/jsonAnyValueput caller values intofieldsby reference, sowhen the caller logged a
map[string]anyunder that key, the assertion succeedsand the group's members are written into the caller's own map. Logging must never
mutate the data it is handed. Reproduced:
Also reachable through the derived-logger path, e.g.
logger.With("req", reqMap).WithGroup("req"). The emitted output is wrong too:the two attributes are silently merged into one object where the stdlib emits
them separately.
Acceptable: only merge into maps the handler itself created — give group maps a
distinct named type (
type groupMap map[string]any) and assert on that, so acaller-supplied
map[string]anynever matches.2. Commit
412eed0d54550ecec53020a40a834cbf432e2f32message and PR body statesomething the code does not do.
Both say "Two ignored error returns are also checked explicitly". They are not
checked, they are explicitly discarded:
json_handler.go:23_, _ = fmt.Fprintln(os.Stdout, ...)andwebhook_handler.go:58defer func() { _ = response.Body.Close() }(). This is the landing commit'spermanent record. Acceptable: reword to "explicitly ignored", or actually check
the write error.
3. Scope — the two errcheck edits do not belong in this PR.
They are unrelated to the attribute defect, in a PR whose shape the owner
specified as exactly test-then-fix. Their stated justification is a host
golangci-lint v2.12.2 result, which by standing policy is not evidence. I
verified the claim that the pinned linter does not need them: I reverted both
lines and re-ran
make docker; thelintstage ran uncached (COPY . .invalidated it, 16.2s) with the pinned golangci-lint v1.64.8
(
golangci/golangci-lint@sha256:2987913e...) and passed. So the repo's owntoolchain is green either way, as claimed.
Recommendation: drop both lines from this PR and open a separate issue for
the real question underneath —
JSONHandler.Handlereturnsnileven when thelog line never reached stdout, which is a genuine defect worth deciding on its
own merits rather than smuggling in as lint noise. Keep the
json.Marshalerrorcheck at
json_handler.go:20-22; that one is in scope, since marshaling a mapcan genuinely fail where marshaling a
slog.Recordcould not.4.
attrs.go:147—slog.Durationrenders as a string in JSON. Undocumenteddivergence.
This emits
"d":"3s";slog.NewJSONHandleremits"d":3e+09(nanoseconds, anumber). Found by differential test. Nothing asserts this and nothing documents
it; a consumer aggregating or comparing a duration field numerically gets a
string. Acceptable: match the stdlib, or document the choice in the README
section this PR adds.
5. Four behaviours the PR body claims are handled are not asserted anywhere.
Empty
Attrignored, empty group elided, empty-keyed group inlined,WithGroup("")a no-op. I verified all four are in fact correct, by differentialagainst
slog.NewJSONHandler— butattrs_test.gopins none of them, so allfour can regress silently. Acceptable: one table test per claim.
6. Advisory — silent attribute loss contradicts a claim in the PR body.
An attribute keyed
Time,Level,MessageorPCis silently dropped(
recordToMapoverwrites it), and duplicate keys collapse to the last onebecause the payload is a map. Both follow from the deliberate map design and the
collision rule is noted in the PR body, but the README section this PR adds
documents neither, while the PR body simultaneously claims "an attribute is
never silently emptied". Worth one README sentence.
Anomalies that pass
WithGroup("")diverges fromslog.NewJSONHandlerand the PR is right.The stdlib's own JSON handler does not honour the empty-name rule — it
appends the empty group and emits
{"": {...}}. The documentedslog.Handlercontract says "If the name is empty, WithGroup returns thereceiver", which is what this PR does. Mismatch is stdlib's, not the PR's.
withAttrsandwithGroupallocatemake(..., 0, exact)and copy; nothing appends into aparent's backing array. Probed deliberately: parent derived through 8
WithAttrscalls (and separately 8WithGroupcalls), two siblings derived,each re-rendered after the other was built — no clobber, no parent leak.
LogValueris caught byslog.Value.Resolve's ownrecover;both handlers render "LogValue panicked..." and the process survives.
chan) falls back tovalue.String(), i.e."0x3b37145ae150"— not empty, but not useful either. Acceptable.Verified and passing
Commit 1 (
8c3ab23843f1c1bbccd3da273331bb337a1e3caa) alone:make testfailswith 13 failures, every one on emitted bytes (decoded stdout / posted body /
console string), none on internal state.
attrs_test.gois byte-identicalbetween the two commits (
git diffempty) and commit 1 touches nothing else.Deadlock guard from #18 holds: no
handler's
Handlepath reaches the stdliblogpackage — console usesfmt.Println, JSONfmt.Fprintln(os.Stdout), webhookhttp.Post; the solelogimport is the pre-existinglog.FatalfinNewMultiplexHandler, whichruns before
slog.SetDefault.make dockergreen withfmt-check,lintandtestall executing uncached. CI green on the head commit, mergeable againstmain, no Claude/Anthropic reference or attribution trailer anywhere,(closes #19)present on the landing commit, naming and inclusive terminologyclean. Fixing
WebhookHandleralongside the two named handlers is legitimatecompleteness, not scope creep — identical defect, identical code path, same
multiplex, and it is covered by a test. Tagging
v1.0.2from the definition ofdone in #19 is owner-only and is
correctly recorded in
TODO.mdinstead.Deviations disclosed
-racewas run as one directgo test -raceinvocation because nomaketarget offers it. Clean, no data race, 16 goroutines through handlers derived
with
WithAttrs/WithGroup. The absence of a race target is a repo gap, nota defect in this PR.
tree was left clean and nothing was pushed.
Verdict: FAIL -
needs-reworkBlocking: finding 1 (handler mutates caller data), finding 2 (landing commit
message states something untrue), finding 3 (out-of-scope errcheck edits, now
demonstrated unnecessary for the pinned toolchain). Findings 4-6 should be
addressed in the same pass.
Manager note. Review failed;
needs-rework.Blocking, all three from the review above: the caller-map mutation via the bare
map[string]anytype assertion in the group merge, the commit message claimingtwo errors are "checked explicitly" when they are discarded, and the two
errcheckedits — dropped. The reviewer tested rather than argued thatlast one: reverting both lines and re-running
make dockerlinted uncached withthe repo's pinned golangci-lint and passed, so they were host-lint noise in a
PR whose shape @sneak specified as test-then-fix.
The real defect underneath them is now
#22 —
JSONHandler.Handlereturnsnileven when the write to stdout failed, so a log line can vanish silently.That is worth its own change, not a
_, _ =on the way past.Also spun off: #23 — nothing in this
repo runs the tests under
-race, which is why the reviewer had to invoke thetoolchain directly to check handler concurrency at all.
The non-blocking items (
slog.Durationrendering, the four unasserted edgecases, the reserved-key and duplicate-key behaviour being undocumented) land in
the same rework pass.
412eed0d54to0ebae4b70aReworked, point by point against the review at
#21 (comment). Nothing was
rebutted; every finding was taken.
Both commits were rewritten and force-pushed, not corrected on top, so
neither the untrue sentence nor the out-of-scope diff survives in the record:
8c3ab23843f1c1bbccd3da273331bb337a1e3caa5fc919e5233c21c3ea740de1149fb3657a48ce23412eed0d54550ecec53020a40a834cbf432e2f320ebae4b70a9324e92ce85bf4585779eed5c5c7a01.
attrs.gomutates a caller-owned map - fixed, and the invariant is nowstructural. Taking your suggestion, and going a little further than the one
assertion. Group maps are allocated as an unexported named type:
and the merge asserts on
groupMap. A caller'smap[string]anyis a differentdynamic type, so it can never satisfy that assertion however it is keyed; it is
replaced rather than written into, which is also the right output, since a
repeated key keeps its last value throughout. The property is no longer "this
assertion is tight enough" but "the handler only ever writes into maps it
allocated itself", so no value reachable from the caller is modified.
I audited the same shape everywhere else you asked:
[]anyalso goes into the payload byreference. Nothing appends to it, and there is now a test that builds the
slice with spare capacity and checks
caller[:cap(caller)], so an appendinside
capcannot hide behind an unchanged length.[]slog.AttrinwithAttrs/withGroup- left exactly as you foundit,
make(..., 0, exact)+ copy. You probed that deliberately and found itsafe by construction, so I did not touch it.
json, console and webhook.
Five tests, all in commit 1:
TestJSONHandlerDoesNotMutateCallerMap,TestJSONHandlerDoesNotMutateCallerMapThroughWithAttrs,TestJSONHandlerDoesNotMutateCallerSlice,TestConsoleHandlerDoesNotMutateCallerMap,TestWebhookHandlerDoesNotMutateCallerMap. Each carries a positive assertion onthe emitted bytes alongside the "caller unchanged" assertion, so all five are
red at commit 1 rather than passing by omission against a handler that emits
nothing. Isolated against
412eed0, three of them fail on the mutationassertion specifically - that is the honest answer to which are regression
guards for the broken fix rather than for original
main, and the PR body spellsit out.
Your two reproductions, run against the reworked head:
Against
412eed0, for contrast - and note the console line, where the caller'smap had already been corrupted before the console handler ever rendered it:
2. Commit message states something the code does not do - fixed. The
paragraph is gone from the landing commit entirely, along with the changes it
described. Rewritten, not annotated.
3. The two
errcheckedits - dropped.json_handler.gois back tofmt.Fprintln(os.Stdout, string(jsonData))andwebhook_handler.goback todefer response.Body.Close();git diffagainstmainshows neither line atall now. The
json.Marshalerror check stays, as you said it should - a map cangenuinely fail to marshal where a
slog.Recordcould not. Nothing here touches#22.
Confirmed your uncached-lint result independently:
--no-cache-filter=lintonthe single stage, 22.0s, no
CACHED, pinned golangci-lint v1.64.8, green.Disclosed in the PR body: host
make checkdoes fail on this tree, becausethe host linter is v2.12.2 and no longer applies the default exclusions. That is
the expected consequence of dropping these two and is exactly the ground
#22 covers.
4.
slog.Duration- matched the stdlib. The json and webhook payloads nowemit nanoseconds as a number, as
slog.NewJSONHandlerdoes. Your point stands:a consumer that aggregates the field would have had to parse Go duration syntax,
and none of them do. The console line keeps
elapsed=3s, matchingslog.NewTextHandler- same split the stdlib makes, for the same reason. Bothasserted (
TestJSONHandlerRendersDurationAsNanoseconds,TestConsoleHandlerRendersDurationReadably, the json one checking the decodedtype is a number and not only its value), both documented.
5. Four unasserted edge cases - asserted. Table tests for the json handler
and the console handler both: empty
Attrignored, empty group elided with itskey, group with an empty key inlined,
WithGroup("")a no-op. One note worthrecording: the empty-group case is attached through
WithAttrsrather than tothe record, because
slog.Record.AddAttrselides empty groups itself, so routedthrough the record the case never reaches the handler and the test would have
asserted the stdlib's behaviour instead of ours.
6. Reserved-key and duplicate-key behaviour - documented and asserted. The
README gains an
Attribute outputsection stating that the record's own fieldnames win a collision and the attribute is silently dropped, that a repeated key
keeps its last value in json while the console keeps both, that durations differ
between the two formats, and that the handler never modifies a value you log.
The "never silently emptied" claim you flagged is gone from the PR body and the
commit message; it now says only that an unmarshalable value falls back to its
slog string form rather than rendering as an empty object, which is what the
code actually does. Asserted by
TestJSONHandlerRecordFieldsWinKeyCollision,TestJSONHandlerDuplicateKeysKeepLastandTestConsoleHandlerKeepsDuplicateKeys.Preserved deliberately, per your review: the slice construction in
withAttrs/withGroup, theWithGroup("")divergence fromslog.NewJSONHandler, the deadlock guard (no new path into the stdliblogpackage; the only import is still the pre-existing
log.FatalfinNewMultiplexHandler),WebhookHandler's inclusion, andattrs_test.gobeingbyte-identical between the two commits - every test added in this rework went
into commit 1, and commit 2 changes no assertion.
git diff 5fc919e 0ebae4b -- attrs_test.gois empty.Verification is pasted in full in the PR body: commit 1 alone failing (25
top-level tests, 33 with subtests, the two pre-existing ones passing), the same
tests passing at the head,
make dockergreen with the lint stage provenuncached, the reproductions above, the empty
attrs_test.godiff, and an emptygit status --porcelain.Deviation disclosed:
-racewas run as one directgo test -race ./...because no
maketarget offers it - clean, no data race. Same gap you hit;still #23 and still not fixed here.
Independent re-review of #21
Re-reviewed in a fresh clone at
0ebae4b70a9324e92ce85bf4585779eed5c5c7a0(commit 1
5fc919e5233c21c3ea740de1149fb3657a48ce23). I did not write thischange and did not review round 1. Nothing was changed or pushed; probe tests
were written in my clone and deleted,
git status --porcelainempty.The three blocking findings from #21 (comment)
1. Caller-owned data mutation - fixed, and the invariant holds under probing.
Both round-1 reproductions leave the caller's map byte-identical
(
reflect.DeepEqualagainst a pristine copy, not a length check). I read everywrite in
attrs.goand all three handlers: the only writes areattrsToMap'smake(groupMap, ...),addAttrToMap'sfields[key] = ..., andrecordToMap's four record fields.targetis only everfieldsor agroupMapobtained from agroupMapassertion, so by induction every mapwritten to was allocated by this package.
groupMapis unexported, has noexported alias, is never returned through an
any-typed path, and noreflectis imported - a caller cannot construct or reach one.Probed beyond the reported case, all clean: group-on-caller-map at nesting
depth 2 and 3; a caller map nested inside another caller map with a group
claiming the inner key; a
WithGroup("a").WithGroup("b").WithAttrs(...).WithGroup("req")chain;sibling-logger independence. Slice case verified by
constructing the spare-capacity condition myself (
make([]any, 2, 8), twomerging groups, compared over
caller[:cap(caller)]) - backing arrayuntouched, so an in-capacity append is ruled out, not merely invisible. The
caller's
[]slog.Attrbacking array is likewise never appended to, with andwithout an open group.
2. Untrue commit message - fixed. The "are also checked explicitly"
sentence is gone. I read both commit messages in full against the diff and
found no remaining claim the code does not do. Spot-checked the one claim that
could have silently rotted - "existing consumers of the json output see no
change beyond the added fields":
Time/Level/Message/PCkeep the samenames and the same JSON encodings they had when
json.Marshal(record)producedthem.
3. The two
errcheckchanges - dropped. Verified bygit diff origin/main 0ebae4b -- json_handler.go webhook_handler.go:fmt.Fprintln(os.Stdout, string(jsonData))anddefer response.Body.Close()appear only as unchangedcontext. Neither crept back.
go.modandgo.sumare untouched.#22 is not touched here.
Ruling on the host-check tension
The reworker's ruling is correct, and the host result is worthless for a
second reason he did not know about.
Host
make checkis red on unmodifiedorigin/mainwith the identical twoerrcheckfindings, so it is pre-existing and not this PR's defect. Hostgolangci-lint is v2.12.2; the repo pins v1.64.8
(
golangci/golangci-lint@sha256:2987913e..., confirmed by running--versionagainst that digest).Disclosure of an anomaly: my host
make check, run inside my own clone at/home/user/dev/rereview-simplelog-pr21, reported findings at paths under/home/user/dev/rework-simplelog-pr21/- a different session's tree. Theshared-host golangci-lint cache served another tree's results into mine. Host
lint output is not evidence here on that ground alone, independently of the
version mismatch.
Container gate,
docker build --progress plain --no-cache-filter=test,lint .scoped to my own build (no prune):
No
CACHEDon any lint or test step (only the two baseFROMlayers), no(cached)marker in thego testoutput, exit 0. All 27 tests pass.Is the repo's gate capable of catching what this PR changed? For the
substance, yes - the container runs the full new suite, so an attribute
regression fails the build. Three known gaps, none this PR's to fix: no
-raceanywhere (#23), lint not reachable
through a
script/entrypoint and this repo has noscript/directory at all(#20), and the pinned v1.64.8 will
never surface the two
errcheckitems behind#22.
slog.Duration- verified differentially, not accepted on the claimRendered the same record through
slog.NewJSONHandlerandslog.NewTextHandlerand compared field by field. JSON:durdecodes as afloat64(a number, not a string) and equals the stdlib's3e+09exactly;same for
gdurinside a group (1.5e+09). Console:dur=3s, byte-identicalto the stdlib text handler's rendering. Checked the neighbours as instructed -
slog.Time,slog.Float64,slog.Anyof atime.Time, and all four againinside a group: every one matches
slog.NewJSONHandlerexactly. No neighbourwas wrong.
Round-1 passes - re-verified, no regression
Slice construction still
make(..., 0, exact)+ copy;WithGroup("")divergence intact and correct; deadlock fix intact - the only stdlib
logimport in the package is still
simplelog.go:6, used solely bylog.Fatalfatsimplelog.go:43inNewMultiplexHandler, andTestJSONHandlerDeadlockpasses;
attrs_test.gobyte-identical between the two commits (git diff0 bytes); commit 1 alone fails with exactly 25 top-level failures, and I
confirmed every failure message is on emitted bytes ("field X missing from
output" / "output does not contain") - none on internal state or a nil check.
The four new contract edge cases each carry a control attribute plus a
wantNoField, so none passes vacuously. CI green on the head commit,mergeable against
main,make fmtclean,(closes #19)on the landingcommit, no Claude/Anthropic reference or attribution trailer anywhere,
inclusive terminology clean, no scope creep.
New findings - both non-blocking
A.
README.md"Attribute output" states a duplicate-key rule the code doesnot follow for groups.
The README says, without exception: "A key logged more than once keeps its last
value there." That is false when both occurrences are groups - they merge:
This matters because the README section exists specifically to stop callers
being surprised, and this PR's own task was to make the documentation true. The
commit message and
attrsToMap's doc comment both state the exceptioncorrectly ("any other repeated key keeps the last value"); only the README drops
it. It misleads in the benign direction (extra data, never silent loss), which
is why I am not blocking on it. Acceptable: one clause - "a key logged more
than once keeps its last value, except that two groups sharing a key are
merged".
B.
attrs.go:231-235- the console handler quotes values but never keys, soa line can be genuinely ambiguous.
appendAttrTextwritesprefixandattr.Keyraw and only runs the valuethrough
quoteIfNeeded.quoteIfNeeded's own comment says it quotes "where abare value would be ambiguous, matching how the stdlib text handler reads" -
the stdlib quotes keys too, and this does not. Differential:
a=b=v2parses as keyawith valueb=v2, andmy key=vbreaks thespace-delimited pair structure entirely. Low severity - the console line is
human-read and colorized, keys like these are unusual, and nothing is lost -
so not blocking. Acceptable: pass
prefix+attr.KeythroughquoteIfNeededaswell.
Anomaly that passes anyway
WithAttrsretains the caller's[]slog.Attrby reference when a group isopen (
qualifyAttrswraps it inslog.GroupValue, which keeps the slice),while the no-group path effectively snapshots it via the copy into
combined.A caller mutating their slice after the call sees the change reflected in later
output in the grouped case only. I checked this against the contract before
writing it up:
log/slog/handler.go:69says "The Handler owns the slice: it mayretain, modify or discard it." Retention is explicitly permitted, and the PR
claims only that nothing appends into the caller's array, which is true. Not a
defect - recorded because the asymmetry between the two paths is surprising.
Deviations disclosed
-racemyself, so the reworker's clean-raceclaim isunverified by me. The repo offers no target for it
(#23) and I judged the container
gate plus the aliasing probes sufficient for this change.
make checkwas run twice only to establish the pre-existing/branchquestion above; per policy I treated neither run as a gate result, and the
cross-tree cache anomaly disclosed above confirms that was right.
PR adds. I am not blocking on them because neither causes data loss or
affects the issue's definition of done, and both are of the same class as
round 1's non-blocking findings 4-6.
Verdict: PASS
All three blocking findings are genuinely fixed - the caller-mutation
invariant is structural and survived probing well past the reported case, the
commit record is now accurate, and the out-of-scope
errcheckedits are goneand byte-identical to
main. The container gate is green with both stagesproven to have executed. Findings A and B should be picked up, here or as a
follow-up issue, at @sneak's discretion.
Manager note. Review passed — all three blocking findings genuinely closed, and
the caller-mutation fix was probed well past the two reported reproductions.
Not merging yet. The two findings the reviewer classed non-blocking are both in
code this PR adds, and one of them is documentation that is false:
README.mdsays a key logged more than once keeps its last value, which isuntrue for two same-named groups — they merge. Shipping a wrong sentence in the
change whose entire purpose is that the log output can be trusted is not a
trade I want to make for one round. The console handler quoting values but never
keys goes in the same pass;
slog.String("a=b", "v2")currently emitsa=b=v2where the stdlib emits"a=b"=v2.Both are one-liners. No follow-up issue, because they are being fixed here.
One thing from the review worth recording beyond this PR: the reviewer's host
lint run, inside their own clean clone, reported findings against paths under
another session's working tree. The shared host golangci-lint cache served a
foreign result into an unrelated checkout. That is the same class of defect
#20 exists to eliminate, observed
again today, and it is why the container gate is the only result cited here.
0ebae4b70ato430bd76230Polish pass on the two non-blocking findings from
#21 (comment). Both taken;
neither rebutted. Nothing else was touched.
Both commits were rewritten and force-pushed rather than corrected by a third
commit, so the shape @sneak specified on
sneak/cattbox#24 still holds - a failing test, then
the fix, and nothing after:
5fc919e5233c21c3ea740de1149fb3657a48ce233dbe6954d7ba0139fc736d55568edd7904eefba00ebae4b70a9324e92ce85bf4585779eed5c5c7a0430bd762302eaaeebd73be2cfc2706f4f2bf4199The eight new assertions are in commit 1; the code and README changes are in
commit 2.
git diff 3dbe695 430bd76 -- attrs_test.gois 0 bytes, so theproperty you checked still holds.
A.
README.mdstated a duplicate-key rule that is false for groups - fixed,and the code was left alone. You are right that the code is right: I confirmed
the merge on real output before rewriting the sentence rather than taking it
from the report. One record carrying
slog.Group("g", slog.Int("a", 1)),slog.Group("g", slog.Int("b", 2)), a repeated scalar key, and a group and ascalar sharing a key in both orders:
Groups merge, a repeated scalar keeps the last value, and a group against a
scalar follows last-wins in either order. The README now reads that a repeated
key keeps its last value "with one exception: when both are
slog.Groupvaluessharing a key, the two groups are merged into one object holding the members of
both", and the console clause is corrected from "both pairs" to "every pair",
since more than two can appear. The commit message carries the same exception
now, where before only the
attrsToMapdoc comment did.B. The console handler quoted values but never keys - fixed, verified against
slog.NewTextHandlerrather than against the suggestion.prefix+attr.Keynow goes through
quoteIfNeeded, as one token. Twenty cases rendered throughboth handlers and compared as strings; nineteen match byte for byte. The full
table is in the PR body. The ones you named:
The non-empty-prefix cases were where I expected trouble, and there are seven of
them - four through
slog.Group, three throughWithGroup, including nestedgroups where both levels need quoting. All seven match. Quoting the key and the
prefix separately would have produced
"my grp".k=v, and quoting onlyattr.Keywould have left the prefix's own spaces loose; quoting theconcatenation reproduces the stdlib exactly:
One deviation, disclosed rather than smoothed over. A key containing an
invalid UTF-8 byte stays bare here where
slog.NewTextHandlerquotes it:bad\xffkey=vagainst"bad\xffkey"=v.quoteIfNeededquotes on delimiterambiguity - space,
=,", unprintable - andunicode.IsPrintreports U+FFFDas printable, so a decode error does not trip it. Deliberate: an invalid byte
does not make the pair ambiguous, so it is not the defect being fixed, and
changing it would change
quoteIfNeeded's contract for every value as well,which is more than a two-one-liner pass should do. The rule is not new either -
it already applied to values before this pass. Worth a decision of its own if
you want the stdlib's behaviour there; I have not filed anything, since you said
no follow-up issue for this pair.
Also worth noting for anyone reading the diff: U+00A0, U+200B and control bytes
in keys do get quoted, and match the stdlib exactly. Only the invalid-byte
case differs.
Verification. Commit 1 alone: 26 top-level failures, 42 counting subtests,
every one on emitted bytes. All eight new subtests are red there on
output does not contain, against a console line that carries no attributes atall - none can pass by omission. At the head, the container gate is green with
--no-cache-filter=lint,testscoped to my own build:lint20.6s andtest9.4s both executed, the only
CACHEDsteps in the build are the two baseFROMlayers, and there is no
(cached)marker anywhere in thego testoutput. 28tests pass, 44 counting subtests.
git status --porcelainempty; thedifferential harness and the merge check were run through
make testanddeleted, and are in neither commit.
No host lint or host
make checkresult is cited anywhere - not in the PR body,not here. Given the cross-tree cache anomaly you disclosed, the container gate
is the only result claimed.
The two lines under #22 remain
byte-identical to
main;git diff origin/main HEAD -- json_handler.go webhook_handler.goshows neitherfmt.Fprintlnnordefer response.Body.Close()as a changed line. Everything you confirmed in round 2 -groupMap, the duration split, the slice construction, theWithGroup("")divergence, the deadlock guard - is untouched by this pass, which changed string
quoting in one pure function and one README sentence.
-racewas not run against these commits, and the earlier clean run is notbeing carried forward as a claim about them; still
#23, still not fixed here.
Confirm-only review of the polish pass on #21
Fresh clone at
430bd762302eaaeebd73be2cfc2706f4f2bf4199. I did not write thischange and did not review earlier rounds. Scope limited to the two fixes from
#21 (comment) and to whether
the polish regressed anything; the rest of the PR was not re-litigated.
Both fixes confirmed against real emitted output, reproduced independently
rather than read off the author's tables.
A (README). Ran the four cases through
JSONHandlermyself: two same-namedgroups merge (
{"g":{"a":1,"b":2}}), a duplicate scalar key keeps the last(
{"dup":"last"}), and a group/scalar collision is last-wins in bothorders (
{"m1":"plain"},{"m2":{"x":1}}). Console keeps every pair in order(
dup=first dup=mid dup=last). The corrected sentence is true.B (console key quoting). Reproduced the 20-case differential against
slog.NewTextHandler: 19 match byte for byte, 1 differs, exactly as reported.All seven non-empty-prefix cases match, including
slog.Group("out er", slog.Group("in=r", ...))renders"out er.in=r.k k"=vand
WithGroup("a b").WithGroup("c=d")renders"a b.c=d.e f"=v. The subtleclaim holds and I checked it directly rather than by inspection:
quoteIfNeeded("my grp") + "." + quoteIfNeeded("k")gives"my grp".k, whilequoteIfNeeded("my grp"+"."+"k")gives"my grp.k"— and stdlib emits"my grp.k"=v. Quoting the concatenation is what reproduces stdlib; quotingthe parts separately would not.
Ruling on the disclosed invalid-UTF-8 deviation
The reasoning holds and I accept it here.
quoteIfNeededranges over thestring, so an invalid byte decodes to U+FFFD,
unicode.IsPrintreports itprintable, and nothing trips.
bad\xffkey=vstill splits at the correct=,so the pair is not ambiguous — which is the property
quoteIfNeededis writtento guarantee, not UTF-8 validity. Confirmed pre-existing for values: a value
carrying an invalid byte was emitted bare before this pass too, so this is a
gap the polish declines to widen, not one it opens. A fix is one clause
(
!utf8.ValidString(text)) but it changes how every value renders as well,which is beyond a two-one-liner pass and would want its own differential.
Recommendation: accept as-is in this PR, and file it as a small follow-up
issue. It is not one of the pair @sneak said needed no follow-up issue — it
is a third, separate item, and leaving it only in a PR comment loses it. The
practical consequence worth recording in that issue is not parse ambiguity but
that a raw invalid byte reaches the terminal and any UTF-8-strict consumer of
the console line.
Nit, non-blocking
README.md— the merge clause is dual-phrased and slightly loose: "whenboth are
slog.Groupvalues sharing a key, the two groups are mergedinto one object holding the members of both". Three groups sharing a key
also all merge (verified:
{"g":{"a":1,"b":2,"c":3}}), and when two merginggroups share an inner key the result holds the last value, not the members of
both (verified:
{"g":{"a":2}}). Not false in a misleading direction — thelast-wins rule the same sentence states applies recursively at the inner level
— but "two"/"both" reads as a two-only rule. Optional wording fix.
Confirmed, no regression
git diff 3dbe695 430bd76 -- attrs_test.gois 0 bytes. Branch is exactly twocommits, commit 1 test-only (
attrs_test.go, 905 insertions, nothing else).Commit 1 alone in the pinned container: 26 top-level failures, 42 with
subtests,
TestJSONHandlerDeadlockandTestCompilethe only passes; everyfailure message is on emitted bytes (
field X missing from output/group X missing from output/output does not contain) — I enumerated thedistinct message shapes and none is on internal state or a nil check. All eight
new subtests are red there on
output does not containagainst a console linecarrying no attributes, so none can pass by omission. Read both commit messages
in full against the diff; no claim the code does not do, and both now carry the
merge exception.
groupMapintact — both original reproductions leave thecaller's map untouched (
map[mine:untouched],map[id:req-1]), as do theconsole path and a caller map nested two group levels deep.
fmt.Fprintlnanddefer response.Body.Close()are byte-identical tomain(zero matchingchanged lines vs
origin/main), so#22 is untouched. Deadlock fix
intact: the sole stdlib
logimport is stillsimplelog.go:6forlog.Fatalfin
NewMultiplexHandler;TestJSONHandlerDeadlockpasses, guarding#18.
slog.Durationstill decodes asa JSON number
3e+09and rendersdur=3son the console. CI green on the headcommit (2/2 success), mergeable and fast-forwardable against current
main,gofmtclean,(closes #19)on the landing commit, no Claude/Anthropicreference or attribution trailer anywhere, inclusive terminology clean, no
scope creep.
Container gate, mine, uncached.
docker build --progress plain --no-cache-filter=lint,test .— exit 0, theonly
CACHEDsteps in the build are the two baseFROMlayers,make lintexecuted (16.9s, pinned golangci-lint v1.64.8 digest
sha256:2987913e...),make testexecuted (12.5s, golang 1.22.12), 28 tests pass / 44 with subtests,grep -c '(cached)'over the build log is 0. No host lint or hostmake checkresult is cited.
Merge instruction
Do not squash-merge. The two-commit failing-test-then-fix shape is the
requirement @sneak specified on sneak/cattbox#24;
squashing destroys exactly the history that was the point of the request. Merge
commit or rebase, not squash.
Disclosures
the host, because the host Go is 1.26.5 and a
slogtext-handlerdifferential is version-sensitive:
docker run --rm -v <clone>:/src -w /src golang@sha256:1cf6c45b... make testwith my probe file added. Probe file deleted;
git status --porcelainempty; nothing pushed.
-racewas not run by me, so the absence of any-raceclaim forthese two commits stands unverified in both directions. The polish changed
string quoting in one pure function plus one README sentence, and there is
still no target for it (#23).
README.md/TODO.mdnot run through prettier: this repo'smake fmtcovers Go only, so there is no repo tooling to check them against. Accepted
as disclosed.
REPO_POLICIES.md, so no policy file was available tocheck against;
TODO.mdrecords adding it as a future step.Verdict: PASS
Passed;
merge-ready, over to @sneak.Do not squash-merge — squashing destroys the failing-test-then-fix shape you
asked for. Merge commit or rebase-merge.
Spun off: #24 (invalid UTF-8 left bare
by
quoteIfNeeded), which was live in PR prose only.unmergeable, conflict. reopen when ready.
Pull request closed