fix(watch): a cluster-wide selection must not collapse named-namespace streams - #255
Merged
Merged
Conversation
…ad to 1.0 The quick start pinned cert-manager v1.20.3, a version that appeared nowhere else in the repo and was never exercised. It also omitted the rollout wait (the controller cannot start until cert-manager issues the admission certificate) and told users to check `Ready` on the GitTarget, which is held at Unknown until first source discovery — the e2e asserts `Validated` for exactly this reason. Also: - Document the Kustomize support that already ships. The write path runs kustomize itself as a pre-commit oracle; the README previously claimed it "cannot infer the original structure of arbitrary templates or overlays", which the code contradicts. - Stop over-hedging delivery. While the watch is connected every change is seen and committed; collapsing to current state only happens across a gap. - Reframe HA from a standalone caveat into a road-to-1.0 list alongside the configuration surface (six CRDs still v1alpha3, no conversion path) and docs. - Move the attribution mode tables into the attribution setup guide, which already opened with the same concept. - Warn that `helm uninstall` removes the cluster-scoped default ClusterProvider, and that the default RBAC grants cluster-wide Secret reads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vate Dependabot covers gomod, github-actions, and Dockerfile FROM lines, and it is healthy — every direct Go dependency is current. The pins it cannot see had drifted, including a cert-manager version in the README that existed nowhere else in the repo. Cluster charts (validated by the e2e suite): - cert-manager 1.19.4 -> 1.21.0. Its three breaking changes (tokenrequest RBAC, cert-manager-edit ACME permissions, metrics Helm values) do not apply: we set no cert-manager values beyond crds.enabled, use a self-signed Issuer, and have no serviceAccountRef. - valkey 0.9.3 -> 0.10.0, kcp-operator 0.7.7 -> 0.8.3. - gitea 12.5.1 -> 12.5.3, deliberately NOT the latest 12.7.0. Gitea 1.26 removed the _csrf form input and cookie from /user/login, which the giteaclient WebSession scrapes to verify SSH signing keys, so 12.6.0+ fails the signing e2e. 12.5.3 carries the same Gitea 1.25.5, so this is a chart-only patch. Devcontainer toolchain (needs an image rebuild to take effect): flux2 2.9.2, flux-operator 0.55.0, helm 4.2.3, task 3.52.0, cosign 3.1.2. cosign is pinned twice and both places move together. Renovate is scoped to only what Dependabot cannot see, so the two never open competing PRs: Flux HelmRelease charts, the devcontainer ENV pins, the cert-manager URL in the README, and the cosign-installer input. It holds gitea below 12.6.0 until the web session helper is reworked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Records the investigation behind unpinning Gitea rather than migrating to Forgejo, read from the upstream sources in external-sources/. The premise for migrating was that we were stuck on chart 12.5.x because Gitea 1.26 removed the `_csrf` form input from /user/login, which our WebSession scrapes. That premise does not survive contact with the source: Gitea and Forgejo made the *same* change, both replacing form-token CSRF with Go's stdlib http.CrossOriginProtection, which admits any request carrying neither Sec-Fetch-Site nor Origin. The fix is a deletion, and it works on either server — so migration and unpinning are not the same piece of work. That leaves a cost comparison Gitea wins: it keeps the literal "gitea" signing namespace (Forgejo switched to the ROOT_URL hostname, the one hazardous change), keeps the session cookie name, keeps an HTTP chart repo, and has the better trusted-key lever — TRUSTED_SSH_KEYS reports the commit's own committer email where Forgejo's instance key reports a fixed configured address, which is what our signing assertion checks. Also records why no Go SDK is adopted on either server: there is no official Forgejo SDK, no SDK can cover the web-UI verification flow, the Gitea SDK's version gating is meaningless against Forgejo, and it would add five dependencies to test-only code that is currently stdlib-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Gitea 1.26 replaced form-token CSRF with Go's stdlib
http.CrossOriginProtection (routers/web/web.go). That check returns nil when a
request carries neither Sec-Fetch-Site nor Origin — every non-browser Go client —
and POST /user/login is exempt besides, being SignOutRequired. So the token our
WebSession scraped is not merely absent, it is unnecessary.
Delete fetchCSRF, extractCSRFToken, csrfInputRE and both form.Set("_csrf", ...)
calls, and move the chart from 12.5.3 (Gitea 1.25.5) to 12.7.0 (Gitea 1.27.0) —
the version whose signing spec failed earlier today. HTML-scraping a hidden input
was the most brittle part of the client, so this is a net reduction in fragility
as well as ~45 fewer lines.
The Renovate guard holding gitea below 12.6.0 goes with it; it existed only to
describe this problem.
Validated on Gitea 1.27.0: task lint, task test (coverage 76.6%, baseline
unchanged), and task test-e2e — 58 passed, 0 failed, including the signing specs
that drive the web session.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Validation climbs a ladder: OpenAPI schema, then CEL XValidation, then the reconciler for anything cross-object. A validating admission webhook is not a rung — it is correct only when the information exists solely at admission time and nowhere on the persisted object. Writing it down once so it stops being re-argued per feature. The reasoning was already settled in practice by the config-plane split, which enforces ClusterProvider.spec.allowedNamespaces on every reconcile rather than at admission: reconcile-time re-evaluates continuously (so it catches a policy tightened after the object was created), the data-plane ordering is what provides the security property, and admission buys only feedback latency in exchange for cert wiring in the chart. Also records why /validate-operator-types is not a counterexample: it captures submitter identity and always allows, so it is not a validation gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sions The record had grown to 474 lines carrying a build order, a test plan, an API dump and open questions that are all either shipped or now owned elsewhere. Cut to the decisions and the reversals, which is the part a finished doc is actually for. Corrects one claim that read as current but never shipped: an earlier draft reserved the name "default" for the in-cluster cluster via CEL. What shipped derives locality from spec.kubeConfig alone, for every name — a provider named "default" may carry a kubeConfig and mirror a remote cluster. Records why the reservation was rejected (it pins a name to a physical cluster, the same silent-retarget hazard the immutability rules exist to prevent). Also drops the "enforced in two places" admission-webhook wording for allowedNamespaces, which shipped as a single reconcile-time gate, and points at the new spec/where-validation-lives.md for the general rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revises the WatchRule source-namespace design. The previous revision gated the new field on kubeConfig presence (remote-only, in-cluster forbidden), which is wrong twice over: it derives an addressing capability from a connectivity setting, so setting a kubeconfig to reach a cluster silently grants namespace addressing; and it welds the in-cluster case shut forever, conflating "unsafe by default" with "impossible". Replaced by an explicit deny-by-default allow-list on the platform-owned object, ClusterProvider.spec.allowedSourceNamespaces, mirroring the existing allowedNamespaces exactly. One mechanism instead of two: an empty allow-list already is a gate, so no separate boolean is needed, and the only capability a boolean uniquely adds — "any namespace" — is the one the doc argues must not ship upstream. Names: sourceNamespace on WatchRule (the longer sourceClusterNamespace was justified only by the remote-only rule, which is gone), and the allowed* prefix retained on the provider because the allow-list IS the gate and the name must foreclose a fail-open reading. Records allowedGitTargetNamespaces + allowedSourceNamespaces as the symmetric pair to adopt at the next API version rather than breaking v1alpha3. Keeps the two code findings that make the upstream ask small: the Git path already uses the source object's namespace, so the write side needs no change, and ClusterWatchRule already watches the source cluster across all namespaces but carries two pre-existing defects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comment claimed AllowsNamespace was "the single authorization predicate shared by the admission webhook and the reconcile-time refusal, so the two can never diverge". There is no such webhook — the proposal was dropped, and checkSourceAuthorization in gittarget_source_cluster.go is the only non-test caller. Replaces it with what actually holds: enforcement is reconcile-time and nowhere else, it returns before DeclareForGitTarget, and that is deliberate rather than incidental because continuous re-evaluation also covers a policy tightened after the GitTarget was created — something admission could not see. Comment-only: no generated CRD drift (it documents a method, not a field), and `task lint` passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ment-and-source-namespace
The dep refresh read v0.8.3 (the appVersion kcp-operator 0.7.7 ships) as the chart version. No such chart exists — kcp-dev/helm-charts publishes nothing above 0.7.x — so the HelmRelease never left SourceNotReady and the e2e source-cluster leg died in setup with "no 'kcp-operator' chart with version matching '0.8.3' found". The earlier fix on this branch was invisible to the merge: it restored the merge-base value, so the three-way merge saw a change only on main's side and took 0.8.3. Merging main in first makes this pin an explicit change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…R pages The single 783-line design had grown past the point where it could be picked up and implemented. Split it into a README that carries the decision and the sequencing, plus one page per shippable PR: - PR 1: collapse the ClusterWatchRule stream scope - PR 2: admit ClusterWatchRule targets - PR 3: the sourceNamespace field and its GitTarget allow-list gate - PR 4: the ClusterProvider source ceiling that binds ClusterWatchRule too PR 4 is not optional cleanup: a multi-tenant deployment runs a ClusterWatchRule per tenant from day one to capture CRDs, so an allow-list that binds only WatchRule would leave the wider path unguarded. Repoint the INDEX entry and the multi-cluster-attribution cross-reference at the new directory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A per-namespace replay produced a `desired` set covering one namespace, but the resulting mark-and-sweep was scoped by (group, resource) only. Every managed document of that type in every other namespace was therefore absent from `desired` — and a sweep deletes what is absent. The scope was present at both ends and discarded in the middle: `targetWatchSpecs` already built per-namespace watch keys, `enqueueReplayResync` passed only `key.GVR` onward, and `resyncPlan`'s predicate matched group and resource while `types.ResourceIdentifier` carried the namespace all along. Replace `ScopeGVR *schema.GroupVersionResource` with `Scope *ResyncScope` (GVR + Namespace) across ResyncRequest and PendingWrite, so the two halves of a scope travel together and a call site cannot pass one without the other. An empty Namespace keeps today's all-namespaces meaning, which is what a ClusterWatchRule's cluster-wide stream gathers. `resyncHealKey` now separates namespaces too, so a parked heal for one namespace no longer replaces another's. The defect is latent today: a GitTarget can only watch one namespace per GVR, because WatchRule.targetRef is namespace-local and a ClusterWatchRule's "" key collapses the type to all-namespaces. It goes live with the first change that fans a GitTarget out across namespaces — which is every remaining PR in docs/design/watchrule-source-namespace/. Hence this lands first. Verified: reverting the namespace half of ResyncScope.Matches fails TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone and the sibling-namespace row of TestResyncScope_MatchesRespectsTypeAndNamespace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Concurrent design-doc pass over the WatchRule source-namespace workstream, committed so it is not carried alongside the PR 2 code change: - README: PR 1 marked landed, PR 2 marked landed, and the compatibility table's PR-number column corrected (it was off by one against the phase split). - PR 1 page: rewritten past-tense as the record of what shipped, with a "what the tree looks like now" section naming ResyncScope, resyncScopeForWatchKey, and the scope.Matches predicate. - PR 2 page: status block describing the landed shape (WatchScopes, one stream per scope at both read sites), and a sharper PR 5 interaction — the ceiling drops the "" key only for namespaced selections, so cluster-scoped rules still emit it. - PR 4 / PR 5 pages: consistency with the above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce pages Follow-up to 9dc9df8, covering the remaining PR #254 review findings. - PR 5: drop the stale "own namespace, plus what the policy admits" row. The overview and PR 4 both settled on no implicit self-namespace exception; PR 5 was the only page still carrying the superseded carve-out, which made the authorization boundary read two ways. - PR 5: attribute the source Namespace informer and the SourceNamespaceAuthorized condition to PR 4, not PR 3. PR 3 is the ClusterWatchRule target-admission fix and introduces neither. - PR 4 / PR 5: settle the unevaluatable-policy contract that the two pages stated in opposite terms. They are one contract with two instances, split on whether a scope is being *established* or *maintained* — failing closed while establishing means never starting a stream, while failing closed on a maintained scope means narrowing to nothing, and a narrowed set is the input to a sweep. Hence a permanent Forbidden is Stalled in the first case and a retained Unknown in the second. Recorded as a table in PR 4, referenced from PR 5, and pinned by a test pair so an implementation cannot satisfy one page while contradicting the other. - README: header said PR 1 landed while the phase table said 1 and 2. - multi-cluster-author-attribution: language on the ASCII fence (MD040). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e streams
`SnapshotNamespaces()` returned nil whenever a WatchedType carried the ""
namespace key, and nil meant all-namespaces at every read site. A WatchRule
scoped to one namespace and a ClusterWatchRule scoped cluster-wide, on the same
GVR and the same GitTarget, therefore folded into a single all-namespaces
stream: the named scope survived only in the plan hash, and `targetWatchSpecs`
substituted `NamespaceOps[""]` for its operation set, so a CREATE-only named
rule co-resident with an UPDATE-only cluster-wide rule lost its filter too.
Under PR 4 that is a gate bypass, not just over-watching. A WatchRule
authorized only for `repo-config` would receive events from every namespace the
credential can read, with its allowedSourceNamespaces check having passed
before the data plane widened it.
Replace `SnapshotNamespaces` with `WatchedType.WatchScopes`, which returns every
scope including the cluster-wide "" rather than collapsing to it, and project
one stream per scope at BOTH read sites — a fix in one path only would leave the
plan hash and the running streams describing different mirrors:
- `targetWatchSpecs` keys a watch per scope, each carrying NamespaceOps[ns], so
the per-namespace operation sets survive.
- `snapshotGVRsFromTable` and `resolveSnapshotGVRForType` both project through
one shared `snapshotGVRScopes` helper, so the whole-target and per-type paths
cannot drift apart. `snapshotGVR.namespaces []string` became a single
`namespace string`, matching targetWatchKey and matching how a dynamic client
List actually takes a namespace ("" = all).
Distinct streams rather than subtracting the named scope from the cluster-wide
one: "all namespaces except team-a" is not expressible as a watch, and the
subtraction would need a sweep scope shaped as a complement, which
git.ResyncScope cannot represent. Each stream's replay now pairs a desired set
with a sweep scoped identically — which is exactly why this had to land after
PR 1.
A cluster-scoped GVR is unaffected: collectWatchRuleSelections hardcodes
ResourceScopeNamespaced, so only a namespaced record can ever acquire a named
namespace key, and a cluster-scoped type still yields exactly one "" scope.
TestBuildWatchedTypeTable_ClusterWideOverridesNamedNamespaces asserted the
collapse as intended ("matching the historic gvrSnapshotEntry collapse") and is
deleted, not skipped. Verified: restoring the collapse fails all three
replacements — the table-level scope test, the targetWatchSpecs stream/op-set
test, and the snapshotGVRsFromTable test — each on its own assertion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Base automatically changed from
docs-validation-placement-and-source-namespace
to
main
July 20, 2026 10:05
sunib
added a commit
that referenced
this pull request
Jul 20, 2026
* ci: validate every pull request, not only those based on main `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> * test(e2e): read the author mode from the Deployment, not from a log grep `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> * chore(dev): make the e2e task surface hard to misuse 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> * diag(watch): name declared streams, and record the attribution investigation 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> * docs(debug): consolidate the attribution investigation into docs/debug/ 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> * docs(design): propose an explicit author for unattributed commits 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> * docs(design): drop the flag, and say "attribution failed" rather than "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> * docs(design): carry attribution outcome explicitly, not in the author 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> * feat(attribution): carry the outcome explicitly, and name unresolved 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> * fix(giteaclient): follow pagination when listing a user's keys 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> * refactor(watch): drop the dead snapshot-resolution path 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> * fix(attribution): let a CommitRequest attach when the two sides disagree 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> * fix(giteaclient): terminate pagination on an empty page, not a short 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> * fix(e2e): initialize the Prometheus client before the attribution report 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> * chore(renovate): track the second devcontainer ENV block 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> * docs: clarify attribution outcomes and prune stale notes * fix(attribution): address review feedback --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
PR 2 of the WatchRule source-namespace workstream. Stacked on #254 (PR 1) — base is
docs-validation-placement-and-source-namespace, notmain.The defect
SnapshotNamespaces()returnednilwhenever aWatchedTypecarried the""namespace key, andnilmeant all namespaces at every read site. A WatchRule scoped to one namespace and a ClusterWatchRule scoped cluster-wide, on the same GVR and the same GitTarget, folded into a single all-namespaces stream. The named scope survived only in the plan hash.The operation sets collapsed the same way:
targetWatchSpecssubstitutedNamespaceOps[""]for the named scope's set, so aCREATE-only named rule co-resident with anUPDATE-only cluster-wide rule lost its filter too.Under PR 4 this is a gate bypass, not merely over-watching. A WatchRule authorized only for
repo-configwould receive events from every namespace the source credential can read, with itsallowedSourceNamespacescheck having passed before the data plane widened it.The fix
SnapshotNamespaces→WatchedType.WatchScopes, returning every scope including the cluster-wide""instead of collapsing to it, with both read sites projecting one stream per scope. Fixing one path only would be worse than no fix — the plan hash and the running streams would then describe different mirrors.targetWatchSpecskeys a watch per scope, each carryingNamespaceOps[ns], so the per-namespace operation sets survive.snapshotGVRsFromTableandresolveSnapshotGVRForTypeproject through one sharedsnapshotGVRScopeshelper, so the whole-target and per-type paths cannot drift apart.snapshotGVR.namespaces []stringbecame a singlenamespace string, matchingtargetWatchKeyand matching how a dynamic clientListactually takes a namespace (""= all).Why distinct streams rather than subtracting the named scope from the cluster-wide one: "all namespaces except
team-a" is not expressible as a watch, and the subtraction would need a sweep scope shaped as a complement, whichgit.ResyncScopecannot represent. Each stream's replay now pairs adesiredset with a sweep scoped identically — which is exactly why this had to land after PR 1.Cluster-scoped types are unaffected.
collectWatchRuleSelectionshardcodesResourceScopeNamespaced, so only a namespaced record can ever acquire a named namespace key; a cluster-scoped type still yields exactly one""scope and one cluster-wide watch.Tests
TestBuildWatchedTypeTable_ClusterWideOverridesNamedNamespacesasserted the collapse as intended ("matching the historicgvrSnapshotEntrycollapse"). It is deleted, not skipped — leaving it green would have meant the fix did not land.Verified by restoring the pre-fix collapse: all three replacements fail, each on its own assertion.
TestBuildWatchedTypeTable_ClusterWideDoesNotCollapseNamedNamespacesTestTargetWatchSpecs_NamedAndClusterWideScopesStayDistinctStreamstargetWatchSpecs(streams + op sets)TestSnapshotGVRsFromTable_NamedAndClusterWideScopesStayDistinctEntriessnapshotGVRsFromTableValidation
task lint— cleantask test— green, coverage 76.6% (baseline unmoved)task test-e2e— full suite in flight at time of opening; will reportAlso re-ran the unexplained PR 1 e2e failure (Commit Request Bundle UC2,
commit_request_e2e_test.go:377) underE2E_LABEL_FILTER='!image-refresh && !bi-directional && !source-cluster && commit-request': all 4 specs passed, so it was a flake, not a regression from PR 1'srenderReconcileCommitMessagesignature change.🤖 Generated with Claude Code