docs: review the configuration surface, and record three consumer asks against the published contract - #272
Merged
Merged
Conversation
Record the review of docs/configuration.md and docs/architecture.md as a future-work proposal, and fix the factual errors it turned up: - cluster-scoped resources use the literal `_cluster/`, not `cluster/` (internal/types/identifier.go, internal/manifestanalyzer/placement.go) - the placement section linked the same file twice under two stale `design/manifest/...` labels, one claiming to be a different document - the type-followability link carried a stale label - the design-document list linked architecture.md to itself; it now points at the support contract, which the list was missing The proposal itself argues the engine learned a lot about a folder — layout, render roots, read-only context, ambiguity, a support verdict — while the API still exposes one bit of it, and orders the work that would close that gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ract Our first downstream consumer links pkg/manifestanalyzer, execs the CLI from a second tool, and runs the reverser as an image — so it feels every seam in what we publish. All three of its asks are one complaint: a distinction our doc comments assert, carried by prose nothing tests, encoded downstream after it went stale. Written for an implementing party, with each claim checked against the tree: - RefusalReason carries no permanence, and the code space was never two-valued — issuesToReasons sets Code from every IssueKind, so a folder refused for invalid-yaml was reported to its owner as "not supported yet". Includes our classification of all eighteen kinds, which no consumer can maintain, and isolates the three that need a decision rather than a lookup. - The JSON report names no analyzer version, the CLI has no --version, and no binary ships — three pins on one release, none of them recorded. - ResourceIdentifier.Key() is a cross-product identity contract with no test at all; its format can change today with every gate green. Also fixes the stale comment the first ask blames: LayoutKustomizeOverlay still said such folders were "refused today, with a forward-looking reason", four lines above the constant saying the scanner adopts them. That sentence is what the consumer's code was written from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds analyzer consumer-contract requirements, proposes structured repository configuration changes, updates documentation links and path conventions, and revises ChangesDocumentation and contract clarification
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/INDEX.md (1)
68-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale open-item count.
The table now contains thirteen open items, including the newly added analyzer-contract document, but the heading still says “Twelve other open items.” Update the count or remove the stale quantifier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/INDEX.md` around lines 68 - 74, Update the open-item heading in docs/INDEX.md to reflect the table’s thirteen entries, or remove the stale numeric quantifier; leave the listed documents and descriptions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/analyzer-consumer-contract-asks.md`:
- Around line 234-243: Clarify the contract around ReportProvenance by
documenting the exact RepoReport and FolderReport JSON nesting and placement of
analyzerVersion. State that each report must include a non-empty
analyzerVersion, including a defined fallback for unreleased or unavailable
versions, and remove or revise omitempty so the field cannot be omitted.
- Around line 124-140: Reconcile the permanence inventory with the complete set
of emitted issue variants, including refused-structural and the special branches
unsupported-kustomize, unresolved-krm, and kustomize-render-refused. Update the
inventory and classification tests to enumerate every variant, then add
branch-level coverage for each per-construct emission path so any newly emitted
issue must have an explicit permanence classification.
In `@docs/future/config-surface-for-a-structured-repository.md`:
- Around line 334-346: Update the namespace-inference example around all.yaml so
it satisfies the documented cohort rule: show ConfigMaps from at least two
existing namespaces in all.yaml before describing a new billing ConfigMap being
appended there. Alternatively, revise the example’s expected destination to the
canonical layout; keep the explanation consistent with the chosen behavior.
---
Outside diff comments:
In `@docs/INDEX.md`:
- Around line 68-74: Update the open-item heading in docs/INDEX.md to reflect
the table’s thirteen entries, or remove the stale numeric quantifier; leave the
listed documents and descriptions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f327355-a20a-4fc3-bacf-ab2c22818298
📒 Files selected for processing (6)
docs/INDEX.mddocs/architecture.mddocs/configuration.mddocs/design/analyzer-consumer-contract-asks.mddocs/future/config-surface-for-a-structured-repository.mdpkg/manifestanalyzer/repo.go
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…nce example Review findings from PR #272, all four verified against the tree before fixing: - The kind count was wrong and hid a real gap. internal/manifestanalyzer defines seventeen IssueKinds, not eighteen, and pkg/manifestanalyzer re-declares only fourteen — so kustomize-render-refused, render-does-not-match-live and unplaceable-edit can reach a consumer through issuesToReasons with no exported constant to match on. Recorded as a second gap to close in the same pass. - A per-constant classification test cannot reach the per-construct branches of unsupported-kustomize, unresolved-krm and kustomize-render-refused, which is the original bug one level down. The ask now requires per-emission-path coverage. - analyzerVersion was specified as a nested, omitempty field, which contradicts the ask it serves: a report must always say what produced it. It is now a non-empty top-level string on both reports, with the fallback chain stated. - The C3 inference example did not satisfy its own precondition. A cohort admits a never-seen namespace only once it holds more than one (configuration.md 652-655), so a bundle of nine ConfigMaps in one namespace inverts the answer. The example now spans two namespaces, and its failure mode is the sharper one: the file is still there and still full of ConfigMaps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/design/analyzer-consumer-contract-asks.md (3)
313-315: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the release platform matrix normative.
“Decision for the implementer” and “the obvious third” do not satisfy the requirement for an explicit platform matrix. Choose and document the required
GOOS/GOARCHtargets, including which binaries receive release attestations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/analyzer-consumer-contract-asks.md` around lines 313 - 315, Update the platform decision in the design document to explicitly define the normative GOOS/GOARCH release matrix, replacing the tentative wording about implementer choice and the “obvious third” platform. Specify which target binaries are built and which receive release attestations.
125-131: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine the exact serialized shape of
Issue.The text says
IssuecarriesPermanenceandActor, but does not specify whether they are top-level fields, their JSON names, or their zero-value/omitemptybehavior. Add the correspondingIssuestruct shape and require tests for it, just as the document does forRefusalReason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/analyzer-consumer-contract-asks.md` around lines 125 - 131, Define the serialized Issue shape in the contract, explicitly showing top-level Permanence and Actor fields, their exact JSON names, and zero-value/omitempty behavior. Add tests covering serialization of these fields, including zero values, matching the documented RefusalReason coverage.
286-297: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftResolve
SchemaVersionsemantics instead of leaving them as open questions.This section asks the implementer to decide what a bump means and how readers handle unknown versions, but does not state the contract. Document which changes require a bump and whether unknown versions must be rejected or parsed best-effort; otherwise consumers will implement incompatible policies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/analyzer-consumer-contract-asks.md` around lines 286 - 297, Update the SchemaVersion section to define its contract explicitly: state which schema changes require a version bump, clarify whether a bump covers field removal, meaning changes, or both, and specify whether readers that know v1 must reject v2 or parse it best-effort. Also explain what breaking changes remain after additive fields are explicitly excluded from bumping.
🤖 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.
Outside diff comments:
In `@docs/design/analyzer-consumer-contract-asks.md`:
- Around line 313-315: Update the platform decision in the design document to
explicitly define the normative GOOS/GOARCH release matrix, replacing the
tentative wording about implementer choice and the “obvious third” platform.
Specify which target binaries are built and which receive release attestations.
- Around line 125-131: Define the serialized Issue shape in the contract,
explicitly showing top-level Permanence and Actor fields, their exact JSON
names, and zero-value/omitempty behavior. Add tests covering serialization of
these fields, including zero values, matching the documented RefusalReason
coverage.
- Around line 286-297: Update the SchemaVersion section to define its contract
explicitly: state which schema changes require a version bump, clarify whether a
bump covers field removal, meaning changes, or both, and specify whether readers
that know v1 must reject v2 or parse it best-effort. Also explain what breaking
changes remain after additive fields are explicitly excluded from bumping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: de6fe501-5ad1-4757-8244-7139a7168073
📒 Files selected for processing (3)
docs/INDEX.mddocs/design/analyzer-consumer-contract-asks.mddocs/future/config-surface-for-a-structured-repository.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/INDEX.md
apiVersion + kind replaces schemaVersion and dissolves the three open questions about what a bump asserts: the Kubernetes API conventions already answer them, in a document every consumer of a GitOps tool has read. The spec/status split turns out to be honest here — spec is the scan request, status the observation — which also gives today's floating "informational" root field a home. Carved out: no metadata (a report has no identity, and the field invites an apply that will never work — kpt's ResourceList sets the same precedent), and never served or registered. Costs one breaking change to the JSON contract, which argues for doing it inside Ask 2 while there is one consumer to coordinate with. It does not replace Ask 2: apiVersion versions the contract, analyzerVersion records the build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r issues
A second team filed four issues against v0.39.1, verified by measurement rather
than by reading. Three of their asks were already covered; five points were not:
- The block a consumer reads first is the one that misleads. "Refusal reason
codes a candidate may carry" lists two of eighteen, one retired, and nothing
says RefusalReason.Code draws from IssueKind — a more direct cause of their
wrong sentence than the Layout comment was. Type the field, fix the header.
- Their permanence enum has two values; ours keeps four. A two-valued enum drops
the not-yet axis, which is the exact distinction that went degenerate.
- --version must print the SchemaVersion it emits, not only the release.
- Their generator{name,version} beats our flat analyzerVersion: a piped report
needs to say which tool, not only which release.
- Key()'s golden test needs a fourth case, core cluster-scoped (/v1/nodes/node-1),
where group and namespace are both empty and degenerate in opposite ways. All
four shapes confirmed by running them.
The two teams disagree on one point — attestation versus checksums — so the doc
now says do both: the attestation is the stronger claim, the checksum is what a
Dockerfile can consume without a token.
Also records that the minimal-version-selection diamond is explicitly not our
bug, so nobody restructures the module to solve it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The KRM envelope is adopted rather than recommended: apiVersion/kind replaces
schemaVersion, spec carries the scan request and status the findings. Taken as a
deliberate breaking change now, while there are two consumers and both are asking
for version clarity in this same document. The provenance field settles as
status.generator{name,version} — the second team's shape, because a piped report
needs to say which tool and not only which release.
The three unclassified refusal codes are decided during implementation: the
session proposes a permanence per raise site and raises each as a review comment
for the maintainer to accept or overturn. They never ship as PermanenceUnknown,
and they no longer block the other fourteen.
Also folds the SchemaVersion section into the envelope decision that answers it,
rather than leaving three open questions beside their own answer.
Co-Authored-By: Claude Opus 5 (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.
Two documents of planning work, plus the factual fixes each one turned up while being written.
What is here
1. The configuration-surface review —
docs/future/config-surface-for-a-structured-repository.mdspec.pathis still one string, while the folder it names has acquired structure: a layout, render roots, read-only context, build directives, an inferred placement convention, and a support verdict. The engine computes all of it; the GitTarget API exposes one bit (GitPathAccepted). The document reviewsconfiguration.mdandarchitecture.md, then orders the work that would close that gap —mode: Observe,status.layout,placement.mode, and movingcommitWindowoffGitProvider.The sharpest case it makes is C2: pointing a GitTarget at
apps/podinforather thanapps/podinfo/overlays/prodgives two render roots and an ambiguous base, so akubectl scalein prod rewrites the base and changes test too. Both spellings reportReady=True; the degradation is a debug log line.2. Three asks from our first downstream consumer —
docs/design/analyzer-consumer-contract-asks.mdThey link
pkg/manifestanalyzer, execmanifest-analyzer --mode scan-repo --format jsonfrom a second tool that does not link us, and run the reverser as an image. All three asks are one complaint: a distinction our doc comments assert, carried by prose nothing tests. One of them shipped a wrong sentence to a real user because of it.RefusalReason. The code space was never two-valued:issuesToReasonssetsCodefrom everyIssueKind, so the live values are eighteen codes plusrefused-structural, and most are fixable today by the person reading the screen. A folder refused forinvalid-yamlwas reported to its owner as "not supported yet". Carries our classification of all eighteen — the part no consumer can maintain — and isolates the three that need a product decision rather than a lookup.--version, and no binary ships. That is a third pin on one release, recorded nowhere.ResourceIdentifier.Key()as a documented contract. Verified: it has no test at all. We can change the format today with every gate green.Written for an implementing party — nothing in either document is implemented here.
Fixes included
Both reviews turned up documentation that was untrue, corrected in the commit that found it:
_cluster/, notcluster/(identifier.go,placement.go).design/manifest/…labels, one captioned as a different document.architecture.mdto itself; it now points at the support contract.LayoutKustomizeOverlaystill said such folders are "refused today, with a forward-looking reason", four lines above the constant saying the scanner adopts them. That sentence is what the consumer's broken code was written from.Validation
task lint,task test(77.7%, baseline 77.8%, within the 0.5% tolerance) andtask test-e2e(69 passed, 0 failed, 22 skipped) all pass.The first e2e run failed in
BeforeSuitewithRan 0 of 91— the local k3d cluster was wedged with agentsNotReadyfor four days, unrelated to this diff.task clean-clusterand a re-run gave the result above.🤖 Generated with Claude Code
Summary by CodeRabbit