Refuse preference reads for email refs naming no known platform user - #9
Merged
Merged
Conversation
An email-form ?user-ref= canonicalizes any well-formed address into a subject -- form, not proof -- so a subject the platform had no record of answered a 200 defaults snapshot, byte-for-byte identical to "this user saved nothing". An agent joining on a commit email the platform never saw then acted on nobody's policy while believing it read the author's: the class default (on_problems) mentioned an author whose saved preference under their real platform identity said never. The earlier unresolved-ref 422 could not catch this shape because the email resolver never reports unresolved. Resolution now carries SubjectProven -- true only for relation-backed resolutions (a resource's sole_user, trigger-author's recursion into one); the email resolver leaves it false, and the zero value is unproven so a future resolver that forgets to claim proof gets the stricter treatment. The ?user-ref= handler demotes an unproven subject with zero user_preference records (any class, a cleared tombstone included) onto the existing 422 path, audited as a new unknown-subject outcome that records the derived subject. The agent-side contract already handles it: the reviewbot class prompt treats a get_preferences error exactly like `never` -- plain login, no mention. The bronzethread bundle reviewbot-notifications-unknown-email pins the TOOL RESULT as an error, per the suite's rule that a scripted reply says the same words whether the read errored or silently defaulted; reviewbot-notifications-default-on-problems remains the non-regression that a relation-proven subject with no saved values still reads class defaults.
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.
Motivation
A pull request author set the reviewbot
notificationspreference toneverthrough a Slack DM session — the save landed, confirmed,source: user— and a later review of their pull request @-mentioned them anyway.Debugging the sessions on the cluster showed the chain: the review session followed the class prompt's join-key protocol and called
get_preferenceswithuser: "email:<the author's commit email>". That commit email is not the email behind the author's platform identity, and the email resolver canonicalizes any well-formed address into a subject — form, not proof — so the read answered a 200 defaults snapshot (notifications: on_problems, source: default) for a subject that exists in name only. The review found problems,on_problemssays mention, and the mention lookup (which failed on the same email but succeeded on the author's display name) resolved the real person the preference read had missed. The session's ownpreference_accessaudit entry records the wrong-subject read asoutcome: ok.The earlier unresolved-ref fix (422, never defaults, for a ref that resolves to no platform user) could not catch this shape: the email resolver never reports unresolved, so the guard was structurally unreachable for the one reference form the reviewbot prompt uses.
pkg/memory/httpsrv's own test suite pinned the hole as intended behavior (TestPreferencesGetUserRef_EmailStillWorksWithNilRelationsasserted 200 for an email with no records at all).Summary of changes
subjectresolve.ResolutiongainsSubjectProven: true only for relation-backed resolutions (a resource'ssole_user,trigger-author's recursion into one); the email resolver leaves it false. The zero value is unproven on purpose — a future resolver that forgets to claim proof gets the stricter treatment.handlePreferencesGetForUserRefdemotes an unproven subject with zerouser_preferencerecords (any class; a cleared tombstone counts as a record) onto the existing 422 path. The existence bar needs no new infrastructure: the user-scope query the handler already runs is the check.preference_accessoutcome,unknown-subject, which — unlikeunresolved— records the derived canonical subject, so the trail still says whom the attempt was about.get_preferencesinstructions say an error means the user could not be identified — treat exactly likenever(plain login, never a guessed mention).reviewbot-notifications-unknown-emailpins the regression end-to-end, asserting on the tool result (lastToolResultIsError: true) per the suite's rule that a scripted reply says the same words whether the read errored or silently defaulted. Before the fix it fails exactly the way the incident happened: the result is a defaults snapshot.reviewbot-notifications-default-on-problemsremains the non-regression that a relation-proven subject with no saved values still reads class defaults.internal/cmd/operator/preferences_wiring_test.goasserted 200 for an email ref with nothing seeded; its claim is aboutnewMemHandlerOptswiring, not resolution semantics, so it now seeds a saved value and keeps all of its wiring/signed-audit pins unchanged.Alternatives considered
sole_useredge is the platform's own linkage authority, and a relation-proven user who never saved anything must keep getting class defaults (pinned by thedefault-on-problemsbundle).Provenance
Ship gate
mage test:unitmage test:integrationmage test:e2eResults:
Regeneration
+kubebuilder:rbacmarker or CRD-shaping field changedconfig/**changedmage fmt:check— clean (no Markdown/TypeScript touched)Coverage
test/e2e/bronzethread/testdata/reviewbot-notifications-unknown-email/)mage test:integration+mage test:e2eare greenBefore requesting review