Skip to content

feat(attribution)!: switch the resolver to the fact index and relabel the metric surface - #287

Merged
sunib merged 43 commits into
mainfrom
feat/attribution-fact-switchover
Jul 29, 2026
Merged

feat(attribution)!: switch the resolver to the fact index and relabel the metric surface#287
sunib merged 43 commits into
mainfrom
feat/attribution-fact-switchover

Conversation

@sunib

@sunib sunib commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Finishes the attribution fact stream. The read side switches to the in-memory index, the v1
keyspace and the deletecollection expander are deleted, and the transport becomes selectable.

Follows #286, which was merged while this was being written; the base is main.

Nothing in #284 or #286 changed behaviour, because nothing read the streams. This is where the
behaviour change lands, and it lands all at once: no install is left half on each path.

The resolver waits instead of polling

ResolveAuthor keeps its grace window, its blocking behaviour, and its outcome classification.
Only the middle changes: register a waiter, check the index, then block on the waiter, the context,
or the grace deadline. Register-then-check is the order that matters — checking first loses a fact
applied in the gap, which is the race the 150ms poll loop papered over by looking again. There is no
Redis call left on this path: the fast case is a map read and the waiting case is a channel receive.

Watches drive the subscription set: a running target watch holds one reference on the
(audit route, group/resource) stream it covers, so the process follows a type while at least one
watch needs it. The release is idempotent, so a watch torn down on both an error path and its
deferred cleanup cannot unfollow a type another watch still needs.

What is deleted

The fact key builders, the SET/GET paths, RecordFact, the SCAN-based size gauge, and the whole
collection expander with its test file. attribution_index.go goes with them: what survived is the
fact shape and the result taxonomy (author_fact.go) and the shared key helpers (key_prefix.go).

Wiring

cmd/main.go builds the index and the transport, starts the follower, and adds
--author-attribution-transport (redis default, memory for a single-pod install). The
--redis-addr validation narrows accordingly: empty is an error only when the Redis transport is
selected. In-memory with an empty address is refused with more than one replica, because the
transport carries facts only within one process — the chart templates --replica-count in for that
check. The four tuning numbers from the design's answered-questions table become flags.

The chart's render guard was refusing what the binary supports: it failed attribution.enabled=true
with an empty queue.redis.addr whatever the transport was, which made transport: memory — a value
values.yaml itself documents as "run attribution with no Valkey at all" — unreachable through the
chart. It now fails only for the transport that needs Redis.

Two bugs found in review, both about naming the wrong person

These are the substantive changes beyond the original scope. Both mis-attribute a deletion to an
innocent person, and neither was visible to the tests as they stood.

1. A collection delete was credited to the last editor. The tiers ranked latest above the
collection's uid set, and those answer different questions: latest says who last wrote an
object, a removal asks who deleted it. For a single-object delete they coincide, because the
delete files its own fact under that uid. A collection delete files one fact about the collection,
so latest was left holding whoever edited the object last — and the uid set the receiver collected
was never read. That is the one thing the expander got right, by overwriting the entry per object.

2. A removal returned before its delete fact arrived. Re-ranking only decides between facts that
are both present. The watch event reliably beats the audit batch carrying the delete — the entire
reason the grace window exists — so at resolve time the only fact present is often the last write.
A per-object match on a removal is now held as a fallback: the wait continues for evidence about
the deletion, and the fallback is returned when the grace expires with nothing better. Waiting never
costs an attribution — the worst case returns what returning early would have, one grace later.

Neither test layer could catch these. The unit fixture built collection facts with no uids, so
it only exercised the scope tier; the e2e specs create and delete as the same actor, so the
wrong answer and the right answer are the same name. Both now have a case where the editor and the
deleter are different people. #2 was caught by CI failing at 4 procs what passed at 1.

Also fixed in review

  • Idle streams never expired. XTRIM MINID rides on the publish path, so a stream whose type
    stops being written is never trimmed again — one immortal key per (route, type) pair that ever
    saw a write. EXPIRE now rides with every XADD; the in-memory transport had the same leak in
    the pod's heap and now drops idle rings.
  • Facts aged from when they were read, not appended — so the follower's replay-on-start handed
    every entry a second full TTL. Apply now reads the append time off the entry's position.
  • Phantom trim gaps. "Behind" is inferred from a read filling its entry budget and was never
    cleared, so ordinary retention surfaced as data loss. A follower that asks and gets nothing is
    caught up.
  • ~25% smaller entries. groupResource (the stream's own name), name and subresource are
    read by no tier; isServiceAccount is a prefix check on the author. auditID stays as the only
    link from a mis-attributed commit back to the audit event that named it.
  • The e2e author-mode probe read "no Redis" as configured-author, silently skipping every
    attribution spec — the failure it exists to prevent, turned on itself.

Breaking: the attribution metric surface is relabelled

Two label breaks land together, deliberately, because the first was unavoidable and taking the
second later would cost a second migration on a label that had been wrong twice. Nothing consumes
these metrics yet — no dashboard ships and no alert rules ship — so the whole cost is one
UPGRADING.md entry, which this PR writes.

result is gone. It crammed two orthogonal questions into one value (which evidence answered,
and who it named) and hid a third:

Metric Was Is
attribution_resolutions_total {result} {tier, actor_kind}
attribution_resolution_wait_seconds {result} {tier, event_kind}

tier is exact, latest, resource_version, name, collection_uid, collection_scope,
absent — the join's own ladder, and the enum in author_fact.go is the source of truth for it, so
the resolver records the branch it actually took. weak split into latest and resource_version
because it covered two different kinds of evidence and the removal path turns on the first
specifically. actor_kind is user/serviceaccount/none, the vocabulary
commits_total{author_kind} already used, and it is now askable of every tier rather than of
exact alone. event_kind is write/removal: a removal holds a fallback and keeps waiting where
a write does not, so the removal wait is the number --author-attribution-grace is tuned from, and
it used to share a series with writes.

exact_deletecollection_item was already replaced by collection_uid and collection_scope in this
branch; collection_scope is new capability rather than a rename, since a deletecollection the API
server sent no body for used to lose its author entirely.

Match coverage is tier!="absent". Anything narrower — result=~"exact_.*|weak" before, or
tier=~"exact.*" after — reads the collection and name tiers as misses, and those tiers named an
actor.

Four families are renamed while the surface is moving: attribution_facts_total,
attribution_fact_index_entries, attribution_collection_without_uidset_total.

The loss paths that were silent now have counters

Three ways a fact could be lost with no symptom, and one piece of metadata that decides how the rest
read:

  • attribution_fact_stream_decode_errors_total{transport} — both transports skipped an
    undecodable entry and advanced past it with no log and no metric. It is the one loss path with no
    other symptom: a trim gap is detectable after the fact and a publish failure is retried by the API
    server, but a skipped entry simply never existed.
  • attribution_fact_follower_errors_total{transport} and
    attribution_fact_follower_last_success_timestamp_seconds — FactIndex.Run retried a wedged
    follower forever and counted nothing, so attribution degraded to committer-authored cluster-wide
    with a rising unresolved rate as the only symptom. The timestamp matters more than the counter,
    and the alert needs both arms: it is not published until the first successful read, so
    time() - <gauge> returns no series for a follower wedged since startup.
  • no_attribution_fact on audit_events_total, in the dropped category — an accepted event
    that produces no fact was counted queued, claiming an append that was never owed. This is the
    only place it can be counted: the aggregated-API create is rejected before publication, so no
    fact-side counter ever sees it. A real e2e run shows 10 of them.
  • attribution_transport_info{transport} — a legend rather than a threshold. A burst of
    unresolved commits after a restart is expected under memory and a bug under redis.

Each ships with its recording site, a manual-reader unit test, and its
interpreting-metrics.md row, per the rule that produced the June
cleanup. This is Phase 1 of
the metrics plan; watch ingestion, the relevance filter
and the dashboard JSON are Phases 2-4 and deliberately not here.

A fact that names nobody is refused, not stored

author is the one required field on the wire, and AuthorFact.UnmarshalJSON now enforces it:
missing, null and "" are one violation and all three refuse the entry, which lands on the
decode-error counter above. The publish gate already refused such an event, but it only speaks for
facts this operator wrote; the read side now holds the same line for facts anybody wrote. That is
what makes {tier!="absent", actor_kind="none"} impossible rather than merely unlikely, which is
what lets coverage be read off the tier alone.

What was measured

  • A proxied aggregated deletecollection carries no response body. Confirmed three independent
    ways: a live e2e run against wardle (collection_scope=2, collection_uid=0), four captured audit
    recordings that were sitting unused in testdata, and the no-Redis run. A body-supplying proxy
    does return the list, which the captures also pin.
  • The in-memory transport works end to end with --redis-addr empty: collection_scope=2,
    facts matched=2, stream gaps=0, no Redis in the picture.
  • The wait-histogram premise did not hold. On main, 80.7% of resolutions already landed in
    under 5ms (mean 113ms) — the suite's report hides this because its smallest bucket is 0.5s. The
    payoff here is the collection tier and the transport choice, not latency. A clean A/B for the
    branch is still outstanding: both branch runs shared a cluster with main's data.

What does not change

attribution_resolutions_total and attribution_resolution_wait_seconds keep their names and
their meanings; only their labels change, as above. Commit order is untouched: one goroutine per (GitTarget, GVR, scope), one FIFO into the branch
worker. Attribution stays optional: a nil Manager.AuthorResolver is configured-author mode.

Deviation from the plan

ResolveAuthor could not keep its signature: the collection tier joins on the object's namespace
and labels, and the six parameters carried neither, so collection_scope would have been
unreachable from production. It takes an AuthorQuery struct. Everything the constraint protected
held. The design record is corrected rather than the code in three places where they disagreed —
the admission webhook's Redis requirement, the tier order, and "on a hit, return".

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added transport-driven author attribution with selectable redis/memory, new --replica-count and --author-attribution-transport flags, plus new attribution capacity/window controls.
    • Improved deletion attribution using collection facts (UID-membership and namespace/label scope), with name-tier attribution for missing UID/RV.
    • Added support for in-memory attribution on a single replica without Redis.

  • Bug Fixes

    • Improved audit-webhook handling for nested paths.
    • Refined attribution fact-stream retention, wake-up, and trim-gap behavior.

  • Documentation

    • Updated Helm/chart values, configuration guidance, metrics, and attribution design docs.

sunib and others added 3 commits July 28, 2026 14:40
@sunib @claude
feat(watch)!: resolve commit authors from the in-process fact index
218a289 
The resolver stops polling Redis and waits on the fact index instead. Only the
middle of ResolveAuthor changes: it registers a waiter, checks the index, then
blocks on the waiter, the context, or the grace deadline. Register-then-check is
the whole point of the order — checking first would lose a fact applied in the
gap, which is the race the 150ms poll loop papered over by looking again. There
is no Redis call left on this path: the fast case is a map read and the waiting
case is a channel receive.

Watches now drive the subscription set. A running target watch holds one
reference on the (audit route, group/resource) stream it covers and releases it
when the watch stops, so the process follows a type while at least one watch
needs it. The release is idempotent, so a watch torn down on both an error path
and its deferred cleanup cannot unfollow a type another watch still needs.

With the read side switched, the v1 keyspace goes: the fact key builders, the
SET/GET paths, RecordFact, the SCAN-based size gauge, and the whole
deletecollection expander with its test file. attribution_index.go goes with
them — what survived is the fact shape and the result taxonomy, now in
author_fact.go, and the shared key helpers, in key_prefix.go.

Two metrics survive the move rather than being retired with the keyspace.
attribution_fact_index_size is now a field read on the sweep instead of a SCAN
of the whole fact keyspace, and attribution_fact_events_total{op} keeps written
and matched; only the expander's deletecollection_expanded op is gone.

BREAKING CHANGE: the exact_deletecollection_item result label is replaced by
collection_uid and collection_scope. The match is now two-tiered and the tiers
carry different confidence: collection_uid is a removal whose uid was in the set
the API server said it deleted, and collection_scope is one matched by
namespace, selector, and window alone. Dashboards selecting
result=~"exact_.*|weak" for match coverage should select result!="absent"
instead, or they will read the collection tiers as misses.

ResolveAuthor's parameter list becomes an AuthorQuery carrying the object's
namespace and labels. The design record said the signature would not change and
that was wrong: the collection tier joins on exactly those two fields, so
without them a body-less deletecollection could never be resolved from
production. Everything the constraint protected held — the grace window, the
blocking behaviour, the outcome classification, and both metric names.

The audit handler's terminal outcome now lands AFTER the append rather than
before it, so an event counts as queued only once its fact is in the log and
audit_events_total{outcome="write_error"} stays reachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
feat(main): select the attribution fact transport
de9a4bd 
cmd/main.go builds the fact index and the transport, starts the follower under
the manager, and hands the watch manager both the resolver and the subscription
set. Attribution stays optional: with --author-attribution=false there is no
index, no follower, and no subscription.

--author-attribution-transport picks where facts travel. Redis is the default
and the production choice: it is the only transport whose facts survive a
restart and the only one that can reach a second process. Memory is for the
single-pod install where running a Valkey StatefulSet to name commit authors is
out of proportion to the benefit. Selecting it explicitly, rather than inferring
it from an empty --redis-addr, keeps existing installs unchanged: an empty
address means attribution is off today, and quietly giving it a second meaning
would change them by inference.

With that flag the --redis-addr validation narrows. Attribution needs a fact
transport, not Redis specifically, so an empty address is an error only when the
Redis transport is selected or the admission webhook is on. In-memory plus an
empty address becomes a supported configuration rather than a rejected one,
which it has to be, or the mode would be unreachable.

The in-memory transport is refused with more than one replica. It carries facts
only within one process, so under two replicas an audit request answered by one
pod leaves a watch running on the other with nothing to join, and every commit
through it would be authored attribution-unresolved with nothing saying why. The
process cannot see its own replica count, so the chart templates --replica-count
in from .Values.replicaCount; the chart's own validate-replica-count.yaml still
fails a chart install outright, and this is the gate for everything installed
another way.

The four tuning numbers from the design's answered-questions table become flags
at their current defaults: the per-type and total index caps, the collection
window, and the collection uid cap. A total cap below the per-type cap is
rejected, since it would make the per-type cap unreachable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs(attribution): record the fact stream as shipped
462778f 
The design record moves to docs/finished/ and flips from "partly built" to
built, with the four PRs it landed in and two more things the code settled that
it had only pointed at: ResolveAuthor taking a query struct, and the replica
count arriving as a flag because the process cannot observe it.

The expander spec keeps §2 — the deletion-as-intent render rule still binds, and
it is what makes the collection window short enough to be safe — and rewrites §5
and §8 to say what replaced them. §6 changes verdict: the hollow-body case was
resolved by scope matching rather than by Option C. The rejection of scope
matching was right at the time and wrong now, because three things bound the
over-attribution it was about, and the third is new: namespace and selector
narrow it, precedence keeps anything with its own fact out, and the window is
short because deletion-as-intent fixes the removal at delete-request time. The
aggregated and metadata-only collection deletes that used to ship
committer-authored now resolve.

interpreting-metrics.md documents the eviction, trim-gap and collection-degraded
counters, the two collection result labels, and — spelled out, because it is
silent otherwise — that a dashboard filtering on result=~"exact_.*|weak" will
now read the collection tiers as misses.

configuration.md and the chart README carry the transport table, the narrowed
--redis-addr rule, and the four tuning flags. The stale "hard dependency in
every mode" comment on RedisStore is corrected in the same pass.

Every markdown reference to the deleted attribution_index.go becomes plain text
rather than a broken link; the historical documents that mention it are left
saying what they said.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR replaces Redis-only author attribution with selectable Redis or memory transports, a bounded fact index, batched audit publishing, await-based watch resolution, collection UID/scope and name-tier matching, replica validation, and updated chart configuration, tests, metrics, and documentation.

Changes

Attribution pipeline

Layer / File(s) Summary
Transport configuration and startup wiring
cmd/main.go, cmd/main_redis_flags_test.go, charts/gitops-reverser/*, docs/configuration.md		 Adds transport selection, replica-count validation, fact-index limits, collection settings, and Redis-optional startup behavior.
Fact model, storage, and indexing
internal/queue/*, internal/telemetry/exporter.go		 Introduces exported fact models, UID-cap parsing, name-tier lookup, stream retention, separate cursor storage, removal-aware waiting, and fact lifecycle telemetry.
Audit fact batching and publication
internal/webhook/*		 Publishes accepted audit facts in per-stream batches and records queued or write-error outcomes after append results are known.
Await-based watch resolution
internal/watch/*, internal/controller/*		 Passes structured author queries to the fact index, waits for facts during the grace window, and manages fact-stream subscriptions per active watch.
Collection attribution and validation
docs/spec/*, test/e2e/*, test/mutationlab/*		 Documents and validates UID-first, scope-based, and name-tier attribution, including aggregated API and generated-name scenarios.
Normalization and supporting documentation
internal/mutationlab/normalize/*, docs/*, .docs-lint-scope		 Normalizes generated names and latency annotations and updates architecture, configuration, design, metrics, and migration documentation.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AuditWebhook
  participant FactTransport
  participant FactIndex
  participant WatchManager
  AuditWebhook->>FactTransport: PublishFacts by route and resource type
  FactTransport->>FactIndex: Follow fact streams
  WatchManager->>FactIndex: Await structured author query during grace window
  FactIndex-->>WatchManager: Return author resolution
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The PR description is detailed, but it does not follow the required template sections like Type of Change, Testing, Checklist, or Related Issues. Rewrite it using the repository template and fill in the missing sections: Type of Change, Testing, Checklist, Related Issues, Screenshots, and Additional Notes.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: switching attribution resolution to the fact index and updating metric labels.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/attribution-fact-switchover

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.90476% with 53 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/queue/fact_index_store.go 63.6% 15 Missing and 1 partial ⚠️
internal/queue/fact_index.go 85.1% 8 Missing and 7 partials ⚠️
cmd/main.go 91.0% 5 Missing and 4 partials ⚠️
internal/queue/author_fact.go 90.0% 4 Missing and 4 partials ⚠️
internal/mutationlab/normalize/normalize.go 86.9% 2 Missing and 1 partial ⚠️
cmd/mutation-capture-lab/main.go 0.0% 1 Missing ⚠️
internal/queue/fact_stream_redis.go 94.1% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

coderabbitai[bot]
coderabbitai Bot reviewed Jul 28, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/webhook/audit_fact_publish_test.go (1)

169-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test the nil-publisher path, not a configured publisher.

This test is named NoPublisherPublishesNothing, but Lines 170-171 configure a publisher and assert that it receives a fact. Construct the handler without FactPublisher and retain the successful-response assertion so configured-author mode is actually covered.

As per coding guidelines, **/*_test.go changes must cover new code with positive and negative tests.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/webhook/audit_fact_publish_test.go` around lines 169 - 178, The test
currently configures a publisher and verifies a publication, so it does not
cover the nil-publisher behavior named by
TestAuditHandler_NoPublisherPublishesNothing. Construct the handler without
setting AuditHandlerConfig.FactPublisher, retain the successful HTTP response
assertion, and remove the publisher-receipt assertion; add or preserve a
separate positive test for configured publishing if needed.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@charts/gitops-reverser/values.yaml`:
- Around line 211-217: Update the preceding enabled-attribution prerequisite
comment in values.yaml to state that Redis configuration is required only when
transport is "redis", while transport "memory" requires a single replica. Keep
the existing transport descriptions and configuration unchanged.

In `@docs/architecture.md`:
- Around line 605-611: Update the Optional attribution section in
docs/architecture.md to reference internal/queue/fact_index.go instead of the
removed attribution_index.go, and revise the state-store wording to explain that
Redis is used by the Redis transport while --author-attribution-transport=memory
keeps facts in an in-process ring and does not require --redis-addr.

In `@docs/design/metrics-observability-plan.md`:
- Line 276: Update the recording-site entry in the metrics observability plan to
reference internal/queue/fact_index.go instead of the removed
attribution_index.go, keeping the surrounding documentation unchanged. Run task
lint-docs after the documentation update.

In `@docs/finished/attribution-fact-stream.md`:
- Around line 51-56: The shipped-history documentation omits the PR identifying
the attribution transport switch-over. In
docs/finished/attribution-fact-stream.md lines 51-56, update the “The
switch-over” entry to reference PR `#287`; in docs/INDEX.md lines 108-117, add
`#287` to the shipped attribution PR list.

In `@docs/spec/deletecollection-attribution-expander.md`:
- Around line 5-24: The document contains stale references to the deleted
expander implementation. In docs/spec/deletecollection-attribution-expander.md
lines 5-24, remove the self-referential related link while retaining the
attribution-fact-stream replacement link; in lines 149-155, replace the legacy
RecordFact/attribution_index.go “stores nothing today” description with the
current single collection-fact behavior or explicitly mark it historical.

In `@internal/queue/redis_store_test.go`:
- Around line 107-111: Remove the stale AttributionIndex test comment
immediately before TestEscapeKeyField, leaving the TestEscapeKeyField
declaration and its coverage unchanged.

In `@internal/webhook/audit_handler.go`:
- Around line 394-400: Update processEvents in internal/webhook/audit_handler.go
(394-400) to track successfully appended stream batches, record queued for their
fact-bearing events, and reserve write_error for failed or unattempted batches
while still returning the publication error for retry. Update
internal/webhook/audit_handler_test.go (630-645) to cover two fact-bearing keys
with failure on the second publish and verify the first append is retained;
update internal/webhook/audit_metrics_test.go (90-94) to assert per-event
queued/write_error metrics; revise docs/interpreting-metrics.md (194-196) to
document partial publication and retry semantics instead of claiming no facts
were written on transport errors.
- Around line 305-306: Update the logging call in the handler around
recordAcceptedOutcomes and logAcceptedFacts so only accepted entries with
acceptedFact.ok true are logged as published. Preserve recording all accepted
outcomes, but filter non-facts—especially configured-author events with a nil
publisher—before invoking logAcceptedFacts.

---

Outside diff comments:
In `@internal/webhook/audit_fact_publish_test.go`:
- Around line 169-178: The test currently configures a publisher and verifies a
publication, so it does not cover the nil-publisher behavior named by
TestAuditHandler_NoPublisherPublishesNothing. Construct the handler without
setting AuditHandlerConfig.FactPublisher, retain the successful HTTP response
assertion, and remove the publisher-receipt assertion; add or preserve a
separate positive test for configured publishing if needed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 36ff990f-9e48-40a7-aebe-e9f075829334

📥 Commits

Reviewing files that changed from the base of the PR and between 7ece731 and 462778f.

📒 Files selected for processing (40)
  • charts/gitops-reverser/README.md
  • charts/gitops-reverser/templates/deployment.yaml
  • charts/gitops-reverser/values.schema.json
  • charts/gitops-reverser/values.yaml
  • cmd/main.go
  • cmd/main_redis_flags_test.go
  • docs/INDEX.md
  • docs/architecture.md
  • docs/configuration.md
  • docs/design/attribution-wait-poll-vs-push.md
  • docs/design/metrics-observability-plan.md
  • docs/design/open-asks-priority.md
  • docs/finished/attribution-fact-stream.md
  • docs/finished/redis-key-schema-v3.md
  • docs/interpreting-metrics.md
  • docs/spec/deletecollection-attribution-expander.md
  • internal/controller/commitrequest_attribution_agreement_test.go
  • internal/queue/attribution_index.go
  • internal/queue/attribution_index_deletecollection_test.go
  • internal/queue/attribution_index_test.go
  • internal/queue/author_fact.go
  • internal/queue/author_fact_test.go
  • internal/queue/fact_index.go
  • internal/queue/key_prefix.go
  • internal/queue/key_prefix_test.go
  • internal/queue/redis_store.go
  • internal/queue/redis_store_test.go
  • internal/telemetry/exporter.go
  • internal/watch/author_resolver.go
  • internal/watch/author_resolver_index_test.go
  • internal/watch/author_resolver_test.go
  • internal/watch/manager.go
  • internal/watch/target_watch.go
  • internal/watch/target_watch_test.go
  • internal/webhook/audit_fact_publish_test.go
  • internal/webhook/audit_handler.go
  • internal/webhook/audit_handler_test.go
  • internal/webhook/audit_identity_test.go
  • internal/webhook/audit_metrics_test.go
  • test/e2e/e2e_suite_test.go
💤 Files with no reviewable changes (3)
  • internal/queue/attribution_index.go
  • internal/queue/attribution_index_deletecollection_test.go
  • internal/queue/attribution_index_test.go

Comment thread charts/gitops-reverser/values.yaml
Comment thread docs/architecture.md Outdated
Comment thread docs/design/metrics-observability-plan.md Outdated
Comment thread docs/finished/attribution-fact-stream.md Outdated
Comment thread docs/spec/deletecollection-attribution-expander.md
Comment thread internal/queue/redis_store_test.go Outdated
Comment thread internal/webhook/audit_handler.go Outdated
Comment thread internal/webhook/audit_handler.go
sunib and others added 4 commits July 28, 2026 15:07
@sunib @claude
test(e2e): attribute an aggregated collection delete through the ward…
8117685 
…le API

The response-body expander gave up entirely on a deletecollection the API server
sent no body for, and that case now resolves by scope. Until now it was proven
only at unit level, against facts a test constructed. This drives it against a
real aggregated API server.

The kube-apiserver PROXIES a request for an aggregated resource and audits the
request it proxied, but never decodes the response it streamed back, so
responseObject is empty. Row 15 of the lab corpus established exactly that for a
flunder create, and a collection delete travels the same proxy path — which
makes wardle a far more honest witness for the body-less case than any fixture.

The spec asserts the AUTHOR rather than the metric tier. Whether a proxied
collection delete returns a body is a fact about the API server, not about this
operator: if some future version did return one, the join silently upgrades to
uid membership and this spec should still pass. The tier that actually fired is
reported into the Ginkgo output instead, so a change in that behaviour is
visible without being a failure.

The suite's attribution report also learns the two new result labels, replacing
exact_deletecollection_item, which no longer exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(queue): expire idle fact streams and stop shipping unread fields
fef99b7 
Two things a Redis client made visible: the stream keys have no TTL, and the
entries carry more than the join reads.

The missing TTL is a leak, not an oversight in naming. MINID trimming is
amortized onto the publish path, so a stream whose type stops being written to
is never trimmed again — nothing appends to it, and the trim rides on the
append. Its key then lives for the life of the Redis instance, one immortal key
per (route, type) pair that ever saw a single write. A namespace-scoped type
garbage-collected once when a namespace is torn down is exactly that shape, and
nothing was left to clean it up. EXPIRE now rides along with every XADD, so a
stream dies one retention horizon after its last append and a busy one keeps
refreshing the deadline. The in-memory transport had the identical leak in the
pod's heap, bounded only by how many types the process had ever seen; it now
forgets a ring whose entries have all aged out.

The payload carried three fields no join tier reads. The group/resource is the
STREAM'S OWN NAME — the index takes its scope from the entry's key and never
from the fact — so every entry repeated its own routing. The object's name and
subresource are joined on by nothing: the tiers key on uid, resourceVersion, or
scope. isServiceAccount went too, for a different reason: it is not evidence,
it is a prefix check on the author that the reader can do for itself, and a
stored copy can disagree with the name beside it.

That is a quarter off a real collection-delete entry, and it is paid three
times over: a fact is broadcast to every process following its type, held for
the whole TTL, and replayed into memory on every restart.

auditID stays, and it is the one field here the join never reads. It is what
ties a commit authored by the wrong person back to the audit event that named
them, which is the single question a mis-attribution investigation asks and the
one thing nothing else in the system can answer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(queue): credit a collection delete to the deleter, not the last e…
fd67198 
…ditor

The tiers ranked the latest fact above the collection's uid set, and those two
answer different questions. The latest tier says who last WROTE an object; a
removal asks who DELETED it.

For a single-object delete the two coincide, because the delete files its own
fact under that uid and overwrites whatever was there. A COLLECTION delete files
one fact about the collection instead, so the uid's latest entry is left holding
whoever happened to edit the object last — and every removal in the collection
resolved to that person, never reaching the actor who actually ran the delete.
The uid set was collected, carried, and never read.

That is the one thing the deleted expander got right: it wrote a per-object fact
for every item in the response body, overwriting the previous editor. Dropping
the expander without re-ranking the tiers regressed it.

Uid membership is the API server stating that THIS request deleted THIS object,
so nothing weaker may answer ahead of it; it now runs before the latest tier.
Scope matching stays below, because it is the weakest evidence the join has and
the only tier that can name the wrong human — an unrelated delete by another
actor during the same window is still claimed by its own fact and never reaches
it.

The unit tests could not catch this: the collection fixture carried no uids, so
it exercised the scope tier only. Neither could the e2e specs, which create and
delete as the same actor, so the wrong answer and the right answer are the same
name. Both now have a case where the editor and the deleter are different
people, because that is the only shape in which this failure is visible — and
its cost is naming an innocent person as the author of a deletion they did not
perform.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(queue): age facts from their append time and stop phantom trim gaps
d72fd70 
Two holes in the TTL and the loss reporting, both found in review.

A fact was aged from when this process READ it rather than from when it was
appended. The case that matters is not exotic: the follower replays the whole
retention window on start, so every entry in it got a second full TTL, and a
process restarting more often than the horizon would keep facts alive
indefinitely — the bound would bound nothing. A follower that falls behind, or
a transport handing back an entry its own retention should have dropped, lands
in the same place. Stream IDs are millisecond timestamps in both transports, so
a position IS a time: Apply now reads the append time off the entry's ID, and
clamps a future one, since that is the only direction that could extend a
fact's life rather than shorten it.

Trim-gap detection inferred "behind" from a read filling its entry budget, and
then never cleared it. The inference is unavoidable — a read that fills the
budget exactly is indistinguishable from one that left more waiting — but the
mark was sticky, so it survived every later empty read. A follower that filled
the budget exactly and then caught up stayed flagged, and ordinary retention
ageing out the entries it had ALREADY read surfaced as a data-loss counter and
a log line naming a stream it never lost anything on. A follower that asks and
is given nothing is caught up by definition, so that is now what clears it, on
both transports. The real gap test still passes, so detection is narrowed
rather than disabled.

The design record also claimed the admission webhook requires Redis. It does
not, deliberately: the webhook is failurePolicy Ignore and the controller is the
real gate, so without Redis it no-ops command-author capture and CommitRequests
claim no actor. That is a supported degraded mode, it pre-dates this design, and
a test pins it — so the record is corrected rather than the code, which would
have broken a shape installs already run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot reviewed Jul 28, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
internal/queue/fact_index_test.go (1)

522-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider covering the other two entryAppendTime branches.

This test pins the "ages from append time" behavior well, but only exercises the past-age and current-time paths. The fallback-to-now-on-unparseable-ID and clamp-future-to-now branches in entryAppendTime aren't directly exercised by a fact-index-level test.

🧪 Example additional cases 
// A malformed/unparseable ID falls back to now rather than never.
malformed := FactEntry{Key: key, ID: "not-a-valid-id", Facts: []AuthorFact{objectFact("alice", "101")}}
index.Apply(t.Context(), malformed)
require.Equal(t, "alice",
	index.Lookup(objectQuery("prod-eu-1", factIndexTestUID, "101", true)).Fact.Author)

// A future-dated ID is clamped to now rather than extending the TTL.
future := FactEntry{Key: key, ID: streamIDAt(time.Now().Add(time.Hour)), Facts: []AuthorFact{objectFact("alice", "101")}}
index.Apply(t.Context(), future)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/queue/fact_index_test.go` around lines 522 - 543, Extend
TestFactIndex_AFactAgesFromWhenItWasAppended to cover entryAppendTime’s fallback
branches: apply a FactEntry with an unparseable ID and verify its fact is
immediately joinable, then apply one with a future-dated ID and verify the ID is
clamped to now rather than extending the TTL. Keep the assertions focused on
fact-index behavior and use distinct facts or reset state as needed to avoid
masking either case.
🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/e2e/deletecollection_intent_e2e_test.go`:
- Around line 216-225: Before deleting the collection in the “deleting the
collection as alice” step, wait for each cleanup-bot annotation performed by
annotateConfigMap to propagate to Git and be attributed to cleanup-bot. Use the
existing repository polling/assertion helpers around the loop over first and
second so DeleteCollection runs only after the bot edit is observable.

---

Nitpick comments:
In `@internal/queue/fact_index_test.go`:
- Around line 522-543: Extend TestFactIndex_AFactAgesFromWhenItWasAppended to
cover entryAppendTime’s fallback branches: apply a FactEntry with an unparseable
ID and verify its fact is immediately joinable, then apply one with a
future-dated ID and verify the ID is clamped to now rather than extending the
TTL. Keep the assertions focused on fact-index behavior and use distinct facts
or reset state as needed to avoid masking either case.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 569a347b-5648-469c-b662-ce5113aa37b3

📥 Commits

Reviewing files that changed from the base of the PR and between 462778f and d72fd70.

📒 Files selected for processing (18)
  • docs/finished/attribution-fact-stream.md
  • internal/queue/author_fact.go
  • internal/queue/author_fact_test.go
  • internal/queue/fact_index.go
  • internal/queue/fact_index_store.go
  • internal/queue/fact_index_test.go
  • internal/queue/fact_stream.go
  • internal/queue/fact_stream_conformance_test.go
  • internal/queue/fact_stream_memory.go
  • internal/queue/fact_stream_memory_test.go
  • internal/queue/fact_stream_redis.go
  • internal/queue/fact_stream_redis_test.go
  • internal/watch/author_resolver_index_test.go
  • internal/watch/author_resolver_test.go
  • internal/webhook/audit_fact_publish_test.go
  • internal/webhook/audit_handler_test.go
  • test/e2e/aggregated_deletecollection_e2e_test.go
  • test/e2e/deletecollection_intent_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
  • internal/webhook/audit_fact_publish_test.go
  • internal/queue/author_fact_test.go
  • internal/watch/author_resolver_index_test.go
  • internal/watch/author_resolver_test.go
  • internal/queue/author_fact.go
  • docs/finished/attribution-fact-stream.md
  • internal/webhook/audit_handler_test.go

Comment thread test/e2e/deletecollection_intent_e2e_test.go
sunib and others added 19 commits July 28, 2026 15:59
@sunib @claude
test(queue): prove one fact serves every GitTarget waiting for it
76b80e5 
The fan-in property had no test of its own: every existing case drives a single
resolver, so nothing proved that waking reaches more than one, or that a second
consumer does not need a second copy of the fact.

Two resolvers park on the same candidate key, one fact is published once, and
both resolve from it. The index then holds that one fact rather than one per
consumer, and a third resolver arriving after it is stored takes the immediate
check instead of blocking.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(webhook): assert the collection join against captured aggregated…
8d006ab 
… deletes

Four recordings of a real deletecollection on wardle flunders were sitting in
testdata unreferenced: nothing had loaded them since the test that captured
them was removed. They are not dead scaffolding, they are the ground truth for
the question this whole tier exists to answer — does a proxied collection
delete carry the set it deleted? — so they are wired in rather than deleted.

They answer it, and they corroborate the e2e run independently: the official
aggregation layer proxies the request and never decodes the response it
streamed back, so it audits with NO body, and the join falls to scope matching.
A body-supplying proxy in front of the extension server does return the list,
and the join upgrades itself to uid membership. A proxy echoing DeleteOptions
looks like a body and carries no items, which has to degrade the same way an
absent one does without failing the parse.

Every one of the body-less shapes is a case the deleted expander produced
nothing at all for, and shipped committer-authored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(queue): make a removal wait for evidence about the deletion
3197419 
Re-ranking the tiers only decided between facts that are both present. It could
not help with the case that actually happens: the watch event reliably arrives
before the audit batch carrying its delete — that is the entire reason the grace
window exists — so at the moment a removal is resolved, the only fact in the
index is often the object's last WRITE.

The resolver returned on the first match, so it answered "who deleted this" with
"who last edited it" whenever anyone had touched the object before it was
removed. No ordering of the tiers could have fixed that, because the right fact
had not been delivered yet.

A per-object match on a removal is now held as a FALLBACK rather than returned,
unless the fact is itself about a deletion. The wait continues for evidence
about the removal — either collection tier, or the object's own delete fact —
and the fallback is returned when the grace expires with nothing better. Waiting
never costs an attribution: the worst case returns exactly what returning early
would have returned, one grace window later, which is the case the grace window
is for.

Found because the e2e spec added with the precedence fix passed at one process
and failed at four: under load the audit batch lost the race often enough to
flip the author to the last editor. Locally it resolved collection_uid=2 and in
CI weak won, which is the same bug seen from both sides.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(e2e): the author-mode probe read "no Redis" as configured-author
b12737f 
The probe mirrors cmd/main.go's mode switch so a spec can tell whether commits
should carry a real actor. It still encoded the old rule — attribution requires
Redis — so an install running attribution over the in-memory transport with an
empty --redis-addr was classified as configured-author.

Every attribution spec then SKIPPED, and the run reported green having asserted
nothing about attribution at all. That is precisely the failure this probe was
built to prevent, turned on the probe itself: it exists so a mode change flips
an assertion instead of silently disabling it. A no-Redis run looked like proof
and was not.

The rule is now "attribution is on AND it has a transport", which is what the
binary validates. Redis with no address is still configured-author; memory with
no address is attribution running normally.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs(attribution): correct the record on tier order and on waiting
0c3e399 
Two claims in the record were wrong, and the implementation contradicted them
rather than the other way round.

It ranked BOTH collection tiers below the latest tier. That credited every
collection-delete removal to whoever last edited the object, and left the uid
set the receiver had gone to the trouble of collecting entirely unread. Uid
membership is the API server stating that this request deleted this object, so
it now sits above latest; scope matching stays below it, being the only tier
that can name the wrong human.

It also said a hit returns. Ordering only decides between facts that are both
present, and the watch event reliably beats the audit batch carrying its delete,
so at resolve time the only fact present is often the object's last write.
Returning on it answered "who deleted this" with "who last edited it". A
per-object match on a removal is held as a fallback instead, and the wait
continues for evidence about the deletion.

configuration.md and interpreting-metrics.md carry the operator-visible halves:
what a removal now waits for, and why collection_uid outranks weak.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(webhook): fail only the streams that did not append
bfb5fbf 
Review caught that a transport failure mid-request was reported as an
all-or-nothing loss, and it is not. Publication is per stream and sequential, so
the batches before the failing one HAVE appended and their facts are in the log
whatever happens next. Counting those events as write_error claimed a loss that
did not occur, on the one outcome an operator is meant to treat as a real
problem.

Each accepted event's outcome is now decided by whether its OWN stream appended.
An event that produced no fact is queued rather than failed: it was carried as
far as it can go and no append was ever owed for it. The request still fails, so
the API server retries the whole batch and the landed facts are appended again —
safe precisely because a fact is keyed data rather than a position in a sequence.

The published-fact log had the same over-claim: it named every accepted event as
published, including those that produced no fact at all, which is every event in
configured-author mode.

Also from review: the chart's attribution prerequisite still said Redis was
required, the metrics doc still described write_error as covering a whole batch,
a test heading outlived the test it described, the expander spec still linked to
itself and described deleted RecordFact behaviour as current, and the shipped
history did not name this PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(e2e): prove the cleanup bot is the last writer before deleting
5e535e7 
The spec's premise is that a DIFFERENT identity wrote the objects last when the
collection is deleted, so that a removal credited to the last writer names the
wrong person and is caught. It requested that edit and deleted immediately,
without waiting for it to land — and if the delete raced ahead, alice was still
the last writer and the assertion passed without exercising the case it exists
for. A test that can pass for the wrong reason is worth less than no test.

It now waits until the cleanup bot's edit is COMMITTED, which is the only
observable proof that it is the current last writer, before running the delete.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(docs): keep the INDEX entry inside the line-length limit
3084b86 
A 129-character line in the shipped-history entry failed markdownlint's MD013.

It reached CI because the local gate was read wrong, not because it was not run:
`task lint` was piped through a grep for golangci and Vale output, which
discarded the markdownlint failure and left the exit code unchecked. The gate is
the exit status.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs(attribution): record what the removal wait costs, per tier
c4a381a 
The removal wait buys a correct deletion author and is paid for in commit
latency. The number belongs in the docs rather than in a dashboard someone
discovers it from.

Measured over one e2e run, by tier: a removal that finds its delete evidence
resolves in about 70ms; one that never does averages about 3.1s before falling
back to the last write. Creates and updates never consult those tiers at all, so
the cost is concentrated in removals for which no delete fact ever arrives — a
graceful pod delete and a status-only removal produce no audit event, so those
spend the grace to end up exactly where they started.

What is deliberately NOT claimed is a before-and-after. Comparing a run of this
against a run of the previous behaviour looked easy and was not: the two runs'
populations differed by more than the change (203 non-exact resolutions against
59, and the specs added alongside this work generate collection deletes that
shift the mix by construction), and the baseline was never captured per tier. A
headline 'the mean wait moved from X to Y' out of those two runs would have been
a workload difference wearing a causal claim's clothes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs(design): work out when a removal should stop waiting for its author
458a03d 
The removal wait fixed a real mis-attribution and introduced a real latency
cost, and the choice about what to do next is a product decision rather than an
implementation detail. This writes out the situations and prices the options.

It also corrects a mental model worth correcting: the tiers are NOT tried in
sequence with a wait each. There is one wait, and every tier is evaluated on
every wake, so the first satisfying answer wins whichever tier it comes from.
The ordering is a preference among facts that are present at the same moment,
not a sequence of attempts. What the wait changed is only which answers count as
satisfying.

Eight situations, and the cost sits in exactly one: a removal for which no
delete fact will ever arrive spends the whole grace to return the answer it
already had. A graceful pod delete emits no audit event at all, so this is not a
corner case in a cluster with pod churn. At the moment of resolution it is
indistinguishable from a fact that is merely late, which is the case worth
waiting for.

Five options priced. The recommendation is a per-route watermark — stop waiting
once the fact stream has demonstrably moved past this event — because it answers
"is a fact still coming?" rather than approximating it with a second timeout
whose correct value lives in the API server's configuration rather than ours.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: the "no audit event" population is the audit policy, not Kubern…
3a6dd24 
…etes

Three records claimed a graceful pod delete and a status subresource update
produce no audit event at all, and treated that as a structural population no
wait could ever resolve. The claim traces to mutation-capture-lab corpus rows 5
and 7 — a measurement, which is why it was trusted.

The measurement is sound and the reading of it was not. The lab runs against the
already-prepared e2e cluster and reuses its audit policy, and that policy drops
`pods` and every `*/status` at level: None as runtime noise. So those rows record
what a cluster configured like this one does not ask for, not what the API server
cannot say. A DELETE on a pod is an audited request like any other, and under the
deletion-as-intent rule that request is exactly the fact the join wants.

This matters for what to do next, not just for accuracy. "Kubernetes cannot tell
us" is a wall. "The policy did not ask" is a setting: it is knowable, it is
per-type and permanent rather than intermittent, and it is exactly what a
per-(route, type) circuit breaker can learn — which is now the leading option for
bounding the removal wait, ahead of the watermark that only prices the transient
case.

It is also an operator-visible gotcha worth stating plainly, so configuration.md
now says it: watching a type your audit policy excludes means every removal on it
spends the full grace and then ships committer-authored, and no operator-side
setting changes that.

The corpus rows are annotated rather than rewritten. Re-capturing either against
a policy that includes those types would settle it by measurement rather than by
reading the policy, which is the honest way to close it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(lab): capture the flunder delete and deletecollection, and say w…
d1f3f4a 
…hat row 7 is for

Two new lab scenarios for the aggregated API, because the question "why do
creates and updates measure but deletes not?" deserved a measurement rather than
an inference: does a proxied delete carry a uid, and does a proxied
deletecollection carry a response body. Each logs its finding whichever way it
falls rather than asserting a preconceived answer.

They are written and NOT yet verified: the lab run failed every scenario in it,
including long-standing ones, so the environment is broken in this cluster and
the run produced no findings at all.

The corpus answers most of it anyway, and the answer is sharper than expected.
An aggregated objectRef carries no name, no uid and no resourceVersion, so a
create produces no fact at all (rejected at the name gate), while an update or a
single delete produces a fact the index then discards as unjoinable. Only the
collection delete works, because a collection fact joins by scope and needs no
uid. Written up in the removal-wait options record, along with the tier that
would fix two of those rows — and the note that this work removed the fact's
name field as "read by no tier", which was true and is exactly the field that
tier would need back.

The pod graceful-delete scenario keeps its construct rather than being swapped:
TestFinalizerDelete already covers the same two-step removal on an AUDITED type,
so replacing it would duplicate that and destroy the corpus's only record of what
an audit-excluded type looks like — which is the population that costs the
resolver its whole grace window. Its doc now says the silence is this cluster's
policy rather than Kubernetes, and points at the audited equivalent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(lab): serve the named audit route, not just the bare path
f76a273 
The lab registered exactly one audit path, /audit-webhook, while the e2e cluster's
audit kubeconfig posts to /audit-webhook/default: the route is NAMED after the
"default" ClusterProvider, because the bare path is the product's shared,
annotation-routed endpoint. Go's ServeMux treats a pattern without a trailing slash
as an exact match, so every audit event the API server sent got a 404.

The store held 133 admission records, 10 watch records and zero audit records, so
every scenario needing an audit event timed out, including the seventeen
long-standing ones. That reads exactly like a broken cluster rather than a one-line
routing mismatch, which is the expensive part: the lab's failure mode was
indistinguishable from the environment's.

Serving the whole subtree fixes it. Row 15 goes from a 91-second timeout to passing
in 6.6 seconds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
fix(lab): stop the corpus churning on timings and generated names
5c34f7b 
Two volatile things reached the golden tree, both found by running a COMPARE pass
rather than trusting a capture: re-capturing alone rewrites whatever it sees and
reports success.

The apiserver.latency.k8s.io/* annotations are dropped whole rather than
placeholder-ed. Both halves of them are nondeterministic: the values are durations
that differ every run, and the keys are present at all only when the request crossed
the API server's slow-request threshold. Rewriting just the values would still leave
a corpus that diffs between a fast run and a slow one. One slow deletecollection is
what put them in the tree.

Generated names are now collected in full, not just as a suffix. The existing rule
rewrites a `name` that sits beside its own `generateName`, which an AdmissionReview
does not: it carries the assigned name at request.name, a sibling of kind and
namespace with no metadata around it. The full name is now rewritten wherever it
appears, including embedded in a requestURI.

Both were pre-existing gaps that only a scenario with a random name and a slow
request would expose, and both would have surfaced later as an unexplained diff on
someone's unrelated change. A corpus whose diff cannot be trusted to mean something
is the one thing this tree must not become.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(lab): capture the aggregated delete and deletecollection
5b91aef 
The two scenarios were written but never verified, because the audit route mismatch
meant the run produced no findings at all. With the lab recording again they answer
what the create half could not, and the answers are now in the corpus rather than
inferred from it.

An aggregated-API single delete IS audited, and its objectRef carries the name from
the URL path but no uid and no resourceVersion, with no response body to recover
either from. So the exact and latest tiers can never match it.

An aggregated deletecollection IS audited, once, name-less, with the selector visible
only in the requestURI and no response body. Its fact carries no uid set, so the join
must fall back to scope matching, which is exactly the case the deleted response-body
expander produced nothing at all for.

Also fixes the helper both scenarios read their records through: it filtered on
Key.Resource, which only audit and admission records carry, because a watch record's
key comes from the object's GroupVersionKind. It was discarding precisely the watch
events these scenarios measure. The collection deletes three flunders rather than two,
so the fan-out reads as a fan-out rather than as a pair.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(lab): capture a generateName create as the aggregated rows' control
0d695a0 
"The name is not available yet" describes both a generateName create and an
aggregated write, so it would be reasonable to expect them to fail the same way.
They do not, and this row is the measurement that says so rather than the reasoning
that claimed it.

A generateName create's audit objectRef is as name-less as an aggregated write's,
and it joins perfectly well, because the policy captures at RequestResponse and the
response body carries the assigned name and uid for IdentityFromAuditEvent to
backfill from. Measured: objectRef name="", hasResponseObject=true.

So the discriminator is the BODY, not the name. Put row 18 beside rows 15a and 15b
and that is visible in the corpus without reading any code, which is the difference
between a claim in a paragraph and evidence in the tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
feat(attribution): restore the fact's name and join on it
8e40dca 
An aggregated-API write is audited with no uid and no resourceVersion, and no body
to recover either from, so the name from the URL path is the only identity it
carries. Such a fact was published and then dropped by the index as unjoinable,
which meant an aggregated update, patch or single delete shipped committer-authored
however ran it. Only the collection delete reached an author, because a collection
fact joins by scope and needs no uid.

AuthorFact.Name is back, and the index files a fact under (namespace, name) when it
carries neither a uid nor a resourceVersion. Lookup consults that tier last, below
the rv-only hatch, reporting AttributionName so the metric says which evidence named
the author.

Last is where it belongs: a name is REUSED after a delete and recreate, where a uid
never is and an rv identifies one specific write, so it is the weakest per-object
evidence there is. Ranking it last costs the stronger tiers nothing, because no fact
carrying a uid or an rv is ever filed there and no query reaches it until every
stronger tier has missed. The TTL is what bounds the wrong answer: it needs the
recreate to happen inside it.

The waiter keys are wired too. Without them a query whose fact arrives late sleeps
out its whole grace beside a fact that would have matched.

Restoring the field is worth being explicit about. It was removed during this work on
the observation that no tier read it, which was true of the code and false of the
domain. "No code reads it" and "nothing could ever read it" are different claims, and
only the second one justifies deleting a field.

This does not reach the aggregated CREATE, whose objectRef carries no name at all, so
nothing is published for any tier to join.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
test(e2e): assert a CommitRequest committed, not merely that it ended
1c67ea5 
Ready=True is not proof of a commit. It is also the benign-rejection state:
rejectCommitRequest sets it deliberately for NoWindowInGrace, WindowMismatch and
AlreadyPresent so that kstatus reads Current rather than Failed.

So the specs' Ready assertion passed on a request that committed nothing, and they
then spent the rest of the two-minute timeout re-reading an empty status.sha before
reporting "Expected <string>: not to be empty" — the least informative message
available, with the actual reason sitting in the condition they never read.

expectCommitRequestCommitted requires the Ready reason to be Committed, and gives up
the moment any other terminal outcome appears: a terminal outcome is final, so
re-reading it cannot change the answer, it only delays the report. The failing
generateName spec now fails in ten seconds naming NoWindowInGrace and its message
instead of in two minutes naming nothing.

That spec still fails. This makes the failure legible, it does not fix it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: record what the branch's loose ends turned out to be
96c0f61 
Five findings, in one place, separating what was measured from what was reasoned:
the lab's audit-route defect that hid every other measurement, the three results
about how an audit event identifies its object, and the one open product race.

Includes the decision to build the name tier and what it still does not reach, plus
two mermaid diagrams: where each verb falls out of the join, and the two-second
window miss. Corrects the removal-wait record's provenance line, which cited
deletecollection recordings that did not exist when it was written, and sharpens its
claim: the objectRef carries no uid or resourceVersion, and no name either unless the
request URL supplies one, which for a single delete it does.

The doc is Vale-clean, so it joins the prose gate rather than sitting outside it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot reviewed Jul 28, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/spec/deletecollection-attribution-expander.md (1)

262-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the UID set explicitly optional in the test contract.

Line [264] says every collection fact carries “the uid set,” but Lines [268-269] require hollow, status, unparseable, and absent bodies to publish facts without one. Change the contract to say “the UID set when present” so the expected behavior is unambiguous.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/spec/deletecollection-attribution-expander.md` around lines 262 - 275,
Update the collection-delete test contract’s first requirement to state that the
fact carries the UID set when present, while retaining the existing actor,
namespace, and selector requirements and the no-UID behavior for hollow, status,
unparseable, or absent bodies.
internal/queue/fact_index.go (1)

85-91: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update Namespace's doc comment — it's no longer collection-tier-only.

"Namespace and Labels serve the collection tier only: they are how a removal finds the deletecollection whose scope covered it." Lookup now also passes query.Namespace into facts.lookupName(query.Namespace, query.Name, cutoff) for the new name tier, and TestFactIndex_NameTierIsScopedToItsNamespace depends on that. The comment should mention the name tier's use of Namespace too, or a future reader will assume it's dead for that path.

As per coding guidelines: "Add or update godoc comments for all exported identifiers."

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/queue/fact_index.go` around lines 85 - 91, Update the exported
`Namespace` field’s godoc comment to state that it is used both for
collection-tier removal lookup and to scope name-tier lookups by namespace. Keep
the existing `Labels` documentation accurate as collection-tier-only, and
preserve the neighboring `Name` field documentation.

Source: Coding guidelines

🧹 Nitpick comments (1)
test/mutationlab/e2e/aggregated_api_test.go (1)

108-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate filtering logic between flunderRecord and flunderRecordNamed.

flunderRecordNamed (with an empty name) is a strict superset of flunderRecord's behavior modulo the isFlunder tightening. Consider having flunderRecord delegate to flunderRecordNamed(records, src, watchType, "") (or removing it if no longer needed) to avoid maintaining two near-identical filters.

Also applies to: 308-332

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/mutationlab/e2e/aggregated_api_test.go` around lines 108 - 122,
Eliminate the duplicated filtering in flunderRecord by delegating to
flunderRecordNamed with an empty name, or remove flunderRecord if callers can
use the named helper directly. Preserve flunderRecord’s current matching
behavior, including its flunders resource or fl-1 name condition, while keeping
the stricter isFlunder behavior in flunderRecordNamed.
🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/design/attribution-removal-wait-options.md`:
- Around line 91-122: Update the aggregated-types section to reflect that the
name-based attribution tier has shipped and resolves aggregated update, patch,
and single-delete events. Revise Row 8 and the surrounding conclusions so only
create remains unattributable, deletecollection remains scope-joined, and
name-based joins are described as current behavior rather than a future
proposal; retain the documented name-reuse limitation.

---

Outside diff comments:
In `@docs/spec/deletecollection-attribution-expander.md`:
- Around line 262-275: Update the collection-delete test contract’s first
requirement to state that the fact carries the UID set when present, while
retaining the existing actor, namespace, and selector requirements and the
no-UID behavior for hollow, status, unparseable, or absent bodies.

In `@internal/queue/fact_index.go`:
- Around line 85-91: Update the exported `Namespace` field’s godoc comment to
state that it is used both for collection-tier removal lookup and to scope
name-tier lookups by namespace. Keep the existing `Labels` documentation
accurate as collection-tier-only, and preserve the neighboring `Name` field
documentation.

---

Nitpick comments:
In `@test/mutationlab/e2e/aggregated_api_test.go`:
- Around line 108-122: Eliminate the duplicated filtering in flunderRecord by
delegating to flunderRecordNamed with an empty name, or remove flunderRecord if
callers can use the named helper directly. Preserve flunderRecord’s current
matching behavior, including its flunders resource or fl-1 name condition, while
keeping the stricter isFlunder behavior in flunderRecordNamed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5f18e2be-be11-4a9d-b935-2f4c94b9371b

📥 Commits

Reviewing files that changed from the base of the PR and between d72fd70 and 96c0f61.

📒 Files selected for processing (41)
  • .docs-lint-scope
  • charts/gitops-reverser/values.yaml
  • cmd/mutation-capture-lab/main.go
  • docs/INDEX.md
  • docs/configuration.md
  • docs/design/attribution-branch-findings.md
  • docs/design/attribution-removal-wait-options.md
  • docs/finished/attribution-fact-stream.md
  • docs/interpreting-metrics.md
  • docs/spec/deletecollection-attribution-expander.md
  • internal/mutationlab/normalize/normalize.go
  • internal/mutationlab/normalize/normalize_test.go
  • internal/queue/author_fact.go
  • internal/queue/fact_index.go
  • internal/queue/fact_index_store.go
  • internal/queue/fact_index_test.go
  • internal/queue/redis_store_test.go
  • internal/watch/author_resolver.go
  • internal/watch/target_watch.go
  • internal/webhook/audit_fact_publish_test.go
  • internal/webhook/audit_handler.go
  • test/e2e/author_mode_test.go
  • test/e2e/commit_request_e2e_test.go
  • test/e2e/deletecollection_intent_e2e_test.go
  • test/e2e/e2e_suite_test.go
  • test/mutationlab/README.md
  • test/mutationlab/corpus/configmap/generate-name-create/admission.create.yaml
  • test/mutationlab/corpus/configmap/generate-name-create/audit.create.yaml
  • test/mutationlab/corpus/configmap/generate-name-create/watch.added.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-delete/audit.delete.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-delete/watch.deleted.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/admission.delete.fl-dc-a.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/admission.delete.fl-dc-b.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/admission.delete.fl-dc-c.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/audit.deletecollection.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/watch.deleted.fl-dc-a.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/watch.deleted.fl-dc-b.yaml
  • test/mutationlab/corpus/flunder/aggregated-api-deletecollection/watch.deleted.fl-dc-c.yaml
  • test/mutationlab/e2e/aggregated_api_test.go
  • test/mutationlab/e2e/configmap_scenarios_test.go
  • test/mutationlab/e2e/workload_scenarios_test.go
💤 Files with no reviewable changes (1)
  • internal/queue/redis_store_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
  • docs/INDEX.md
  • internal/watch/author_resolver.go
  • test/e2e/deletecollection_intent_e2e_test.go
  • docs/finished/attribution-fact-stream.md
  • internal/queue/author_fact.go
  • internal/watch/target_watch.go
  • charts/gitops-reverser/values.yaml
  • docs/interpreting-metrics.md
  • docs/configuration.md

Comment thread docs/design/attribution-removal-wait-options.md Outdated
sunib and others added 2 commits July 28, 2026 20:54
@sunib @claude
fix(queue): let a removal reach its delete fact when the fact is keye…
81e0b56 
…d by name

A removal was waiting out its whole grace for evidence the index already held, and
that wait blocks the watch shard's serial goroutine, so every later event of the type
queued behind it. Measured on the e2e cluster, which runs a 10s grace: three removals
in one commit-request run spent 20.18s between them, mean 6.73s, all resolving weak.

The consequence was a spec failing for a reason unrelated to what it tests. A
CommitRequest created 105ms after a Deployment reported NoWindowInGrace, because the
Deployment's event had not been processed yet: three removals from the previous
specs' cleanup were ahead of it, each burning a grace. The window could not open, so
there was nothing for the request to attach to.

Whether a delete fact carries a uid depends on what the API server answers the
request with, and it answers differently for different deletes. Both shapes are in
the corpus: configmap/finalizer-delete returns the ConfigMap, so the uid is
recoverable; configmap/owner-ref-cascade returns a Status, so the fact's only key is
its name. A kubectl delete deployment is the second shape.

Restoring the name field stored those facts but left them unreachable for the
population that needs them most. Lookup returns as soon as the removal ladder yields
anything, and the uid tier yields the object's last WRITE fact, so the name-keyed
DELETE fact below was never consulted. The caller then held the write fact and waited
the full grace for the evidence sitting one tier down.

lookupRemoval now applies the ordering rule the removal path already states elsewhere:
a fact about the DELETION outranks a fact about a write, whichever key each is filed
under. The object's own delete fact wins from the uid tier, then from the name tier,
and only then does the held write fact answer. A name-keyed WRITE still does not jump
the queue.

Total resolver wait across the same run: 21.24s before, 1.63s after, with two
resolutions now landing on the name tier that previously reached nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: root-cause the CommitRequest window race and record the fix
55f5262 
Replaces the open question with the measurement that answered it. Neither of the two
candidate explanations was right: the window was not slow to open and the request's
grace was not too short. The write's event had not been processed, because removals
ahead of it were each waiting out a ten-second grace for evidence already in the index.

Carries the histogram before and after, the two delete-response shapes from the corpus
that decide whether a delete fact has a uid at all, and the note that this leaves the
head-of-line block itself unfixed: a removal that must genuinely wait still stalls its
shard, and the two directions for that are the design call, not this change.

Also records that the pre-branch probe produced no answer. Its bring-up failed with the
manager rejecting the API server's audit client certificate, which is worktree
provisioning rather than a product difference; the comparison turned out to be
unnecessary once the cause was measured directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
coderabbitai[bot]
coderabbitai Bot reviewed Jul 28, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/design/attribution-branch-findings.md`:
- Around line 277-280: Update the timing claim in the measurement findings to
match the recorded table: describe the three weak resolutions as consuming
roughly 20 seconds in aggregate, or replace it with accurate per-removal
durations from the recorded measurements. Remove the unsupported assertion that
each removal waited a full ten seconds.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 395312de-2bcf-4023-bf34-5f72a4b4b254

📥 Commits

Reviewing files that changed from the base of the PR and between 96c0f61 and 55f5262.

📒 Files selected for processing (3)
  • docs/design/attribution-branch-findings.md
  • internal/queue/fact_index.go
  • internal/queue/fact_index_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/queue/fact_index.go

Comment thread docs/design/attribution-branch-findings.md Outdated
sunib and others added 14 commits July 29, 2026 05:38
@sunib @claude
docs: describe attribution's two halves, and why neither knows about …
c33ed01 
…types

A reference for what the publish side and the join side each do, exactly: one audit
event in, zero or one fact out, filed under the keys it happens to have; one watch
event in, the tiers walked strongest-first to name an author. A flowchart per half in
the style of the findings record, the tier table, and the two rules that are easy to
miss.

Also answers the question that prompted it: nothing in the path special-cases a type.
Every branch is on the verb, on whether the event is a removal, or on which fields are
present. The type appears once, as the index partition, where it is never read to
choose a behavior.

That is a requirement rather than an accident, and the corpus is the argument. Two
ConfigMap deletes in the same cluster produce different shapes: one is answered with
the object and keeps its uid, the other with a Status and has only its name. A rule of
the form "ConfigMaps behave like this" cannot express a difference that belongs to the
request rather than the type. The same property is why aggregated APIs work without
being mentioned anywhere in the code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: say why attribution is split in two, and that filing picks one …
2f8d8bf 
…branch

Three additions to the two-halves reference, all asked for by review.

Filing is a CHOICE, not a fill-in. The fact keeps every field it recovered, but `file`
is a switch on the strongest key present and the first case wins: a fact with a uid is
not also filed under its name or its resourceVersion. The argument is reachability —
the watch side always carries the strongest keys, so a weaker duplicate entry could
never be the thing that answers a query, and it would be paid for on every replica
following the type, for the whole TTL, and again on every restart. The one branch that
files twice serves two different questions about the same object rather than hedging.

The wait, and what changed about it. Why a grace window exists at all (audit is
batched, so the first lookup is a near-guaranteed miss), why the waiter is registered
before the index is read, why a removal holds a write fact as a fallback rather than
answering with it, and what that cost: the wait runs on the shard's serial goroutine,
so it is head-of-line blocking. Carries the before-and-after numbers and names the part
that is still open.

Why it is split at all, from the fact-stream record: the audit endpoint must answer
fast and keep answering during a deploy; the fact and the watcher that needs it land in
different replicas under HA, so a per-type stream with independent cursors is the
primitive rather than an in-process channel; rollouts are the normal state, and a
resumable stream replays the retention window where publish-and-subscribe drops facts
silently; and the batching delay belongs to the API server, so the resolver must wait
on a signal rather than poll.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: diagram the wait, and say plainly that filing is about memory
494ee06 
"Reachability" was jargon for a plain argument, and the plain one is shorter: a watch
event always knows its object's uid, so a uid-keyed fact always answers from a uid tier,
and a second copy of it under its name would never be read. Storing it costs an entry on
every replica following the type, for the whole TTL, and again on every restart replay,
and buys nothing.

Adds a sequence diagram for the wait, because the point that matters is easy to state
and hard to see from the code: the resolver does not ask again, it arms a signal and
sleeps until a fact that could match it arrives.

Also spells out the Go mechanics, since they carry the guarantees rather than merely
implementing them. The registry is key to set-of-resolvers, which is what makes the
fan-out a join through an index rather than a broadcast: one resolver registers under
several keys, one per tier it could resolve through, and an applied fact wakes only the
resolvers under the keys it filled. The channel is struct{} with buffer 1 so a signal
sent while a resolver is mid-recheck survives; the send is non-blocking so the applying
goroutine is never slowed and the send is safe under the registry lock; the select
covers the waiter, the context and the deadline together; and unregister is deferred,
with a len() on the registry so a test can prove nothing leaks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: list the attribution metrics, the gaps, and the exact_user wart
927fb20 
Names every metric on the path with its labels and the question it answers, then says
what cannot be seen, because the honest half is the second one.

The gap that matters: a fact published and filed under nothing records nothing. `file`
matches no case, returns no keys, and `Apply` counts neither a drop nor a reason, so
`written` minus `matched` conflates "nobody needed it" with "nobody could ever have
joined it". That is exactly the population behind the window race — name-only delete
facts were published and discarded while no counter moved — so it would have been a
dashboard reading rather than a corpus reading. Two smaller gaps noted too: nothing
measures the queueing delay a slow resolution imposes on the events behind it, and the
publish-side tier distribution is not counted.

On exact_user versus exact_serviceaccount: it is a real wart. `result` names the TIER
everywhere else, so `exact` is the only value that also encodes who the actor was,
which means summing two series to count exact resolutions and no way at all to ask the
actor kind of a name or collection resolution. The decisive argument is that
commits_total already carries author_kind as its own label with those values, so the
two metrics disagree about the shape of one distinction.

Recommends result becoming tier-only with a sibling actor_kind label, and records why
it is not done here: it is a breaking metric change that deserves its own change and an
UPGRADING entry rather than arriving as a side effect of an attribution fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: propose a consolidated attribution metric surface
1e9676f 
Written because this release has already broken the `result` label:
exact_deletecollection_item is gone with the deletecollection rework, and `name` is
new. The usual reason to leave a metric label alone is that changing it breaks
consumers, and that cost is paid once per break — so finishing the job in the release
that is already breaking costs one migration where deferring it costs two, on a label
that will have been wrong twice. Nothing consumes these yet, which will not stay true.

Renames: result splits into tier and actor_kind, which is how commits_total already
models the same distinction; weak splits into latest and resource_version so the label
is exactly the tier ladder, one value per rung, rather than lumping the object's own
last write together with the rv-only hatch. fact_events_total becomes facts_total
because "events" already means two other things here, index_size names its unit, and
collection_degraded says what it counts instead of implying something broke.

Adds the three things nothing can currently see. A fact the index can file under no key
is discarded silently, so written-minus-matched conflates "nobody needed it" with
"nobody could ever have joined it" — the population the window race lived in for a
whole branch while no counter moved. The publish-side tier distribution, which is the
aggregated-API story and is currently only visible in a debugger. And the head-of-line
delay a slow resolve imposes on the events behind it, which is what actually broke a
spec while the wait histogram timed each resolution in isolation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: trace the fact-stream record's claims to metrics, and find seve…
3e2b9aa 
…n more gaps

Re-reading the shipped design against the metric surface finds seven operational
claims with nothing measuring them, two of which are questions that record leaves
explicitly open and says cannot be settled without data.

The two open ones are the reason this is worth doing rather than filing: the record
asks whether one entry per type per request is the right granularity, and notes that
nothing bounds one entry's size today, so a histogram of facts per entry is what
decides whether a ceiling is needed. It also asks whether the per-type stream count
stays reasonable, which a gauge of the subscription set answers from any real install.
Neither can be argued to a conclusion.

The one that would fail silently is the fact follower. When it fails it logs and
retries with a backoff and nothing counts it, so a follower that is flapping or wedged
degrades attribution to committer-authored across the board with a rising unresolved
rate as the only symptom and nothing pointing at the cause. A last-successful-read
timestamp is better than a counter there, because only "has not read in ten minutes" is
an outage.

Restart warmth is the one an HA decision rests on. "A restart costs nothing" depends on
replaying the retention window before the first event needs it, and nothing measures
whether that holds. The record's own last open question, whether a replica must warm
its index before taking over a watch, calls the ordering a small decision with a
visible effect; the effect is only visible with a metric.

Plus three cheap ones: TTL expiry is not counted where eviction is, so nothing says
whether the TTL or the cap is binding; the waiter registry already has a len() written
for tests that would export as live head-of-line pressure; and nothing says which
transport an install is running, though the two have different failure modes.

Notes what is already covered so nobody adds it twice: a failed XADD propagates out of
the audit handler as outcome="write_error".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: design the stream-count metrics so "reasonable" becomes measurable
bd2e0c1 
A count on its own cannot answer whether the per-type stream count stays reasonable,
because the same number is fine or fatal depending on what it costs. Four metrics
instead of one: the size, the pressure, the cost, and the churn.

Reading the follower settled what actually scales. The XREAD argument list is O(streams)
and is re-issued every block period whether or not anything is happening, but gap
detection is O(streams that are BEHIND) rather than O(streams), because detectGaps skips
targets that are caught up. So the set size alone never says the follower is struggling
and the number of streams behind does, before any facts are lost.

streams_followed is labelled by route and deliberately not by type: routes are bounded
by how many clusters an install mirrors, types are the unbounded thing being measured,
and a per-stream gauge would become the scaling problem it exists to detect. Splitting
by route also separates the two ways the number grows, which have different answers.
Placement is free, on the reference-counted set's existing change notification.

The read histogram needs its outcome label to be useful at all: Next blocks for the
whole period when idle, so an unlabelled histogram would be dominated by the idle case
and say nothing. Read the delivered bucket against the gauge and the question is one
query.

Read together the four distinguish three answers that look identical today: the count
grows harmlessly, the XREAD argument list is the cost and the fix is sharding readers,
or the follower cannot keep up and stream count is a symptom. Only the last is urgent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: design the three cheap metrics, and correct a wrong claim about…
ab497d9 
… one

Gives the expiry counter, the waiting gauge and the transport info gauge the same
treatment as the stream metrics: name, type, labels, placement, and what each
distinguishes.

One of them was described wrongly the first time and the correction is the reason this
was worth doing rather than copying the sketch. I wrote that factWaiterRegistry.len()
could be exported directly as "resolvers waiting". It cannot: len() returns the number
of candidate KEYS holding a waiter, and one resolver registers under several keys, one
per tier its query could resolve through. So it over-counts blocked resolvers by roughly
the tier fan-out, by a factor that varies with which tiers each query supports, and it
would have been wrong in the way that never gets caught — it moves in the right
direction. What is wanted is a count of live waiters kept in register and unregister,
which already hold the lock.

TTL expiry gets the pairing that makes it useful: against evictions it says which
constraint is binding, and the shape where both are near zero on a busy install is a
stalled follower rather than a healthy one.

Resolvers-waiting gets the reading that keeps it from being alarming: a blocked resolver
is expected, since the whole design assumes the fact is late. The number that matters is
the sustained level against the active shard count, because at that point every shard is
blocked.

Transport info gets the argument for why an info gauge is not decoration here: the same
symptom means different things under each transport, and a burst of unresolved commits
after a restart is expected under the in-memory one and a bug under Redis.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib @claude
docs: cut the metrics proposal to a first release, and record what it…
f3f452c 
… got wrong

Review was right on every checkable point, and four of them were verified against the
code rather than accepted. The draft expanded to thirteen new families at once and
several of its signals would have produced misleading diagnoses.

published minus filed is not delivery loss: published counts every fact appended for
every type while filed would count only facts on streams this process follows, and
replay re-files the same facts, so the second number can exceed the first with nothing
wrong. Two counters over different populations do not subtract.

unfilable would have been a permanently zero series. The publish gate rejects an event
with no resolvable name unless it is a collection verb, so every fact reaching the index
has a name, a uid, an rv, or is a collection fact, and with the name tier each of those
files somewhere. The default branch is unreachable. The aggregated create the draft
pointed at never becomes a fact at all, which is why the replacement counts it on the
ingestion side, as a bounded no_attribution_fact outcome on the existing frozen audit
vocabulary.

The named failure metric does not exist: write_error is a value on audit_events_total,
per event, not on the request-level audit_eventlist_* families. An alert on the drafted
name would have reported zero forever.

facts_filed_total{tier} conflates two models, because a uid+rv fact files under both
exact and latest, so tiers do not partition facts. resolvers_waiting would count
registrations rather than blocked resolvers, since Await registers before its first
lookup and most resolutions never block. streams_behind means "the last read filled its
entry budget", not a backlog depth. replay_seconds has no boundary to measure, because
nothing gates serving on a warm index.

The review also found the silent drop the "every silent drop gets a counter" principle
should have caught first: both transports discard an undecodable stream entry and
advance the cursor past it with no log and no metric. Unlike a trim gap it cannot be
detected afterwards, and unlike a publish failure the API server does not retry it. That
counter is now in the first release.

Also records that metrics-observability-plan.md calls itself canonical while specifying
result values and fact ops that do not exist and describing the index as living in
Redis. It drifted when the fact stream landed, so this proposal has to update it rather
than sit beside it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sunib
feat(metrics)!: split the attribution result label into tier and …
2de2904 
…`actor_kind`

`result` crammed two orthogonal questions into one value — which evidence
answered, and who it named — and hid a third. Counting exact resolutions meant
summing `exact_user` and `exact_serviceaccount`, and the actor kind could not be
asked of any other tier at all, while `commits_total{author_kind}` one metric
over already modelled the same distinction correctly.

So `result` is gone. `attribution_resolutions_total` carries `tier` plus
`actor_kind` (`user`/`serviceaccount`/`none`, the `commits_total` vocabulary),
and the wait histogram carries `tier` plus `event_kind`. The label sets differ on
purpose: the actor kind is a property of the answer and belongs on the census,
while the event kind only changes what a wait MEANS, and putting both on both
would multiply a histogram that already carries the type triple for no reading
anyone would make.

`weak` splits into `latest` and `resource_version`, in the enum rather than at
the metric boundary. They are different evidence — the object's own last write
against a fact that carried a resourceVersion and no uid — the removal path turns
on `latest` specifically, and a lookup that cannot say which tier answered is the
thing this change exists to fix. `AttributionResult` is now the tier vocabulary
itself, so the resolver records the branch it actually took.

`event_kind` comes from `ExactCapable`: a removal holds a fallback and keeps
waiting for evidence about the deletion where a write does not, so an absent
write and an absent removal used to be one series. The removal wait is the number
anyone tuning --author-attribution-grace actually needs, and the measurement that
found the window race had to infer it from the shape of a distribution.

Four renames ride along while the surface is moving: `attribution_facts_total`,
`attribution_fact_index_entries`, and
`attribution_collection_without_uidset_total`.

Nothing consumes these metrics yet, so the break costs one UPGRADING entry today
against a consumer migration after the first published dashboard.

Note for review: architecture.md, interpreting-metrics.md and the metrics
proposal here also carry docs edits that were already uncommitted in the working
tree when this landed (the fact-stream transport reconciliation). They could not
be separated cleanly, because the same paragraphs are edited by this change.
@sunib
feat(metrics): count the attribution loss paths that were silent
ad6d0a8 
Three ways a fact or an event could be lost with no counter, and one piece of
metadata that decides how any of them read.

**Undecodable stream entries.** Both transports did the same thing with an entry
they cannot decode: `continue`, then advance the cursor past it. That is the
right call — it can never decode, and stalling the follower on it would cost
every later fact on that stream — but it made this the one loss path with NO
symptom at all: unlike a trim gap it is not detectable after the fact, and unlike
a publish failure the API server does not retry it.
`attribution_fact_stream_decode_errors_total{transport}` and a log line are the
whole fix, recorded in the shared decode path so neither transport can drift.

**Follower health.** `FactIndex.Run` logs a transport failure and retries with a
backoff, because a follower that returned would leave attribution dead for the
life of the process. Nothing counted it, so a wedged follower degraded
attribution to committer-authored cluster-wide with a rising unresolved rate as
the only symptom. The last-success timestamp matters more than the counter: only
it separates "erroring occasionally while making progress" from "has read nothing
in ten minutes", and only the second is an outage. Idle rounds count as success,
so a quiet cluster does not read as a wedged follower.

**`no_attribution_fact` on `audit_events_total`.** An accepted event that yields
no fact was counted `queued`, which claimed an append that was never owed and
buried the whole unattributable population under the busiest value on the
counter. It is `Dropped`, not `Error` — nothing failed, and the e2e invariant
gating on `category="error"` stays intact. The handler is the only place it can
be counted: the aggregated-API create is rejected before publication, so no
fact-side counter can ever see it.

**`attribution_transport_info{transport}`**, an info gauge recorded by the
follower, which is the one goroutine that runs for the life of the process and
holds the transport. It is interpretive metadata rather than a signal: a burst of
unresolved commits after a restart is expected under the in-process transport and
a bug under Redis, so reading any metric above without it is reading it without
knowing the contract.

`FactFollower` gains `TransportKind()` for the label — on the follower half
because the index is what records follower health, and a counter that cannot say
which transport erred says half of what an operator needs.

Note for review: metrics-observability-plan.md and INDEX.md here also carry docs
edits that were already uncommitted in the working tree when this landed, for the
same reason as the preceding commit.
@sunib
docs(metrics): make the follower alert fire when it has NEVER succeeded
f66d28a 
Three corrections from review, all in the reading rather than the recording.

The wedged-follower alert could not fire for the outage it exists for. The
last-success gauge is not published until the follower's first successful read,
so `time() - <gauge> > 600` returns NO SERIES for a follower wedged since
startup — a transport unreachable at boot — rather than a large number. That is
the same failure the proposal already records as the worst kind a monitoring
change can have: an expression that reports nothing forever. The alert now carries
a second arm on the gauge's ABSENCE while `attribution_transport_info` says a
follower is running, with `for: 10m` so an ordinary restart's gap does not trip
it. The gauge itself is left honest: stamping it at start would drop the arm at
the cost of claiming a success that never happened, which reads as health through
the first ten minutes of every outage.

`actor_kind="none"` on a matched tier was undocumented rather than possible. An
audit event whose user cannot be resolved never becomes a fact — the publish gate
refuses it, and it is counted `no_attribution_fact` — so every fact that reaches
the index names someone, which is why coverage can be read off the tier alone.
Said out loud in the field guide, and pinned by a metric assertion on the
resolver's defensive branch: if such a fact ever did arrive, it is recorded with
its tier and `actor_kind="none"`, not as a named actor.

Two phase-boundary contradictions: the proposal still listed
`watch_event_queue_seconds` as shipped while its own banner and the canonical plan
put it in Phase 2 (the argument for it is kept, the claim is not), and the plan's
§9 preamble had every phase shipping dashboard panels and alerts while Phase 4
holds both. Phases 1-3 ship instruments, tests, and field-guide rows; the §8 alert
sketches are the specification Phase 4's rules are written from.
@sunib
feat(attribution): make "a fact names somebody" a wire contract, not …
e9aaefc 
…a hope

`author` is the one required field on an attribution fact: a fact exists to name
an actor, so a fact that names nobody is not a weak fact, it is not a fact.
`AuthorFact.UnmarshalJSON` now says so — missing, `null`, and `""` are one
violation and all three are refused.

Go cannot state this as a type. Every type has a zero value that is
constructible without going through any constructor, and `encoding/json` writes
exported fields straight past one anyway, so there is no "string of at least one
character" to declare. The constraint has to live at a boundary, and the decode
path is the boundary that matters: the publish gate already refuses an event
whose user cannot be resolved, but it only speaks for facts THIS operator wrote.
The read side now holds the same line for facts anybody wrote.

The refusal is at ENTRY granularity, deliberately. An entry carrying an
authorless fact came from a different version, a different producer, or a hand
edit — that is a protocol violation rather than a low-quality fact, and it is
better counted and logged loudly, which the decode-error counter shipped for,
than half-absorbed by dropping one fact out of a batch and keeping the rest.

What this buys the metric surface: every fact reaching the index names someone,
so `{tier!="absent", actor_kind="none"}` is now impossible rather than merely
unlikely, and reading attribution coverage off the tier alone is sound. The
resolver's authorless branch stays for the zero-value paths and keeps recording
`actor_kind="none"` rather than counting a named actor.
@sunib
fix(chart): let attribution.transport=memory actually run without Redis
b08fd23 
The chart documents `transport: memory` as the way a single-pod install runs
attribution with no Valkey at all, and then refused to render it: the guard
failed on `attribution.enabled=true` with an empty `queue.redis.addr` whatever
the transport was. The binary has the narrower rule — Redis is required for the
transport that uses it — so the chart was refusing a configuration the operator
supports, which made a documented value unreachable.

The guard now matches the binary. The memory transport's other requirement, a
single replica, needs no check here: validate-replica-count.yaml already refuses
`replicaCount > 1` for the whole chart, so a second condition would be one that
can never fire.

Also in this change, from the review and from reading UPGRADING against what
actually lands:

- UPGRADING gains an entry for the transport switchover itself. It is what a
  reader upgrading INTO 0.41.0 needs and it was missing entirely: the transport
  table, the default that needs no action, the facts-in-flight window (the v1
  keyspace is gone and the streams are a new keyspace, not a migration of it),
  and the new index/collection knobs.
- The metric entry now says the decode-error counter also covers the fact
  contract, and that the follower liveness alert needs both arms.
- attribution-removal-wait-options.md described the name tier as a proposal; it
  shipped in this branch, and its table said an aggregated update or single
  delete is unattributable, which is no longer true.
- attribution-branch-findings.md said three removals "each waited a ten-second
  grace" against its own table's 20.18s total, and its measurements are labelled
  in the pre-rename vocabulary. Both corrected in place; the measurement stands,
  only the names moved.
@sunib sunib changed the title feat(attribution): switch the resolver to the fact index and delete the v1 keyspace feat(attribution)!: switch the resolver to the fact index and relabel the metric surface Jul 29, 2026
@sunib
test(controller): widen the encryption-secret recreation budget to 3.…
930f25f 
…5 intervals

The spec waits for a controller to recreate a deleted age-key Secret, and that
recreation rides a 10s PERIODIC requeue rather than a watch — the controller
deliberately runs no control-plane Secret informer, so nothing enqueues the
GitTarget when the Secret disappears. The budget was `2*RequeueStreamSettleInterval
+ timeout`, exactly 30s, which covers two ticks and nothing else; a CI runner busy
enough to drop one failed the spec on timing alone, twice, months apart.

It is now 3.5 intervals plus the timeout, 45s. The extra room is free on every run
that was going to pass — Eventually returns as soon as the Secret is back — and is
only ever spent by a run that was going to fail anyway.

It stays a bound rather than a generous number, because the budget is this spec's
implicit SLO: recreation must still happen within about four ticks of the
deletion, so a regression that makes the requeue path slow rather than broken
still fails here.
@sunib
sunib merged commit 928df2d into main Jul 29, 2026
19 checks passed
@sunib
sunib deleted the feat/attribution-fact-switchover branch July 29, 2026 18:13
@coderabbitai coderabbitai Bot mentioned this pull request Jul 29, 2026
Merged
@github-actions github-actions Bot mentioned this pull request Jul 28, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sunib