fix(repositories): stop declaring org-enforced signoff so updates can apply - #128
Conversation
… apply GitHub rejects a repository PATCH with 422 whenever web_commit_signoff_required appears in it while the org enforces signoff -- measured directly against the API, sending the field at its own current value of true still fails. The Terraform provider omits the field only while it is left unconfigured, and upjet builds that configuration from forProvider and initProvider alike, so the create-only seed kept the field in every update payload and the write path stayed blocked. Nothing is lost by omitting it: the org setting is what applies signoff to a new repository, and it is the same setting that makes the field unwritable. Fixes #112 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verification recordRoot cause, measured (not inferred). Two no-op PATCHes against
So the trigger is the field's presence, and the "cannot be disabled" wording is misleading — nothing RED/GREEN.
Redundancy of the removed seed. Every repository in this org already carries Open verification gap — stated plainlyThe deployed provider embeds I attempted to settle it by temporarily removing the field from the live managed resource and watching This does not change whether the PR is correct — the removed declaration is provably redundant and is
If they do not, the field is unconditionally sent by v6.6.0 and #112 is upstream-blocked rather than |
@coderabbitai review |
|
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 47 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: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughRepository guidance now states that organization-enforced fields must be absent from both Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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.
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 `@deploy/repositories/kustomization.yaml`:
- Around line 39-47: Update the comment in agent-plugins.yaml near the
webCommitSignoffRequired guidance to state that the field is declared nowhere in
the repository configuration and signoff is inherited from the organization.
Remove the outdated reference to the shared patch while preserving the
surrounding configuration guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c04ffb7-ff83-41e4-b7aa-5aae96b825c5
📒 Files selected for processing (3)
AGENTS.mddeploy/repositories/kustomization.yamltests/repository-update-policy.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the instructions defined in
AGENTS.md.Keep this repository's
AGENTS.mdsynchronized with the actualdeploy/layout and shared engineering contract; do not maintain a redundant.github/copilot-instructions.mdfile.
Files:
AGENTS.md
**/AGENTS.md
📄 CodeRabbit inference engine (GEMINI.md)
Follow the coding guidelines and instructions defined in
AGENTS.md.
Files:
AGENTS.md
tests/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Before every pull request, run
tests/admin-team-policy.sh,tests/declarative-coverage.sh, andtests/repository-update-policy.sh; all must pass.
Files:
tests/repository-update-policy.sh
deploy/{repositories,labels,teams}/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
When adopting an existing repository, issue-label set, or team, set
crossplane.io/external-nameto the live name and use a management policy that excludesDeleteto observe and late-initialize without recreate or deletion risk.
Files:
deploy/repositories/kustomization.yaml
deploy/repositories/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
After adoption, active
Repositoryresources must useObserve/Create/UpdatewithoutLateInitialize; do not declare organization-enforced fields such aswebCommitSignoffRequiredin eitherforProviderorinitProvider.
Files:
deploy/repositories/kustomization.yaml
deploy/**/*.yaml
📄 CodeRabbit inference engine (AGENTS.md)
deploy/**/*.yaml: Verify every new Crossplane resource kind and field against the authoritative provider CRDs and generated examples; local schema validation is unavailable, so do not rely solely on local kustomize validation.
Preserve the documented deployment layout: repositories, teams, team memberships, team-repository grants, labels, provider configuration, external secret, and top-level kustomization belong in their corresponding paths.
Before every pull request, ensure the rendered deployment manifests build cleanly withkubectl kustomize deploy/.
Files:
deploy/repositories/kustomization.yaml
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: devantler-tech/.github
Timestamp: 2026-07-27T14:26:44.160Z
Learning: Manage GitHub organization, repository, team, and label configuration declaratively by editing `deploy/` and shipping a PR; never write managed configuration through the GitHub UI or `gh api`. Reading via `gh api` is allowed, and applying an existing label is triage rather than configuration.
Learnt from: CR
Repo: devantler-tech/.github
Timestamp: 2026-07-27T14:26:44.160Z
Learning: Use GitHub Issues as the roadmap of record, triage incoming issues into the roadmap/epic structure, and have implementation pull requests use `Fixes `#N``.
📚 Learning: 2026-07-10T14:42:56.622Z
Learnt from: devantler
Repo: devantler-tech/.github PR: 90
File: deploy/archived-repositories/kustomization.yaml:11-16
Timestamp: 2026-07-10T14:42:56.622Z
Learning: In this repo’s deployment setup, `deploy/` kustomize manifests are rendered unconditionally (no feature-flag/conditional-render/apply primitive) and Flux applies from `main`. For irreversible or ordering-sensitive rollouts in `deploy/` (e.g., changes under `deploy/archived-repositories/` that require prerequisites handled in separate PRs), reviewers should not expect a mechanical CI guard. Instead, require the PR to remain in draft until the required prerequisite one-click actions (e.g., org `required-workflow` ruleset repointing) are completed, with the required rollout ordering explicitly documented in the PR body (per the documented bold line). Only then should the PR be promoted/merged.
Applied to files:
deploy/repositories/kustomization.yaml
📚 Learning: 2026-07-27T11:40:06.755Z
Learnt from: devantler
Repo: devantler-tech/.github PR: 127
File: deploy/repositories/agent-plugins.yaml:14-15
Timestamp: 2026-07-27T11:40:06.755Z
Learning: When reviewing Crossplane GitHub `Repository` resources under `deploy/repositories/*.yaml`, note that removing `LateInitialize` prevents newly observed/undeclared fields from being copied into `spec.forProvider`. As a result, those newly added settings become unmanaged until explicitly declared. However, fields that were late-initialized before this policy change may already exist in `spec.forProvider`; they are then owned by the provider field manager (not Flux `kustomize-controller`) and may remain in provider update payloads. Require comments to clearly distinguish these two cases so reviewers know whether a field is newly unmanaged vs. pre-existing provider-owned data.
Applied to files:
deploy/repositories/kustomization.yaml
🔇 Additional comments (5)
AGENTS.md (1)
47-50: 🗄️ Data Integrity & IntegrationVerify the deployed provider behavior before relying on this rule.
The fix depends on
terraform-provider-github v6.6.0omittingwebCommitSignoffRequiredwhen both provider blocks are absent, but the PR context says this is still unconfirmed. Validate the installed provider’s update path and one real repository update; otherwise existing 422 loops may persist despite clean manifests.deploy/repositories/kustomization.yaml (2)
58-80: LGTM!
81-83: 🗄️ Data Integrity & IntegrationVerify
hasDownloadsagainst the installed Repository CRD.This adds
spec.forProvider.hasDownloadsto every renderedRepository. As per coding guidelines, verify the field in the authoritative provider CRD and generated namespaced examples; localkubectl kustomizerendering is insufficient.Source: Coding guidelines
tests/repository-update-policy.sh (2)
61-83: 🗄️ Data Integrity & IntegrationCheck live CR state in addition to rendered manifests.
This assertion proves only that Kustomize output no longer declares the key. A field previously late-initialized into live
spec.forProvidercan remain provider-owned after manifest removal and still be included in update payloads, so existing 422 failures may continue. Verify that the reconciled live resources no longer contain the field before treating this test as proof of recovery.Based on learnings, previously provider-owned fields can remain in live
spec.forProviderafter a manifest stops declaring them.Source: Learnings
85-85: LGTM!
Five repository files and the archived-repositories kustomization still described webCommitSignoffRequired as supplied by the shared patch. That patch no longer sets it, so the guidance would have invited the rejected field back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai review |
|
✅ Action performedReview finished.
|
ReadinessPentad at Blast-radius check before merging a write-path repair. Restoring the write path makes every
So the only pending writes this unblocks are the intended ones: the declared topics on User evaluation. These manifests have no locally exercisable runtime surface — they are consumed by
The one thing I could not settle pre-merge is whether Promoting on that basis. |
Seven of the twenty Repository resources have been failing every update with 422 "Commit signoff is enforced by the organization and cannot be disabled", so declared repository configuration has not reached GitHub for them. The accepted explanation was that the field is rejected whenever it appears in an update, so #128 removed the declaration entirely. The live cluster shows the opposite. upjet builds the Terraform configuration from forProvider; an absent optional bool takes the provider's zero value of false; false against a live true is a permanent diff, so the payload carries web_commit_signoff_required: false and GitHub rejects it. The error names disabling, not presence. Declaring the live value leaves nothing to diff, so Terraform omits the field from the payload and the update applies. Measured, at two-minute resolution: 2026-07-27T04:33Z #125 merges, shared patch still declares the field 2026-07-27T04:34:5x nine write-enabled repos record LastAsyncOperation=Success 2026-07-27T14:42Z #128 removes the declaration 2026-07-27T14:44:4x the same repos begin recording AsyncUpdateFailure (422) Ten repositories still reconcile today only because the provider had already late-initialized the value into their spec, where Flux's removal could not reach it. The seven that never accumulated that residue are exactly the seven failing, and six of them differ from a working repository by this one field. So the write path currently depends on undeclared residue rather than on anything in this repository. platform-tenant-template is the one active resource holding no adopted values, so it cannot fall back on a previously observed visibility once updates apply again; its live value is pinned explicitly. tests/repository-update-policy.sh now pins the inverted invariant. Fixes #112 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…136) * chore: claim #123 * fix(repositories): declare org-enforced signoff so updates can apply Seven of the twenty Repository resources have been failing every update with 422 "Commit signoff is enforced by the organization and cannot be disabled", so declared repository configuration has not reached GitHub for them. The accepted explanation was that the field is rejected whenever it appears in an update, so #128 removed the declaration entirely. The live cluster shows the opposite. upjet builds the Terraform configuration from forProvider; an absent optional bool takes the provider's zero value of false; false against a live true is a permanent diff, so the payload carries web_commit_signoff_required: false and GitHub rejects it. The error names disabling, not presence. Declaring the live value leaves nothing to diff, so Terraform omits the field from the payload and the update applies. Measured, at two-minute resolution: 2026-07-27T04:33Z #125 merges, shared patch still declares the field 2026-07-27T04:34:5x nine write-enabled repos record LastAsyncOperation=Success 2026-07-27T14:42Z #128 removes the declaration 2026-07-27T14:44:4x the same repos begin recording AsyncUpdateFailure (422) Ten repositories still reconcile today only because the provider had already late-initialized the value into their spec, where Flux's removal could not reach it. The seven that never accumulated that residue are exactly the seven failing, and six of them differ from a working repository by this one field. So the write path currently depends on undeclared residue rather than on anything in this repository. platform-tenant-template is the one active resource holding no adopted values, so it cannot fall back on a previously observed visibility once updates apply again; its live value is pinned explicitly. tests/repository-update-policy.sh now pins the inverted invariant. Fixes #112 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(repositories): point signoff rationale at the tracking issue * docs(agents): bound LateInitialize residue to the adoption phase CodeRabbit: the previous wording let 'late-initialized values land in forProvider' read as an ongoing behaviour. Once LateInitialize is removed no newly observed field is copied in again, which is precisely why a resource adopted without a field can never acquire it and the config must supply it. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…epos (#138) * chore: claim #123 * fix(repositories): declare org-enforced signoff so updates can apply Seven of the twenty Repository resources have been failing every update with 422 "Commit signoff is enforced by the organization and cannot be disabled", so declared repository configuration has not reached GitHub for them. The accepted explanation was that the field is rejected whenever it appears in an update, so #128 removed the declaration entirely. The live cluster shows the opposite. upjet builds the Terraform configuration from forProvider; an absent optional bool takes the provider's zero value of false; false against a live true is a permanent diff, so the payload carries web_commit_signoff_required: false and GitHub rejects it. The error names disabling, not presence. Declaring the live value leaves nothing to diff, so Terraform omits the field from the payload and the update applies. Measured, at two-minute resolution: 2026-07-27T04:33Z #125 merges, shared patch still declares the field 2026-07-27T04:34:5x nine write-enabled repos record LastAsyncOperation=Success 2026-07-27T14:42Z #128 removes the declaration 2026-07-27T14:44:4x the same repos begin recording AsyncUpdateFailure (422) Ten repositories still reconcile today only because the provider had already late-initialized the value into their spec, where Flux's removal could not reach it. The seven that never accumulated that residue are exactly the seven failing, and six of them differ from a working repository by this one field. So the write path currently depends on undeclared residue rather than on anything in this repository. platform-tenant-template is the one active resource holding no adopted values, so it cannot fall back on a previously observed visibility once updates apply again; its live value is pinned explicitly. tests/repository-update-policy.sh now pins the inverted invariant. Fixes #112 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(repositories): point signoff rationale at the tracking issue * docs(agents): bound LateInitialize residue to the adoption phase CodeRabbit: the previous wording let 'late-initialized values land in forProvider' read as an ongoing behaviour. Once LateInitialize is removed no newly observed field is copied in again, which is precisely why a resource adopted without a field can never acquire it and the config must supply it. * fix(repositories): correct stale private declarations before restoring writes ascoachingogvaner and wedding-app are public and serve live sites, but both declared visibility: private — carried in from the template they were written against, never true of either repository. While updates were write-blocked the divergence was inert; restoring the write path is what would make it act, so the declaration is corrected first and the two stay Observe-only until promoted separately. The kustomization grouped them under '# Private repos.', which described neither their visibility nor their management policy. fleet-gitops, which is genuinely private and fully managed, moves in with the actively-managed set. Part of #123 * docs(repositories): state the current management rationale, not the migration The two observe-only declarations described the change this PR makes rather than the state it leaves behind. Both now say why write management is still disabled and where enabling it is tracked. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Why
Our declarative GitHub configuration has not been able to change any repository setting since it
was set up. Every update is rejected by GitHub, so declared settings that differ from the live repo
simply never take effect — two repos have had their declared topics sitting unapplied for weeks, and
the discovery surface they were written for has never existed.
The previous two attempts at this both narrowed the problem without fixing it. This one is based on a
direct measurement against the GitHub API rather than an assumption about how the setting behaves.
What
Stops declaring the commit-signoff setting in our configuration at all.
GitHub refuses the whole update whenever that setting is mentioned, even when we send the exact
value the repository already has. Our organisation already enforces signoff on every repository —
including every new one — so declaring it bought us nothing and was the very thing blocking the write
path. Removing it lets everything else we declare actually apply, and loses no protection.
The existing safety check is updated to match: it now fails if the setting reappears anywhere, so
this cannot silently regress.
Fixes #112