auth_events: record the reporting principal, and distinguish direct from unrecorded - #50
Merged
ualtinok merged 1 commit intoSep 18, 2026
Conversation
There was a problem hiding this comment.
1 issue found across 7 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/gate.sh">
<violation number="1" location="scripts/gate.sh:356">
P3: The new floor contradicts the gate’s documented measured total: the comments say 614, while `run_expect` now enforces 622. Update the nearby measurement commentary to the verified 622 count so future floor changes are based on one consistent value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # THE FLOOR IS RATCHETED AGAINST THE MERGE TARGET BY `assert_floor_not_lowered` BELOW, | ||
| # because a floor alone does not defend the property it exists for. See that function. | ||
| run_expect 621 "workspace unit + integration" \ | ||
| run_expect 622 "workspace unit + integration" \ |
There was a problem hiding this comment.
P3: The new floor contradicts the gate’s documented measured total: the comments say 614, while run_expect now enforces 622. Update the nearby measurement commentary to the verified 622 count so future floor changes are based on one consistent value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gate.sh, line 356:
<comment>The new floor contradicts the gate’s documented measured total: the comments say 614, while `run_expect` now enforces 622. Update the nearby measurement commentary to the verified 622 count so future floor changes are based on one consistent value.</comment>
<file context>
@@ -353,7 +353,7 @@ assert_floor_not_lowered() {
# THE FLOOR IS RATCHETED AGAINST THE MERGE TARGET BY `assert_floor_not_lowered` BELOW,
# because a floor alone does not defend the property it exists for. See that function.
-run_expect 621 "workspace unit + integration" \
+run_expect 622 "workspace unit + integration" \
cargo test --locked --workspace --features credentials-core/test-support
</file context>
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.
Closes the gap in #42.
auth_eventsalready hadprincipal_kindandprincipal_id; the read surface never filled them, so every consumer-report row in both our stores is NULL.The three states, which is the actual ask
A direct caller genuinely has no name — that is an answer, not a gap. Before this, it rendered identically to "we did not record", so an absent principal meant nothing and could not be read as evidence either way.
AuthEventPrincipalis typed, followingreporter_source's shape, so a raw consumer string remains unrepresentable at the call site rather than merely rejected.Mutations
Two from the implementer, one I ran myself because it targets the design point rather than the plumbing:
One correction to the commit as returned
The floor was left at 621 while the tree measured 622. Both the gate (622 >= 621) and the ratchet (621 >= master's 621) passed, so nothing went red — and the new test was unprotected: delete it later and the gate still passes. That is precisely the defect the floor exists to catch, sitting inside the change that added the test. Floor set to the measured 622, red-checked (623 fails, 622 passes).
Worth noting because a floor that is merely not lowered is not the same as a floor that is correct, and only the first of those is mechanically enforced.
A red gate I chased and a claim I withdrew
Two full-gate runs failed on
manifest_lock_aba_regressionmembers — different members each time (:1449, then:1334) — in a file my diff touches zero lines of. The fixtures useDuration::from_millis(100)TTLs, so real elapsed time crosses them when the machine is busy.I then measured master passing a full gate while mine failed twice, and started writing that up as a branch-caused regression with a plausible mechanism (my new async test adds parallel work, lengthening wall time inside those 100ms windows). Run 3 on my branch passed, which kills it. The load differed across the comparison because I was generating the load myself with back-to-back gate runs — master ran at 2.4, mine at 3.9. That is not a branch measurement, and I withdraw it.
Third sighting of this class. #33 fixed one member by converting the race to arithmetic (making the work outlast the TTL by construction); these are the survivors. Filing separately rather than widening this PR.
Verification
bash scripts/gate.shexit 0,workspace floor 622 >= origin/master 621 at e97bc73,Cargo.lockunchanged, one commit over master.No migration — the columns already existed.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fills the
principal_kindandprincipal_idcolumns inauth_events, which the read surface never populated, so every consumer-report row was NULL. Route-bound reports now recordreservedwith the module id, direct callers recorddirectwith no id, and pre-change rows stay NULL — so a direct caller reads as an answer, not a gap.Notes
report_auth_failurenow accepts the principal and records it through the new typedAuthEventPrincipal, so raw consumer strings can't be written at the call site.Written for commit 9c82d31. Summary will update on new commits.