feat: make unresolved attribution explicit and harden operations - #257
Merged
sunib merged 17 commits intoJul 20, 2026
Merged
Conversation
`ci.yml` triggered on `pull_request: branches: [main]`, so a stacked PR — one based on the previous PR's branch rather than on main — never matched and ran no validation at all. PR #255 (PR 2 of the WatchRule source-namespace stack) merged having run only CodeRabbit and GitGuardian: no lint, no unit tests, no e2e. Retargeting does not rescue it. When the base branch merges, GitHub retargets the still-open PR onto main and emits `pull_request` with action `edited`, which is not in the default trigger set (opened/synchronize/reopened), so nothing re-runs. The PR is then mergeable into main having never been validated by this pipeline. Drop the `branches` filter so validation follows the pull request rather than its base. This costs some runner time on stacked PRs and is what makes a stack reviewable one PR at a time — the workstream in docs/design/watchrule-source-namespace/ is five stacked PRs, so without it four of the five would merge unvalidated. The trust model is unchanged: `pull_request` still grants no secrets and no registry writes, and GitHub independently forces a read-only token on fork PRs. docs/ci-overview.md already documents the trigger as `pull_request`, so this makes the workflow match the documented design. Verified with actionlint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`configuredAuthorModeEnabled` decided which commit-author assertion applies by
shelling out to `kubectl logs deployment/gitops-reverser --since=30m` and
grepping for a startup banner, returning false on any kubectl error. That is a
probe that fails OPEN on the exact question the assertion turns on, in at least
three ways:
- a controller up longer than 30 minutes has its banner age out of the window;
- `kubectl logs deployment/...` can error or pick the wrong pod mid-rollout;
- a spec that redeploys the controller in configured-author mode (the
quickstart specs do) leaves the banner in the shared log window, so a later
spec reads a mode that is no longer current.
It matters more than a normal flaky helper because both author values are
ambiguous in isolation: "GitOps Reverser" is BOTH the configured-author identity
AND the fallback used when attribution finds no matching audit fact
(git.DefaultCommitterName). A wrong probe therefore silently swaps the
assertion for a weaker one that passes, instead of failing.
Read the Deployment's container args, which are the actual input to the mode
switch in cmd/main.go, and fail loudly when they cannot be read. The decision is
factored into a pure `configuredAuthorModeFromArgs` with unit coverage, so the
default-handling is pinned: cmd/main.go defaults BOTH `--author-attribution`
(true) and `--redis-addr` ("valkey:6379") to enabled, so an absent flag means
attribution mode and only an explicit opt-out selects configured-author mode.
Found while investigating an attribution failure on PR 2; see
docs/design/watchrule-source-namespace/pr2-e2e-attribution-investigation.md.
This is NOT the cause of that failure — the deployment there was genuinely in
attribution mode and the probe genuinely returned the right answer — but it made
the failure take far longer to interpret than it should have.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR introduces explicit attribution outcomes across watch, controller, CommitRequest, and Git flows; adds unresolved-author diagnostics and documentation; creates a dedicated E2E Taskfile; paginates Gitea listings; and updates CI, tooling, configuration, and repository references. ChangesAttribution outcome flow
E2E infrastructure
Gitea pagination
Repository maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/author_mode_test.go (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename test to follow the required naming convention.
As per coding guidelines, tests must use the naming convention
TestFunctionName_Scenario(t *testing.T). Since this is a table-driven test covering multiple cases, consider appending_TableDrivenor_AllScenariosto the function name to reflect the scenario being covered.♻️ Proposed refactor
-func TestConfiguredAuthorModeFromArgs(t *testing.T) { +func TestConfiguredAuthorModeFromArgs_AllScenarios(t *testing.T) {🤖 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/e2e/author_mode_test.go` at line 11, Rename TestConfiguredAuthorModeFromArgs to follow the required TestFunctionName_Scenario convention, using a suffix such as TableDriven or AllScenarios to indicate its table-driven coverage; update only the test function name and any references to it.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.
Nitpick comments:
In `@test/e2e/author_mode_test.go`:
- Line 11: Rename TestConfiguredAuthorModeFromArgs to follow the required
TestFunctionName_Scenario convention, using a suffix such as TableDriven or
AllScenarios to indicate its table-driven coverage; update only the test
function name and any references to it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 93eecab1-ae7b-40e7-9a58-d39b491bc0d6
📒 Files selected for processing (3)
.github/workflows/ci.ymltest/e2e/author_mode_test.gotest/e2e/e2e_suite_test.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Two ergonomics fixes for footguns that each cost a real debugging cycle in the session that landed PR 2. Rename test/e2e/Taskfile.yml to Taskfile-e2e.yml. Every path in that file is relative to the REPO ROOT, because the root Taskfile includes it with `dir: .`. While it was named Taskfile.yml, running `task` from inside test/e2e/ stopped Task's upward search there and executed with test/e2e/ as the working directory, so every one of those paths silently resolved somewhere that does not exist. The observed damage: `task clean-cluster` deleted the k3d cluster (cwd-independent) while its `rm -rf .stamps/cluster/<ctx>` hit a nonexistent relative path, leaving a stale `ready` stamp that made the next `prepare-e2e` skip cluster creation and fail with "No nodes found for given cluster". With no Taskfile.yml in test/e2e/, the search always reaches the repo root and the working directory is right no matter where the command is typed. docs/tasks-overview.md records why, so it does not get renamed back. Add a PreToolUse hook refusing `task … | …` when pipefail is unset. A pipeline reports the LAST command's status, so `task prepare-e2e | tail -15 && task test-e2e` reported success for a failed prepare and ran the suite against a cluster that was never created — surfacing much later as an unrelated-looking SynchronizedBeforeSuite failure. The root Taskfile sets pipefail for commands inside tasks, which does nothing for a pipeline typed at the shell. .gitignore un-ignores .claude/hooks/ because the tracked settings.json points at the hook: ignoring it would leave everyone else with a hook reference to a file they do not have, failing every Bash call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…igation
An e2e failure while landing PR 2 — a commit that should have been authored
jane@acme.com was authored "GitOps Reverser" — took far too long to diagnose, and
was wrongly blamed on PR 2 three separate times. This adds the instrumentation
that settled it and writes down the answer.
Findings, measured rather than argued:
- Attribution silently loses ~10% of actors under parallel load. The resolver
waits DefaultAttributionGraceWindow (3s) for a matching audit fact and then
commits as the configured committer. Measured fact-delivery lag on an IDLE
cluster is 835-1101ms, bimodal — the signature of the apiserver's
--audit-webhook-batch-max-wait=1s. So a 3s budget runs against a ~1s fixed
batching delay with ~2s of headroom, and under load the tail crosses it.
Resolver wait by result: weak 0.001s, exact_user 0.533s, absent 2.268s.
- PR 2 is NOT the cause. Attribution loss rates over ~85 resolutions per arm are
8.0% with PR 2 and 9.6% without; configmaps alone lose 10.0% on baseline. The
spec is ~10%-flaky on its own, which no three-sample comparison could separate
from an effect.
- The loss is invisible by construction: git.DefaultCommitterName ("GitOps
Reverser") is ALSO the configured-author identity, so a lost-actor commit is
indistinguishable from a correct configured-author commit by inspection.
targetWatchSpecs now names every declared stream as
"<gvr>@<namespace|*cluster-wide*>=<ops>" instead of logging a bare count. A GVR
appearing twice for one GitTarget means the same object is delivered on two
streams — legitimate scoping after PR 2, but invisible in a count. This is how
the real case-C fan-out was found (unsupported-folder-dest carries both a
cluster-wide and a named configmaps stream).
hack/attribution-diagnostics.sh answers the question that splits the diagnosis:
is the fact ABSENT from Redis (never delivered) or PRESENT but written too late
for the grace window? Fact keys carry a 10-minute TTL, so the remaining TTL
back-computes when each landed.
docs/design/watchrule-source-namespace/ gains the resolution, the full session
log including the reversals, and a watch-construction overview answering how
watches are built and at what cardinality (one raw watch per
(GitTarget, GVR, namespace) — no reuse across GitTargets).
The grace-window fix itself is deliberately NOT in this commit: it is a product
behaviour change (a write with no audit fact would wait longer before
committing) and deserves its own review.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three sprawling working documents in design/watchrule-source-namespace/ replaced by one condensed page, plus the watch-construction reference moved alongside it. The originals grew by accretion during the investigation and carried several conclusions that were later overturned; keeping them would mean keeping three versions of the same story with no signal about which is current. docs/debug/ is a new lifecycle folder, registered in INDEX.md: "this is wrong now" — open investigations into live defects, retired to finished/ once fixed. It sits between design/ (still deciding) and finished/ (already happened), which had no home for a known-broken behaviour someone must not re-investigate from scratch. attribution-loss.md states the defect, how far the 7-10% figure can be trusted and why, and then two tables that are the point of the exercise: what was RULED OUT with the evidence that killed each hypothesis (PR 2, late fact arrival, watermark gating, key shape, webhook routing, audit policy, replay events, load, "just flaky"), and what was FIXED along the way (CI trigger, author-mode probe, Taskfile rename, task-pipe hook, per-stream logging, the after-suite report, the histogram le-label bug). The ruled-out table is the part worth keeping. Every row cost a cycle, and several were things I asserted confidently before measuring. Also corrects the grace-window comment in config/deployment.yaml: raising 3s to 10s left the loss rate unchanged, so it buys headroom and observability, NOT a fix. And fixes the after-suite report's bucket queries — Prometheus renders le as "1.0"/"3.0"/"10.0", so %g matched nothing above 0.5 and produced a plausible but entirely wrong distribution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When attribution is enabled but no audit fact matches, the commit is authored by the configured committer — the same "GitOps Reverser" identity that every commit carries in configured-author mode. So a silent attribution failure is byte-identical in Git history to correct configured-author behaviour, and the only way to see the ~7-10% loss is a Prometheus counter nobody looks at. That ambiguity is what made the loss so slow to diagnose (docs/debug/ attribution-loss.md). Proposes authoring those commits as `unattributed <unattributed@gitops-reverser.invalid>` while leaving the committer as the operator — using git's author/committer split for exactly what it is for: the operator really did make the commit, and the honest answer to "on whose behalf" is "we do not know". Configured-author mode is untouched, since there the committer genuinely is the author. Checked the interaction most likely to make this risky: commit-window grouping matches on UserInfo.Username, and today unattributed events share Username == "" so they already group with each other and not with attributed ones. A sentinel renames that equivalence class without splitting or merging any window. Does NOT fix the underlying loss — makes it legible, and worth doing first because it turns an invisible correctness failure into one visible in `git log`, and gives the open investigation a labelled instance tied to an object and time rather than a counter increment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… "unattributed"
Two revisions after review.
No flag. The earlier draft proposed --author-attribution-placeholder defaulting
on. The only thing such a flag can do is restore silently mislabelled
authorship, which is not a preference to preserve behind an opt-out — it is the
bug. Attribution is already gated by --author-attribution: an operator who does
not want actor names turns that off and gets configured-author mode, where the
committer genuinely is the author. A second flag would only add an incoherent
third state: attribution on, but lie about it when it fails.
"unattributed" was too passive — it reads as "this operator does not do
attribution", which is exactly the confusion with configured-author mode the
change exists to remove. The reader must be able to tell the operator TRIED and
failed, because that is a defect to investigate rather than a configuration they
chose.
Settled on three distinct strings, because UserInfo feeds three places and one
string does not serve them:
Username attribution-failed (window grouping key AND
the grouped commit MESSAGE
body — must stand alone
there, where a bare
"unknown" says nothing)
DisplayName unknown (attribution failed) (the git author Name a
human reads; leads with
what they care about,
follows with why)
Email attribution-failed@gitops-reverser.invalid
Verified parentheses and spaces pass isSafeSignatureField, so the display form
is valid in a commit object and in a signed payload.
With no opt-out, adopters WILL see these commits appear, so the release note is
now mandatory rather than advisable, and must point at the metric and at
docs/debug/attribution-loss.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… string
Revised after review. Every point raised was verified against the code and every
one held; two would have shipped silent regressions.
The structural change: stop encoding attribution outcome in the author string.
Add an explicit AttributionOutcome (not_attempted | resolved | unresolved) on the
event and pending write, and let the git author identity, the author_kind metric,
and CommitRequest window matching all READ it. The first draft inferred outcome
from UserInfo.Username, which is load-bearing in three places it did not account
for.
HIGH — CommitRequest attachment. The draft claimed this was untouched; it is not.
A fallback CommitRequest attaches with Author == "" and matchesWindow compares
the author string exactly, so today it coincides with an unattributed window's
empty author and attaches. A naive sentinel breaks that coincidence and the
request silently lands as its own commit. Now matched on outcome, which also
turns today's accidental match-on-empty-string into stated behaviour. Integration
test required, not just a unit test.
HIGH — author_kind. authorKind() classifies by string: not empty and not a
serviceaccount prefix means "user". A sentinel username would therefore be
counted as a real named actor, so failed attribution would render in dashboards
as IMPROVED user attribution — actively masking the defect rather than merely
staying silent. Adds `unresolved` as a fourth kind, driven by the outcome, with
metric docs and dashboards updated in the same change.
"failed" -> "unresolved". AttributionAbsent collapses no-fact-existed (correct
behaviour), cancelled waits, Redis read errors and malformed values —
matchFactKey discards the error deliberately. "Failed" asserts a fault we cannot
prove. "Unresolved" says we tried and did not arrive at an actor, which is all
the evidence supports; a genuine `failed` state can be split out later if the
index grows typed outcomes.
Resolver contract. ResolveAuthor documents ok=false as "commit as the configured
committer". Returning a placeholder instead would overturn that contract while
leaving its documentation intact — the exact failure mode this investigation was
caused by. The signature returns the outcome explicitly instead, which also
removes the unsound "non-nil resolver means attribution is configured" inference.
Documentation inventory. architecture.md ("the author identity is never
invented"), configuration.md, attribution-setup-guide.md, interpreting-metrics.md
and UPGRADING.md all promise the committer fallback and must move together. Also
notes Username reaches user-authored per-event templates, not only grouped
messages, so that surface needs compatibility coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…authors in Git
Attribution failed silently. When it ran and found no fact, the commit was
authored by the configured committer — the same "GitOps Reverser" identity used
in configured-author mode — so a lost actor was byte-identical in Git history to
a deployment that simply does not do attribution. The only way to see the ~7-10%
loss was a Prometheus counter nobody reads. Implements
docs/design/attribution-outcome-and-author.md.
The change is NOT just a sentinel string. Attribution outcome is now an explicit
value carried on Event and PendingWrite:
not_attempted configured-author mode; the committer legitimately IS the author
resolved a fact named the actor
unresolved attribution ran and did not arrive at an actor
Three consumers read the outcome instead of inferring it from the author string,
which is load-bearing in places a naive sentinel would have broken:
- Commit author. `unresolved` renders
`unknown (attribution unresolved) <attribution-unresolved@gitops-reverser.invalid>`
while the committer stays the operator — git's author/committer split used for
exactly what it is for. RFC 2606 reserves .invalid, so it can never collide
with a real address.
- author_kind gains `unresolved`. Classifying it as "user" would have made a lost
actor indistinguishable from a named one, so a DEGRADING attribution path would
have read in dashboards as an improving one.
- CommitRequest attachment matches on outcome. A fallback CommitRequest carries an
empty author, which today coincides with an unattributed window's empty author
and attaches by accident; once the window carries the sentinel those strings
differ and the request would have silently landed as its own commit. Matching on
the outcome keeps them together and makes the old accident explicit.
ResolveAuthor returns the outcome rather than an ok bool whose documented meaning
("commit as the configured committer") the placeholder would have overturned
while leaving the doc intact — the exact class of mismatch that made this
investigation slow. It also removes the unsound "non-nil resolver means
attribution is configured" inference.
Two defects found while implementing, neither in the review:
- Adding attributionNotAttempted left setCommitRequestAttributed's switch
unhandled, so the AuthorAttributed condition stopped being set at all.
- The existing attributionCommitter message claimed "the validate-operator-types
webhook is not configured", which is now only true for not-attempted. A
configured-but-missing webhook would have sent operators to the wrong place.
Both messages are now accurate.
Docs moved together, as they must: architecture.md ("the author identity is never
invented" -> never fabricated as a PERSON, with both no-actor cases spelled out),
configuration.md, attribution-setup-guide.md, interpreting-metrics.md.
Verified: e2e attribution report shows the split working — 3 resolutions
succeeded after waiting longer than the 3s default grace, and absent waits now
separate fast returns (a fact with no author) from grace-ceiling ones (no fact
ever arrived), which the old combined view could not distinguish.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ListUserKeys issued GET /users/{login}/keys with no pagination, so it saw at
most one page. Verified against both sides:
- Server: GetListOptions does PageSize: ToCorrectPageSize(limit), and
ToCorrectPageSize clamps `size <= 0` back to DefaultPagingNum — 30 by default,
MaxResponseItems 50. There is NO "return everything" mode for this endpoint.
(gitea modules/setting/api.go, services/convert/utils.go,
routers/api/v1/utils/page.go)
- SDK, for the record: code.gitea.io/sdk documents ListOptions.Page = -1 as
"disables pagination", but it merely sends page=0&limit=0, which the server
clamps straight back to 30. So adopting the SDK would NOT have avoided this;
its escape hatch is ineffective on this endpoint and actively misleading.
The consequence is not a truncated listing but a wedged workflow: this listing
backs FindUserKeyByTitle, which EnsureUserKeyAsAdmin uses to decide whether a
title is taken. Past one page, "ensure" cannot see the key it needs to replace,
so it tries to create and fails `422 Key title has been used` for a key that
demonstrably exists.
The e2e suite hits this deterministically. It registers ~24 seeded transport
keys per full run against a Gitea that outlives the run, so from the second run
onward the admin user holds more than one page. The long-lived, stable-titled
`playground` key then becomes unreachable and the playground spec fails at
BeforeAll — which is why it only ever appeared after several runs on one cluster,
and why `task clean-cluster` "fixed" it: that resets Gitea to zero keys rather
than fixing anything.
Requests limit=50 (MaxResponseItems) to keep round trips low, and stops on the
first short page. An empty page also terminates, guarding against a server that
ignores the limit.
Verified: both new tests fail against the unpaginated implementation — one for a
title on the last of three pages, one for a listing that exactly fills a page.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
resolveSnapshotGVRs, resolveSnapshotGVRForType, snapshotGVRsFromTable, snapshotGVRScopes and the snapshotGVR type had no production callers — only each other and their tests. The desired set is no longer gathered through this path; it is the spliced materialization, so scope_resolve.go kept a parallel, never-executed projection of the same watched-type table. Found while implementing PR 2, where it caused real cost: the change had to be applied to BOTH read sites and verified at both, and one of PR 2's three regression tests asserted this projection specifically. That test goes with the code, which is the honest outcome — with the path gone there is exactly ONE consumer of WatchedType.WatchScopes (targetWatchSpecs), so the "both read sites must agree" invariant PR 2 was written to defend no longer has two sides to disagree. Kept: retainedWatchedTypes, typeWobbling, gvkListSummary and desiredFromObject, which are all live from target_watch.go and the splice. 243 lines removed. No behaviour change — nothing called any of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/giteaclient/keys.go (1)
43-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a hard cap on pagination iterations.
The loop only terminates on a short/empty page; if Gitea ever returns full pages indefinitely (bug, misconfiguration, or API change), this loops without bound, growing
allunboundedly untilctxis cancelled elsewhere (if at all). Notably, the test harness added inkeys_test.gohad to add its ownrequests > 5guard against this exact failure mode, but production code has no equivalent safeguard.A small max-page cap keeps the fix cheap while eliminating the unbounded-growth risk entirely.
🛡️ Proposed fix
+// maxKeyPages bounds ListUserKeys against a misbehaving server that never returns a short page. +const maxKeyPages = 1000 func (c *Client) ListUserKeys(ctx context.Context, login string) ([]PublicKey, error) { var all []PublicKey base := "/users/" + PathEscape(login) + "/keys" - for page := 1; ; page++ { + for page := 1; page <= maxKeyPages; page++ { var keys []PublicKey path := fmt.Sprintf("%s?page=%d&limit=%d", base, page, keyPageSize) code, raw, err := c.Do(ctx, http.MethodGet, path, nil, &keys) if err != nil { return nil, err } if code != http.StatusOK { return nil, unexpectedStatus(http.MethodGet, path, code, raw) } all = append(all, keys...) if len(keys) < keyPageSize { return all, nil } } + return nil, fmt.Errorf("ListUserKeys: exceeded %d pages for %q", maxKeyPages, login) }🤖 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/giteaclient/keys.go` around lines 43 - 62, Update ListUserKeys to enforce a small maximum pagination-page limit in addition to the existing short-page termination condition. Stop and return an appropriate error once the cap is reached, preventing unbounded requests and growth of all while preserving normal aggregation for pages within the limit.internal/controller/commitrequest_finalize.go (1)
134-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a distinct
Reasonfor "capture disabled" vs "capture ran and found nothing."Both
attributionCommitterandattributionNotAttemptedsetcrReasonCommitterFallback; only theMessagetext differs. SinceReasonis the field consumers typically filter/alert on (not free-textMessage), this collapses two operationally distinct causes back together — the same conflation this PR is otherwise removing.♻️ Proposed fix: add a dedicated reason for the disabled-capture case
case attributionNotAttempted: setCommitRequestCondition(cr, ConditionTypeAuthorAttributed, metav1.ConditionFalse, - crReasonCommitterFallback, + crReasonCommitterFallback, // or a new crReasonCaptureDisabled, if downstream tooling should distinguish "command-author capture is disabled (the validate-operator-types webhook is not "+ "configured); committed as the configured committer")🤖 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/controller/commitrequest_finalize.go` around lines 134 - 145, Introduce a distinct reason constant for the attributionNotAttempted (capture disabled) case and use it in that setCommitRequestCondition call, while retaining crReasonCommitterFallback for attributionCommitter (capture ran but found nothing). Keep the existing messages and condition type/status unchanged.
🤖 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 `@hack/attribution-diagnostics.sh`:
- Line 48: Update the Python formatting expression around the TOTAL output to
assign the literal label to a Python variable first, then reference that
variable in the f-string. Preserve the existing spacing and total formatting
while avoiding single quotes inside the Bash single-quoted python3 -c block.
In `@internal/controller/commitrequest_finalize.go`:
- Around line 60-81: Update commitRequestAttribution and its gitOutcome method
so the enum’s zero value cannot map to git.AttributionResolved: add an explicit
unset/invalid value before attributionFromAdmission, or handle the zero value as
git.AttributionNotAttempted. Preserve the existing mappings for
attributionFromAdmission, attributionCommitter, and attributionNotAttempted.
---
Nitpick comments:
In `@internal/controller/commitrequest_finalize.go`:
- Around line 134-145: Introduce a distinct reason constant for the
attributionNotAttempted (capture disabled) case and use it in that
setCommitRequestCondition call, while retaining crReasonCommitterFallback for
attributionCommitter (capture ran but found nothing). Keep the existing messages
and condition type/status unchanged.
In `@internal/giteaclient/keys.go`:
- Around line 43-62: Update ListUserKeys to enforce a small maximum
pagination-page limit in addition to the existing short-page termination
condition. Stop and return an appropriate error once the cap is reached,
preventing unbounded requests and growth of all while preserving normal
aggregation for pages within the limit.
🪄 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: 76bef3ea-9ec1-4eba-97e4-70a0f6cc0ad5
📒 Files selected for processing (42)
.claude/hooks/block-unguarded-task-pipe.sh.claude/settings.json.coverage-baseline.gitignoreTaskfile.ymlconfig/deployment.yamldocs/INDEX.mddocs/architecture.mddocs/attribution-setup-guide.mddocs/ci-overview.mddocs/configuration.mddocs/debug/README.mddocs/debug/attribution-loss.mddocs/debug/watch-construction.mddocs/design/attribution-outcome-and-author.mddocs/design/support-boundary/finished/higher-level-krm-documents.mddocs/interpreting-metrics.mddocs/spec/e2e-bi-directional-corner.mddocs/tasks-overview.mdhack/attribution-diagnostics.shinternal/controller/commitrequest_controller.gointernal/controller/commitrequest_finalize.gointernal/git/attribution_outcome_test.gointernal/git/commit.gointernal/git/commit_request_attach.gointernal/git/commit_request_attach_loop.gointernal/git/open_window.gointernal/git/pending_writes.gointernal/git/types.gointernal/giteaclient/keys.gointernal/giteaclient/keys_test.gointernal/watch/author_resolver.gointernal/watch/author_resolver_test.gointernal/watch/manager_snapshot_test.gointernal/watch/scope_resolve.gointernal/watch/scope_resolve_test.gointernal/watch/snapshot_stream_type_test.gointernal/watch/target_watch.gotest/e2e/Taskfile-e2e.ymltest/e2e/e2e_suite_test.gotest/e2e/setup/argocd/README.mdtest/e2e/setup/argocd/values.yaml
💤 Files with no reviewable changes (3)
- internal/watch/snapshot_stream_type_test.go
- internal/watch/scope_resolve_test.go
- internal/watch/scope_resolve.go
sunib
changed the title
…ree about why nobody was named
In the DEFAULT deployment no CommitRequest could attach to any commit window, so
the user's message was silently dropped and the change landed under the
generated one. Two independent causes, both introduced with the explicit
outcome.
1. AttributionNotAttempted was "not_attempted", so it was NOT the zero value of
AttributionOutcome. Most producers of an Event never assign the field —
reconcile, resync, bootstrap, and configured-author mode's early return in
attachAuthor (the only assignment site in the non-test tree) — and all of
them mean "no actor was sought". The zero value was therefore a silent fourth
state equal to none of the three named outcomes. Three comments asserted it
already WAS AttributionNotAttempted and were false (types.go, target_watch.go,
author_resolver.go). The constant is now "" so that it genuinely is, which
makes all three true at once instead of patching one site. Nothing serializes
it: it reaches no CRD field and no metric label, and authorKind branches on
the typed value.
2. matchesWindow compared the two outcomes for exact equality. They are produced
by DIFFERENT, INDEPENDENTLY CONFIGURED subsystems — the window's by
mirrored-resource attribution (--author-attribution), the request's by
command authorship (--admission-webhook), which cmd/main.go documents as
independent — so equality couples the two flags:
watch attr | webhook | window | CommitRequest | equality
off | off | not_attempted | not_attempted | match
off | on, miss | not_attempted | unresolved | NO MATCH
on, unres. | off | unresolved | not_attempted | NO MATCH
on, unres. | on, miss | unresolved | unresolved | match
The two middle rows are real deployments that attach on main. Matching now
goes through AttributionOutcome.NamesActor: an outcome either produced an
actor to compare or it did not, and WHY it did not is a property of the
producer's configuration, not of the commit.
The author comparison stays UNCONDITIONAL, with the outcome class as an
additional guard rather than a replacement. Skipping it when neither side
names an actor looks equivalent — both authors are "" — but makes
cross-author attachment depend on the outcome fields being right;
TestAttach_ForeignWindowIsNotStolen caught exactly that, with bob's request
claiming alice's window.
Note this is the opposite call from openWindow.canAppend, which compares the
enums directly and should: both of its sides come from the same watch
pipeline. Intra-subsystem versus cross-subsystem.
Also settles what the unresolved sentinel is scoped to. commitOptionsFor already
DERIVES UnresolvedAuthor() at the write path from the carried outcome rather
than storing it on the event, so the sentinel is the git author header and
nothing else — not window grouping, not the grouped message body, not
{{.Username}}. types.go documented the opposite and was the stated rationale for
the three-string design. Stamping it onto event.UserInfo would change the commit
text of every deployment that has attribution misses and force user templates to
special-case a magic token; an unresolved event now renders {{.Username}} empty
exactly as configured-author mode always has, while git log names the gap.
Tests. The gap that let this ship is that every existing case in
commit_request_attach_test.go left BOTH sides at the zero value, so "" == ""
passed and the production combination was never exercised.
- TestCommitRequestAndWindowOutcomesAgree drives each side from its REAL
producer: the controller's gitOutcome() projection, and the watch package's
actual resolver / an actual zero git.Event. It lives in internal/controller
because that is the only package that imports both.
- TestMatchesWindow_AcrossIndependentlyConfiguredSubsystems walks every pair
either side can produce and names the deployment that produces it, including
the different-authors-with-unset-outcomes case that pins the author check as
unconditional.
- TestAttributionZeroValueIsNotAttempted pins the coupling everything rests on.
- The two tests that hand-injected a state production never produces
(window.Author = sentinel; the sentinel in a {{.Username}} template) now
assert production reality instead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…one, and cap the loop The keys pagination added in 2f29c39 terminated on `len(keys) < keyPageSize`, which assumes the server's page size equals the one requested. It does not: MAX_RESPONSE_ITEMS is operator-configurable and ToCorrectPageSize clamps every request down to it, so against a server whose cap is below 50 EVERY page is short and the loop stops after page 1 with no error. Measured against a server clamping to 30 with a user holding 100 keys: `keys=30 (of 100)`, FindUserKeyByTitle found=false — byte-for-byte the original failure (`422 Key title has been used` for a key that demonstrably exists). It passed only because it happened to match an unpinned server default; the e2e lab sets no [api] block in gitea-values.yaml, so nothing held that default in place. Only an empty page terminates now. Correct for any server page size, at one extra round trip. (Gitea does emit X-Total-Count and Link, but Client.Do returns (int, []byte, error) and discards headers, so that route needs a signature change.) Separately the loop had no cap. Against a server that ignores `page` and returns a full page forever it issued 7193 requests in 1.5s before the context deadline, growing `all` by 50 structs each time. All callers pass deadlines, so it degraded to an undiagnosable "context deadline exceeded" rather than a hang. A maxKeyPages guard names the cause instead. The comment at the old termination was wrong on both halves and is rewritten with the fix: a short page is not necessarily the last page, and the server that loops forever is one that ignores `page`, not one that ignores the limit. The stranded "// ListUserKeys returns every public key…" doc comment, left above `const keyPageSize` when the new block was inserted, now documents the const it actually sits on. ListRepoHooks had the identical unpaginated bug — /repos/{owner}/{repo}/hooks paginates the same way. Latent only because e2e creates a fresh repo per spec and never accumulates a page of hooks on one repo. Both now go through one listPaginated helper, so the termination rule is stated once. Tests: a server that clamps below the requested limit (the regression above), a server that ignores `page` (the cap), and the exact-page-multiple case. The fakes now honour `page` rather than serving the same body forever — three of them would have tripped the new cap, which is the correct client behaviour against an unrealistic fake. Also fixes the handler-goroutine t.Fatal in this file's fakes: Fatal from an httptest handler runs runtime.Goexit on that goroutine, killing the connection mid-response, so the client reports a spurious EOF on top of the real message — two reported errors for one fault, misleading one first. Failures are now recorded and asserted on the test goroutine (or reported with t.Errorf, which does not Goexit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Four e2e legs failed on run 29745528349 — quickstart-install, source-cluster,
image-refresh and bi-directional — with every selected spec PASSING and the
suite then panicking in SynchronizedAfterSuite:
Test Panicked: invalid memory address or nil pointer dereference
test/e2e.queryPrometheus helpers.go:70
test/e2e.reportAttributionStats e2e_suite_test.go:153
test/e2e.init.func18 e2e_suite_test.go:80
reportAttributionStats, added in c55c33b, queries Prometheus without calling
ensurePrometheusClient() — unlike assertNoAnomalousAuditOutcomes on the very
next line. promAPI is only ever set by that lazy initializer, so on any leg
whose selected specs never touched a metric helper it was still nil.
The four failing legs are exactly the small ones (1, 3, 6 and 11 specs);
full-manager (43) and full-core (17) passed only because their specs had
already initialized the client. A pure function of leg size and spec selection,
not of what the legs test — which is why it read as unrelated flakiness.
Worth noting the blast radius: the panic aborted the AfterSuite node, so the
GATING assertNoAnomalousAuditOutcomes never ran on those four legs at all.
queryPrometheus now reports an uninitialized client as a named error rather
than nil-dereferencing. A panic there aborts whatever node is running, and from
an AfterSuite that fails an entire leg with every spec green, which is
unreadable from the console log.
Also parses the author-attribution flag with strconv.ParseBool, as Go's flag
package does. configuredAuthorModeFromArgs matched only the literal
`--author-attribution=false`, so a `--set attribution.enabled=0` (or f, F,
False, FALSE) would flip the probe to ATTRIBUTION mode while the controller ran
configured-author — precisely the silent assertion swap a678dc7 exists to
prevent. The `["--redis-addr="]` row is dropped: with attribution defaulting
true that arg set is fatal at startup ("redis-addr is required when
author-attribution is enabled"), so it documented a mode that cannot exist.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TILT_VERSION, DLV_VERSION, GOPLS_VERSION and STATICCHECK_VERSION
(.devcontainer/Dockerfile) had no custom manager and are invisible to Dependabot
too, so those four pins sat in exactly the "silently rotted" class the config's
own description says it covers. The first ENV block is fully covered by 13
working regexes; this one was simply missed.
Datasource per how each is actually installed, not by habit:
- tilt downloads a GitHub release asset -> github-releases tilt-dev/tilt
- the other three are `go install <module>@$VERSION`, so the `go` datasource
resolves the same module-proxy versions the build uses:
github.com/go-delve/delve
golang.org/x/tools/gopls (its own module, versioned apart from x/tools)
honnef.co/go/tools (staticcheck's module path, not dominikh/go-tools
releases, whose tags use a different scheme)
NODE_MAJOR is left alone: it is a deliberate major-only pin tracking an LTS
line, not a rotting version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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 `@internal/controller/commitrequest_attribution_agreement_test.go`:
- Line 69: Rename the affected tests to follow the TestFunctionName_Scenario
convention: in internal/controller/commitrequest_attribution_agreement_test.go,
rename TestCommitRequestAndWindowOutcomesAgree to
TestCommitRequest_OutcomesAgree and TestAttributionOffLeavesZeroOutcome to
TestAttributionOutcome_OffLeavesZeroOutcome; in
internal/git/commit_request_attach_test.go, rename
TestAttributionZeroValueIsNotAttempted to
TestAttributionOutcome_ZeroValueIsNotAttempted. Update only the test names and
preserve their implementations.
🪄 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: 14c3ebef-ebe4-442a-bc3d-8c5451610a59
📒 Files selected for processing (19)
.github/renovate.jsondocs/configuration.mddocs/design/attribution-outcome-and-author.mdinternal/controller/commitrequest_attribution_agreement_test.gointernal/git/attribution_outcome_test.gointernal/git/commit_request_attach.gointernal/git/commit_request_attach_test.gointernal/git/open_window.gointernal/git/types.gointernal/giteaclient/bootstrap.gointernal/giteaclient/bootstrap_test.gointernal/giteaclient/keys.gointernal/giteaclient/keys_test.gointernal/giteaclient/pagination.gointernal/watch/author_resolver.gointernal/watch/target_watch.gotest/e2e/author_mode_test.gotest/e2e/e2e_suite_test.gotest/e2e/helpers.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/giteaclient/keys.go
- internal/git/types.go
- internal/git/open_window.go
- docs/configuration.md
- test/e2e/author_mode_test.go
- test/e2e/e2e_suite_test.go
- internal/git/commit_request_attach.go
- internal/watch/target_watch.go
- docs/design/attribution-outcome-and-author.md
- internal/watch/author_resolver.go
There was a problem hiding this comment.
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 `@internal/controller/commitrequest_controller_unit_test.go`:
- Around line 261-264: Update the comment immediately preceding
TestCommitRequestReconcile_LookupMissClaimsNoActor so its first line begins with
that exact test identifier, while preserving the existing explanation on
subsequent lines.
🪄 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: 0033837d-cae7-459b-ab63-e4fa58c4f0b0
📒 Files selected for processing (47)
README.mdapi/v1alpha3/commitrequest_types.gocharts/gitops-reverser/templates/validate-operator-types-webhook.yamlcharts/gitops-reverser/values.yamlcmd/main.goconfig/deployment.yamlconfig/webhook/validating-webhook.yamldocs/INDEX.mddocs/TODO.mddocs/UPGRADING.mddocs/architecture.mddocs/attribution-setup-guide.mddocs/configuration.mddocs/design/metrics-observability-plan.mddocs/design/residual-e2e-flakes-2026-06-19.mddocs/finished/documentation-triage.mddocs/finished/redis-key-schema-v3.mddocs/finished/watch-first-ingestion-architecture.mddocs/future/design-commit-request-phase-2.mddocs/future/idea-unify-pending-write-kinds.mddocs/interpreting-metrics.mddocs/spec/README.mddocs/spec/commitrequest-admission-authorship.mddocs/spec/commitrequest-design.mddocs/spec/commitrequest-multi-finalize-design.mddocs/spec/deletecollection-attribution-expander.mddocs/spec/status-conditions-guide.mddocs/tasks/bounded-queue.mdhack/attribution-diagnostics.shinternal/controller/commitrequest_controller.gointernal/controller/commitrequest_controller_unit_test.gointernal/controller/commitrequest_finalize.gointernal/controller/commitrequest_kstatus_test.gointernal/controller/constants.gointernal/git/attribution_outcome_test.gointernal/git/commit_request_attach.gointernal/git/commit_request_attach_loop.gointernal/git/resync_flush.gointernal/git/resync_push_test.gointernal/git/types.gointernal/queue/attribution_index_deletecollection_test.gointernal/watch/target_watch.gointernal/watch/target_watch_test.gointernal/webhook/validate_operator_types_handler.gotest/e2e/commit_author_attribution_e2e_test.gotest/e2e/commit_request_e2e_test.gotest/e2e/e2e_suite_test.go
💤 Files with no reviewable changes (5)
- docs/tasks/bounded-queue.md
- docs/design/residual-e2e-flakes-2026-06-19.md
- docs/finished/documentation-triage.md
- docs/future/design-commit-request-phase-2.md
- docs/spec/commitrequest-multi-finalize-design.md
🚧 Files skipped from review as they are similar to previous changes (11)
- config/deployment.yaml
- internal/controller/commitrequest_finalize.go
- internal/git/commit_request_attach_loop.go
- docs/configuration.md
- internal/git/attribution_outcome_test.go
- internal/watch/target_watch.go
- internal/git/types.go
- internal/git/commit_request_attach.go
- hack/attribution-diagnostics.sh
- test/e2e/e2e_suite_test.go
- internal/controller/commitrequest_controller.go
sunib
changed the title
sunib
changed the title
sunib
deleted the
feat/watchrule-src-ns-pr3-clusterwatchrule-target-admission
branch
July 20, 2026 16:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.