report: record the consumer-asserted reporter source as a closed vocabulary - #27
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
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="crates/credentials-module/src/main.rs">
<violation number="1" location="crates/credentials-module/src/main.rs:5209">
P2: When a stale report carrying `reporter_source` is recorded, this test still passes because it checks only state and the newest event. Assert that the audit-event set remains unchanged after the stale call, including that no `reporter_source` is persisted, to pin the required silent no-op behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .expect("bump the record version"); | ||
| surface | ||
| .report_auth_failure(7, ¶ms(401, 1)) | ||
| .report_auth_failure(7, ¶ms(401, 1, Some("relay_message_parse"))) |
There was a problem hiding this comment.
P2: When a stale report carrying reporter_source is recorded, this test still passes because it checks only state and the newest event. Assert that the audit-event set remains unchanged after the stale call, including that no reporter_source is persisted, to pin the required silent no-op behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/main.rs, line 5209:
<comment>When a stale report carrying `reporter_source` is recorded, this test still passes because it checks only state and the newest event. Assert that the audit-event set remains unchanged after the stale call, including that no `reporter_source` is persisted, to pin the required silent no-op behavior.</comment>
<file context>
@@ -5200,7 +5206,7 @@ mod tests {
.expect("bump the record version");
surface
- .report_auth_failure(7, ¶ms(401, 1))
+ .report_auth_failure(7, ¶ms(401, 1, Some("relay_message_parse")))
.await
.expect("a stale report is accepted, not errored");
</file context>
38e4dc2 to
11d063a
Compare
|
All four findings addressed in Finding 1 (stale-report test gap): the suggested fix — "assert the audit-event set remains unchanged after the stale call" — is backwards. Finding 3 (type-enforced unwritability): accepted and it's the right escalation. Findings 2 and 4: field doc comment added, plus the two wire-contract sites in Full gate re-run at |
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
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="crates/credentials-core/src/store.rs">
<violation number="1" location="crates/credentials-core/src/store.rs:3023">
P2: `ck auth events` drops `AuthEvent::reporter_source` from its output, so operators cannot see the source this field records through the documented event workflow. Update the CLI event rendering to include the optional reporter source.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| pub provider_status: Option<u16>, | ||
| pub detail: Option<String>, | ||
| /// Consumer-asserted, unverified; from `ReporterSource::as_str`, never raw consumer input. | ||
| pub reporter_source: Option<String>, |
There was a problem hiding this comment.
P2: ck auth events drops AuthEvent::reporter_source from its output, so operators cannot see the source this field records through the documented event workflow. Update the CLI event rendering to include the optional reporter source.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/store.rs, line 3023:
<comment>`ck auth events` drops `AuthEvent::reporter_source` from its output, so operators cannot see the source this field records through the documented event workflow. Update the CLI event rendering to include the optional reporter source.</comment>
<file context>
@@ -3003,6 +3019,8 @@ pub struct AuthEvent {
pub provider_status: Option<u16>,
pub detail: Option<String>,
+ /// Consumer-asserted, unverified; from `ReporterSource::as_str`, never raw consumer input.
+ pub reporter_source: Option<String>,
pub record_version: Option<u64>,
/// Whether this observation actually changed the credential. False for a report
</file context>
…bulary A report_auth_failure now carries an optional reporter_source naming which observation path saw the failure. The need comes from anthropic-auth#175: the same 401 can arrive from a direct provider response or from a WebSocket relay event, and the relay path itself has a strong arm (a structured status field) and a weak arm (a regex over message text). The weak arm is where a parse false-positive will eventually come from, and the version fence cannot stop a wrong-but-current report -- so when one happens, the forensic question is which path produced it, and auth_events could not answer that. The column is consumer-asserted and the vault does not vouch for it, which is why it is a NEW column rather than a widening of detail: every existing detail value is a variant name from an enum this repo owns, and a reader of that column may rely on it recording what the vault observed. The two kinds of statement stay structurally apart (issue cortexkit#26). The value set is a vault-owned closed enum, not a validated string. Charset validation was proposed and rejected for a concrete reason: a 40-character lowercase hex token satisfies ^[a-z0-9_]{1,64}$, so a character-class filter passes exactly the secrets that do not look like secrets. With a closed set, consumer input is unwritable by construction -- and the enum is now carried as a typed Option<ReporterSource> through AuthObservation itself, so raw consumer strings cannot reach ANY auth_events writer, not just the read surface. The store renders the stable label only at the final SQL boundary; an unrecognised wire value becomes "unrecognised" and the submitted string is dropped without being persisted or logged. The load-bearing test reports a token-shaped string and asserts it appears in NO string column of auth_events. The rider never reaches the state machine: the fence, the stale-vs-latch arm choice, and rate limiting are unchanged. A superseded-version report is a STATE no-op that still records its applied=false diagnostic row -- deliberately, since a stale report leaving no trace is the exact case auth_events was created to fix -- and the row retains the mapped reporter_source, which the test pins. Wire compatibility is serde-default: a params object without the field deserializes to None and writes NULL, documented in the runbook as distinct from "unrecognised" (omitted vs sent-and-refused). The ck-auth events listing renders the source as a trailing src= field only when present, so legacy NULL rows print byte-identically to the pre-column format. Seeded values: direct, relay_status_field, relay_message_parse. The three-way split (rather than one merged relay label) is deliberate: the column exists to forensicate the weak parse arm, so the weak arm must be distinguishable by construction. Documented in the runbook and the wire contract with the same exhaustive-match pin the other vocabularies carry; adding a variant without documenting it fails the build.
11d063a to
618caf7
Compare
|
Round 2 addressed in The CLI one was the real catch: the runbook's documented workflow for this table is Module-level op doc now carries Full gate at |
MEASURED, NOT ASSUMED: the only principal holding grants is prefrontal-core, and it holds three -- github_app: read, signing:agent-assertion: read and sign -- with no capability handle. So the documented answer to 'how does a consumer observe a credential coming back' (poll credential.status, which reads plaintext metadata and never mints) was UNREACHABLE for the one consumer that asked the question, on issue #6, two weeks ago. Its only route to a named credential was get_scoped, which mints on a stale record: polling for repair meant repeatedly buying upstream token exchanges, which is the cost this surface exists to avoid. THE REFUSAL IS DELIBERATELY NOT A REFUSAL. An unauthorized or unknown scoped call returns the IDENTICAL body an unresolvable handle returns -- ready false, version and mark omitted -- from a single constructor, so the two cannot drift apart into an oracle for which credential ids exist. A surface that answered 'no grant' differently from 'no such credential' would enumerate the vault for any principal that could reach it. The discrimination goes to auth_events, named with the principal, exactly as get_scoped does: silent on the wire, loud in the log -- the split I got wrong in the route-epoch path, where the drop was silent in both. VERIFIED HERE RATHER THAN TAKEN FROM THE REPORT: no-grant arm returns a DISTINGUISHABLE body -> 4 tests RED, incl. scoped_status_unknown_and_no_grant_are_ indistinguishable_on_the_wire, by name wire-key pin UNTOUCHED and green -- the reply shape does not move, only the request shape, so no consumer announcement is owed force_..._for_test #[cfg(test)], absent from release Sign-only test uses a prefix where the principal holds Sign and NOT Read, so it cannot pass under either rule The conflict on merge was the module header: this branch predates #27, which added reporter_source? to the same doc line. Both belong; kept both.
2 participants
Implements #26 as agreed: your counter-proposal shape, seeded with the three-way set the consumer confirmed.
The shape:
ReporterSourceclosed enum inaudit.rsbeside the other vocabularies:direct,relay_status_field,relay_message_parse, reservedunrecognised. Out-of-contract input maps toUnrecognised; the submitted string is dropped without being persisted or logged anywhere.auth_events.reporter_sourcecolumn (migration 8).detailkeeps its vault-observed meaning; the new column is consumer-asserted-unverified, doc-commented as such at the struct and in the runbook.report_auth_failure; absent → NULL, old consumers untouched.reporter_sourceis still a silent no-op (asserted).Verification (all on this tree,
gate.shexit 0):params.reporter_source.as_deref())NULLfor the columnleft: None, right: Some("unrecognised")relay_message_parseremovedThe load-bearing test reports a 40-char lowercase token-shaped string — the exact case that passes
^[a-z0-9_]{1,64}$— and asserts it appears in no string column of any row, not merely thatreporter_sourcecame back mapped. First draft of that assertion checked one column while its message claimed all of them; strengthened before this went up.Consumer side: anthropic-auth confirmed they'll thread
{status, source}out of the relay parse (the arm is known at match time) and assertdirecton the direct path, so the seeded set reflects what will actually be reported.Base is
6094a4e(one behind —0aa6f79touches onlyscripts/check-doc-status.py, no overlap).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Records the consumer-asserted source of an auth failure so a relay parse false positive is distinguishable from the direct path and the structured relay field.
report_auth_failurenow accepts an optionalreporter_sourcewith a closed vocabulary (direct,relay_status_field,relay_message_parse,unrecognised), stored in a new nullableauth_events.reporter_sourcecolumn via migration 8.detailkeeps recording only vault-observed values.unrecognised; the submitted string is dropped, never persisted or logged.Noneand writesNULL.auth_events, not merely inreporter_source.Written for commit 618caf7. Summary will update on new commits.