tests: enumerate the skip codes and the RBAC grants instead of listing them by hand - #100
Open
TamasSzigeti wants to merge 42 commits into
Open
tests: enumerate the skip codes and the RBAC grants instead of listing them by hand#100TamasSzigeti wants to merge 42 commits into
TamasSzigeti wants to merge 42 commits into
Conversation
…en list
engine.SkipCodes() enumerates sixteen values. The reachability half of
TestEverySkipReasonCarriesItsOwnCode — the loop whose own comment says
"every code the engine can emit must be reachable, or it is documentation
for a state that cannot happen" — then hard-coded ten of them.
The six it omitted were pool-disabled, gone, uncordoned,
autoscaler-not-live, protected-pod and too-many-pods. Five were asserted
by constant somewhere else in the package, by luck rather than design.
protected-pod was asserted nowhere at all: grep for SkipProtectedPod
across the test files returned nothing, while the branch that assigns it
sits beside three tests of the excluded-namespace path that all pin the
sentence — strings.Contains(a.SkipReason, "payments") — and never the
code.
The sentence is the half that is explicitly not public. The code is
published as binpack_nodes_skipped{code="protected-pod"} and documented
in docs/reference/metrics.md, so folding that branch into a neighbour or
reusing a nearby code leaves every existing test green while the series
stops existing and any alert keyed on it silently never fires again. The
vocabulary guard would not catch it either: TestEveryLabelValueBinpack
CanProduceIsDocumented compares the enumerator with the page, and both
would still agree about a value nothing emits.
So the loop now ranges over engine.SkipCodes() and the six missing
fixtures are added. Three of them — the pool switched off, a pod binpack
must not evict, and a node over the blast-radius cap — reach Decide and
are now cases in the table. The other three cannot: Decide assesses the
nodes the snapshot carries, so it can never see a node that is gone; it
passes resuming false, so eligibility's marked-but-schedulable branch is
unreachable from it; and it refuses above the assessments when the
autoscaler is not live, returning a decision code rather than a per-node
skip, which is the emptiness that stops a dead autoscaler zeroing the
node gauges.
Those three are recorded rather than dropped from the enumerator, in the
shape the differential harness's notReachedByGenerator uses: a map from
the code to the reason it cannot be reached here and the name of the test
that adjudicates it instead. Deleting them to quiet the loop would take a
published label value out of the vocabulary the reference is checked
against, which is the failure the enumerator exists to prevent. The
record is read in both directions — a code that starts reaching Decide
fails until the record catches up — because an entry claiming coverage
that has moved is worse than a gap, being believed.
TestExcludedNamespaceProtectsItsNode gains the assertion its own subject
implies. Sabotaged by making the branch assign SkipAnnotated instead: the
enumerated loop's new fixture fails and names the substitution, where
before the change nothing in the package noticed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
R3-160 records a three-way agreement between what the executor can do, what the chart grants and what the reference documents, guaranteed by four named tests. All four read documents. None reads the code, and TestTheRBACReferenceMatchesWhatTheExecutorDoes says in its own comment that it "checks the third side of an agreement the chart and the code already keep between them" — an agreement nothing kept. Both documents are hand-maintained against the code, so a verb removed from both stays consistent and wrong, and a verb added to the code and to neither is consistent and missing. Either way the suite is green, and the symptom is the one capability_doc_test.go already describes for the operator-managed case: binpack holds its lease, serves its metrics, publishes decisions, and 403s on its first node patch. TestTheChartGrantsWhatTheCodeCalls closes that corner. It drives all five writes binpack's own code makes — executor.Cordon, Annotate, HandBack and Evict, plus the decision Event the direct reporter creates — against a writer that records what an RBAC rule would have to grant, and compares that with the chart in both directions: a recorded pair the chart does not grant is a write nobody has been given permission for, and an act-gated pair the chart grants that nothing exercised is either a grant that has outlived its caller or a caller this test has stopped driving, at which point the first direction is comparing a shrunken set and passing for it. The recorder derives group and resource from the object through the scheme rather than being told per call site what each call is for. A recorder carrying its own description of Cordon and Evict would be a second implementation of the executor's call shapes, agreeing with the chart about a set of writes neither is any longer a reading of. It satisfies executor.Writer and controller.eventWriter, so a method added to either has to be answered before the test compiles. It lives in internal/controller rather than beside the other RBAC tests because directReporter is unexported and that is the only package that can see all five call sites. Verified by sabotage, three ways: withdrawing pods/eviction: create from the chart, adding a nodes/status patch to Cordon, and dropping Evict from the driven calls each fail it, and each names the thing that moved. The second half is the reader those guards run on. rulePairs matched apiGroups:/resources:/verbs: line by line and pulled values out of `[...]`, so a rule written as a block sequence — the ordinary Kubernetes idiom, and what a YAML formatter or a pasted upstream example produces — yielded nothing and was skipped in silence. Both consumers iterate the parsed set and guarded only with len(granted) == 0, which a partial parse is not: reformatting one rule shrinks the iteration and the tests pass. Demonstrated before the change by reformatting the chart's nodes: patch rule into block style and deleting it from docs/reference/rbac.md altogether. That is the only mutating verb binpack holds on cluster state, and an operator running rbac.create: false would have written a role without it. Both R4-017 guards stayed green. So the four hand-rolled readers of these two files become one. internal/rbacdoc parses YAML — the technique internal/collect's tests already used, which fatals rather than dropping a rule — and every way of finding nothing there is an error rather than a smaller answer: a documented block that declares apiGroups and decodes to no rules is this reader dropping one, and says so. It takes no *testing.T, for the reason internal/mother takes none. Each guard then gets a count in chart_test.go's shape rather than a non-empty check: thirty pairs against the thirty-six the chart renders, and two against the act block's two. A reader that stops seeing rules fails there instead of quietly checking the remainder. internal/executor's package doc says what the same sabotage says: its enumeration is now executable rather than only written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TamasSzigeti
marked this pull request as ready for review
August 26, 2026 10:17
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f26ef639b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tely
Four findings from review, all valid, all the same class as the ones this
PR set out to close: a reader that returns part of a permission set, and a
comparison that reads the shortfall as less to check.
**A nested conditional truncated the guarded block.** Section cut at the
first `{{- end }}`, which is the wrong one as soon as a guard contains
another — and this chart already nests leader election inside
`.Values.rbac.create`, so asking for that guard returned twenty-five of
the thirty-six pairs the file holds. Nothing asked it for that guard,
which is the only reason it had not been noticed. The harm the review
named is the one that matters: a nested `if` inside the act block drops
every rule after it from `gated`, so a later mutating permission could
move outside the opt-in guard with the reverse check still green. The
scanner now tracks depth. `template` is not an opener, unlike `define`
and `block`, which is the distinction that makes the count right.
**A partial decode was accepted.** The fallback unmarshal's error was
discarded, and encoding/json fills in what it can before reporting the
first field it could not: a page whose second rule has a malformed
`resources:` decodes to two rules of which one grants nothing, and Grants
emits nothing for it while the reader reports success. That is the line
reader's disappearing rule arrived at from the other side. An error now
discards the value rather than qualifying it, and a block that names
apiGroups and does not decode as rules is reported rather than skipped.
The review's own example — `verbs: [create, 1]` — does not reproduce:
sigs.k8s.io/yaml coerces the scalar to a string and returns no error at
all. The mechanism is real for a shape that changes a node's *type*, and
worse than described, because the rule survives into the result carrying
an empty field.
**The unconditional write was compared against the union.** Roles is
deliberately the union of every branch, which is the right reading of
what a chart could ever grant and the wrong one of what a default install
grants. dryRun defaults to true, and an evaluation reports its decision on
the node whatever it is set to, while every executor write is reached only
through advance — so moving the decision event's `create` inside
rbac.allowDraining left every assertion here green and would have 403ed
the first `--once` report on a default install. Verified: it did. The
driven writes are now split along the line the chart itself draws, and the
always-issued half is checked against the chart with that block removed.
**And the count guard could not see a deleted rule.** It measures the
reader, not the chart, and the two shrink the set identically. Deleting
the autoscaler-status ConfigMap rule — the object binpack's whole
no-cloud-credentials design rests on — cost three pairs, stayed above
thirty, and left the suite green. The chart-to-page comparison is now
read in both directions, which is what the page's own banner claims
anyway: chart ⊆ page says a role written from it is not missing a grant,
page ⊆ chart says it does not ask for one nobody renders, and only the
second notices the deletion. The two sets agree exactly at thirty-six
today. The count stays, with its comment corrected to say what it does
not catch.
Each fix re-run against the sabotage that motivated it. The nesting one
cannot fail against this chart, so it is shown instead by what it
returns: `.Values.rbac.create` now yields all thirty-six pairs, and a
nested `if` inside the act guard no longer hides the rule beneath it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69c8aa0eae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three more from review, all valid, all the same shape as the last round:
a comparison whose two sides are not readings of the same question.
**An act-only write was checked against the union rather than the guard.**
The previous commit fixed the unconditional half and left this one: `acting
⊆ granted` says a mutating write is granted *somewhere*, which `nodes:
patch` moved one line above `{{- if .Values.rbac.allowDraining }}` still
satisfies. A default install would then hold a verb that can cordon a node
while docs/reference/rbac.md promises it holds none — "without the Act
group it holds no verb that can cordon a node or evict a pod, whatever its
configuration says". The comparison is now an equality against the gated
set. The cli count guard did fire on that sabotage, incidentally and with
the wrong explanation, so its message now names both ways of getting there.
**And the pair could not say at what scope.** Grants flattens Role and
ClusterRole into group/resource/verb, so moving the events rule out of the
ClusterRole and into the namespaced autoscaler-status Role left every
comparison green. That Role is scoped to discovery.autoscalerNamespace,
kube-system by default; decision events about a Node are filed under
`default`, where `kubectl describe node` looks for them. Every write
binpack's own code makes needs a cluster-scoped grant — nodes are
cluster-scoped objects, eviction reaches whichever namespaces the chosen
node hosts — so the comparison now runs over the ClusterRole's rules alone.
That also settles where the act grants come from. A block lifted out of its
document carries no kind, so Section cannot tell a ClusterRole rule from a
namespaced one; the gated set is the difference between the chart rendered
with the guard and rendered without it, which is scope-aware because both
sides are. Section keeps its one caller, the reference comparison, where
kind is not the question.
**And the skip-code loop ran one way only.** It iterates SkipCodes(), so a
code removed from the enumerator is not visited, and every other check is
keyed on the enumerator too. Deleting SkipProtectedPod from it and from
docs/reference/metrics.md left the whole suite green with the branch still
emitting "protected-pod": no longer pre-initialised as a series, no longer
documented, still published. The codes Decide actually produced are now
compared back against the enumerator, which is the direction
TestEverySkipCodeDecideProducesIsEnumerated asks of its own smaller table
and this one had not been asking of its larger one.
Each verified against the sabotage that motivated it, and the five from
the previous rounds re-run to confirm they still fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7b0c7c91a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nothing The ClusterRole filter fixed the write side and left the read side flattened. Grants keys on group, resource and verb, which is the whole identity of a cluster-wide grant and not of a namespaced one: this chart renders two Roles into two different namespaces — the one the operator names through discovery.autoscalerNamespace, where the cluster-autoscaler publishes its status, and binpack's own — so a rule in the wrong one is granted where binpack does not read. Moving the ConfigMap read from the autoscaler-status Role to the leader-election Role left every test in this repository green. Any install whose release namespace differs from the autoscaler's — the ordinary case, since the autoscaler publishes into the namespace it runs in — would then 403 on every status read and report no autoscaler for ever, which is ADR-0004's entire basis failing from an install that came up clean. The namespaces cannot be compared: the chart writes both as Helm expressions and HelmToYAML renders them to the same placeholder. The names can, because what distinguishes the two is a literal suffix on a templated prefix, and chart_test.go already keys on `-autoscaler-status` for the binding half of this same promise. So Role carries its metadata name, and the new guard sits beside the binding test it completes: a Role bound in the right namespace that grants nothing binpack needs there fails exactly as one bound in the wrong namespace does. Two assertions, because either alone is satisfiable. The namespaced Roles must grant disjoint sets — the general form, and it needs no list. And the ConfigMap read must belong to the autoscaler-status Role specifically, because disjointness is satisfied by the two Roles swapping their rules wholesale. Writing it surfaced one more disappearance. decode skipped every document with no rules, so a Role stripped of its last rule vanished from the parse — and the guard above reported "found 1 namespaced Role, this test asserts nothing" for what was really "one of them now authorises nothing". That is the reader disappearing again, one level up from the rules: an empty Role is still an object an install creates and binds. A document declaring itself a Role or a ClusterRole is now kept whatever it grants, and only a document that is neither and carries no rules is skipped — which is the RoleBindings, and Section's lifted blocks, which have no kind to declare. The ten sabotages this pull request has accumulated across four rounds were re-run together; all ten still fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e5316282
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… granted Five more from review, all valid, and four of them the same flattening the last two rounds have been peeling apart one layer at a time. **A grant can be wrong on its own terms, and no comparison sees it.** Rule dropped resourceNames, so adding `resourceNames: [cluster-autoscaler-status]` to the ConfigMap rule left chart and page agreeing, every count above its floor and every scope check satisfied — while the controller's cache, which lists and then watches, held no permission at all. resourceNames restricts a request by the name in its path and list and watch requests carry none; the chart's own RBAC comment says exactly that, and is why it scopes the autoscaler-status Role by namespace rather than naming the object. The field is now parsed, carried into the pair so a narrowed rule is a different permission from the same triple unrestricted, and rbacdoc.Unauthorizable reports the combination that authorises nothing. That is the first guard here that is not a comparison, and it needed to be: two documents can agree about a rule neither of them makes work. **One anchor is not two.** The namespaced Roles had to grant disjoint sets and the ConfigMap read had to be the autoscaler-status Role's — which the Lease and core Event rules moving into that same Role satisfies, leaving the leader-election Role empty and no replica able to acquire leadership. Each Role is now anchored to what its own namespace is for, neither may grant nothing, and each anchor asserts both presence in the right Role and absence from every other. The ConfigMap anchor names get, list and watch rather than accepting any one of them, for the reason internal/collect's grantsRead gives: the cache lists and then watches, so a rule short of any of the three fails at runtime rather than being a narrower version of the same permission. **The page's grants were compared as a union too.** Documented has recorded each snippet's kind since the scope fix and this discarded it immediately, so moving the events.k8s.io rule out of the reference's ClusterRole snippet and into the leader-election Role's left both directions green — and a role written from the page would grant it in binpack's namespace while decision Events about a Node are filed under `default`. Compared per kind now, and a snippet naming neither kind fails rather than being placed by default. That needed one line of the page: the Act section's YAML block did not say which object its two rules belong in. Neither did it before this pull request, which is a gap in a page whose whole purpose is to be written into a role by hand — an operator was told to grant `nodes: patch` and left to infer where. **And the recorder's writes are the chart's problem even though no interface bounds them.** The long-running path hands events to client-go's broadcaster, which creates and patches; dropping events.k8s.io/events: patch from the chart and the reference together left them agreeing and left every decision after the first failing to aggregate. The verbs are named once in reporter_test.go, where the broadcaster test proves them by driving the real recorder, and read here. Bounding a library's writes and asking whether the chart permits the ones it is observed to make are different questions; only the first is impossible. The stale v0.36.3 citation in the block above the one this touched was refreshed to v0.36.4 and re-read there: LeaseLock still uses get, create and update at that version. Fifteen sabotages now, across five rounds. All fifteen re-run together and all fifteen fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f51cb664f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… dropped Five more from review, all valid. Two are the flattening again, one level finer; two are fields the model omitted; and one is the direction none of these comparisons had ever run in. **Nothing asked whether a granted permission is one binpack needs.** Every check so far asks whether something binpack does is permitted. Adding `delete` to the Node read rule in the chart and the reference together left the two agreeing, left internal/collect's read equality unchanged — the rule still carries get, list and watch, so it still "grants read" — and was never reached by the acting equality, while the service account gained cluster-wide permission to delete Nodes. "binpack removes no object, ever" is internal/executor's package doc, R4-003's availability argument and this page's own "what binpack is never granted" section, and the grant is where it is actually enforced. The ungated ClusterRole is now an equality against the reads binpack performs and the writes it makes unconditionally, with the reads derived from collect.TemplateSources so the two tests that name them cannot drift. **And two fields the model dropped.** nonResourceURLs is ResourceNames' mirror image: a rule granting `get` on `["*"]` contributed no pair at all, so it passed every comparison and every count while the service account gained access to endpoints nothing asks for. It is modelled now, which means the surplus check above catches it rather than a rule of its own. **The namespaced Roles were still compared as one on the page's side.** Identity is finer than kind and the chart's half already knew it; the page's did not, so moving the ConfigMap rule between the reference's two Role snippets left both directions green while an operator following the page granted the cache permissions in binpack's namespace instead of the autoscaler's. Both documents are matched against one closed list of Role suffixes, and a block matching none of them fails rather than being placed by default. That needed the page to say which Role each block is, which it did not: the namespaced blocks described their namespaces and left the objects unnamed. They now name both, which is what an operator writing two Roles by hand actually needs to know. **The leader-election anchor covered only the Lease.** Moving the core event rule into the autoscaler-status Role left both Roles nonempty and disjoint and satisfied every Lease anchor, while leader election announces itself in binpack's own namespace. Anchored now, and worth stating: an anchor is a claim about one rule, so adding rules to a Role means adding to its anchor. **And the recorder's observed methods ran one way.** recorderMethods is read by the RBAC test as the verbs the chart must grant, so a method the sink observes that the list does not name is a request an install makes and nobody asked permission for. Dropping Patch from the list alongside both documents left the sink still observing it and nothing saying so. Both directions now; the Update case the test was written for is the same check, stated generally. Twenty-two sabotages across six rounds, re-run together. All twenty-two fail — including one this sweep initially reported as missed, which turned out to be the sweep's own restore step reverting the fix before it ran. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fe32ead43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Five more from review, all valid. Four are the same widening: this suite had learned to read what a rule grants and never what makes the rule apply. **A Role grants nothing until a binding names it.** Pointing the autoscaler-status RoleBinding at the leader-election Role's name installs cleanly — the API server does not resolve roleRef at admission — and every assertion here stayed true, because the Role does grant what it says and nothing reaches it. rbacdoc models bindings now, and every Role the chart renders has to be named by one: both halves of roleRef, since a RoleBinding may reference either kind and `kind: ClusterRole` with a Role's name resolves to nothing. A RoleBinding is also only effective in its own namespace, so one created away from the Role it names is checked too. **And that needed the namespaces this package twice said could not be compared.** They can. HelmToYAML replaced every action with one constant, so every templated field equalled every other; deriving the placeholder from the action's own text instead makes two expressions two scalars. It answers "are these the same expression" and never "which namespace", which is the question worth asking: the leader-election Role and the --leader-election-namespace flag have to move together, whatever they move to. Moving that Role to the autoscaler's namespace left it nonempty, disjoint and correctly anchored while no replica could acquire leadership. **The leader-election grants were not held to their own guard.** Roles is deliberately the union of every branch, so removing `.Values.leaderElection.enabled` from around them is invisible to anything asking what they grant — they are still granted, now to everybody. The reference tells an operator running a single replica they may omit Leases entirely; the chart has to let them. Checked against the render with that guard off, the same way the act rules are. **An aggregationRule would make every one of these comparisons fiction.** Kubernetes' aggregation controller owns the rules of a ClusterRole that declares one and replaces them with the union of whatever matches the selectors, so the rules written in the chart are overwritten and what is granted is decided by labels on objects this package never reads. Refused at the parse, where the alternative is to model a controller. **And the skip-code enumerator was assumed to be a set.** Every guard turns it into one, and a set absorbs a duplicate: two constants sharing a value collapse to a single enumerated code, each fixture goes on asserting its own constant, and the reference updated to the resulting vocabulary agrees. What is lost is the distinction — two operational causes an alert cannot tell apart, published under one label value. Asserted for the three hand-written slices; Diagnoses is built from a map keyed by the code, which is the shape to prefer and the reason it needs no guard. Twenty-eight sabotages across seven rounds, re-run together. All twenty-eight fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f16962693
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t see Five more from review, all valid, and two of them are directions this suite has now had to learn three times. **A binding grants to somebody.** The model checked which role a binding names and never who it names it for. Pointing the ClusterRoleBinding's subject namespace at the autoscaler's produces a valid binding for a ServiceAccount that does not exist there, and binpack's own account holds nothing — with every assertion about roleRef and about what the Roles grant still true. Subjects are modelled now: kind, name and namespace, with the namespace compared against the leader-election Role's rather than a literal, since that one is already pinned to the namespace the deployment elects in. Two pins to one expression is a chain; two literals would be two guesses. **Surplus verbs, again, one scope down.** The ClusterRole learned last round that no check had an opinion about a permission binpack does not need. The namespaced Roles had not: `delete` added to the ConfigMap rule in the chart and the reference together satisfied the per-identity comparison, disjointness and every minimum anchor at once. The anchors are equalities now. The leader-election Role's expected set carries the three Lease verbs that are granted and never exercised, because docs/reference/rbac.md takes and defends that position — narrowing the grant is a decision for the page to make first, not a test to force. **And rbac.create was unheld, the same way leaderElection.enabled was.** Roles unions every branch, so moving an object outside that guard leaves every comparison unchanged while an operator who chose to manage RBAC elsewhere receives chart-managed permissions anyway. The chart CI job renders with it off and checks only that templating succeeds, which an object rendered outside the guard also does. **A bare template invocation was being deleted.** `template` was in the control-keyword list, and it does not structure anything — it invokes a named template and emits whatever that renders. So a helper injecting one extra rule would have been erased from the document this package compares, while the installed chart granted it. It is not control flow any more, and a line that is nothing but an action is refused: a placeholder in its place lies about the document's shape and deleting the line lies about its contents. A field's value is the opposite case and has its own entry point. **And the uniqueness guard covered three of five vocabularies.** drain.AbandonCodes and controller.EventReasons are published, slice-backed and iterated as sets by their own reference guards. The metrics one moves to internal/metrics, where the four metric-label vocabularies were already gathered; EventReasons carries its own, because internal/metrics imports internal/engine and the purity rule keeps that package out of it. Thirty-five sabotages across eight rounds, re-run together; all thirty-five fail. The sweep is checked in beside these notes rather than described, since reconstructing it from prose is how a verification harness rots. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestRBACCreateFalseRendersNoRBACAtAll asked whether rbacdoc.Roles and rbacdoc.Bindings returned an error, and read that as "the guard removed everything". Both do refuse a document with no roles in it, so it passed — and it would have gone on passing if either ever stopped, because the assertion was about a contract stated nowhere and held by nothing. What the test needs to know is whether an object survived the guard, and the rendered text says that directly. The ordinary render is now checked for the three roles it has always carried too, so the second half cannot pass against a chart that lost them. It also corrects the record. The previous commit's message says the sabotage sweep is "checked in beside these notes". It is not: it lives outside the repository, in the private working directory this project keeps ungitignored material out of, and nothing in the repository should point at it. The claim was false in the way this whole change is about — an assertion about where something is, held by nothing — and it is the second time in this branch that a sentence written to summarise verification turned out to be the least verified thing in it. Thirty-five sabotages, re-run; all thirty-five still fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c7cc364e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hree lists share
Three more from review, all valid, and the smallest round yet.
**roleRef names an API group, and Kubernetes rejects a binding that gets it
wrong.** The model discarded it, so a mistyped or missing apiGroup left the
kind and name still the pair this suite expects. Helm validates neither, and
roleRef is immutable — the failure lands at install and no upgrade repairs
it. Modelled and asserted against the one value RBAC accepts.
**Three enumerators share one label namespace and were only checked
separately.** binpack_drains_abandoned_total{reason} carries a drain's own
codes, the skip codes a revalidation can end a drain with, and the verdicts
that can. AbandonStuck taking SkipBackoff's value leaves every per-list
uniqueness guard green, and a reference updated to the resulting vocabulary
agrees with it — the series then means both "wedged on a finalizer" and "in
backoff", which is precisely the distinction PUBLIC-01 split these codes
apart to make. Checked as one set now, from the same three lists the
reference's abandonment table is checked against, so a value added to the
namespace is covered without being named.
**And Without models a guard by deleting it, which an else branch makes a
lie.** A rule in the else branch renders exactly when the guard is false —
the case Without exists to describe — while Roles unions it with the true
branch and Without removes both. A mutating grant added there would be
classified as gated and held by every default install. Modelling the false
render means rendering the chart; refusing is the honest alternative, and it
is the same answer this package now gives to a bare template invocation.
Only the guard's own else: a nested block's is inside content that is either
kept whole or removed whole.
Thirty-nine sabotages across nine rounds, re-run; all thirty-nine fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934281a59e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ally is Five from review. Four were valid gaps; one was already covered, and the change made for it improves a message rather than closing a hole. **The reachability exemptions were prose.** decidedElsewhere says of three skip codes that Revalidate reaches them instead, and nothing checked that claim against anything that runs: a value added to SkipCodes(), to docs/reference/metrics.md and to that map satisfied all three at once, on the strength of the sentence in the map. A misspelling, or a branch nobody wrote, could be documented and pre-initialised as a public label for ever with no runtime path producing it. The Revalidate fixtures are extracted from vocabulary_test.go and driven, and every key in the record has to be a code one of them emits. That is the same defect the record exists to prevent, one level up. It was written to stop a code being deleted from the enumerator to quiet a loop, and it did — while accepting a code that was never real. **A RoleBinding naming a ClusterRole is legal.** It installs cleanly and grants that ClusterRole's rules inside one namespace only, so the ClusterRoleBinding turned into a RoleBinding left the counts matching, the ClusterRole marked bound, every namespace and subject check passing, and binpack without the cluster-wide Node and Pod read this file is about. The binding's own kind is now checked against the kind it references. **A subject count in aggregate is not a subject count.** One binding with none and another with two satisfied it, and the binding with none grants its whole Role to nobody. Per binding now. **And subjects have an API group too**, kind-specific in a way roleRef's is not: empty for a ServiceAccount, which is a core object, and the RBAC group for a User or a Group. A subject that acquires the wrong one decodes unchanged in every other field and the API server refuses the binding. The fifth — an enumerator holding the empty string — is refused already. TestNoLabelCarriesProse gathers the pre-initialised series and rejects `=""`, and all four vocabularies are pre-initialised, so both halves of the scenario fail today. It is refused in the vocabulary guard as well because that test is about a rendered series and this one is about the vocabulary, and a reader of the enumerator should not have to know which other file holds it well-formed. No hole closed. Forty-four sabotages across ten rounds, re-run; all forty-four fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/controller/vocabulary_test.go
Lines 30 to 35 in 30d1885
Compare emitted event reasons back to the enumerator
This check only proves EventReasons() ⊆ documentation, not that every reason the controller emits remains enumerated. For example, removing ReasonWouldDrain from EventReasons() and its reference entry leaves the reporting paths in report.go emitting it and leaves their fixtures green because they compare against the same constant, while this loop simply stops visiting it; the public Event reason is then undocumented despite the suite passing. Add the reverse comparison from reasons reached by the reporting fixtures to EventReasons().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rs nowhere Four from review, all valid, and one of them is the mirror of a refusal this reader already makes. **`define` declares a template and renders nothing.** It was in the control-keyword list, so the substitution deleted its opening and closing actions and left the body behind as though the chart had emitted it. A ClusterRole moved into a define whose invocation was forgotten is then read, compared and found correct, while Helm renders only its dangling binding and the install comes up with no cluster read at all. Refused now, which is the mirror of the bare invocation this already refuses: one emits what is not written here, the other writes what is not emitted. **A binding names an account, and nothing compared it with the pod's.** The older raw-template check requires the name expression to *contain* `include "binpack.serviceAccountName"`, which a suffix satisfies — `…}}-other` on all three subjects left the suite green while the Deployment went on running as the unsuffixed account and held none of these permissions. Compared with the Deployment's own expression now, so the two have to move together, and the Deployment dropping the field at all fails rather than reading as agreement. **Event reasons were held in one direction.** The doc guard proves EventReasons() ⊆ the reference, so it visits what the enumerator holds and has no opinion about what it does not: dropping ReasonWouldDrain from the enumerator and from the pages left report.go emitting it, left its fixtures green — they compare against the same constant — and left the loop simply not visiting it. The reason is then written onto nodes and documented nowhere. Every Reason constant the package declares is now required to be enumerated, read from the declarations the way internal/cli's conventions guard reads node keys, with a count so a parse that stops seeing them fails instead of passing. **And an empty reason is refused.** Unlike the metric label values, nothing downstream catches this one — a series with an empty label is rejected when it is gathered, but an Event carries whatever reason it is given, and the doc guard would look for `` in the reference, which every code fence satisfies. Forty-nine sabotages across eleven rounds, re-run; all forty-nine fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ccf28555c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…each object Four from review, all valid. **Two feature guards were flattened into one union.** Roles unions every branch and Without models a guard by deleting its block, so neither can say that two values must both be true. Nesting the act rules inside another feature's guard is invisible to both: the union still holds them, the difference between the two renders still reports them as gated on rbac.allowDraining, and an install that opted in to acting and out of the other feature renders neither nodes: patch nor pods/eviction: create and 403s on its first drain. The nesting is read from the template now — GuardsAround — and each feature states which guards it may depend on. Both hang off rbac.create and off nothing else, which is what the reference promises: acting is a decision about acting and leader election is a decision about replicas. Writing that turned up a second thing. Every reader here finds a guard by its first occurrence, so a value used for two blocks would have one of them read and the other silently ignored. GuardsAround refuses that rather than picking one. **Nothing asserted that two rendered objects are two objects.** Duplicate manifests keep the counts balanced, collapse into the same map entries and dedupe through every grant comparison; Helm then asks the API server to create one cluster-scoped object twice and the install fails outright. Asserted by kind, namespace and name. **A namespace on a ClusterRoleBinding was read as nothing to check.** The namespace comparison was scoped to RoleBindings, so the cluster-scoped case fell through the condition entirely — and a ClusterRoleBinding carrying a namespace is refused by the API server, taking every cluster-wide permission with it. **And the metric vocabularies were held in one direction**, the way the Event reasons were until last round. Dropping AbandonNotRemoved from drain.AbandonCodes() and its row from the reference leaves the drain code emitting it and its fixtures green, while the guard stops visiting it: the series is published, documented nowhere, and no longer pre-initialised, so it appears for the first time on the day it fires — which is the case a pre-initialised zero exists to prevent. Every Skip, Verdict, Code and Abandon constant the two packages declare must now be enumerated, read from the declarations with a count so a parse that stops seeing them fails. Fifty-four sabotages across twelve rounds, re-run; all fifty-four fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/controller/vocabulary_test.go
Lines 37 to 40 in 696dbf7
Reject an empty Event action
Fresh evidence after adding the empty-reason guard is that ActionConsolidate is appended to the documentation loop but is not included in the non-empty validation below. If that constant becomes "", all emission fixtures still compare against the same constant and this check searches for ``````—which ordinary Markdown fences satisfy—so the suite stays green while every emitted Event loses the action that groups consolidation events. Validate the action as non-empty alongside the reasons.
binpack/internal/metrics/reference_test.go
Lines 133 to 137 in 696dbf7
Limit abandonment verdicts to those the counter emits
The abandonment counter can receive only VerdictInfeasible and VerdictBlocked when revalidation returns no skip code, as reflected by the initialization in internal/metrics/metrics.go, but this allowed set includes every value from engine.Verdicts(), including skipped and drainable. Adding either of those rows—or any future verdict—to the binpack_drains_abandoned_total{reason} table therefore passes the reverse documentation check even though that series can never be emitted; use the same two-verdict set as the counter.
binpack/internal/controller/vocabulary_test.go
Lines 37 to 40 in 696dbf7
Reject obsolete Event reasons in the reference
This containment check proves only that each current value appears somewhere in the entire reference corpus; it never verifies the reverse direction or that the occurrence is in the Event-reason catalogue. Renaming a reason, adding the new spelling to any reference page, and leaving the old row in docs/reference/cli.md therefore keeps every vocabulary test green while the CLI reference still advertises a field selector that no Event can match. Parse that table and compare it exactly with EventReasons().
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…a catalogue Five from review, all valid, and one of them is a defect in the guard added last round rather than a gap beside it. **GuardsAround pushed on `if` and popped on every `end`.** depthDelta counts `with`, `range` and `block` as openers, so a template using any of them unwound the stack through guards that were still open and reported the wrong nesting. It is also the reason the finding matters at all: a `with` over an empty value, or a `range` over nothing, suppresses everything inside it exactly as a false condition does, so both features could sit inside one and still report `[.Values.rbac.create]`. Every construct that opens a block is on the stack now, named by its keyword so an expectation listing a condition cannot match one — and a close with nothing open is an error rather than a silent truncation. **A ClusterRole is cluster-scoped too.** The namespace check added last round was written for bindings, and the role beside it went unasserted: a namespace on the ClusterRole leaves every grant, identity, binding and count assertion true while the API server refuses the object and binpack installs with no cluster-wide read at all. **The Event catalogue was checked by containment.** The guard proves each reason appears somewhere in the reference corpus, which a mention on any page satisfies and which says nothing about what else the table lists. Renaming a reason, documenting the new spelling elsewhere and leaving the old row in docs/reference/cli.md kept every vocabulary test green while that table went on advertising a --field-selector no Event can match — and that table is where an operator copies the selector from. Compared exactly now, in both directions, because a catalogue is not a mention. **The action was left out of the vocabulary it belongs to.** It is appended to the documentation loop and was missing from the emptiness check beside it; an Event whose action is empty has lost the field that groups every consolidation event, which is the same defect as an empty reason on the field next to it. **And the abandonment table allowed four verdicts where the counter creates two.** A revalidation that returns no skip code ends the drain on its verdict, and only infeasible and blocked mean stop — skipped carries a code by construction and drainable is the drain continuing. The counter has always pre-initialised those two; the reference guard allowed all four, so the table could document a series that can never exist. The pair is named once in internal/metrics and read by both. Fifty-nine sabotages across thirteen rounds, re-run; all fifty-nine fail, and each failure read rather than counted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/metrics/reference_test.go
Lines 162 to 165 in 1b18e23
Require every evaluation code in its catalogue table
For the evaluation-code table, this verifies only that each row is allowed, not that every allowed code still has a row. Removing the none-feasible row remains green because that value is still backticked in the prose immediately below, satisfying TestEveryLabelValueBinpackCanProduceIsDocumented, while this loop sees only the remaining valid rows; compare the rows under the evaluation anchor with engine.DecisionCodes() in both directions so the advertised catalogue cannot omit a live series.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hat grants nothing
Six from review, all valid. Two are in readers added over the last two
rounds, and one is a hazard the previous round's fix created.
**GuardsAround asked what encloses a feature and never what it contains.**
Those are different questions and only the first was being asked, so
`{{- if .Values.rbac.allowDraining }}{{- with … }}` on one line leaves the
feature enclosed by exactly what it should be while everything it guards
sits inside a construct that can suppress it. GuardsWithin asks the second,
and each feature now states what it may contain as well as what it may sit
inside. The same commit fixes openedBlock, which returned after the first
opener on a line while depthDelta counted them all — my first attempt at
this finding fixed only that, and the sabotage went from failing for the
wrong reason to not failing at all.
**A rule that grants nothing is a rule Kubernetes refuses.** `- verbs:
[get]` with neither resources nor nonResourceURLs decodes without error and
flattens to no pair, so every comparison, count and surplus check reads it
as nothing to say — while the API server rejects the whole role and the
install has none of its permissions. Silent in both directions at once,
which is this package's own definition of what it must not do.
**Single-sourcing abandonmentVerdicts made it one place to be wrong.** It
is both the pre-initialised zero series and the reference guard's allowed
set, so a verdict added there and to the table satisfies the zero-series,
reverse-documentation and uniqueness checks together while no failed step
ever carries it. Which verdicts can is decided in internal/executor, and a
test there now derives them from the assessments the engine produces — a
drainable node is the drain continuing, and a skipped one carries a code
that gets published instead of the verdict.
**A const spec with no expression repeats the previous one.** The parsers
skipped those, so an alias was invisible: two branches emitting one value,
one of them absent from every count. Evaluated now.
**Actions are half the Event vocabulary.** The declaration audit read
`Reason*` and each check named the single current action by hand, so a
second one would be public, undocumented, unaudited and permitted to be
empty or duplicate. Discovered alongside the reasons.
**And a code table has to list every code.** Checking that each row is
allowed leaves the table free to lose one: the `none-feasible` row removed
stayed green because the value is still backticked in the prose below,
which satisfies the every-value-is-documented guard — and that guard
searches the page while the table is the closed set an operator reads out
of. Compared both ways, except where a table is partial by design, which is
now stated rather than assumed.
Sixty-five sabotages across fourteen rounds, re-run; all sixty-five fail,
and each failure read rather than counted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 565c69142b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…part
Five from review, all valid.
**Requiring three tokens to appear is not requiring the helper to use
them.** `{{- $_ := dig "discovery" "autoscalerNamespace" … -}}kube-system`
reads the setting, discards it and returns the default, satisfying every
token check — and both RBAC objects go on agreeing with the helper while the
rendered config points binpack somewhere else. The helper is now held to its
shape: one action, no literal beside it, and the dig inside it. Structural
rather than evaluated, because evaluating means rendering the chart and
`make check` may have no helm; the chart CI job renders, and this holds the
shape that makes the render right.
**The write audit recognised a Writer only when it was called `w`.** The
spelling is this file's convention, not a rule, so an entry point taking
`writer Writer` and driven like every other left its conditional write
unexamined. The receiver names come from the signatures now.
**A Markdown fence may be tildes**, and a snippet written that way was
absent from the reference reader while the backtick blocks kept its
non-empty guard satisfied.
**And a role needs the API version it is served under.** A manifest missing
or misspelling it decodes to the same kind, the same name and the same
rules — every comparison agrees and Kubernetes refuses the object. Required
of the chart's manifests and not of the documentation fragments, which carry
none by design.
**The last is a guard refusing correct code, and it is one I introduced.**
Reasons and actions were merged before deduplicating, and Kubernetes stores
and filters them as separate fields: `reason=Consolidate` and
`action=Consolidate` are two selectors, not a collision. Merging them would
have refused a legitimate reason for sharing a name with an unrelated
action. Each field is now a set on its own.
A hundred and nineteen sabotages across twenty-six rounds, re-run; all
behave as intended.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b8a4540fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…count that is one
Three from review, all valid, and the smallest round in ten.
**A binding needs its API version too.** Roles got that check last round and
the bindings beside them, in the same file and for the same reason, did not:
a manifest missing or misspelling it decodes to the same kind, name, roleRef
and subject, so every assertion agrees while Kubernetes refuses the object
and the Role it names is bound to nobody. That is the eighth time in this
pull request that half a symmetry was fixed.
**An assignment emits nothing.** Last round's shape check requires one
action, no literal beside it and the dig inside it —
`{{- $_ := dig "discovery" "autoscalerNamespace" … -}}` satisfies all three
and returns the empty string. The shape I wrote it against was the very one
I had described in the comment, and I checked the parts of it that were
convenient to check.
**And a manifest has to be the kind it is named for.** serviceaccount.yaml
changed to another valid kind still reads the same metadata, so the
Deployment and every binding went on agreeing about a name nothing creates,
and a default install has no ServiceAccount and a pod Kubernetes will not
start. The manifest is now required to declare `apiVersion: v1` and
`kind: ServiceAccount` before its identity is read.
A hundred and twenty-two sabotages across twenty-seven rounds, re-run; all
behave as intended.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f53d36cac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t circuit
Four from review, all valid, and all four are against work from the last two
rounds.
**A search for text matches a comment.** The check added last round requires
serviceaccount.yaml to declare `kind: ServiceAccount`, and
`# kind: ServiceAccount` above a changed field satisfies it — the same
comment-versus-field defect fixed twice already, in the helper reading the
pod's account and again in the one reading the leader-election flag, and
written a third time in the check that closed a different hole. It decodes
now, through rbacdoc.Objects, which also gives the apiVersion.
**A template assigns with `=` as well as `:=`.** Last round rejected the
declaration form; `{{- $ = dig … -}}` has one action, no literal beside it,
the dig inside it, and renders nothing. Comparisons in a template are `eq`
and `ne`, so an `=` outside `==`, `!=`, `>=` and `<=` is an assignment.
**And `&&` does not evaluate its right operand.** The unconditional-write
audit walked an if's condition with its caller's state, which reads the
whole condition as always evaluated — so `if annotated && w.Patch(…) != nil`
was classified unconditional while the patch happens only for annotated
nodes. The right operand of `&&` and `||` is conditional now.
**A fence may put whitespace before its info string**, which the pattern
required to follow the delimiter immediately.
A hundred and twenty-six sabotages across twenty-eight rounds, re-run; all
behave as intended. One needed a shell fix first: a `$D` escaped inside the
sweep's heredoc left that entry editing a file named `$D`, which reported a
miss for a guard that works.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0cd27232
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three documents in this repository said `resourceNames` cannot restrict a list or a watch, because those requests carry no object name. The chart's RBAC comment said it, docs/reference/rbac.md said it at length under a heading promising to explain why, and rbacdoc.Unauthorizable enforced it — so the tightest rule an operator could write for the autoscaler's status ConfigMap was refused by binpack's own test suite as authorising nothing. It is not true, and the sentence disproving it was two lines away in every one of the three. The API server derives the name of a collection request from an exact-match metadata.name field selector when the request carries one — requestinfo.go sets requestInfo.Name from RequiresExactMatch, on a path no feature gate guards — and RuleAllows then matches resourceNames against it like any other name. binpack's cache sends exactly that selector, which each of the three documents went on to say in its next breath while explaining how little is read. A name-restricted ClusterRole would in fact have authorised every read binpack makes. The chart still renders a namespaced Role, for the reason that survives checking: a grant conditional on the client's own query stops working the day the selector is dropped, and a ClusterRole carries the name into every namespace rather than the one the autoscaler publishes into. Neither is visible in the rule. That is now what the comment and the page say. Two verbs do carry no name however the client phrases the request. A create has none because the object does not have one yet, and a deletecollection is the verb the server picks precisely when a delete arrived without one; neither has a selector escape hatch. Unauthorizable is those two now, which makes it a claim about the grant rather than about the caller — and no rule the chart renders trips it, so it is verified by sabotage: a resourceNames restriction on the eviction create fails the suite, and dropping create from the set makes that sabotage pass again. Four guards widened alongside, each verified the same way. The rendered ServiceAccount name was being placeholder-substituted twice, comparing a value against its own re-encoding. The vocabulary reader could not evaluate `string(sep)` for a named rune constant, only a literal one. The unconditional-write walk read a statement following an early return as unconditional, which is the shape a real write behind a guard clause takes. And the reference's action pattern matched an alphabetic run rather than the backticked token, so a legal Event action spelled with a dash was invisible to the guard that checks it is documented. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
rbacdoc rendered the chart by substituting a derived scalar for every Helm
action and decoding what was left. That is an evaluator for a template
language, written to avoid a dependency the project already has: CI installs
helm and runs `helm template` in its own job. It came to four hundred lines
against a hundred-and-fifty-line chart, and it answered a question about the
text of a template rather than about the manifests an install applies — the
two agree only until somebody writes a guard the scanner did not anticipate.
Most of what it cost was paid in the guards built on it. The namespace check
asserted the field was exactly `include "binpack.autoscalerNamespace" .` with
nothing but `quote` after it, and grew a clause every time somebody thought of
another expression that mentions the helper and renders elsewhere. The helper
check asserted one action, no literal beside it, a `dig` within it and not an
assignment — four guesses about what Helm would do, three of them added after
a shape got past the previous version. Feature independence was read off
`{{- if }}` nesting. Each is now one render with a value this test chooses and
one look at what came out, which is the operator's own question and is right
however the chart is written.
Two of them got stronger rather than shorter. The namespace was compared as an
expression, so it could say the flag and the Role moved together and never
which namespace either named; both sides are now real namespaces. And the
quoting hazard was a proxy — does the value mention `quote` — where the test
now renders with the namespace set to `true` and to `123`, both legal DNS-1123
labels, and looks at what Kubernetes would receive.
That last one needed a guard the decode cannot provide. sigs.k8s.io/yaml
converts YAML to JSON and coerces a bool into a string field, so `namespace:
true` comes back from Objects as "true" and matches a correctly quoted render
exactly. The API server does not: its manifest path decodes strictly and
answers `cannot unmarshal bool into Go struct field
ObjectMeta.metadata.namespace of type string`, which was verified against the
strict serializer rather than assumed from the lenient one. rbacdoc.Mistyped
decodes into `any` and reports the YAML type, which is the distinction the
strict decoder makes.
Rendering also put the chart outside `go test`'s view, since it now reaches
Helm through a subprocess and the cache is decided by what the test binary
opened. A template edit returned the previous run's verdict — every guard here
silently stopping the moment the thing it guards changed, which is the failure
this whole change is against. Render reads the chart's files into a digest and
the read is the point, not the digest: sabotaging the key alone leaves the
files opened and the cache correct, so the sabotage that demonstrates this
removes the call.
Asking the install rather than the template found one gap that was open before
this commit. serviceAccount.name moves the bindings and, with the Deployment
naming the release directly, would not have moved the pod — the chart's own
default account *is* the release name, so a default render cannot tell the two
apart, and the bindings would grant an account the pod does not use. The
external-account case now checks both halves.
make test needs helm from here on. It fails rather than skips when helm is
absent, because a skipped comparison between the chart and the reference page
reads in a log exactly like one with nothing to report. CONTRIBUTING says so,
and both workflows install it — including release.yaml, whose own check was
comparing against every job in the file, so the chart job's helm stood in for
the release job that runs `make check`. A tag would have failed at its verify
step with that check green.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2acbad90aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… text Four guards that read something adjacent to what they were about. The autoscaler-status check confirmed the process is told which namespace to read by matching `autoscalerNamespace: <value>` in the manifests. A ConfigMap that dropped the field and kept a templated comment above it satisfies that substring exactly, and the install it describes is the one the whole test exists to forbid: the Role follows the setting, the process falls back to kube-system, and every status read is a 403. It now decodes the rendered ConfigMap's config.yaml through v1alpha1.Load, so what is compared is the value binpack would run on rather than a string that appears near it. The release job's setup check compared the build job's actions against the whole release job. A setup step is available to a command only if it has already run, so moving `azure/setup-helm` below `run: make check` left this green and the tag failing at its verify step — the same masking as the job-versus-file one this replaced last commit, one level further down. It now slices the job at the verification command. vocab classified a binary expression by its operands, and only `+` produces a string. `const ReasonsAreCompared = "a" == "a"` has two string operands and a bool result, so it was read as something that might be a string, rejected by the evaluator for its operator, and reported as unevaluable — which failed the vocabulary guard over valid package code whose name shared a prefix. The operator decides the result, so it is checked first; `1 << 4` is covered by the same clause rather than by the operand check that was added for it. And the external-account render was held to less than the default one. Its loop skipped every subject that was not already a correctly-kinded ServiceAccount, which made three failures invisible: a binding with no subjects grants nobody, a subject of another kind grants somebody else, and a ServiceAccount subject resolves per namespace, so the right name in the wrong one is as inert as the wrong name. A binding omitted on that path was invisible too, since nothing compared the two renders. Both now go through one helper that requires exactly one correctly named and namespaced ServiceAccount subject per binding, and the sets of bindings are compared with each other. That path is rendered by no other test, so each of the four shapes is verified by its own sabotage rather than by the pair the helper obviously catches. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830cdf7d6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hey read
A vocabulary is usually declared through its own type, and `DecisionCode("x")`
is as ordinary a spelling as `DecisionCode = "x"`. vocab recognised only a
conversion to the predeclared `string`, so those constants were dropped from
the declarations — silently, which is the failure this package exists to
prevent. A member omitted from the declarations shrinks both sides of the count
that holds an enumerator honest, so the guard passes while the value is
published and documented nowhere. Conversions to any of the package's own names
for a string are now string expressions, in the classifier and the evaluator
both.
The Markdown fence expression accepted either delimiter as the close of either
opener, anywhere on a line. A backtick block whose body mentioned three tildes
— in a YAML comment, or a piece of quoted output — ended there, and because the
prefix already held a valid rule the block decoded and every grant after that
line was dropped without a word. RE2 has no backreference to say "the same
delimiter", so this is a line scanner now: CommonMark's rule, the same
character, at least as long, alone on its line.
Documented did not refuse an aggregationRule, though decode refuses one on the
chart's own manifests for a reason that applies harder here. A ClusterRole
carrying one does not grant what is written in it — the aggregation controller
replaces those rules with the union of whatever its selectors match — and this
page is written for the operator who is not using the chart, so nothing else in
their install would catch it. Their flattened grants agree with the chart
exactly and they hold whatever labels elsewhere in their cluster decide.
The unconditional-write audit found the Writer by parameter name, so `writer :=
w` hid the call that followed. The entry point stayed in the driven set, and a
fixture that missed the branch left the recorder contributing no grant at all —
every RBAC comparison passing over a call the production path makes. Aliases
are followed to a fixpoint now, deliberately not scoped per function: a name
that shadows one elsewhere is treated as a writer too, which errs towards
examining a write that need not have been.
renderedConfig picked the ConfigMap by kind and decoded it. Which object the
process actually loads is a chain — `--file` names a path, the path has to fall
inside a volumeMount, the mount has to name a volume, the volume has to be a
ConfigMap of that name in the pod's own namespace, and the key has to be there
— and a break anywhere leaves the pod on its own defaults or not starting.
Pointing the volume at a name nothing creates left the autoscaler-namespace
comparison green. The chain is traced end to end, and each of its six links has
its own sabotage.
And every render in the suite installed into the chart's default namespace, so
every assertion about one accepted that literal string: a subject changed from
.Release.Namespace to binpack-system passed all of them, and an install
anywhere else binds an account in a namespace its pod does not run in. A
ServiceAccount subject resolves per namespace, so that is the difference
between a chart installable anywhere and one that works in exactly one place.
The helpers take a render rather than a value map now, and one test installs
into another namespace and holds the account, the pod, the bindings, the
namespaced Roles, the leader-election flag and the mounted ConfigMap to it.
Twenty sabotages, in notes rather than the tree. Two of them tested the wrong
thing first and were rewritten: the fence needed a valid rule *before* the
stray delimiter, and the alias needed a direct Patch, since a SubResource call
is matched by its shape whatever its receiver is called.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76b7ff53b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Each of these reads the right document and stops before the thing that decides the answer. A ConfigMap volume with `items` renames what it projects: `key: missing, path: config.yaml` puts nothing in the container while `data["config.yaml"]` exists and decodes cleanly. The chain is followed through the projection now, so the key read is the key Kubernetes would look for — and a path the items do not project is a file the pod does not have. A Markdown fence's info string carries more than the language. `title=`, a highlight range, an anchor — renderers here and everywhere else still treat the block as YAML, and anchored at the end this reader did not, so grants written in such a block left the reference silently while the plain blocks kept the non-empty guard satisfied. `yaml` is the first word now rather than the whole string, and `yamlish` is still not YAML. A rune constant need not be written as a character. `const letter rune = 65` is the same constant as 'A' and Go converts it the same way, as it does an alias of one and arithmetic over them; all three were absent from the rune map and their conversions were reported as unevaluable, which fails the vocabulary guard over code that compiles. Integer constants are evaluated to a fixpoint now, in the shape the string ones already were. A documented block may declare a real `kind` as well as carry its scope comment, and the two were not compared — the header was assigned over the decode. A snippet headed `# ClusterRole` and declaring `kind: Role` was compared as the ClusterRole it is not, so it could satisfy every grant check while what the page tells an operator to apply is namespaced, and binpack holds none of its cluster-wide node and pod access. Kubernetes reads `resourceNames` as a set and this keyed it as a list, so the same restriction in two orders produced two pairs: one missing permission and one surplus, reported to an operator who had reordered a line and changed nothing. Sorted and compacted on a copy now. No comparison in this repository can isolate that — reordering both sides at once is a change the other guards reject for their own reasons — so it has a unit test rather than a sabotage. And the unconditional-write audit was per-function, which does not compose. An unexported helper writing unconditionally satisfies the walk, and if its only call is behind a branch then driving the entry point need not reach the write: the fixture records no grant, the chart is never asked for it, and the branch that does call it 403s. Every Writer-bearing function must now be exported — and so driven in its own right — or reachable by unconditional calls from one that is. Present code passes: each helper has at least one caller that reaches it outright. Sabotages in notes. One of them again tested the wrong thing first: a helper writing through SubResource is caught by the subresource audit whatever its call site, so it took a plain Patch on another resource to show the gap. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e0273bdd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… too far
A conversion's operand is an integer expression, not only a character literal
or a name. `string(rune(65))`, `string(letter + 1)` and `string((letter))` all
compile, all convert to the same string, and all were reported as unevaluable —
which fails the vocabulary guard over code Go accepts. The operand goes through
the integer evaluator now, and only what is not an integer expression falls
through to the string one. The literal `string(65)` in the report is not a case
this repository has: go vet's stringintconv refuses it, so `go test` never
compiles it.
A closure body runs where it is called, not where it is written. The write walk
descended into a FuncLit carrying the declaration site's state, so `write :=
func() { w.Patch(…) }` invoked only from a branch read as unconditional — and a
literal has no name, so the helper graph added last commit could not reach it
either. Conservatively conditional: a write that needs a closure is a write
that needs its own named function, which is what executor.go promises to be.
A mountPath means different things with and without subPath. Without one it is
the directory the volume is mounted at; with one it is the single file
projected there. Read as a directory either way, a mount that gained `subPath:
config.yaml` still resolved --file to a key that exists — while in the
container that path is not a path at all, because its parent is the file. A
subPathExpr is refused rather than guessed at: Kubernetes expands it from the
container's environment, and the guess that looks right is the assumption this
helper exists to stop making.
A documented block that declares an apiVersion has to declare one Kubernetes
still serves. Its grants and its identity satisfied every comparison here while
an operator applying it was refused at create and held none of the permissions
the page had spent a section explaining.
And binpack_drains_abandoned_total's prose was only ever checked forwards. Its
table is compared both ways; everything else the counter carries — every skip
code, and the two verdicts that end a drain without one — is described below it
instead, and prose was only asked whether each produced value appeared
somewhere on the page. That cannot see a stale claim: rename a reason, document
the new spelling anywhere, and the paragraph explaining the old one survives,
describing a series that will never appear again. The section is read as a
closed claim now, with the words in it that are not values named and asserted
to still be there.
That last one found a defect in its own first draft, which is the reason the
list is short. Cut at the next `## ` the section ran on through `### Pools` and
swallowed another metric's table, so `pool` had to be excused as a word that is
not a reason — when the truth was that it is a different metric's label and
this had no business reading it. An exception list is where an over-broad
reader hides. Cut at the next heading of any level, the list is two words.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7129b007ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… splitter Two of these are the same defect: a rule learned in one place and not in the other. The helper-call audit did not know that `&&` skips its right operand, so `if ready && helper(w) != nil` recorded an unconditional edge to a helper the fixture may never call. The write walk did know — it had been taught the previous commit, along with closures, which the helper walk also did not know. Two walkers over the same syntax, diverging twice in two commits, each divergence a call that looked unconditional and was not. They are one traversal now, and what "conditional" means is one question with one answer: loop bodies, select cases, closure bodies, the right operand of a short-circuit, and everything after a statement that can return. internal/cli split rendered manifests on the literal bytes "\n---\n", which is one spelling of a separator among several. YAML ends a document at `---` alone on its line and neither trailing whitespace nor a following comment changes that. `helm template` normalises the comment form away when it re-emits each document, and passes a trailing space straight through — so `--- ` merged two documents into one chunk, the decoder read the first, and a second ConfigMap of the same name was invisible to every assertion about the configuration the pod loads. rbacdoc already had this right; it now exports it, and nothing splits a manifest stream by hand. The integer evaluator dropped a const spec with no expression, though Go repeats the previous one and `declarations` already carried that for the string constants. `const ( letter rune = 65; repeated )` left `repeated` unknown, so `string(repeated)` was unevaluable and the vocabulary guard failed over a refactor Go compiles. And the alternate-namespace install checked everything except where the pod lands. The Deployment's own metadata.namespace was not decoded at all, so it could be pinned while the ServiceAccount, the bindings, the Roles, the ConfigMap and the leader-election flag all followed the release — the pod created in one namespace, claiming an account that exists only in another, and never starting. Sabotages in notes. The first attempt at the short-circuit one again passed for the wrong reason: inserting the branch at the top of Cordon made Cordon's own tail call conditional, so the audit reported patchNode rather than the helper under test. The entry point is added beside the others now, and the check reads the name in the message. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff5d405097
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ocument
`const ( CodeA = string('A' + iota); CodeB )` is how a sequence of related
constants is ordinarily written, and `iota` was looked up in the map of
declared names like any other identifier — never found, so neither member could
be evaluated and the vocabulary guard failed over code Go compiles to "A" and
"B". It is the one identifier whose value is the expression's position, so the
position is carried alongside the expression: a spec with no expression repeats
the previous one, and the same syntax then evaluates once per place it stands.
And a documented fence may hold a whole stream. Decoded as a single document,
the first role came back and every later one was discarded in silence — so a
permission an operator receives from a block they copy entire was absent from
both directions of the comparison, along with every check made of it: a
malformed rule, an aggregationRule, a wrong apiVersion. Each document is read
now, and the fence's header describes its first: a later one is measured
against its own leading comment, or its own `kind`, and only falls back to the
fence's when it says nothing about itself.
The skip path was restructured on the way. Split into "did anything decode" and
"was this a rule list", it reads as a decision about the block rather than as
an error dropped on the floor — which is what it had become once the loop body
turned into a function, and what nilerr called it.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0e87a91de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…fications Three readers here had grown into partial implementations of things that are fully specified elsewhere, and every review round extended one of them. The findings were right each time; the code they were right about should not have existed. internal/vocab was a Go constant evaluator. It handled literals, concatenation, conversions to the predeclared string and to a package's own names for one, character literals, named runes, integer literals, arithmetic, shifts, parentheses, alias chains resolved to a fixpoint, specs that repeat the one above them, and iota — six hundred lines, arrived at one review round at a time, chasing a definition that go/constant already is. It asks the type checker now. Imports are stubbed rather than resolved, because resolving them means compiling this module's dependencies from source and internal/controller took ninety seconds of it. The standard library is not stubbed: constants are declared in terms of it, and `15 * time.Second` has no value without `time` — which is not a guess, it is what the completeness guard reported the first time everything was stubbed. That guard is the rule the old evaluator was built around, kept: the syntax says how many constants a package declares, and every one has to come back with a value, so a stubbed import cannot quietly shrink a vocabulary. The package now has its own tests — a fixture declaring every shape that was found missing over six rounds, and a package that does not compile, which must be an error rather than a shorter answer. rbacdoc was becoming a Markdown parser. It accepted tilde fences, `yml`, any capitalisation, leading spaces, info-string metadata and multi-document streams, each added after a review found a block it had skipped in silence. That permissiveness was bought to avoid missing a grant — so the fix is to stop needing it: one spelling is read, and anything that looks like a YAML fence and is not that spelling is an error naming the line. A stream inside a fence is refused too, which also settles a question that had no good answer — which of several documents a fence's leading `# ClusterRole` comment describes. The chart tests were modelling kubelet: `items` renaming projected keys, `subPath` turning a mount path into a file, `subPathExpr` expanding from the container's environment. The chart uses none of them, and the model was wrong twice before a reviewer said so. They are refused now, with a message saying what is not modelled — a smaller thing to be wrong about than a model of somebody else's contract. Two review findings are answered by the deletions: a fence can no longer hold a stream, so a later document's scope header is not a question. Two are fixed where they stand. `Documents` required whitespace before a separator's comment, because `---#comment` is not a comment to YAML and splitting there handed the decoder two fragments that each parse while the stream as written is refused. And an exported method with a Writer parameter was treated as an already-driven root though the entry-point audit skips methods, so its write satisfied both static walks while no fixture performed it. requireBoundTo also checks where each binding points, not only whom it names. A binding is two halves and either wrong makes it inert; the audit that checked roleRef in full ran on one render, so a value-dependent path could change it and grant nothing with every subject check passing. The line counts barely move for two of the three. What moves is what this code is answerable for. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faafd0e532
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Which files are in a package is go/build's question. A glob answers it wrongly for the one case that matters: `codes_windows.go`, or a file behind `//go:build windows`, is not in the binary binpack ships here, and its constants were being read into a vocabulary they can never appear in. MatchFile answers for the same context the compiler uses, filename suffixes included — the comment this replaces claimed the shape would not need revisiting if such a file arrived, which was wrong twice over. What Kubernetes permits in a rule depends on the kind holding it — nonResourceURLs are a ClusterRole's alone — and a documented fragment carries its kind in a leading comment rather than a field. The shape check ran before that comment was read, so it saw an empty kind and skipped the half that depends on one: a namespaced fragment carrying nonResourceURLs was accepted, and assembling the advertised Role from it gets the object refused. The kind is resolved first now. A manifest names itself, and the header above it is prose. Substituted into a block that declares apiVersion and kind, it let a manifest with no metadata.name satisfy every grant and identity comparison while the API server refuses the object whole — the section reads as a list of permissions the operator then does not hold. Only a fragment, which has no metadata to carry a name, takes one from its header. And a RoleBinding resolves its Role in its own namespace. Matched on kind and name alone, a binding moved elsewhere found the Role it had left behind and read as bound — reachable on the external-account path, which the comprehensive namespace audit does not render. Each verified against a sabotage, and the two that narrow what is accepted against a positive control as well: nonResourceURLs in a ClusterRole fragment, and a manifest that does name itself, both still pass. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/cli/chart_test.go
Lines 554 to 556 in a3d8b0a
Restrict installed namespaces to installation commands
This builds the allowlist from every --namespace occurrence in the installation guide, not just from the helm install commands that create namespaces. Consequently, changing the documented helm uninstall command to the stale namespace binpack adds binpack to installed and is never rejected—the validation loop only examines kubectl commands—and a long-form kubectl --namespace binpack in this same guide would similarly authorize its own typo. Derive this set only from the actual install commands before validating the remaining examples.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…stray kind A three-clause loop runs its post statement after each iteration, so a loop that runs none never reaches it — as conditional as the body, and not walked at all. `for ; ready; w.Patch(…)` was a write both static audits stepped over, and a fixture whose loop does not execute records no grant for it either. A binding subject's apiGroup is kind-specific: a ServiceAccount's is the core one, spelled empty, and the API server refuses a subject that says otherwise. requireBoundTo checked the subject's kind, name and namespace and not its group, so the external-account render — which no other binding audit exercises — could name it correctly and still be refused at create. And decode kept a rendered document of an unknown kind when it carried rules. It counted as a role: the role and binding counts were satisfied, BindingKindFor reads every kind but ClusterRole as namespaced so a roleRef could resolve to it, and a rule repeating an existing grant is deduplicated by every comparison here — while `helm install` fails, because that API version serves no such kind. decode reads only rendered manifests now, so there is no fragment for it to be confused with: an unknown kind carrying rules is refused by name. The two narrowing fixes have positive controls beside their sabotages — a correct ServiceAccount subject still passes, and a rendered document that is not a role and carries no rules is still ignored rather than refused. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/metrics/reference_test.go
Lines 314 to 317 in e83c156
Validate every repeated code-table claim
When the reference contains the same anchor more than once, this selects only its first occurrence and never examines later tables. For example, a refactor can add an updated table before the old one, after which every current value is found and allowed while the stale table continues advertising a removed label value; the forward documentation check is also satisfied by the new table. Find all occurrences of the anchor or reject duplicates so a second closed-set claim cannot escape the reverse comparison.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Reading one fence spelling and refusing the others left a gap between them: a block opened with a bare fence is neither, so it was skipped in silence. Usually that costs a grant the reference then appears to be missing, which fails — but a surplus or duplicate block disappears entirely, and the page goes on presenting it as configuration to copy. These pages do use bare fences, for log output and shell, so the label cannot simply be required: an unlabelled block is read for rule fields instead, and refused if it has them. Volume mounts nest, and a container resolves a path through the most specific of them. This took the last match in list order, which is worse than either answer: a nested volume declared before the outer one left the check reading a ConfigMap the process never receives. Refused rather than resolved, for the reason `subPath` and `items` are — the chart has always had one mount covering that path, and which of several wins is Kubernetes' rule to state. And the Deployment was selected by kind alone. `helm template` renders whatever the template says, so `apps/v2` renders perfectly and is refused at create: the install gets every object it needs except the one that runs binpack, and every namespace, account, flag and mount assertion here stays green about a pod that does not exist. Each verified by sabotage, with a positive control for the fence change: the log-output block that is not YAML is still ignored rather than refused. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
left a comment
There was a problem hiding this comment.
💡 Codex Review
binpack/internal/cli/chart_test.go
Lines 564 to 566 in 237bff3
Derive installed namespaces only from install commands
This pattern treats every --namespace occurrence in the installation how-to as a namespace that an install creates, rather than limiting the scan to the Helm install/upgrade commands. If a kubectl diagnostic or uninstall example in that same page uses --namespace typo, installed["typo"] becomes true and the later repository-wide validation accepts that broken namespace everywhere, including on the line that introduced it, even though no install command creates it. Parse the actual installation commands before building this allowlist.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+361
to
+364
| if binding.Kind == "RoleBinding" && role.Metadata.Namespace != binding.Metadata.Namespace { | ||
| return false | ||
| } | ||
| return role.Kind == binding.RoleRef.Kind && role.Metadata.Name == binding.RoleRef.Name |
There was a problem hiding this comment.
Fresh evidence after the prior roleRef fix is that this check only requires a RoleBinding and its Role to share a namespace; it never requires that namespace to be the one where the permission is needed. If the serviceAccount.create=false path moves both the leader-election Role and RoleBinding together to another namespace, this predicate still resolves the role, the binding identity comparison still matches kind/name, and the subject checks still pass, but the external ServiceAccount receives Lease permissions only in the wrong namespace and leader election gets a 403. Compare the rendered Role namespaces with their intended release/autoscaler namespaces in this alternate render.
Useful? React with 👍 / 👎.
|
|
||
| // readGrants is every resource the rules grant read on, sorted and deduplicated. | ||
| // | ||
| // Read means get, list and watch together: binpack's cache lists and then |
There was a problem hiding this comment.
This requires get, list, and watch to coexist in one PolicyRule, although Kubernetes authorizes from the union of all matching rules. If an equivalent chart refactor puts get in one rule and list/watch in another for the same API group and resource, readGrants omits that resource and fails the exact-read checks even though the effective permission is unchanged. Accumulate verbs by group/resource across all roles and rules before deciding whether the complete read grant exists.
Useful? React with 👍 / 👎.
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 guards, one mechanism: a list written by hand beside an enumerator that already existed, so whatever the list omitted went unheld. Each commit replaces the list with the enumeration and adds the count guard that stops the enumeration degrading to nothing.
tests: pin every skip code from the enumerator, not from a hand-written listengine.SkipCodes()enumerates sixteen values. The reachability half ofTestEverySkipReasonCarriesItsOwnCode— the loop whose own comment says "every code the engine can emit must be reachable, or it is documentation for a state that cannot happen" — hard-coded ten of them.SkipProtectedPodwas one of the six omitted. It is published asbinpack_nodes_skipped{code="protected-pod"}, documented indocs/reference/metrics.md, and asserted by nothing:grep -rn SkipProtectedPod --include='*_test.go'returned nothing, while the three tests of the excluded-namespace path all pin the sentence —strings.Contains(a.SkipReason, "payments")— which is explicitly not the public half.The loop failing first, before any fixture:
Three of the six reach
Decideand are now cases in the table. The other three cannot:Decideassesses the nodes the snapshot carries, so it never sees a node that is gone; it passesresumingfalse, soeligibility's marked-but-schedulable branch is unreachable; and it refuses above the assessments when the autoscaler is not live, returning a decision code rather than a per-node skip — the emptiness that stops a dead autoscaler zeroing the node gauges.Those three are recorded rather than deleted from the enumerator, in the shape the differential harness's
notReachedByGeneratoruses: code → why it cannot be reached here, and the test that adjudicates it instead. The record is read in both directions, so a code that starts reachingDecidefails until it catches up.Sabotage — the finding's own mechanism, the branch reusing a neighbouring code:
TestExcludedNamespaceProtectsItsNodestayed green under that sabotage before this change, and now carries the code assertion its subject implies.tests: hold the RBAC agreement in three directions, not twoNothing pinned what the code calls
R3-160 records a three-way agreement — what the executor can do, what the chart grants, what the reference documents — guaranteed by four named tests. All four read documents.
TestTheRBACReferenceMatchesWhatTheExecutorDoessays in its own comment that it "checks the third side of an agreement the chart and the code already keep between them", and nothing kept it.TestTheChartGrantsWhatTheCodeCallsdrives all five writes binpack's own code makes —executor.Cordon,Annotate,HandBack,Evict, and the decision EventdirectReportercreates — against a writer that records(apiGroup, resource, subresource, verb), then compares with the chart in both directions.It cannot fail against present code, so it is verified by sabotage:
The recorder derives group and resource from the object through the scheme rather than being told per call site what each call is for — a recorder carrying its own description of
CordonandEvictwould be a second implementation of the executor's call shapes. It satisfiesexecutor.Writerandcontroller.eventWriter, so a method added to either must be answered before it compiles.It lives in
internal/controllerrather than beside the other RBAC tests becausedirectReporteris unexported and that is the only package that can see all five call sites.The reader those guards ran on
rulePairsmatchedapiGroups:/resources:/verbs:line by line and pulled values out of[...], so a rule written as a block sequence — the ordinary Kubernetes idiom, and what a YAML formatter or a pasted upstream example produces — yielded nothing and was skipped in silence. Both consumers iterate the parsed set and guarded only withlen(granted) == 0, which a partial parse is not.Demonstrated before the change by reformatting the chart's
nodes: patchrule into block style and deleting it fromdocs/reference/rbac.mdaltogether:That is the only mutating verb binpack holds on cluster state, and an operator running
rbac.create: falsewould have written a role without it. The same sabotage against the new reader:So the four hand-rolled readers of these two files become one.
internal/rbacdocparses YAML — the techniqueinternal/collect's tests already used — and every way of finding nothing is an error rather than a smaller answer. It takes no*testing.T, for the reasoninternal/mothertakes none.Each guard then gets a count in
chart_test.go:136's shape rather than a non-empty check: thirty against the thirty-six pairs the chart renders, and two against the act block's two.internal/executor's package doc now says what the sabotage says — its enumeration is executable rather than only written down.Verification
make checkgreen on each commit; green onorigin/mainbefore anything changed.golangci-lint v2.12.2(the CI pin, not the local 2.13.1):0 issues.go.mod/go.sumare untouched andtest/differentialis unaffected.Public surface
None.
SkipProtectedPodand everybinpack_nodes_skipped{code=…}value are unchanged — this PR adds the assertion that they are produced, and moves nothing.