feat(ci): gate sizeable API changes behind API-owner review - #3167
Conversation
Add a deterministic api-gate tool (cmd/api-gate, internal/apigate) and a workflow that require an approving review from an API owner (lllamnyp or kvaps) when a PR introduces a new API group, a new resource, or a breaking schema change to an existing resource. Detection diffs the checked-in OpenAPIv3 schemas (apps cozyrds + CRD manifests) and the apiserver storage registrations; no network or LLM calls. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new CI gate to protect the Cozystack API surface. By comparing the API definitions between the merge base and the PR head, the system automatically identifies significant changes—such as new API groups, new resources, or breaking schema modifications—and mandates an approval from designated API owners before the PR can be merged. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the api-gate tool, which compares the Cozystack API surface between two checkouts to detect sizeable changes (new API groups, new resources, or breaking schema changes). The review feedback highlights several critical improvements for the schema diffing and parsing logic, including handling cases where head is nil to avoid false positives, detecting added type constraints on previously unrestricted fields, correctly identifying transitions of additionalProperties to false, supporting hyphens in API server storage plurals, and preventing silent bypasses when no resources are loaded.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return out | ||
| } | ||
|
|
||
| func diffNode(path string, base, head Schema, out *[]string) { |
There was a problem hiding this comment.
When a schema constraint (such as additionalProperties or items) is completely removed in the head version, head becomes nil. Currently, the code proceeds to diff this against a non-nil base, which leads to false-positive breaking change reports (e.g., claiming types were narrowed or fields were removed, when in fact the schema became completely unrestricted/wider). Adding an early check for head == nil prevents these false positives.
| func diffNode(path string, base, head Schema, out *[]string) { | |
| func diffNode(path string, base, head Schema, out *[]string) { | |
| if head == nil { | |
| return | |
| } |
| bt, ht := schemaTypes(base), schemaTypes(head) | ||
| if len(bt) > 0 { // base constrained the type at all | ||
| if removed := setDiff(bt, ht); len(removed) > 0 { | ||
| *out = append(*out, fmt.Sprintf("%s: type narrowed, no longer accepts %s", path, strings.Join(removed, "/"))) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current type-narrowing check only runs if len(bt) > 0. If the base schema had no type constraint (meaning it was unrestricted and accepted any type), and the head schema adds a type constraint (e.g., type: string), this is a breaking change because it now rejects other types. However, the current logic completely misses this. We should check if len(ht) > 0 and handle the case where len(bt) == 0 as a type constraint addition.
| bt, ht := schemaTypes(base), schemaTypes(head) | |
| if len(bt) > 0 { // base constrained the type at all | |
| if removed := setDiff(bt, ht); len(removed) > 0 { | |
| *out = append(*out, fmt.Sprintf("%s: type narrowed, no longer accepts %s", path, strings.Join(removed, "/"))) | |
| } | |
| } | |
| bt, ht := schemaTypes(base), schemaTypes(head) | |
| if len(ht) > 0 { | |
| if len(bt) == 0 { | |
| *out = append(*out, fmt.Sprintf("%s: type constraint added (previously unrestricted)", path)) | |
| } else if removed := setDiff(bt, ht); len(removed) > 0 { | |
| *out = append(*out, fmt.Sprintf("%s: type narrowed, no longer accepts %s", path, strings.Join(removed, "/"))) | |
| } | |
| } |
| if b, h, ok := schemaChild(base, head, "additionalProperties"); ok { | ||
| diffNode(path+"{}", b, h, out) | ||
| } |
There was a problem hiding this comment.
In OpenAPI, additionalProperties can be a boolean (e.g., additionalProperties: false). Restricting additionalProperties to false (when it was previously true, absent, or a schema) is a breaking change because it rejects any undeclared fields. Currently, schemaChild only extracts map-based schemas and ignores boolean values, meaning the tool completely misses when additionalProperties is set to false. We should explicitly check for this transition.
if base["additionalProperties"] != false {
if head["additionalProperties"] == false {
*out = append(*out, fmt.Sprintf("%s: additionalProperties restricted to false", path))
} else if b, h, ok := schemaChild(base, head, "additionalProperties"); ok {
diffNode(path+"{}", b, h, out)
}
}| // apiserverStorageRe matches `<var>["<plural>"]` storage registrations, e.g. | ||
| // | ||
| // coreV1alpha1Storage["tenantsecrets"] = ... | ||
| var apiserverStorageRe = regexp.MustCompile(`(\w+Storage)\["([a-z0-9]+)"\]`) |
There was a problem hiding this comment.
The regular expression apiserverStorageRe uses [a-z0-9]+ to match the plural name of the registered storage. However, Kubernetes resource plurals can contain hyphens (e.g., ip-addresses or security-groups). If a future resource with a hyphen is registered in apiserver.go, this regex will fail to match it. Updating the character class to [a-z0-9-]+ makes the parser more robust and future-proof.
| var apiserverStorageRe = regexp.MustCompile(`(\w+Storage)\["([a-z0-9]+)"\]`) | |
| var apiserverStorageRe = regexp.MustCompile(`(\w+Storage)\["([a-z0-9-]+)"\]`) |
| } else if !os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("read %s: %w", apiserverPath, err) | ||
| } | ||
|
|
||
| return snap, nil | ||
| } |
There was a problem hiding this comment.
If LoadSnapshot is run in an incorrect directory, or if the directory structure changes such that the glob patterns no longer match any files, the function will return an empty Snapshot without any error. This would cause the CI gate to silently succeed (reporting "No sizeable API changes detected") even if there are major breaking changes. Adding a check to ensure that at least one resource is loaded (or returning an error if the snapshot is empty) prevents this silent bypass mode.
} else if !os.IsNotExist(err) {
return nil, fmt.Errorf("read %s: %w", apiserverPath, err)
}
if len(snap) == 0 {
return nil, fmt.Errorf("no API resources found in %s; please verify the repository path and directory structure", dir)
}
return snap, nil
}
📝 WalkthroughWalkthroughAdds an ChangesAPI Review Gate
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
internal/apigate/parse.go (1)
140-170: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffRegex scan can overcount non-code text
apiserverStorageRewill also match commented-out or string-literalStorage["..."]text, which can emit falseNewGroup/NewResourcefindings. Parse tokens/AST, or strip comments before matching.🤖 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/apigate/parse.go` around lines 140 - 170, The APIServer storage scan in ParseAPIServerStorages is matching raw text too broadly, so commented-out or string-literal Storage["..."] fragments can be counted as real resources. Tighten the extraction around apiserverStorageRe by parsing Go tokens/AST or otherwise ignoring comments and string literals before scanning, while keeping the existing resourceKey deduping and storageVarGroups lookup intact.
🤖 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 @.github/workflows/api-review-gate.yaml:
- Around line 37-38: The workflow currently uses the pull request base branch
tip instead of the actual merge base, which can skew the API gate results.
Update the logic in api-review-gate.yaml around the BASE_SHA/HEAD_SHA setup to
compute the true merge base with git merge-base first, then use that commit to
materialize the base worktree before running the comparison. Keep the HEAD_SHA
handling as-is and make sure any later steps that read the base commit use the
merge-base-derived symbol/value.
- Around line 33-40: The workflow currently exposes GH_TOKEN too broadly and
leaves checkout credentials persisted, allowing PR-controlled steps like the go
build and api-gate steps to read a live token. Move GH_TOKEN out of the
job-level env and scope it only to the specific gh api step that needs it, and
update the checkout action to use persist-credentials: false. Apply the same
change anywhere the token or checkout credentials are reused in this workflow so
only the gate step can access them.
- Around line 91-97: The approval check in the workflow is using all historical
APPROVED reviews, so stale approvals can still satisfy the gate. Update the
`approvals` query in the review gate job to reduce `gh api
repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews` results to each
reviewer’s latest review state before filtering for `APPROVED`. Keep the fix
localized to the `approvals` assignment in
`.github/workflows/api-review-gate.yaml`, preserving the existing gate logic
while ensuring only the latest state per `user.login` counts.
In `@internal/apigate/parse.go`:
- Around line 91-96: The CRD version parsing in parse.go is treating every entry
in spec.versions as active because the inner versions struct only reads name and
schema. Update the parsing logic in the CRD handling path that builds
Resource.Versions to also capture the served flag and filter out versions where
served is false, so only callable API versions are classified and diffed. Make
sure the fix applies to the version extraction logic around the Versions struct
and any later use that appends into Resource.Versions.
- Around line 127-170: ParseAPIServerStorages currently skips any matched
*Storage prefix that is not present in storageVarGroups, which can hide newly
added API groups. Update the logic around apiserverStorageRe and
ParseAPIServerStorages so an unknown storage variable prefix is reported loudly
instead of being silently ignored, either by returning an error/finding or by
otherwise surfacing the unexpected prefix for review. Keep the existing handling
for known prefixes like coreV1alpha1Storage and sdnV1alpha1Storage.
In `@internal/apigate/report.go`:
- Around line 57-59: The plain-text report path is still emitting Markdown
because `identity()` always returns emphasized names. Update `Report` and
`identity()` so formatting is conditional on the requested output mode: keep
emphasis only for markdown and return unwrapped names for text. Make sure both
the main line formatting and the grouped bullet handling in `Report` use the
same format-aware identity logic.
In `@internal/apigate/schemadiff.go`:
- Around line 111-116: The handling in schemadiff.go around schemaChild,
diffNode, and the items/additionalProperties branches should distinguish
explicit removal and boolean-state changes instead of recursing whenever either
side has a child schema. Update the comparison logic so typed child schemas
removed on the head side are treated as intentional relaxations, and add
explicit handling for additionalProperties boolean transitions such as
true/false so they are classified deliberately rather than as nil-vs-map diffs.
- Around line 45-49: The schema diff logic in schemadiff.go currently treats
adding an explicit type on an unconstrained base schema as safe. Update the
comparison in schemaTypes/setDiff handling so that when the base has no type but
head introduces one, it emits a breaking finding for the path (covering
transitions like empty schema to string/object). Add a regression test around
the schemadiff path to verify len(bt) == 0 and len(ht) > 0 is reported as
breaking.
---
Nitpick comments:
In `@internal/apigate/parse.go`:
- Around line 140-170: The APIServer storage scan in ParseAPIServerStorages is
matching raw text too broadly, so commented-out or string-literal Storage["..."]
fragments can be counted as real resources. Tighten the extraction around
apiserverStorageRe by parsing Go tokens/AST or otherwise ignoring comments and
string literals before scanning, while keeping the existing resourceKey deduping
and storageVarGroups lookup intact.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46daaf2f-ad71-4b55-9cfd-2e7a374326a4
📒 Files selected for processing (11)
.github/workflows/api-review-gate.yamlcmd/api-gate/main.gointernal/apigate/classify.gointernal/apigate/classify_test.gointernal/apigate/model.gointernal/apigate/parse.gointernal/apigate/parse_test.gointernal/apigate/report.gointernal/apigate/schemadiff.gointernal/apigate/schemadiff_test.gointernal/apigate/snapshot.go
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the schema-diff core has confirmed false-positive/false-negative bugs in exactly the breaking-change cases this gate exists to catch, entire-resource removal is silently ungated, and the workflow's approval check can be satisfied by a stale/superseded review.
Business context: Adds a deterministic CI gate requiring an API-owner review whenever a PR introduces a new API group, a new resource, or a breaking schema change to the Cozystack API surface.
Blockers
B1: Type-constraint diffing is wrong in three related ways
File: internal/apigate/schemadiff.go:42-50
Issue: (a) When a nested schema (items/additionalProperties) is entirely removed on head, diffNode is called with a nil head and misreports the removal as "type narrowed" plus a spurious "field X was removed" for every nested property — even though dropping the constraint is a widening, not a break. (b) When base has no type keyword (unconstrained) and head adds one, the if len(bt) > 0 guard skips the check entirely, so a real breaking change is never reported. (c) When base has a type and head removes it (becoming unconstrained), the same block reports "type narrowed" — backwards, since removing a constraint is safe.
Evidence: Reproduced all three with diffSchema directly:
itemsschema entirely removed →["...type narrowed, no longer accepts object", "...field \"x\" was removed"](should be empty).- base
{}(any type) → head{"type":"string"}→[](should flag "type constraint added"). - base
{"type":"string"}→ head{}(any type) →["...type narrowed, no longer accepts string"](should be empty).
Impact: The gate both waves through real breaking changes (b) and forces unnecessary owner review on safe relaxations (a, c) — undermining trust in the signal either way.
Fix: GuarddiffNodeagainst a nil head; treat an empty type set on head as "unconstrained" (never narrowing) instead of subtracting against it; treat an empty type set on base plus a non-empty set on head as "type constraint added".
B2: additionalProperties: false is never detected as breaking
File: internal/apigate/schemadiff.go:111-113, schemaChild (internal/apigate/schemadiff.go:223-230)
Issue: schemaChild only extracts map-shaped schemas; a boolean additionalProperties: false never satisfies its type assertion, so the recursion at line 111-113 is skipped entirely regardless of what the base allowed.
Evidence: base {"additionalProperties": true} → head {"additionalProperties": false} on the same property → diffSchema returns []. Restricting additionalProperties to false rejects any previously-accepted undeclared field — a textbook breaking change that ships with zero signal.
Fix: Explicitly compare the boolean transition (true/absent/schema → false) before falling through to the map-schema recursion.
B3: Removing an entire resource or API group is not gated, and this isn't disclosed
File: internal/apigate/classify.go:35-92
Issue: All three findings loops (NewGroup, NewResource, Breaking) iterate only sortedResources(head); nothing checks for a (group, plural) present in base and absent from head. Deleting an entire CRD/ApplicationDefinition — arguably the single most disruptive kind of API change — produces zero findings.
Evidence: Classify(snap(pgBase), snap()) (Postgres resource entirely removed) returns nil. This matches the explicit test TestClassify/removed_resource_is_not_sizeable_by_these_rules, so it's intentional — but the PR description's definition of "sizeable" never mentions this exclusion, and the workflow's job summary/report text gives no hint that a full resource removal sailed through unreviewed.
Impact: An operator reading "this PR makes a sizeable API change" absence as "the API surface is safe" is misled for the one case that matters most.
Fix: Either add a fourth category (ResourceRemoved/GroupRemoved) to the gate, or state the exclusion explicitly in the workflow's PR-facing report text so a passing gate never implies "nothing was removed".
B4: Approval isn't scoped to the current head commit or to each reviewer's latest review state
File: .github/workflows/api-review-gate.yaml:94-97
Issue: gh api .../reviews --jq '[.[] | select(.state=="APPROVED") | .user.login] | unique' collects every historical APPROVED review from an owner, with no filter on commit_id and no "latest state per user" reduction.
Evidence: Direct read of the jq filter — two independent ways to slip a sizeable change through: (1) owner approves at commit A, a new commit B lands adding the breaking change, gate still passes because it only checks "was there ever an APPROVED review" — GitHub's own "dismiss stale approvals on push" setting is not reproduced here; (2) owner approves, then later submits CHANGES_REQUESTED on the same commit, the earlier APPROVED review is still in the list and still satisfies the gate.
Fix: Filter to select(.commit_id == env.HEAD_SHA) and reduce to each reviewer's most recent review state before checking for APPROVED.
B5: Base checkout diffs against the base branch's current tip, not the PR's actual merge-base
File: .github/workflows/api-review-gate.yaml:37-38,54-57
Issue: BASE_SHA: ${{ github.event.pull_request.base.sha }} is the live tip of the target branch, not git merge-base <base> <head>.
Evidence: Confirmed on this PR right now: gh api repos/cozystack/cozystack/pulls/3167 --jq .base.sha → a31a73cb8, while git merge-base origin/main origin/ci/gate-api-changes → b25a1327, ten commits apart. Any API-relevant commit that lands on main while this PR is open gets diffed against this PR's unrelated head, producing false positives or false negatives that have nothing to do with the PR's actual change.
Fix: Materialize the worktree at git merge-base "$BASE_SHA" "$HEAD_SHA" instead of $BASE_SHA directly.
B6: A real first-party cozystack.io CRD is outside every configured glob
File: internal/apigate/snapshot.go:33-39
Issue: packages/system/cozystack-scheduler/charts/cozystack-scheduler/crds/cozystack.io_schedulingclasses.yaml (the SchedulingClass CRD) doesn't match any entry in crdGlobs — it lives under charts/*/crds/ rather than a */definitions/ directory like the other typed-group manifests.
Evidence: Verified the file exists and its group: is cozystack.io, matching the exact API-group family the PR's own coverage table claims to gate. filepath.Glob against the five configured patterns does not match it.
Fix: Add its glob to crdGlobs (and add a snapshot fixture regression test so a future misplaced CRD directory is caught here, not by an incident).
B7: Two more breaking-change vectors this resource type already uses are unchecked
File: internal/apigate/parse.go:116-121 (served version), internal/apigate/schemadiff.go (whole file, CEL)
Issue: (a) ParseCRDs never reads versions[].served; the "removed served version" check in classify.go:111 only looks at whether the version key still exists in spec.versions[], not whether it's still served. The standard Kubernetes CRD deprecation flow (flip served: true → false, keep the version block, remove it later) is invisible to this exact, by-name feature. (b) x-kubernetes-validations (CEL rules) are never diffed, even though backups.cozystack.io_backupjobs.yaml, strategy.backups.cozystack.io_etcds.yaml, and strategy.backups.cozystack.io_mariadbs.yaml already ship CEL rules today — adding or tightening one can reject previously-valid updates with no signal from this gate.
Fix: Track served alongside the schema per version and flag served: true → false as the version-removal case; add a CEL-rule diff (at minimum: flag any change to x-kubernetes-validations as breaking, conservative-by-default like the rest of the file).
Tests gate
None of B1, B2, B6, or B7 have any test coverage, despite schemadiff_test.go and parse_test.go already establishing a table-driven convention for exactly this kind of case (safe vs. breaking, additive vs. narrowing). Per the existing convention, each of these needs a case that fails on the current code and passes after the fix — not just a happy-path addition.
Non-blocking follow-ups
internal/apigate/parse.go:140—apiserverStorageRe's[a-z0-9]+character class excludes hyphens; no currently-registered plural has one, so this is forward-looking, but a future hyphenated plural (e.g.ip-addresses) would silently fail to parse. Widen to[a-z0-9-]+.- Given how many subtle correctness bugs surfaced in ~150 lines of hand-rolled JSON-Schema diffing, consider whether a purpose-built CRD-compatibility checker (e.g. the class of tool OpenShift's
crd-schema-checkerrepresents) would carry less ongoing bug surface than maintaining this by hand — not a blocker for this PR, but worth a look before this becomes a required check other teams depend on.
| return out | ||
| } | ||
|
|
||
| func diffNode(path string, base, head Schema, out *[]string) { |
There was a problem hiding this comment.
B1(a) — diffNode proceeds with a nil head when a nested schema (items/additionalProperties) is entirely removed, misreporting the widening as a break. See B1 in the review body for the full mechanism and repro.
| // Type narrowing: any type the base accepted that head no longer accepts | ||
| // is breaking. Widening (head adds types) is safe. | ||
| bt, ht := schemaTypes(base), schemaTypes(head) | ||
| if len(bt) > 0 { // base constrained the type at all |
There was a problem hiding this comment.
B1(b)/(c) — the len(bt) > 0 guard skips the "type constraint added to a previously-unconstrained field" case entirely, and separately mishandles head becoming unconstrained as a narrowing. See B1 in the review body.
|
|
||
| // Map values (additionalProperties as a schema) and array items: recurse so | ||
| // nested breaks in maps/lists are caught. | ||
| if b, h, ok := schemaChild(base, head, "additionalProperties"); ok { |
There was a problem hiding this comment.
B2 — additionalProperties: false never reaches this recursion (schemaChild only extracts map-shaped schemas), so restricting it to false is never flagged as breaking. See B2 in the review body.
| } | ||
|
|
||
| // Breaking changes: resources present in both, whose schema regresses. | ||
| for _, res := range sortedResources(head) { |
There was a problem hiding this comment.
B3 — this loop (and the two above it) iterate only head; nothing checks for a resource present in base and gone from head, so removing an entire resource/CRD produces zero findings. See B3 in the review body.
| # Latest review state per reviewer; an APPROVED review by any API | ||
| # owner satisfies the gate. GitHub forbids self-approval, so a | ||
| # sizeable PR authored by an owner still needs the other owner. | ||
| approvals="$(gh api \ |
There was a problem hiding this comment.
B4 — this query isn't scoped to commit_id == HEAD_SHA and doesn't reduce to each reviewer's latest state, so a stale or superseded approval still satisfies the gate. See B4 in the review body.
| # Space-separated GitHub logins allowed to satisfy the gate. Edit here to | ||
| # change who can approve sizeable API changes. | ||
| API_OWNERS: "lllamnyp kvaps" | ||
| BASE_SHA: ${{ github.event.pull_request.base.sha }} |
There was a problem hiding this comment.
B5 — pull_request.base.sha is the base branch's current tip, not the PR's merge-base; confirmed on this PR right now (10 commits apart). See B5 in the review body.
| // manifests for the typed API groups. Kept explicit (rather than a repo-wide | ||
| // scan) so the gate's surface is auditable and a new manifest home is a | ||
| // deliberate one-line addition here. | ||
| var crdGlobs = []string{ |
There was a problem hiding this comment.
B6 — crdGlobs misses packages/system/cozystack-scheduler/charts/cozystack-scheduler/crds/cozystack.io_schedulingclasses.yaml, a first-party cozystack.io CRD. See B6 in the review body.
| Origin: origin, | ||
| Versions: map[string]Schema{}, | ||
| } | ||
| for _, v := range doc.Spec.Versions { |
There was a problem hiding this comment.
B7(a) — versions[].served is never read, so flipping a version to served: false (the standard CRD deprecation step) is invisible to the "removed served version" check. See B7 in the review body.
Fix schema-diff correctness: guard nil head on removed child schemas, flag adding a type to a previously-unconstrained field, stop reporting a head becoming unconstrained as narrowing, detect additionalProperties:false, and only recurse items/additionalProperties when both sides carry a schema. Gate resource and group removal as new categories. Filter CRD versions to served ones so the deprecation flow surfaces as a removed version, and flag added x-kubernetes-validations (CEL) rules. Discover first-party cozystack.io CRDs by walking crds/ and definitions/ dirs so a relocated CRD (e.g. cozystack-scheduler) is covered, and error on an empty snapshot. Surface unmapped apiserver storage prefixes and allow hyphens in plurals; drop Markdown from text-mode reports. Workflow: diff against the true merge base, count only each reviewer's latest review on the current head commit, and scope the token to the approval step with persist-credentials:false. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Thanks for the thorough review — all seven blockers were real. Addressed in the latest commit, each with a regression test that fails on the prior code and passes now. Blockers
Inline / non-blocking
On the second follow-up (an off-the-shelf CRD-compatibility checker like |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/apigate/schemadiff.go`:
- Around line 131-146: The schema diff logic in diffNode is missing
breaking-change detection for head-only typed child schemas under
additionalProperties and items. Update the additionalProperties and items
branches so head-only map-shaped schemas (for example, a typed schema introduced
where base was absent/true) are treated as restrictive changes and recorded as
breaking, while preserving the existing recursion only when both sides carry
compatible map schemas via bothMapSchemas. Use the diffNode, bothMapSchemas, and
the additionalProperties/items handling blocks to locate the fix.
In `@internal/apigate/snapshot.go`:
- Around line 52-55: The CRD group filter in snapshot.go is using a raw suffix
check with crdGroupSuffix, which can incorrectly accept names like
fakecozystack.io as first-party. Update the logic around crdGroupSuffix and the
HasSuffix check in the CRD discovery path to require a dot boundary before
cozystack.io, so only groups matching *.cozystack.io are treated as first-party.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3df12b7-c007-4a26-a3bb-a761fe413e13
📒 Files selected for processing (10)
.github/workflows/api-review-gate.yamlinternal/apigate/classify.gointernal/apigate/classify_test.gointernal/apigate/model.gointernal/apigate/parse.gointernal/apigate/parse_test.gointernal/apigate/report.gointernal/apigate/schemadiff.gointernal/apigate/schemadiff_test.gointernal/apigate/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/apigate/parse.go
- .github/workflows/api-review-gate.yaml
- internal/apigate/schemadiff_test.go
Scope the gate's own files (cmd/api-gate, internal/apigate, the workflow) to the API owners in CODEOWNERS, and compile the detector from the trusted base checkout when present (head fallback only during bootstrap), so a PR cannot neuter the gate by editing its own detector code. Detect two more breaking vectors: removing nullable:true (rejects null) and adding a oneOf/allOf/not composition constraint. Both covered by regression tests. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Follow-up review addressed. 1 (blocking) — a PR could neuter its own gate. Correct, and the bootstrap constraint is real (the merge base has no
Per the repo's convention against static-file/grep tests, this one is verified procedurally rather than with a parsing test — the e2e check is: open a PR that short-circuits 2 (recommended) — 3 (recommended) —
|
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — six of the seven prior blockers are fixed and verified, but one issue of the same class survives: a schema-shaped items/additionalProperties constraint added where base had none is still undetected.
Fixed and verified
- Type-constraint diffing (nil-head, type-added-to-unconstrained, type-removed) — fixed, covered by new table-driven cases.
additionalProperties: falsetransitions — fixed, covered (true→false, absent→false, false→true safe).- Resource/group removal — fixed;
RemovedGroup/RemovedResourcecategories added symmetrically to the existing new-group/new-resource logic, with matching tests. - Approval no longer satisfied by a stale or superseded review — fixed; the
jqfilter now scopes tocommit_id == HEAD_SHAand reduces to each reviewer's latest state. - Base checkout now diffs against the true
git merge-base, not the base branch's live tip. - The previously-missed first-party CRD is covered: discovery now walks
crds//definitions//manifests/directories by location +cozystack.io-suffixed group instead of a fixed glob list, with a dedicated regression test for a CRD under a chart'scrds/dir. servedversion handling and CEL (x-kubernetes-validations) rule diffing are both implemented and tested.
Blockers
B1: A schema-shaped items/additionalProperties constraint added where base had none is still undetected
File: internal/apigate/schemadiff.go:151-167 (bothMapSchemas, internal/apigate/schemadiff.go:276-283)
Issue: bothMapSchemas only recurses when both base and head carry a map-shaped child schema. The boolean additionalProperties: false transition is now handled separately, but a typed items/additionalProperties schema introduced where base had none (base absent, or head restricts a previously-unconstrained array/map) still requires both sides to already have one, so the recursion — and therefore any finding — never fires.
Evidence: Reproduced directly against the current code:
- base
{"type":"array"}(noitems) → head{"type":"array","items":{"type":"object","properties":{"foo":{"type":"string"}}}}→diffSchemareturnsnil. An array element that was previously unconstrained (e.g. an integer) is now rejected by the newitemsschema — a breaking change with zero signal. - base
{"type":"object"}(noadditionalProperties) → head{"type":"object","additionalProperties":{"type":"string"}}→ same result:nil, despite restricting previously-unconstrained extra values to strings.
None of the existingadditionalProperties/itemstest cases inschemadiff_test.gocover this direction — every case there is about removing/relaxing a nested schema, never about adding one from an absent state.
Impact: This is the same failure mode B1(b) from the previous round covered for the top-leveltypekeyword, just one level of nesting deeper — a real breaking change ships with no gate signal.
Fix: Mirror the top-level type-diff fix: when head carries a map schema atitems/additionalPropertiesand base does not (or base's value is the permissive default), diff against an empty schema rather than skipping.
Tests gate
B1 above has zero coverage in schemadiff_test.go despite that file's now-extensive table-driven convention covering the sibling cases (safe relaxation, boolean-false restriction, type-added-to-unconstrained). Per the existing convention this needs a case that fails on the current code and passes after the fix.
Non-blocking follow-ups
internal/apigate/snapshot.go:55—strings.HasSuffix(res.Group, crdGroupSuffix)matches any group literally ending incozystack.io, not just*.cozystack.io(no separating-dot check), so a hypothetical vendored group likefakecozystack.iowould be misclassified as first-party. No group in the repo currently triggers this, so it's forward-looking.
| // misreport that relaxation as a break. | ||
| if base["additionalProperties"] != false && head["additionalProperties"] == false { | ||
| *out = append(*out, fmt.Sprintf("%s: additionalProperties restricted to false (undeclared fields no longer accepted)", path)) | ||
| } else if b, h, ok := bothMapSchemas(base, head, "additionalProperties"); ok { |
There was a problem hiding this comment.
B1 — bothMapSchemas requires both sides to already carry a map schema before recursing, so a typed items/additionalProperties schema introduced where base had none (base absent or permissive) is never diffed. Repro: base {"type":"object"} → head {"type":"object","additionalProperties":{"type":"string"}} returns no finding. See the review body for the full mechanism and the items variant.
childSchemas now recurses into items/additionalProperties whenever head defines a schema, diffing against an empty base when base had none. This catches constraining a previously-unconstrained array element or map value (base absent/true -> schema), while still treating additionalProperties:false -> schema and schema removal as safe relaxations. Also tighten the first-party group check to cozystack.io and its dot-delimited subdomains so a lookalike like fakecozystack.io is not gated. Both covered by tests. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Both addressed. B1 (blocking) — constraint added to a previously-open
Tests: Non-blocking — group suffix. Tightened:
|
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 @.github/CODEOWNERS:
- Around line 3-11: The CODEOWNERS rules still leave the CODEOWNERS file itself
covered by the catch-all owner, so the review gate can be changed without
API-owner approval. Update the CODEOWNERS entries to add an explicit ownership
rule for the CODEOWNERS file itself, using the existing API owners (`@kvaps` and
`@lllamnyp`), so changes to the ruleset require their review just like the
API-gate paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 225825a1-efd9-42ee-bcb3-c6859b300edb
📒 Files selected for processing (6)
.github/CODEOWNERS.github/workflows/api-review-gate.yamlinternal/apigate/parse_test.gointernal/apigate/schemadiff.gointernal/apigate/schemadiff_test.gointernal/apigate/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/apigate/snapshot.go
- .github/workflows/api-review-gate.yaml
- internal/apigate/schemadiff_test.go
- internal/apigate/parse_test.go
| # The API review gate must not be editable without an API-owner review, or a | ||
| # PR could neuter the detector (it is compiled from the head checkout during | ||
| # bootstrap) and slip a sizeable API change through unreviewed. Scoping these | ||
| # paths to the API owners overrides the catch-all above (last match wins), so | ||
| # changes here require @kvaps or @lllamnyp specifically. Enable "Require review | ||
| # from Code Owners" in branch protection for this to bind. | ||
| /cmd/api-gate/ @kvaps @lllamnyp | ||
| /internal/apigate/ @kvaps @lllamnyp | ||
| /.github/workflows/api-review-gate.yaml @kvaps @lllamnyp |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the CODEOWNERS file and nearby repo context.
git ls-files .github/CODEOWNERS .github | sed 's#^`#FILE` #'
echo
echo "---- .github/CODEOWNERS ----"
cat -n .github/CODEOWNERS
echo
echo "---- Search for other CODEOWNERS references ----"
rg -n --hidden --glob '!**/.git/**' 'CODEOWNERS|Require review from Code Owners|api-review-gate|api gate|apigate' .Repository: cozystack/cozystack
Length of output: 5144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line-numbered context around the relevant CODEOWNERS entries.
sed -n '1,40p' .github/CODEOWNERS | cat -nRepository: cozystack/cozystack
Length of output: 935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check CODEOWNERS semantics in docs if present in repo.
rg -n --hidden --glob '!**/.git/**' 'CODEOWNERS' README* docs .github 2>/dev/null || trueRepository: cozystack/cozystack
Length of output: 1666
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.github/CODEOWNERS')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if line.strip() and not line.lstrip().startswith('#'):
print(f"{i}: {line}")
PYRepository: cozystack/cozystack
Length of output: 422
Protect .github/CODEOWNERS too.
The API-gate paths are covered, but .github/CODEOWNERS itself still falls under the broad * rule. Add an explicit /.github/CODEOWNERS Andrei Kvapil (@kvaps) @lllamnyp`` entry so changes to the ruleset also require an API-owner review.
🤖 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 @.github/CODEOWNERS around lines 3 - 11, The CODEOWNERS rules still leave the
CODEOWNERS file itself covered by the catch-all owner, so the review gate can be
changed without API-owner approval. Update the CODEOWNERS entries to add an
explicit ownership rule for the CODEOWNERS file itself, using the existing API
owners (`@kvaps` and `@lllamnyp`), so changes to the ruleset require their review
just like the API-gate paths.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the remaining blocker from the prior round is fixed and verified; no blockers remain.
Fixed and verified
items/additionalPropertiesschema constraints added where base had none (base absent or permissive) are now caught: the child-schema helper requires only the head side to carry a schema, diffing it against an empty base when base has none, while a base-only schema is still correctly treated as a relaxation. Covered by new table-driven cases (adding additionalProperties schema to open map is breaking,adding items schema to unconstrained array is breaking, plus thefalse -> schemarelaxation case).- The first-party group check now requires an exact match or a dot-delimited
.cozystack.iosubdomain rather than a bare suffix, so a lookalike group name can't be misclassified as first-party. Covered by a dedicated test. - Build,
go vet, the full test suite, andgolangci-lintare all clean.
Non-blocking follow-up
.github/CODEOWNERSitself isn't covered by the new API-owner-scoped rules (/cmd/api-gate/,/internal/apigate/,/.github/workflows/api-review-gate.yaml), so it still falls under the repo-wide catch-all with a broader owner set. Since this file is the source of truth for those very protections, consider adding an explicit/.github/CODEOWNERSentry scoped to the API owners as well — otherwise the protection can be loosened by an approval from outside that set. Low practical risk given the primary tamper defense (building the detector from the trusted base checkout) doesn't depend on it.
…ate (#3184) ## What this PR does Closes two coverage gaps found while shadow-observing the API-owner-review gate (#3167) against real PRs in the first hours after merge: 1. **CRD discovery was gated on directory name** (`crds/`, `definitions/`, `manifests/`), so a CRD shipped inside a chart's `templates/` dir — a valid, existing convention when the CRD is wrapped in a `{{- if }}` guard — was invisible to the gate. This was not hypothetical: `#3149` (cozyplane) adds four brand-new CRDs (`VPC`, `VPCBinding`, `VPCPeering`, `Port`) exactly this way, and none of them tripped a review requirement. 2. **Groups served only by an external aggregated apiserver had no representation at all.** The same `#3149` also registers `sdn.cozystack.io` via a `kind: APIService` pointing at a prebuilt image built outside this repository — no CRD, no vendored Go types, nothing to diff. ### Fix - CRD/APIService discovery is now content-based: every `.yaml`/`.yml` file under `packages/` and `internal/` is a candidate, and whether it's kept depends on whether it parses into the right `kind` with a first-party group — not which directory it sits in. - Both parsers now tolerate a Helm directive line (`{{- if }}`, `{{- end }}`, a comment block) sharing a `---`-delimited document with otherwise-valid YAML, rather than dropping the whole document. This matters because the closing `{{- end }}` of a wrapping conditional has no `---` before it, so it lands in the same document as the manifest it closes. - Added a fourth resource source, `SourceAPIService`, for `kind: APIService` manifests registering a first-party group. Like the existing static Go-backed groups, these carry no checked-in schema, so they participate only in new-group / new-resource / removal detection — never a fabricated breaking-change diff against a schema this repo can't see. Verified against the real `feat/cozyplane` branch: the gate now reports all four new CRDs, plus the removal of the old `securitygroups`/`APIService` registration for the same group — a full API-surface migration that was previously invisible end to end. ### Release note ```release-note fix(ci): the API-owner-review gate now discovers CRDs and APIService registrations by content instead of directory name, closing a gap where a CRD or an externally-served aggregated API group could ship without triggering the required review. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Discovery now includes APIService registrations, enabling detection of groups/versions exposed only via aggregated APIService manifests (including merging versions from multiple templates). * **Bug Fixes** * Improved YAML discovery to tolerate mixed/templated multi-document streams: non-CRD/non-APIService documents no longer stop discovery, and embedded template directives are handled more reliably. * File processing is more resilient during snapshot loading (includes a safety cap on YAML size). * **Tests** * Added coverage for APIService discovery, templated neighbor documents, and correct exclusion of third-party manifests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds a CI gate that requires an approving review from a designated API owner (
lllamnyporkvaps) when a pull request makes a sizeable change to the Cozystack API surface. Sizeable means one of:Detection is fully deterministic — no LLM or network calls, so it runs on every PR.
How it works
cmd/api-gate+internal/apigate— a small Go tool that loads the API surface from two checkouts (merge base and PR head) and classifies the delta. Every resource's schema is normalized to a parsed OpenAPIv3Schema; the tool only unwraps it from its two container formats and reads identity (group/kind/plural) from the wrapper.apps.cozystack.io)packages/system/*-rd/cozyrds/*.yamlspec.application.openAPISchemacozystack.io,backups.*,network.*,gateway.*)*.cozystack.ioCRD discovered under acrds/ordefinitions/directory inpackages/andinternal/spec.versions[].schema.openAPIV3Schemacore.*,sdn.*)pkg/apiserver/apiserver.gostorage registrationsCRDs are found by location + content rather than a fixed file list, so a first-party CRD that moves (e.g. into a chart's
crds/dir) stays covered. OnlyservedCRD versions count as live API surface, so the standard deprecation step (served: true → false) registers as a removed version. An empty snapshot is treated as an error, never a silent pass.Breaking classification is semantic, not textual. Additive changes (new optional field, new enum value, widened type, relaxed or removed constraint, description/default edits) pass. Flagged as breaking: removed fields, type narrowing (including adding a type to a previously-unconstrained field), enum-value removal, newly-required fields, added/tightened
pattern/min/max/length constraints,additionalPropertiesrestricted tofalse, addedx-kubernetes-validations(CEL) rules, and removed served versions..github/workflows/api-review-gate.yamlbuilds the tool, diffs the PR head against its true merge base, and — when the change is sizeable — requires anAPPROVEDreview from an API owner. Only each reviewer's latest review on the current head commit counts, so a stale or superseded approval no longer satisfies the gate. It re-runs onpull_request_reviewso an approval flips the check without a new push. The token is scoped to the approval step alone (persist-credentials: falseon checkout) so PR-controlled build steps can't read it. The allowlist lives in one place (API_OWNERS). The job runs on every PR (no path filter) so it can safely be marked a required check.Follow-up needed after merge
Mark "Require API owner review for sizeable API changes" as a required status check in
mainbranch protection for the gate to block merges.Tests
Table-driven unit tests cover the schema-diff rules, the classifier (new group / new resource / breaking, including the double-count and pure-removal edge cases), all three parsers, and an on-disk end-to-end snapshot test. Verified against the real repo tree: identical trees pass; injected new-group/new-resource/breaking changes are all caught; an additive-only edit passes.
Release note
Summary by CodeRabbit