Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ The sixth dimension of the 1:1 audit, `go run ./cmd/audit_1to1/ -scope=paths`, a

Five checks, three of which gate. Four need no network and no suite run (`make audit-1to1-paths`, run by `make analyze` and by CI's generated-artifacts job); the endpoint one needs 250 documentation pages, so it runs only when asked for (`make audit-1to1-paths-endpoints`) and nothing schedules it:

- **Has the path ever been observed.** An action whose owning package issued no request has never had its request seen by anything. Held at package grain, because nothing on the wire names an action, and the report says so beside the number (`actions_observed_grain`): 990 of 1082 means 990 actions whose _owner_ issued something, which is a regression guard and not per-action assurance. A silent package means either that nothing drives it or that the inventory is stale, and the finding cannot tell those apart. A package silent for a reason is held to a declaration with a category and a reason in `cmd/audit_1to1/internal/paths/declarations.go`, and a declaration that no longer describes the tree is itself a finding. One entry today: `internal/tools/adminspecs` declares specs whose handlers live in other packages, so its 92 actions' requests are recorded under the packages that make them. An action whose owner names no package fails too: nothing in the catalog validates that field, and such an action used to be classified unmapped, which the gate ignored and `-gaps-only` dropped, so it could be neither counted nor seen. The owner `tools` is the catalog's own exception, for the orchestration package rather than a domain under it, and resolves to `internal/tools`. An owner that names a real package and the wrong one is a lie no version of this can catch, since the recording joins on that name and nothing else.
- **Has the path ever been observed.** An action whose owning package issued no request has never had its request seen by anything. Held at package grain, because nothing on the wire names an action, and the report says so beside the number (`actions_observed_grain`): 1082 of 1082 means 1082 actions whose _owner_ issued something, which is a regression guard and not per-action assurance. A silent package means either that nothing drives it or that the inventory is stale, and the finding cannot tell those apart. A package silent for a reason is held to a declaration with a category and a reason in `cmd/audit_1to1/internal/paths/declarations.go`, and a declaration that no longer describes the tree is itself a finding. No package is declared today: `internal/tools/adminspecs` was the one entry, for declaring 92 specs whose handlers live in other packages, until each of those actions was given the domain package its route names as its owner. Naming the owner where the handler lives is the better answer of the two, because a declaration excuses a join that cannot be made while a true owner makes it, and the sharper join is what put `admin.system_hook_list` on R-PAGE's list below. An action whose owner names no package fails too: nothing in the catalog validates that field, and such an action used to be classified unmapped, which the gate ignored and `-gaps-only` dropped, so it could be neither counted nor seen. The owner `tools` is the catalog's own exception, for the orchestration package rather than a domain under it, and resolves to `internal/tools`. An owner that names a real package and the wrong one is a lie no version of this can catch, since the recording joins on that name and nothing else.

**The grain sharpens to the action where an end-to-end run speaks** (`e2e_observation`, `make audit-1to1-paths-e2e`). The unit recording cannot name an action because the httptest server answers on its own goroutine; a Docker run can, because the harness stamps a trace id into each MCP call, the server's span carries it back with the route the dispatcher actually chose, and every GitLab request the handler made is a child of that span. The receiver counts those children per trace and the harness writes the count on the `dispatch` line it already emits (`e2ecalls.Dispatch.Requests`), so `-e2e-calls <dir>` answers, per action, which were seen issuing a request and which ran and issued none. It **reports and never gates**, and both halves of that are about what a missing record means: the shards are a byproduct of a run CI does not schedule and never commits, so failing on their absence would fail every push, and the counts are floors, since the batching span processor drops silently when its queue overflows. A positive claim is therefore solid and a negative one is a lead. A dispatch carrying a refusal reason is kept off the lead list, because the server declining to run something is not a handler that could not build a request.
- **Does the document validate.** Every raw GraphQL document in the source against the pinned schema, through `cmd/internal/graphqldocs`, shared with `make check-graphql-documents`. It judges the document and never the values: four of the nine broken tools sent a document the schema refuses and the other five sent an accepted document carrying a value GitLab does not have, and the second half is caught by `internal/testutil`'s validating transport, which checks the variables with the document on every request a test drives. Neither substitutes for the other.
Expand All @@ -350,7 +350,7 @@ Five checks, three of which gate. Four need no network and no suite run (`make a

**It reports and does not gate**, for a different reason than the REST joins: the pin is exactly what GitLab serves, so the oracle is not incomplete, but a field GitLab offers is a candidate for the surface rather than a defect in it, GitLab adds fields weekly, and this dimension has **no tier oracle and no deprecation oracle** — the schema declares no tier (GitLab gates a GraphQL field at resolve time) and the pin carries no `@deprecated`, because `cmd/internal/graphqlintrospect` drops the directives and descriptions on decode. Both blind spots are stated on the report rather than silently absorbed, and both are closed by one sidecar record written from the introspection fetch that already carries the data. Nine of the eleven domains are Premium or Ultimate surfaces, so most of these findings sit on licensed ground: a `tier` is omitted from a finding rather than emitted empty, since an empty field would read as a condition that was checked and found absent. The one sub-class that **does** gate, and on every run, is a mutation payload whose `errors` **no field of the decoder reads**, which drops GitLab's account of a refused mutation and reports success; the condition is the decoder and not the document, because a payload that selects its errors and decodes none loses them just as completely, while the reverse is already a hard failure of the always-empty leg. A finding is answered by an entry in `cmd/audit_graphql_shapes/sent_declarations.go`, keyed by package, schema type and field, and a stale declaration fails the run on the terms every declaration table here is held to; two of the nine answer the work item a notes query names only to reach its notes widget, which is 68 rows. The record is deliberately uncommitted and not freshness-gated: a schema re-pin would churn it every time.

- **Does a list say where it ends.** R-PAGE, added because a blind spot was found by accident and every dimension above was green on it: `internal/tools/impersonationtokens` answers `user.list_impersonation_tokens` with a bare array of tokens while GitLab serves twenty at a time, so a caller cannot tell it has one page and cannot ask for the second. None of the six could see it, because all six compare a **published field** against the SDK struct, the documentation or the entity record, and GitLab's pagination is a field of no entity: an offset page arrives in the `X-Page`, `X-Next-Page`, `X-Per-Page`, `X-Total` and `X-Total-Pages` response headers, and a keyset page in a `Link`. The oracle is the same live record read through its params rather than its entities: 308 of its 2110 routes declare `per_page`, 304 with `page` beside it (offset) and 4 with a `cursor` or `page_token` instead (keyset, counted apart and raising no finding, since a page-and-total block is the wrong shape for one; none is reached by a recorded request today). An action is judged when its output is a **collection envelope** — exactly one content field, a list of objects, with this server's own framing taken out — which is where "the route declares the params but the action reads a single object" is answered structurally rather than one declaration at a time: a project carrying `shared_with_groups` is a single-object read, and admitting those turned 24 findings into 79. Of 268 collection-reading actions, 224 publish a pagination block, 20 sit in a package no paginated endpoint was recorded for, and **24 are findings**, 14 of them naming an action whose own route declares `per_page`. The join is the package, like the observation check's, and `pagination_endpoint_grain` says so beside the number; the 10 findings that matched a sibling's endpoint are declared in `cmd/audit_1to1/internal/paths/pagination_declarations.go` with the route the record holds for each (`GET /projects/:id/issues/:issue_iid/participants` declares neither param; `GET /projects/:id/languages` answers with an object rather than an array; `project.target_branch_rule_list` is read over GraphQL and has no REST route at all), and there is deliberately no category for "GitLab pages this and we have decided not to" — that is the finding. It **reports and does not gate**: a finding is a surface change, and `request_paginates` on each says whether the fix is one output field or an input that has to offer `page` and `per_page` first. Its declaration table gates like every other one here.
- **Does a list say where it ends.** R-PAGE, added because a blind spot was found by accident and every dimension above was green on it: `internal/tools/impersonationtokens` answers `user.list_impersonation_tokens` with a bare array of tokens while GitLab serves twenty at a time, so a caller cannot tell it has one page and cannot ask for the second. None of the six could see it, because all six compare a **published field** against the SDK struct, the documentation or the entity record, and GitLab's pagination is a field of no entity: an offset page arrives in the `X-Page`, `X-Next-Page`, `X-Per-Page`, `X-Total` and `X-Total-Pages` response headers, and a keyset page in a `Link`. The oracle is the same live record read through its params rather than its entities: 308 of its 2110 routes declare `per_page`, 304 with `page` beside it (offset) and 4 with a `cursor` or `page_token` instead (keyset, counted apart and raising no finding, since a page-and-total block is the wrong shape for one; none is reached by a recorded request today). An action is judged when its output is a **collection envelope** (exactly one content field, a list of objects, with this server's own framing taken out), which is where "the route declares the params but the action reads a single object" is answered structurally rather than one declaration at a time: a project carrying `shared_with_groups` is a single-object read, and admitting those turned 24 findings into 79. Of 268 collection-reading actions, 224 publish a pagination block, 19 sit in a package no paginated endpoint was recorded for, and **25 are findings**, 15 of them undeclared. `admin.system_hook_list` is the newest and shows what the join costs when an owner is coarse: it was in the not-asked-about count while its owner was `adminspecs`, which records nothing, and naming `systemhooks` as its owner put it against the one paginated endpoint that package records, `GET /hooks`, which is the endpoint the action itself calls. The join is the package, like the observation check's, and `pagination_endpoint_grain` says so beside the number; the 10 findings that matched a sibling's endpoint are declared in `cmd/audit_1to1/internal/paths/pagination_declarations.go` with the route the record holds for each (`GET /projects/:id/issues/:issue_iid/participants` declares neither param; `GET /projects/:id/languages` answers with an object rather than an array; `project.target_branch_rule_list` is read over GraphQL and has no REST route at all), and there is deliberately no category for "GitLab pages this and we have decided not to": that is the finding. It **reports and does not gate**: a finding is a surface change, and `request_paginates` on each says whether the fix is one output field or an input that has to offer `page` and `per_page` first. Its declaration table gates like every other one here.

What none of it can do is check that GitLab answers a **specific request** the way our output struct expects: the shape check reads what GitLab's document says an endpoint returns, never what an instance returned for a call we made. The layers stack: the pinned schema and the inventory catch a request that cannot work, a real instance catches a response we misread, and the other five rules keep catching the surface we failed to expose. One more limit is worth knowing because it hid a defect: the inventory records which parameter names an endpoint was sent, never which of them were sent together, so a combination GitLab refuses is invisible here. `gitlab_get_catalog_resource` was in that state (`id` and `full_path` are both optional and GitLab accepts exactly one), and the answer to that class is a handler that refuses the combination and a schema that says so, not an inventory of every combination the fixtures happen to use.

Expand Down
18 changes: 9 additions & 9 deletions cmd/audit_1to1/internal/paths/declarations.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ const (
// A declaration that no longer describes the tree is a finding too: a package
// that has since recorded a request, or that no longer owns any action, leaves
// a claim behind that a later reader would trust.
var declaredSilentOwners = map[string]silentOwnerDeclaration{
"adminspecs": {
Category: categoryRecordedElsewhere,
Reason: "declares the instance-administration specs whose handlers live in the domain packages " +
"(topics, settings, system hooks and twenty more), so every request it owns an action for is " +
"recorded under the package that issues it. Its own silence says nothing about whether those " +
"requests were seen, which is why the count of silent actions is read package by package.",
},
}
//
// The table is empty, and an empty one is the healthy state rather than an
// unfinished one. It held a single entry until then: internal/tools/adminspecs
// declared the 92 instance-administration actions while their handlers, and so
// their requests, live in the twenty-three domain packages the routes name.
// Each of those actions now names the package it routes to, which is the answer
// a declaration can only approximate, since a declaration excuses a join that
// cannot be made and a true owner makes it.
var declaredSilentOwners = map[string]silentOwnerDeclaration{}
16 changes: 10 additions & 6 deletions cmd/audit_1to1/internal/paths/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,16 @@
// What no version of this can catch is an owner that names a real package and
// the wrong one, because the recording joins on that name and nothing else.
//
// A package may nevertheless be silent for a reason, and internal/tools/adminspecs
// is the whole of it today: it declares specs whose handlers live in other
// packages, so its requests are recorded under the package that made them.
// Such a package is held to a declaration with a reason, the way -scope=sdk
// holds a client-go service to one, and a declaration that no longer describes
// the tree is itself a finding.
// A package may nevertheless be silent for a reason: it may declare specs
// whose handlers live in other packages, so that its requests are recorded
// under the packages that made them. Such a package is held to a declaration
// with a reason, the way -scope=sdk holds a client-go service to one, and a
// declaration that no longer describes the tree is itself a finding. No
// package is declared today. internal/tools/adminspecs was the one entry, for
// exactly that reason, until each of its 92 admin actions was given the domain
// package its route names as its owner; naming the owner where the handler
// lives is the better answer, because a declaration only excuses a join that
// cannot be made while an owner that is true makes it.
//
// # Does the document validate
//
Expand Down
12 changes: 7 additions & 5 deletions cmd/internal/requestinventory/coverage.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,13 @@ type Owner struct {
// catalog's ownership metadata rather than in any test.
//
// Silent is weaker than "never exercised" for a second reason beyond the
// coarse grain, and internal/tools/adminspecs is the whole of it today: a
// package may declare specs whose handlers live in other packages, and the
// request is then recorded under the package that made it while the count of
// silent actions blames the one that declared them. Reading the silent list as
// a work list means reading it package by package, not action by action.
// coarse grain: a package may declare specs whose handlers live in other
// packages, and the request is then recorded under the package that made it
// while the count of silent actions blames the one that declared them.
// internal/tools/adminspecs was that case for 92 actions until each of them was
// given the domain package its route names as its owner. Reading the silent
// list as a work list still means reading it package by package, not action by
// action.
type Coverage struct {
Total int
Covered int
Expand Down
Loading
Loading