fix: retrospective review fixes for merged PR #57#109
Merged
Conversation
Retrospective review of merged PR #57 found a still-live leak in its own self-review fix (commit 6ae5b94, "structured fallback so the data-loss guard does not leak sibling secrets"). sanitizeObservation falls back to structuredScrub/walkScrub only when the byte-level scrub turned a secret-keyed UNQUOTED scalar into invalid JSON. That fallback's secretKeyRE was anchored (`^(...)$`), matching only a bare key like "token" — but real tool/API responses almost always use a compound key: "access_token", "refresh_token", "client_secret", "auth_token". For `{"access_token":12345678}`, the byte-level scrub still breaks JSON validity (substring "token" inside "access_token" matches secretPattern) and triggers the fallback, but the fallback's exact-key gate then fails to match "access_token", falls through to walkScrub's default case, and returns the numeric secret completely unredacted while still reporting valid JSON — the exact leak this fallback exists to prevent. Fix: drop the anchors so secretKeyRE substring-matches a key the same way secretPattern already does at the byte level, keeping the two redaction paths consistent. Adds TestSanitizeObservation_CompoundSecretKeyNotLeaked covering access_token / refresh_token / client_secret / a nested-array case.
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
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.
Retrospective
/code-reviewsweep of merged PR #57 ("fix: retrospective-review follow-ups for v0.9.18 (#48-#54)") that shipped without pre-merge review.The one confirmed still-live bug is in PR #57's own self-review fix (commit
6ae5b94, "structured fallback so the data-loss guard does not leak sibling secrets").sanitizeObservationfalls back tostructuredScrub/walkScrubonly when the byte-level regex scrub turned a secret-keyed unquoted scalar into invalid JSON (e.g.{"token":12345678}→{"token":[REDACTED]}). That fallback'ssecretKeyREwas anchored (^(...)$), so it only matched a bare key liketoken— but real tool/API JSON almost always uses a compound key:access_token,refresh_token,client_secret,auth_token. For{"access_token":12345678}, the byte-level scrub still breaks JSON validity (the substringtokeninsideaccess_tokenmatchessecretPattern, same as the byte-level scrub always has), correctly triggering the fallback — but the fallback's exact-key gate then fails to matchaccess_token, falls throughwalkScrub's default case, and returns the numeric secret completely unredacted while still reporting valid JSON. That defeats the very guarantee the fallback was written to provide, and is a straight secret-leak into the persistedobservations.jsonlcorpus.Fix: drop the anchors so
secretKeyREsubstring-matches a key the same waysecretPatternalready does at the byte level, restoring consistency between the two redaction paths. AddedTestSanitizeObservation_CompoundSecretKeyNotLeakedcoveringaccess_token/refresh_token/client_secret/ a nested-under-array case; all fail before the fix and pass after.Also reviewed (no further still-live issues found): monorepo-root project-id resolution (
inject,observer run-once,dispatchInjectContext), whole-word user-prompt confidence-demotion scanning, escaped-JSON secret redaction,observer stoppid re-verify before SIGKILL,doctor's homunculus obs path,instinct promotepreview-by-default,chooseBasebranch_strategy trimming, andclaudemd --writestale-proposal clearing — all correct as merged. The two items PR #57 explicitly deferred (observer grandchild process-group kill; a precise correction-of-assistant demotion signal) were already resolved by PR #58.Test plan
go build ./...go test ./...(all packages green)golangci-lint run ./...(0 issues)gofumpt -lon changed files (no diff)