diff --git a/CLAUDE.md b/CLAUDE.md index 8186d1b66..c98b51b70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ gitlab-mcp-server/ ├── cmd/ │ ├── server/ # MCP server entry point and --shutdown support │ ├── audit_1to1/ # Consolidated 1:1 SDK↔API parity audit (R-INPUT/R-OUTPUT/R-ACTION/R-META/R-ENUM + merge; R-PATH's `shapes.sent` and `shapes.typed.unsurfaced` list the fields GitLab sends that we do not publish, each with the entity condition and license tier read from `gitlab-api-live.json`, the list the field-by-field review reads, and `sent_declarations.go` answers the ones the record lists and the endpoint does not send; R-PAGE's `pagination.unpaginated` lists the actions that hand a model a list and no way to ask for the rest of it, judged against the 308 routes the same record says GitLab pages, and `pagination_declarations.go` answers the ones whose own route declares no `per_page`; -scope=sdk gates the service universe, the raw-GraphQL exemptions and the enum values; -scope=paths is R-PATH, the only rule that reads the request a handler builds rather than the surface it publishes — see Request paths below) -│ ├── audit_action_ids/ # Holds every canonical action ID the server publishes to a model against the IDs the catalog builds: the `RelatedActions` of an `ActionSpec`, the first argument of every `toolutil.HintAction` call, and a dotted ID spelled inside a `Usage` line or an individual tool's `Description`. Loaded through `cmd/internal/goprogram` and folded by the type checker rather than matched as text, because the IDs are package-local constants, fifty packages keep them in a metadata table with a lowercase `related` field, one keeps that table as a map to an anonymous struct, several hand the list in as a parameter, and two build an ID by concatenating a domain constant onto a name; a value it cannot fold is named rather than passed over. The oracle is the catalog built at Ultimate twice, self-managed and GitLab.com, since Orbit's group is contributed only for GitLab.com and one build alone reports its six IDs as dead. It answers whether an ID resolves and never whether it is the right one, and a registered alias is reported apart rather than counted a finding, since `gitlab_execute_action` resolves one and `gitlab_find_action` publishes canonical IDs. It reports and does not gate (make audit-action-ids) +│ ├── audit_action_ids/ # Holds every canonical action ID the server publishes to a model against the IDs the catalog builds: the `RelatedActions` of an `ActionSpec`, the first argument of every `toolutil.HintAction` call, and a dotted ID spelled inside a `Usage` line or an individual tool's `Description`. Loaded through `cmd/internal/goprogram` and folded by the type checker rather than matched as text, because the IDs are package-local constants, fifty packages keep them in a metadata table with a lowercase `related` field, one keeps that table as a map to an anonymous struct, several hand the list in as a parameter, and two build an ID by concatenating a domain constant onto a name; a value it cannot fold is named rather than passed over. The oracle is the catalog built at Ultimate twice, self-managed and GitLab.com, since Orbit's group is contributed only for GitLab.com and one build alone reports its six IDs as dead. It answers whether an ID resolves and never whether it is the right one, which is a permanent limit: being the right ID is a claim about the object an action reaches, and no set of names carries it. A registered alias is judged by where it is written, since both facts about one are true: `gitlab_execute_action` resolves it and `gitlab_find_action` publishes canonical IDs. In a `RelatedActions` entry or a `HintAction` argument it is a finding, with the canonical ID named beside it, because those are handed to a model as the ID to call next and an alias there can be followed once and never looked up; in a `Usage` line or a description it is reported apart and not counted, because naming an alias can be the substance of the sentence (`issue.update`'s usage says dynamic execute also accepts `issue.close` and `issue.reopen`). It reports and does not gate (make audit-action-ids) │ ├── audit_catalog_first/ # Enforces catalog-first registration invariants (ADR-0004) │ ├── audit_dead_consts/ # The half of `unused` staticcheck cannot reach: it judges a const group as one unit, so a member sharing a declaration with a member that is read is never looked at, and no `unused` setting changes it under golangci-lint v2. That group is the prevailing shape here (the action-ID block every domain keeps, the assertion-message block beside it in the tests), and the first run found twenty-five constants nothing reads. It loads `./internal/...` and `./cmd/...` through `cmd/internal/goprogram` with the test variants, since a constant a test reads is read and forty-odd packages hand their whole ID block to the catalog test through an `export_test.go`, which is also its one blind spot, a block exported wholesale being read wholesale. Exported constants are out of scope, because one may be read from a package behind a build tag this load leaves out; the packages carrying a GOOS- or GOARCH-constrained file are read again under each operating system and architecture pair the release builds, because a constant only the Windows half reads is read and so is one only the arm64 half reads, and setting the operating system alone would keep the host's architecture and leave an `_arm64.go` file out on amd64. A constant kept although nothing reads it is declared in `declarations.go` with its reason, keyed `package:name` at package scope and `package:Func.name` or `package:Type.Method.name` inside a function, so an entry for the one never excuses a local constant sharing its name; the stale judgement is scoped to the packages the run loaded (make check-dead-consts, step 4 of `make analyze`) │ ├── audit_discovery_completeness/ # Audits discovery metadata (aliases/usage/related/param-guidance/sibling-cluster; input-enum candidates) — META-001 @@ -832,6 +832,8 @@ Developers add normal GitLab actions through domain-local `ActionSpecs` and the Find combines canonical `domain.action` IDs, domain/action names, aliases, natural-language stopword filtering (removing frequent non-informative words), synonyms, fuzzy matching, and segmented matching for multi-intent prompts. Models should use `gitlab_find_action` to retrieve exact schemas, then execute the canonical action ID returned by find. See `docs/concepts/dynamic-tools.md` and ADR-0011. +**The `related_actions` a result carries are narrowed to the session that reads them.** The curated lists in the `ActionSpec`s are written against the whole catalog and every session is served a narrower one, so a Free instance used to be handed cross-links to Premium actions and an `--exclude-tools` deployment links to actions it had removed. `Registry.publishedRelatedActions` decides by what following the link would answer: one that resolves here is published under its canonical ID whatever spelling it was written in, one a filter withheld is kept because asking for it produces the narrowing rather than a typo correction (`withheldActionMessage`), and one neither can explain is dropped, since `unknown action` plus near-miss suggestions reads as a spelling correction and teaches a model the capability is absent. Measured at the Free tier the dropped set is 4 links in 3 packages, all of them Free actions naming Premium siblings. + **What one find call may cost is bounded, in three places that belong together.** The query is capped at `dynamic.MaxSearchQueryLength` (256 characters, published as the schema's `maxLength` and refused, never truncated), because a search costs the word count times the catalog three times over (lexical, fuzzy, and one pass per segment window) and a request body may be 4 MiB. The scoring passes check the context, so an abandoned POST stops costing and a deadline can end one; a cancelled search returns the error rather than an empty result. And the handler takes `toolutil.WithActionDeadline` itself, because it is registered directly rather than through one of the `WrapAction` functions and so passed through no deadline at all. `BenchmarkFind_PathologicalQuery` is what keeps a scorer change from quietly raising the ceiling. ### Enterprise tool gating diff --git a/cmd/audit_action_ids/doc.go b/cmd/audit_action_ids/doc.go index 4df9dc1b1..8a667c50c 100644 --- a/cmd/audit_action_ids/doc.go +++ b/cmd/audit_action_ids/doc.go @@ -43,13 +43,27 @@ // cross-linked to the first is silent here, because the first resolves. What // this narrows is the field to the IDs that cannot work at all. // -// An ID that is a registered alias rather than a catalog ID is reported apart, -// under "alias", and is not counted as a finding. gitlab_execute_action -// resolves an alias, so a hint naming one works today; gitlab_find_action -// publishes canonical IDs, so a model that looks the name up in a listing does -// not find it. Which of those two facts should decide is a question for the -// layer that fixes the cross-links, and this command's job is to put both sets -// in front of it rather than to settle it. +// That limit is permanent, and the reason is worth stating so nobody tries to +// close it here: the oracle is the set of IDs, and being the right ID is a +// claim about the object an action reaches, which no set of names carries. +// Reading each list against the parameters its own action requires is what +// answers it, and that is a review rather than a rule. A membership check is +// also blind to a cross-link that resolves for this tree and not for the +// session reading it, which is why the projection filters what it publishes +// (Registry.publishedRelatedActions in internal/tools/dynamic) instead of +// leaving that to a gate here. +// +// An alias is judged by where it is written, which is the one thing this used +// to leave open. Both facts about an alias are true: gitlab_execute_action +// resolves one, and gitlab_find_action publishes canonical IDs and so lists it +// under no name. In a RelatedActions entry or a HintAction argument the second +// decides, because those are handed to a model as the ID to call next and an +// alias there can be followed once and never looked up, so it is a finding +// with the canonical ID named beside it as the fix. In a Usage line or a +// description the first decides, because naming an alias can be the substance +// of the sentence: issue.update's usage says that dynamic execute also accepts +// issue.close and issue.reopen. Those stay reported apart, under "alias", and +// are not counted as findings. // // # It reports and does not gate // diff --git a/cmd/audit_action_ids/report.go b/cmd/audit_action_ids/report.go index ed115024f..67cbc15ec 100644 --- a/cmd/audit_action_ids/report.go +++ b/cmd/audit_action_ids/report.go @@ -14,7 +14,12 @@ import ( // schemaVersion is the shape of the work list this writes. A later layer reads // the file to know which cross-links to fix, so a change to the shape has to // be visible to it. -const schemaVersion = 1 +// +// Version 2 moved an alias written into a related entry or a hint argument out +// of alias_references and into findings, carrying its canonical target in the +// same `canonical` field. The field set is unchanged and the counts are not: +// a reader that compares two runs across this line is comparing two rules. +const schemaVersion = 2 // dottedToken matches an action-ID-shaped token inside prose. The shape alone // is far too generous, which is why every match is also held to a domain the @@ -75,10 +80,21 @@ type Report struct { // classify holds every site against the oracle and builds the report. // -// The three outcomes are deliberately kept apart. A canonical ID is silent, an -// alias is reported without being counted a finding (gitlab_execute_action -// resolves it, gitlab_find_action does not publish it, and which of those -// decides is not this command's question), and anything else is a finding. +// The three outcomes are deliberately kept apart. A canonical ID is silent, +// anything the catalog has never heard of is a finding, and an alias is judged +// by where it was written. +// +// That last rule is the one this command deferred while there was nothing to +// settle it with. Both halves of the old reasoning are true at once: +// gitlab_execute_action resolves an alias, and gitlab_find_action publishes +// canonical IDs and so lists it under no name. What decides between them is +// the site. A related entry and a hint argument are structured fields that the +// discovery tools hand a model as the ID to call next, and an alias there is a +// cross-link a model can follow once and can never look up, so it is a +// finding. A Usage line or a description is prose, where naming an alias can +// be the whole point of the sentence: issue.update's usage says that dynamic +// execute also accepts issue.close and issue.reopen, which is true, useful, +// and would be a defect under one rule for both. Those stay reported apart. func classify(sites []site, ids *oracle) Report { report := Report{ SchemaVersion: schemaVersion, @@ -119,7 +135,12 @@ func (r *Report) judge(at site, candidate string, ids *oracle) { finding := Finding{Package: at.Package, File: at.File, Line: at.Line, Kind: at.Kind, ID: candidate} if canonical, isAlias := ids.alias(candidate); isAlias { finding.Canonical = canonical - r.AliasRefs = append(r.AliasRefs, finding) + if isProseKind(at.Kind) { + r.AliasRefs = append(r.AliasRefs, finding) + return + } + r.Findings = append(r.Findings, finding) + r.Summary.ByKind[at.Kind]++ return } finding.Closest = closestID(candidate, ids.sorted) @@ -269,10 +290,10 @@ func editDistance(left, right string) int { // to act on them, then the two buckets that are not findings, then what the // run saw. func writeReport(out io.Writer, report Report, verbose bool) { - writeGroups(out, report.Findings, "resolves to no action") + writeGroups(out, report.Findings, findingVerb) if verbose { - fmt.Fprintln(out, "=== registered aliases, not catalog IDs ===") - writeGroups(out, report.AliasRefs, "alias of") + fmt.Fprintln(out, "=== aliases named in prose, not catalog IDs ===") + writeGroups(out, report.AliasRefs, aliasVerb) writeUnresolved(out, report.Unresolved) } writeStale(out, report.StaleExemptions) @@ -292,8 +313,14 @@ func writeStale(out io.Writer, stale []string) { } } -// writeGroups prints findings under one `=== package ===` heading each. -func writeGroups(out io.Writer, findings []Finding, verb string) { +// writeGroups prints findings under one `=== package ===` heading each, each +// row read with the verb its own outcome deserves. +// +// The verb is per row rather than per list because the findings list now holds +// two outcomes: an ID nothing resolves, and an alias written where a catalog +// ID belongs. Printing the second under the first's verb would say a string +// resolves to nothing while naming what it resolves to. +func writeGroups(out io.Writer, findings []Finding, verb func(Finding) string) { current := "" for _, finding := range findings { if finding.Package != current { @@ -301,10 +328,22 @@ func writeGroups(out io.Writer, findings []Finding, verb string) { fmt.Fprintf(out, "=== %s ===\n", current) } fmt.Fprintf(out, " %s:%d %s %q %s%s\n", - finding.File, finding.Line, finding.Kind, finding.ID, verb, trailer(finding)) + finding.File, finding.Line, finding.Kind, finding.ID, verb(finding), trailer(finding)) } } +// findingVerb reads one finding: an alias standing in for the canonical ID, or +// a string the catalog has never heard of. +func findingVerb(finding Finding) string { + if finding.Canonical != "" { + return "is an alias, not the catalog ID" + } + return "resolves to no action" +} + +// aliasVerb reads a row of the prose bucket, where naming an alias is allowed. +func aliasVerb(Finding) string { return "alias of" } + // trailer renders whatever a finding knows beyond the ID itself. func trailer(finding Finding) string { if finding.Canonical != "" { @@ -330,7 +369,7 @@ func writeUnresolved(out io.Writer, unresolved []Unresolved) { // writeSummary prints what the run saw. func writeSummary(out io.Writer, summary Summary, verbose bool) { - fmt.Fprintf(out, "%s: %d published ID(s) resolve to nothing in %d package(s); %d alias reference(s); %d site(s) not folded; %d stale exemption(s)\n", + fmt.Fprintf(out, "%s: %d published ID(s) to fix in %d package(s); %d alias(es) named in prose; %d site(s) not folded; %d stale exemption(s)\n", toolName, summary.Findings, summary.Packages, summary.AliasHits, summary.Unresolved, summary.Stale) fmt.Fprintf(out, " judged %d published ID(s) against %d catalog ID(s) and %d alias(es)\n", summary.Judged, summary.CatalogIDs, summary.Aliases) diff --git a/cmd/audit_action_ids/report_test.go b/cmd/audit_action_ids/report_test.go index 8fc867aa7..5a161e948 100644 --- a/cmd/audit_action_ids/report_test.go +++ b/cmd/audit_action_ids/report_test.go @@ -28,34 +28,44 @@ func stubOracle() *oracle { return ids } -// TestClassify_ThreeOutcomes_AreKeptApart holds the split the whole report -// rests on: an ID the catalog has is silent, a registered alias is reported -// without being counted a finding, and anything else is a finding. -func TestClassify_ThreeOutcomes_AreKeptApart(t *testing.T) { +// TestClassify_FourOutcomes_AreKeptApart holds the split the whole report +// rests on: an ID the catalog has is silent, anything it has never heard of is +// a finding, and an alias is one or the other depending on where it is +// written. In a structured field it is a finding carrying the canonical ID as +// the fix; in prose it is reported without being counted, because a sentence +// may be about the alias. +func TestClassify_FourOutcomes_AreKeptApart(t *testing.T) { report := classify([]site{ {Package: "p", File: "p/a.go", Line: 1, Kind: kindRelated, Value: "demo.get", Resolved: true}, {Package: "p", File: "p/a.go", Line: 2, Kind: kindRelated, Value: "demo.fetch", Resolved: true}, {Package: "p", File: "p/a.go", Line: 3, Kind: kindHint, Value: "demo.gone", Resolved: true}, - {Package: "p", File: "p/a.go", Line: 4, Kind: kindRelated, Expr: "helper(x)"}, + {Package: "p", File: "p/a.go", Line: 4, Kind: kindUsage, Value: "Dynamic execute also accepts demo.fetch.", Resolved: true}, + {Package: "p", File: "p/a.go", Line: 5, Kind: kindRelated, Expr: "helper(x)"}, }, stubOracle()) - if report.Summary.Findings != 1 || report.Findings[0].ID != "demo.gone" { - t.Errorf("findings = %+v, want the one dead ID", report.Findings) + if report.Summary.Findings != 2 { + t.Fatalf("findings = %+v, want the structured alias and the dead ID", report.Findings) + } + if report.Findings[0].ID != "demo.fetch" || report.Findings[0].Canonical != "demo.get" { + t.Errorf("first finding = %+v, want demo.fetch naming demo.get as the fix", report.Findings[0]) } - if report.Summary.AliasHits != 1 || report.AliasRefs[0].Canonical != "demo.get" { - t.Errorf("alias references = %+v, want demo.fetch resolved to demo.get", report.AliasRefs) + if report.Findings[1].ID != "demo.gone" || report.Findings[1].Canonical != "" { + t.Errorf("second finding = %+v, want the dead ID with no canonical target", report.Findings[1]) + } + if report.Summary.AliasHits != 1 || report.AliasRefs[0].Kind != kindUsage { + t.Errorf("alias references = %+v, want only the alias named in prose", report.AliasRefs) } if report.Summary.Unresolved != 1 || report.Unresolved[0].Expression != "helper(x)" { t.Errorf("unresolved = %+v, want the expression named", report.Unresolved) } - if report.Summary.Judged != 3 { - t.Errorf("judged = %d, want the three folded IDs", report.Summary.Judged) + if report.Summary.Judged != 4 { + t.Errorf("judged = %d, want the three folded IDs and the prose token", report.Summary.Judged) } if report.Summary.Packages != 1 { t.Errorf("packages with findings = %d, want 1", report.Summary.Packages) } - if report.Summary.ByKind[kindHint] != 1 { - t.Errorf("findings by kind = %v, want the hint counted", report.Summary.ByKind) + if report.Summary.ByKind[kindHint] != 1 || report.Summary.ByKind[kindRelated] != 1 { + t.Errorf("findings by kind = %v, want the hint and the related entry counted", report.Summary.ByKind) } } @@ -210,12 +220,17 @@ func TestEditDistance_KnownPairs_AreTheLevenshteinDistance(t *testing.T) { // TestWriteReport_Verbose_AddsTheBucketsThatAreNotFindings holds what the two // report modes say. The quiet one is the work list; the verbose one adds the -// aliases and the sites that could not be folded, which are the audit's own -// blind spot rather than a clean answer. +// aliases named in prose and the sites that could not be folded, which are the +// audit's own blind spot rather than a clean answer. +// +// The two alias rows are both here on purpose, because each is printed with a +// verb of its own: the one written into a cross-link is a finding and is read +// as a spelling to correct, the one written into a sentence is not. func TestWriteReport_Verbose_AddsTheBucketsThatAreNotFindings(t *testing.T) { report := classify([]site{ {Package: "p", File: "p/a.go", Line: 3, Kind: kindHint, Value: "demo.gone", Resolved: true}, {Package: "p", File: "p/a.go", Line: 2, Kind: kindRelated, Value: "demo.fetch", Resolved: true}, + {Package: "p", File: "p/a.go", Line: 5, Kind: kindUsage, Value: "Execute also accepts demo.fetch.", Resolved: true}, {Package: "p", File: "p/a.go", Line: 4, Kind: kindRelated, Expr: "helper(x)"}, }, stubOracle()) @@ -227,10 +242,21 @@ func TestWriteReport_Verbose_AddsTheBucketsThatAreNotFindings(t *testing.T) { if strings.Contains(quiet.String(), "helper(x)") { t.Error("the quiet report printed the unresolved bucket") } + if strings.Contains(quiet.String(), "alias of") { + t.Error("the quiet report printed the prose alias bucket") + } var loud bytes.Buffer writeReport(&loud, report, true) - for _, want := range []string{"demo.gone", "demo.fetch", "helper(x)", "exemptions that excuse nothing", "judged by kind"} { + for _, want := range []string{ + "demo.gone", + `"demo.fetch" is an alias, not the catalog ID demo.get`, + "aliases named in prose", + `"demo.fetch" alias of demo.get`, + "helper(x)", + "exemptions that excuse nothing", + "judged by kind", + } { t.Run(want, func(t *testing.T) { if !strings.Contains(loud.String(), want) { t.Errorf("the verbose report left out %q", want) diff --git a/cmd/audit_action_ids/source.go b/cmd/audit_action_ids/source.go index ce0f7e011..61a260d06 100644 --- a/cmd/audit_action_ids/source.go +++ b/cmd/audit_action_ids/source.go @@ -610,23 +610,61 @@ func (w *walker) recordListCall(kind string, call *ast.CallExpr) { } // isListCopy reports whether a call does nothing but hand back a list already -// recorded where it was written, which is what cloneStrings(spec.RelatedActions) -// is. Following such a call into its body finds a loop over a parameter and no -// literal, so recognizing the copy is the difference between a quiet -// pass-through and a site reported as unfoldable. +// recorded where it was written. Following such a call into its body finds a +// loop over a parameter and no literal, so recognizing the copy is the +// difference between a quiet pass-through and a site reported as unfoldable. +// +// Two shapes qualify, and the second is a narrowing rather than a copy: +// cloneStrings(spec.RelatedActions) is handed the list itself, and +// Registry.publishedRelatedActions(entry) is handed the value it hangs off and +// returns the subset one session may be shown. Both are judged where the IDs +// are written, which is the whole reason this is safe: a pass-through can drop +// an ID or respell it, and the declaration it came from is still read. What +// neither shape can prove is that the body adds no ID of its own, so a literal +// written inside one is a hole in this audit rather than a finding. That hole +// was accepted for the copy and is the same size here. func (w *walker) isListCopy(call *ast.CallExpr) bool { if len(call.Args) == 0 { return false } for _, arg := range call.Args { - selector, isSelector := ast.Unparen(arg).(*ast.SelectorExpr) - if !isSelector || !w.isIDListRead(selector) { + if !w.carriesRecordedIDList(arg) { return false } } return true } +// carriesRecordedIDList reports whether one argument of a call is an ID list +// this walk records where it is written, or a value carrying one. +// +// The second half is deliberately narrow: a bare name whose type is a struct +// of this module with an ID-list field of its own. Anything looser would +// silence a call that was handed nothing to do with action IDs and returned a +// list of them. +func (w *walker) carriesRecordedIDList(arg ast.Expr) bool { + switch expr := ast.Unparen(arg).(type) { + case *ast.SelectorExpr: + return w.isIDListRead(expr) + case *ast.Ident: + structType, ok := w.structType(w.pkg.TypesInfo.TypeOf(expr)) + return ok && hasIDListField(structType) + default: + return false + } +} + +// hasIDListField reports whether a struct declares a field this walk reads as +// a list of canonical action IDs. +func hasIDListField(structType *types.Struct) bool { + for field := range structType.Fields() { + if _, isIDList := idListFieldNames[strings.ToLower(field.Name())]; isIDList && isStringSlice(field.Type()) { + return true + } + } + return false +} + // recordAppend records the arguments of an append: the first is the list being // grown and is followed back through the same rule, the rest are elements, // unless the call spreads a slice, in which case that slice is a list too. diff --git a/cmd/audit_action_ids/source_test.go b/cmd/audit_action_ids/source_test.go index e5b1fafaa..a74d322cf 100644 --- a/cmd/audit_action_ids/source_test.go +++ b/cmd/audit_action_ids/source_test.go @@ -287,6 +287,72 @@ func options() toolutil.ActionSpecOptions { } } +// TestCollectSites_AListNarrowedByAProjection_IsPassedOver holds the second +// shape the pass-through rule accepts: a call handed the value an ID list +// hangs off, returning the subset one caller may be shown. The dynamic +// registry narrows its cross-links that way before publishing them, and the +// IDs it narrows are declared in the catalog and judged there. +// +// The negative half is what keeps the rule from being a blanket silence: a +// call handed a value carrying no ID list has had nothing recorded for it, so +// the list it returns lands in the unresolved bucket like any other. +func TestCollectSites_AListNarrowedByAProjection_IsPassedOver(t *testing.T) { + t.Run("the value carries an ID list", func(t *testing.T) { + sites := collectFixture(t, `package fixture + +import "github.com/jmrplens/gitlab-mcp-server/v3/internal/toolutil" + +type entry struct { + ID string + RelatedActions []string +} + +func published(e entry) []string { + out := make([]string, 0, len(e.RelatedActions)) + for _, id := range e.RelatedActions { + if id != "" { + out = append(out, id) + } + } + return out +} + +func options(e entry) toolutil.ActionSpecOptions { + return toolutil.ActionSpecOptions{RelatedActions: published(e)} +} +`) + + if got := unresolvedExprs(sites); len(got) != 0 { + t.Errorf("unresolved = %v, want none: the narrowed list is declared and judged where it was written", got) + } + }) + + t.Run("the value carries none", func(t *testing.T) { + sites := collectFixture(t, `package fixture + +import "github.com/jmrplens/gitlab-mcp-server/v3/internal/toolutil" + +type label struct { + Name string +} + +func published(l label) []string { + out := make([]string, 0, 1) + out = append(out, l.Name) + return out +} + +func options(l label) toolutil.ActionSpecOptions { + return toolutil.ActionSpecOptions{RelatedActions: published(l)} +} +`) + + if got := unresolvedExprs(sites); len(got) == 0 { + t.Error("a list built from a value carrying no action IDs was passed over silently") + } + }) +} + // TestCollectSites_ShapesWithNoIDInThem_AreHandled walks the shapes that // carry no action ID and must not be mistaken for one: a struct literal // written positionally, a list taken out of a slice of lists, and a read of a diff --git a/docs/development/tool-surfaces-and-action-core.md b/docs/development/tool-surfaces-and-action-core.md index 9c34a001c..7b10c8509 100644 --- a/docs/development/tool-surfaces-and-action-core.md +++ b/docs/development/tool-surfaces-and-action-core.md @@ -332,6 +332,26 @@ spec wraps a route, route-local aliases, tags, usage, related actions, and parameter guidance become defaults; explicit `ActionSpecOptions` values may add or override the metadata for that spec. +### Cross-links + +`RelatedActions` carries **canonical action IDs**, never individual tool names. +Both resolve for `gitlab_execute_action`, because an individual tool name is +registered as an alias of its action, and only the canonical ID is published by +the discovery tools this field reaches a model through, so a tool name written +here is a cross-link a model can follow once and can never look up. +`cmd/audit_action_ids` reports one as a finding and names the canonical ID +beside it; an alias spelled inside a `Usage` line or a description is prose and +is reported apart, since naming an alias can be the substance of the sentence. + +Write the list against the whole catalog and do not try to gate it by tier. +What one session is shown is narrowed for it at registration: +`Registry.publishedRelatedActions` in `internal/tools/dynamic` drops a +cross-link this session cannot run and cannot be told anything useful about, +which is an action above the resolved tier or one `--exclude-tools` removed, +and keeps one a filter withheld, because asking for that one is answered with +the narrowing rather than with `unknown action`. A cross-link that resolves is +published under its canonical ID whatever spelling it was written in. + ## When Adding A GitLab Action 1. Add or update the typed handler in the appropriate domain package. diff --git a/internal/tools/action_specs_test.go b/internal/tools/action_specs_test.go index 6c7d0b9dd..fa0fbe511 100644 --- a/internal/tools/action_specs_test.go +++ b/internal/tools/action_specs_test.go @@ -392,9 +392,27 @@ func assertSurfacePolicyMetaProjection(t *testing.T, spec toolutil.ActionSpec, h return metaRoutes } +// surfacePolicyRelatedSpec is the action the spec under test cross-links to. +// +// It belongs in the group because the dynamic registry publishes a cross-link +// only when the session it serves can run it: a related ID the catalog does +// not hold is dropped rather than handed to a model as a next step it would be +// told is unknown. Without this the projection assertion would be checking +// that a dead link survives. +func surfacePolicyRelatedSpec() toolutil.ActionSpec { + return toolutil.NewActionSpec("archive", toolutil.ActionRoute{ + Handler: func(context.Context, map[string]any) (any, error) { return map[string]any{"ok": true}, nil }, + InputSchema: map[string]any{"type": "object", "properties": map[string]any{}}, + }, toolutil.ActionSpecOptions{ + Usage: "Archive a project reversibly.", + OwnerPackage: "projects", + IndividualTool: toolutil.IndividualToolSpec{Name: "gitlab_project_archive", Title: "Archive Project"}, + }) +} + func assertSurfacePolicyDynamicProjection(t *testing.T, spec toolutil.ActionSpec) { t.Helper() - group, err := actioncatalog.GroupFromSpecs(actioncatalog.GroupOptions{ToolName: "gitlab_project"}, []toolutil.ActionSpec{spec}) + group, err := actioncatalog.GroupFromSpecs(actioncatalog.GroupOptions{ToolName: "gitlab_project"}, []toolutil.ActionSpec{spec, surfacePolicyRelatedSpec()}) if err != nil { t.Fatalf("GroupFromSpecs() error = %v", err) } diff --git a/internal/tools/dynamic/register.go b/internal/tools/dynamic/register.go index 4466b43e3..7ea48a390 100644 --- a/internal/tools/dynamic/register.go +++ b/internal/tools/dynamic/register.go @@ -674,7 +674,7 @@ func (r *Registry) Search(ctx context.Context, _ *mcp.CallToolRequest, input Sea Destructive: entry.Destructive, RequiredParams: append([]string(nil), entry.RequiredParams...), Usage: usageHintForEntry(entry), - RelatedActions: relatedActionsForEntry(entry), + RelatedActions: r.publishedRelatedActions(entry), Score: match.score, LowConfidence: match.lowConfidence, AmbiguousWith: append([]string(nil), match.ambiguousWith...), @@ -709,7 +709,7 @@ func (r *Registry) Describe(_ context.Context, _ *mcp.CallToolRequest, input Des if !ok { return toolutil.ErrorResult(r.unknownActionMessage("catalog describe", id)), DescribeOutput{}, nil } - descriptions = append(descriptions, describeEntry(entry)) + descriptions = append(descriptions, r.describeEntry(entry)) } output := DescribeOutput{Count: len(descriptions), Actions: descriptions} @@ -753,7 +753,7 @@ func (r *Registry) Find(ctx context.Context, req *mcp.CallToolRequest, input Fin } results := make([]FindResult, 0, len(matches)) for _, match := range matches { - description := describeEntry(match.entry) + description := r.describeEntry(match.entry) result := FindResult{ ID: description.ID, Tool: description.Tool, @@ -2289,6 +2289,11 @@ func normalizedLimit(limit int) int { // describeEntry renders one action as a find or describe result. // +// It hangs off the registry rather than standing on its own because of the +// cross-links: which of an action's related IDs this session may be told about +// is a property of the session, and [Registry.publishedRelatedActions] is what +// knows it. +// // The two schemas are handed out as they are, not copied. Both are frozen: // the input schema is the process-shared derivation [dynamicInputSchema] // returns, and the output schema belongs to the route of a catalog shared by @@ -2297,7 +2302,7 @@ func normalizedLimit(limit int) int { // still reach every pooled server; the honest form is to share the map and say // that nothing may write into it. A caller that needs to mutate one copies it // with [toolutil.CloneSchemaMap]. -func describeEntry(entry actionEntry) ActionDescription { +func (r *Registry) describeEntry(entry actionEntry) ActionDescription { inputSchema := dynamicInputSchema(entry) return ActionDescription{ ID: entry.ID, @@ -2308,7 +2313,7 @@ func describeEntry(entry actionEntry) ActionDescription { Destructive: entry.Destructive, RequiredParams: append([]string(nil), entry.RequiredParams...), Usage: usageHintForEntry(entry), - RelatedActions: relatedActionsForEntry(entry), + RelatedActions: r.publishedRelatedActions(entry), ParamGuidance: cloneParameterGuidance(entry.Route.ParameterGuidance), InputSchema: inputSchema, OutputSchema: entry.Route.OutputSchema, @@ -2566,6 +2571,87 @@ func relatedActionsForEntry(entry actionEntry) []string { return append([]string(nil), actionUXMetadataByID[entry.ID].RelatedActions...) } +// publishedRelatedActions is the cross-link list one session is shown: the +// curated IDs of [relatedActionsForEntry], spelled canonically, with the ones +// this session could be told nothing useful about removed. +// +// The curated lists are written against the whole catalog, and every session +// sees a narrower one. Nothing used to reconcile the two, so a Free instance +// was handed cross-links to Premium actions it does not serve; the same holds +// for an action --exclude-tools removed, for one a narrow credential cannot +// reach, and for one read-only mode withdrew. What a model does with such a +// link is call it, and what it gets back decides whether the link cost it +// anything, so the rule is drawn there rather than at "is it in the catalog": +// +// - It resolves here. Published, as the canonical ID rather than as written, +// which is the spelling [Registry.Find] lists and the only one a model can +// look back up; an alias resolves for execute and is findable under no name. +// - It does not resolve, but the registry knows why. Published as written, so +// that asking for it produces [Registry.withheldActionMessage] naming the +// narrowing. That message is worth more than silence: it tells the caller +// the capability exists and what to widen, and dropping the link would +// leave a model concluding the server cannot do it at all. +// - Neither. Dropped. Nothing here can say more than "unknown action", whose +// near-miss suggestions read as a spelling correction, and a model that +// believes one concludes the capability is missing. That is the tier case, +// since the tier filter runs before the catalog is built and so never +// reaches the withheld lists, and the --exclude-tools case, where naming +// the action would contradict the exclusion as well as mislead. +// +// A published list therefore shrinks with the surface instead of pointing off +// it, and the entry's own order is kept: the curation is a workflow order, and +// re-sorting it would lose the only thing it says. +func (r *Registry) publishedRelatedActions(entry actionEntry) []string { + related := relatedActionsForEntry(entry) + if len(related) == 0 { + return nil + } + published := make([]string, 0, len(related)) + seen := make(map[string]struct{}, len(related)) + for _, id := range related { + spelling, ok := r.publishableRelatedAction(id) + if !ok { + continue + } + if _, duplicate := seen[spelling]; duplicate { + continue + } + seen[spelling] = struct{}{} + published = append(published, spelling) + } + if len(published) == 0 { + return nil + } + return published +} + +// publishableRelatedAction resolves one cross-link to the spelling it should be +// published under, or reports that it should not be published at all. +func (r *Registry) publishableRelatedAction(id string) (string, bool) { + if target, resolved := r.resolveAction(id); resolved { + return target.ID, true + } + if r.actionWithheld(id) { + return id, true + } + return "", false +} + +// actionWithheld reports whether the catalog carried this action and one of +// the deployment's filters took it away, under either cause. +// +// [Registry.withheldActionMessage] keeps the two causes apart because only one +// of them is something the caller can act on. Here the question is narrower: +// whether asking for the action produces an answer that explains itself. +func (r *Registry) actionWithheld(action string) bool { + key := strings.ToLower(strings.TrimSpace(action)) + if _, byScope := r.withheldByScope[key]; byScope { + return true + } + _, byOperator := r.withheldByOperator[key] + return byOperator +} + func cloneParameterGuidance(guidance map[string]toolutil.ParameterGuidance) map[string]toolutil.ParameterGuidance { if len(guidance) == 0 { return nil diff --git a/internal/tools/dynamic/register_test.go b/internal/tools/dynamic/register_test.go index 56fd8bfcb..34ca4108f 100644 --- a/internal/tools/dynamic/register_test.go +++ b/internal/tools/dynamic/register_test.go @@ -648,12 +648,27 @@ func customCatalogForDynamicTest(t *testing.T) *actioncatalog.Catalog { catalog := actioncatalog.NewCatalog() group := actioncatalog.NewGroup(actioncatalog.GroupOptions{ToolName: "gitlab_custom"}) group.SetAction(actioncatalog.Action{Name: "inspect", Aliases: []string{"custom.lookup"}, Tags: []string{"bespoke"}, Route: customCatalogRouteForDynamicTest()}) + // The cross-link target belongs in the catalog: a related action the + // catalog does not hold is one this session cannot run, and + // Registry.publishedRelatedActions drops it rather than publishing a dead + // end. Without it this fixture would assert the route metadata survives by + // asserting a link that no longer should. + group.SetAction(actioncatalog.Action{Name: "audit", Route: customCatalogSiblingRouteForDynamicTest()}) if err := catalog.AddGroup(group); err != nil { t.Fatalf("AddGroup() error = %v", err) } return catalog } +func customCatalogSiblingRouteForDynamicTest() toolutil.ActionRoute { + return toolutil.ActionRoute{ + Handler: func(_ context.Context, params map[string]any) (any, error) { + return map[string]any{"target": params["target"]}, nil + }, + InputSchema: map[string]any{"type": "object", "required": []any{"target"}, "properties": map[string]any{"target": map[string]any{"type": "string"}}}, + }.WithUsage("Use to audit what custom.inspect reported.") +} + func customCatalogRouteForDynamicTest() toolutil.ActionRoute { return toolutil.ActionRoute{ Handler: func(_ context.Context, params map[string]any) (any, error) { @@ -5823,12 +5838,12 @@ func TestCompatibilityAliasAndDescriptionBranches(t *testing.T) { t.Fatalf("dedupeActionAliases() = %+v, want one normalized alias", aliases) } - description := describeEntry(actionEntry{ID: "missing.action", Tool: "gitlab_missing", Domain: "missing", Action: "action", Route: toolutil.ActionRoute{OutputSchema: map[string]any{"type": "object"}}}) + registry := NewRegistry(testRoutes(t)) + description := registry.describeEntry(actionEntry{ID: "missing.action", Tool: "gitlab_missing", Domain: "missing", Action: "action", Route: toolutil.ActionRoute{OutputSchema: map[string]any{"type": "object"}}}) if description.InputSchema["additionalProperties"] != true || description.OutputSchema["type"] != "object" { t.Fatalf("describeEntry(fallback) = %+v, want fallback input schema and cloned output schema", description) } - registry := NewRegistry(testRoutes(t)) - if got := describeEntry(registry.entries[0]); got.InputSchema["type"] == "" || got.Example.Tool != executeActionToolName { + if got := registry.describeEntry(registry.entries[0]); got.InputSchema["type"] == "" || got.Example.Tool != executeActionToolName { t.Fatalf("describeEntry(success) = %+v, want schema and dynamic execute example", got) } if got := compactSchemaJSON(nil); got != "" { @@ -8375,6 +8390,162 @@ func TestExecute_WithheldActionNamesTheCauseInsteadOfCallingItUnknown(t *testing }) } +// crossLinkCatalog builds a catalog whose one described action points at every +// kind of cross-link target: a sibling the catalog holds, that same sibling +// under an alias, an action a filter took away, and an action nothing here has +// ever heard of. +func crossLinkCatalog(t *testing.T) *actioncatalog.Catalog { + t.Helper() + route := func() toolutil.ActionRoute { + return toolutil.ActionRoute{ + Handler: func(context.Context, map[string]any) (any, error) { return map[string]any{}, nil }, + InputSchema: map[string]any{"type": "object", "properties": map[string]any{}}, + } + } + catalog := actioncatalog.NewCatalog() + group := actioncatalog.NewGroup(actioncatalog.GroupOptions{ToolName: "gitlab_widget"}) + group.SetAction(actioncatalog.Action{ + Name: "inspect", + Route: route().WithRelatedActions( + "widget.audit", + "widget.lookup", + "widget.repair", + "widget.hidden", + "widget.retired", + ), + }) + group.SetAction(actioncatalog.Action{Name: "audit", Route: route()}) + group.SetAction(actioncatalog.Action{Name: "repair", Aliases: []string{"widget.lookup"}, Route: route()}) + // An action whose every cross-link is one this session cannot be told + // about, so the published list is empty rather than one short. + group.SetAction(actioncatalog.Action{Name: "orphan", Route: route().WithRelatedActions("widget.retired")}) + if err := catalog.AddGroup(group); err != nil { + t.Fatalf("AddGroup() error = %v", err) + } + return catalog +} + +// crossLinks is the related-action list one session is shown for widget.inspect. +func crossLinks(t *testing.T, registry *Registry) []string { + t.Helper() + _, described, err := registry.Describe(t.Context(), nil, DescribeInput{Action: "widget.inspect"}) + if err != nil { + t.Fatalf("Describe() error = %v", err) + } + if described.Count != 1 { + t.Fatalf("Describe() count = %d, want 1", described.Count) + } + return described.Actions[0].RelatedActions +} + +// TestDescribe_RelatedActions_CanonicalizeWhatResolvesAndDropWhatCannotBeExplained +// pins the rule a cross-link is published under, which is about what a model +// gets back when it follows one. +// +// The curated lists are written against the whole catalog and every session +// sees a narrower one, so a Free instance used to be handed links to Premium +// actions and an --exclude-tools deployment links to actions it had removed. +// Following one answers "unknown action" plus near-miss suggestions, which +// reads as a spelling correction and teaches a model the capability is absent. +// An action a filter withheld is the exception: asking for that one produces a +// message naming the narrowing, which is worth more than silence. +func TestDescribe_RelatedActions_CanonicalizeWhatResolvesAndDropWhatCannotBeExplained(t *testing.T) { + t.Run("nothing withheld", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t)) + want := []string{"widget.audit", "widget.repair"} + if got := crossLinks(t, registry); !slices.Equal(got, want) { + t.Fatalf("RelatedActions = %v, want %v: an alias published under its canonical ID, once, and the two dead links gone", got, want) + } + }) + + t.Run("withheld by token scope", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t), WithWithheldActions([]string{"widget.hidden"}, nil)) + want := []string{"widget.audit", "widget.repair", "widget.hidden"} + if got := crossLinks(t, registry); !slices.Equal(got, want) { + t.Fatalf("RelatedActions = %v, want %v: a scope-withheld link stays, since asking for it explains the narrowing", got, want) + } + }) + + t.Run("withheld by the operator", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t), WithWithheldActions(nil, []string{"widget.hidden"})) + want := []string{"widget.audit", "widget.repair", "widget.hidden"} + if got := crossLinks(t, registry); !slices.Equal(got, want) { + t.Fatalf("RelatedActions = %v, want %v: an operator-withheld link stays for the same reason", got, want) + } + }) + + t.Run("an action no filter reported stays dropped", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t), WithWithheldActions([]string{"widget.hidden"}, nil)) + if got := crossLinks(t, registry); slices.Contains(got, "widget.retired") { + t.Fatalf("RelatedActions = %v, must not carry widget.retired: nothing here could answer for it", got) + } + }) + + t.Run("an action with no cross-links publishes none", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t)) + _, described, err := registry.Describe(t.Context(), nil, DescribeInput{Action: "widget.audit"}) + if err != nil { + t.Fatalf("Describe() error = %v", err) + } + if got := described.Actions[0].RelatedActions; len(got) != 0 { + t.Fatalf("RelatedActions = %v, want none", got) + } + }) + + t.Run("an action whose every cross-link is dropped publishes none", func(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t)) + _, described, err := registry.Describe(t.Context(), nil, DescribeInput{Action: "widget.orphan"}) + if err != nil { + t.Fatalf("Describe() error = %v", err) + } + if got := described.Actions[0].RelatedActions; len(got) != 0 { + t.Fatalf("RelatedActions = %v, want none: its one link is an action nothing here could answer for", got) + } + }) +} + +// TestFind_RelatedActions_AreTheSameOnesDescribeWouldPublish holds the other +// discovery tool to the same rule. Find renders its results through a separate +// assembly, and a filter applied to one of the two would leave the surface +// telling a model two different things about the same action. +func TestFind_RelatedActions_AreTheSameOnesDescribeWouldPublish(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t)) + _, found, err := registry.Find(t.Context(), nil, FindInput{Query: "widget inspect", Limit: 5}) + if err != nil { + t.Fatalf("Find() error = %v", err) + } + for _, result := range found.Results { + if result.ID != "widget.inspect" { + continue + } + if want := crossLinks(t, registry); !slices.Equal(result.RelatedActions, want) { + t.Fatalf("Find() RelatedActions = %v, want the described %v", result.RelatedActions, want) + } + return + } + t.Fatalf("Find() results = %+v, want widget.inspect among them", found.Results) +} + +// TestSearch_RelatedActions_AreTheSameOnesDescribeWouldPublish holds the third +// assembly, the catalog search the meta surface registers, to the rule too. +func TestSearch_RelatedActions_AreTheSameOnesDescribeWouldPublish(t *testing.T) { + registry := newCatalogRegistry(crossLinkCatalog(t)) + _, output, err := registry.Search(t.Context(), nil, SearchInput{Query: "widget inspect", Limit: 5}) + if err != nil { + t.Fatalf("Search() error = %v", err) + } + for _, result := range output.Results { + if result.ID != "widget.inspect" { + continue + } + if want := crossLinks(t, registry); !slices.Equal(result.RelatedActions, want) { + t.Fatalf("Search() RelatedActions = %v, want the described %v", result.RelatedActions, want) + } + return + } + t.Fatalf("Search() results = %+v, want widget.inspect among them", output.Results) +} + // TestWithheldKeySet_NormalizesKeysAndDropsBlankOnes verifies how the withheld // list is indexed: each key is lowercased and trimmed, and a key that is blank // or only spaces is not indexed at all. diff --git a/internal/tools/runnercontrollers/action_specs.go b/internal/tools/runnercontrollers/action_specs.go index 649669dd6..fb441f8b0 100644 --- a/internal/tools/runnercontrollers/action_specs.go +++ b/internal/tools/runnercontrollers/action_specs.go @@ -101,31 +101,31 @@ var ( listControllerMeta = runnerControllerActionMetaEntry{ usage: "List every registered runner controller on the instance (admin-only, experimental API). Use to discover controller IDs and their enabled/disabled/dry_run state before getting, updating, or deleting one. Supports offset and keyset pagination for large fleets.", aliases: []string{"list runner controllers", "runner controller fleet", "registered runner controllers"}, - related: []string{toolControllerGet, toolControllerCreate, toolControllerUpdate, toolControllerDelete}, + related: []string{actionControllerGet, actionControllerCreate, actionControllerUpdate, actionControllerDelete}, description: "List registered runner controllers (admin-only, experimental API) with offset or keyset pagination. Returns: controllers with id, description, state, created/updated timestamps, plus pagination metadata. See also: gitlab_runner_controller_get, gitlab_runner_controller_create, gitlab_runner_controller_update.", } getControllerMeta = runnerControllerActionMetaEntry{ usage: "Fetch one runner controller by numeric controller_id, including its live connection status (admin-only, experimental API). Use after gitlab_runner_controller_list to inspect a specific controller before updating or deleting it.", aliases: []string{"get runner controller", "runner controller details", "runner controller connection status"}, - related: []string{toolControllerList, toolControllerUpdate, toolControllerDelete}, + related: []string{actionControllerList, actionControllerUpdate, actionControllerDelete}, description: "Get one runner controller by controller_id (admin-only, experimental API). Returns: the controller with id, description, state, connected flag, and created/updated timestamps. See also: gitlab_runner_controller_list, gitlab_runner_controller_update, gitlab_runner_controller_delete.", } createControllerMeta = runnerControllerActionMetaEntry{ usage: "Register a new runner controller with an optional description and initial state (enabled, disabled, or dry_run). admin-only, experimental API. Use to onboard a controller into the agentic runner control plane before it connects.", aliases: []string{"create runner controller", "register runner controller", "onboard runner controller"}, - related: []string{toolControllerGet, toolControllerUpdate, toolControllerList}, + related: []string{actionControllerGet, actionControllerUpdate, actionControllerList}, description: "Register a new runner controller (admin-only, experimental API) with optional description and state (enabled/disabled/dry_run). Returns: the created controller with id, description, state, and timestamps. See also: gitlab_runner_controller_get, gitlab_runner_controller_update, gitlab_runner_controller_list.", } updateControllerMeta = runnerControllerActionMetaEntry{ usage: "Update an existing runner controller's description or state (enabled, disabled, or dry_run) by controller_id. admin-only, experimental API. Use to pause (disabled), resume (enabled), or stage (dry_run) a controller in the runner control plane.", aliases: []string{"update runner controller", "edit runner controller", "set runner controller state"}, - related: []string{toolControllerGet, toolControllerList, toolControllerDelete}, + related: []string{actionControllerGet, actionControllerList, actionControllerDelete}, description: "Update a runner controller's description or state (enabled/disabled/dry_run) by controller_id (admin-only, experimental API). Returns: the updated controller with id, description, state, and timestamps. See also: gitlab_runner_controller_get, gitlab_runner_controller_list, gitlab_runner_controller_delete.", } deleteControllerMeta = runnerControllerActionMetaEntry{ usage: "Permanently remove a runner controller by controller_id (destructive, admin-only, experimental API). Use to decommission a controller from the runner control plane. Verify the controller_id with gitlab_runner_controller_list first.", aliases: []string{"delete runner controller", "remove runner controller", "decommission runner controller"}, - related: []string{toolControllerGet, toolControllerList, toolControllerUpdate}, + related: []string{actionControllerGet, actionControllerList, actionControllerUpdate}, description: "Delete a runner controller by controller_id (destructive, admin-only, experimental API). Returns: a success confirmation. See also: gitlab_runner_controller_list, gitlab_runner_controller_get, gitlab_runner_controller_update.", } ) diff --git a/internal/tools/runnercontrollers/action_specs_test.go b/internal/tools/runnercontrollers/action_specs_test.go index e86b9b23a..8f954c7b4 100644 --- a/internal/tools/runnercontrollers/action_specs_test.go +++ b/internal/tools/runnercontrollers/action_specs_test.go @@ -145,19 +145,29 @@ func TestActionSpecs_EveryToolCarriesItsOwnDiscoveryMetadata(t *testing.T) { } } -// TestActionSpecs_RelatedActionsNameRegisteredTools holds every cross-link to -// the set this package registers. Nothing in the repository validates these -// strings, so a misspelled one is a dead end a model follows once and abandons. -func TestActionSpecs_RelatedActionsNameRegisteredTools(t *testing.T) { +// TestActionSpecs_RelatedActionsNameRegisteredActions holds every cross-link to +// the canonical action IDs this package registers. A misspelled one is a dead +// end a model follows once and abandons. +// +// The canonical IDs rather than the individual tool names: the discovery tools +// that publish this field hand out canonical IDs, so a tool name written here +// would be a cross-link a model cannot look up in any listing it is shown. +func TestActionSpecs_RelatedActionsNameRegisteredActions(t *testing.T) { byTool := runnerControllerSpecsByTool(t, ActionSpecs(testutil.NewTestClient(t, runnerControllerActionHandler()))) + actions := make(map[string]string, len(byTool)) + for tool, spec := range byTool { + actions["runner."+spec.Name] = tool + } for tool, spec := range byTool { t.Run(tool, func(t *testing.T) { for _, related := range spec.RelatedActions { - if _, ok := byTool[related]; !ok { + named, ok := actions[related] + if !ok { t.Errorf("%s names related action %q, which this package does not register", tool, related) + continue } - if related == tool { + if named == tool { t.Errorf("%s names itself as a related action", tool) } } diff --git a/internal/tools/runnercontrollers/markdown.go b/internal/tools/runnercontrollers/markdown.go index 3df186d9c..f25d5279f 100644 --- a/internal/tools/runnercontrollers/markdown.go +++ b/internal/tools/runnercontrollers/markdown.go @@ -8,12 +8,20 @@ import ( "github.com/jmrplens/gitlab-mcp-server/v3/internal/toolutil" ) -// Canonical action IDs the hints name, the one form every surface resolves: a -// controller action is projected under the runner domain. +// Canonical action IDs the hints and the RelatedActions cross-links name, the +// one form every surface resolves: a controller action is projected under the +// runner domain. +// +// The individual tool names in the const block of action_specs.go are not an +// alternative spelling for either: the discovery tools that publish both +// fields hand out canonical IDs, and a tool name written into a cross-link is +// findable under no name a listing shows. const ( actionControllerGet = "runner.controller_get" actionControllerList = "runner.controller_list" + actionControllerCreate = "runner.controller_create" actionControllerUpdate = "runner.controller_update" + actionControllerDelete = "runner.controller_delete" actionControllerTokenList = "runner.controller_token_list" actionControllerScopeList = "runner.controller_scope_list" ) diff --git a/internal/tools/runners/action_specs.go b/internal/tools/runners/action_specs.go index 25bd85640..58d8ab68e 100644 --- a/internal/tools/runners/action_specs.go +++ b/internal/tools/runners/action_specs.go @@ -194,8 +194,22 @@ type runnerActionMetaEntry struct { // resetScopedRegTokenEntry builds the runnerActionMetaEntry for a scoped runner // registration-token reset action. The group and project variants share the same // sentence structure; only the scope name, the scope-ID parameter name, and the -// two sibling related-action IDs differ. -func resetScopedRegTokenEntry(scope, scopeParam, related1, related2 string) runnerActionMetaEntry { +// two siblings differ. +// +// Each sibling arrives as two parameters because it is published under two +// names: the canonical action ID the cross-link carries, which is what the +// discovery tools list, and the individual tool name the "See also" sentence +// names, which is what a model can call on that surface. +// +// Two plain strings rather than one struct carrying both, and the ID halves +// keep the "related" prefix in their names, because both are what +// cmd/audit_action_ids reads them through: it folds a helper's arguments back +// to the constants its callers pass and cannot fold a field selected off a +// struct, and it follows a parameter out to its callers only when the +// parameter's own name says it carries action IDs. Tidied either way, these +// two cross-links leave the audit's sight and are reported as sites it could +// not read. +func resetScopedRegTokenEntry(scope, scopeParam, relatedFirstAction, firstTool, relatedSecondAction, secondTool string) runnerActionMetaEntry { return runnerActionMetaEntry{ usage: fmt.Sprintf( "Reset a %s's runner registration token by %s (deprecated registration flow). "+ @@ -208,11 +222,11 @@ func resetScopedRegTokenEntry(scope, scopeParam, related1, related2 string) runn "renew " + scope + " registration token", "regenerate " + scope + " runner token", }, - related: []string{related1, related2}, + related: []string{relatedFirstAction, relatedSecondAction}, description: fmt.Sprintf( "Reset a %s's runner registration token by %s (deprecated). "+ "Returns: the new registration token and its expiry. See also: %s, %s.", - scope, scopeParam, related1, related2, + scope, scopeParam, firstTool, secondTool, ), } } @@ -225,110 +239,114 @@ var runnerActionMeta = map[string]runnerActionMetaEntry{ "gitlab_runner_list": { usage: "List runners owned by the authenticated user. Use type, status, tag_list, and scope filters to narrow results when the prompt asks for the caller's own runners. Prefer runner.list_all for an instance-wide admin view.", aliases: []string{"list owned runners", "my runners", "show my runners", "browse my runners"}, - related: []string{"gitlab_runner_list_all", "gitlab_runner_get", toolRunnerListProject, "gitlab_runner_list_group"}, + related: []string{actionRunnerListAll, actionRunnerGet, actionRunnerListProject, actionRunnerListGroup}, description: "List runners owned by the authenticated user, with type/status/tag/scope filters and pagination. Returns: runners with id, name, type, status, paused/shared/online flags, plus pagination metadata. See also: gitlab_runner_get, gitlab_runner_list_all, gitlab_runner_list_project.", }, "gitlab_runner_list_all": { usage: "List every runner registered on the instance (requires an admin token). Use when the prompt asks for a fleet-wide or instance-level inventory rather than the caller's own runners.", aliases: []string{"list all runners", "instance runners", "show all runners", "browse instance runners"}, - related: []string{"gitlab_runner_list", "gitlab_runner_get"}, + related: []string{actionRunnerList, actionRunnerGet}, description: "List every runner on the instance (admin token required), with type/status/tag/scope filters and pagination. Returns: runners with id, name, type, status, flags, plus pagination metadata. See also: gitlab_runner_list, gitlab_runner_get.", }, "gitlab_runner_get": { usage: "Get the full configuration of one runner by its numeric runner_id. Use after a runner.list result or when the prompt names a concrete runner to inspect tags, lock state, access level, and timeout.", aliases: []string{"get runner", "runner details", "show runner", "fetch runner"}, - related: []string{"gitlab_runner_list", "gitlab_runner_update", "gitlab_runner_jobs", toolRunnerListManagers}, + related: []string{actionRunnerList, actionRunnerUpdate, actionRunnerJobs, actionRunnerListManagers}, description: "Get full configuration for one runner by numeric runner_id. Returns: runner details including tags, locked, access level, maximum timeout, contact time, and associated groups/projects. See also: gitlab_runner_list, gitlab_runner_update, gitlab_runner_jobs.", }, // Runner lifecycle actions. "gitlab_runner_update": { usage: "Update a runner's configuration by runner_id. Set description, paused, tag_list, locked, access_level, or maximum_timeout. Pass only the fields the prompt asks to change (e.g. paused to pause or resume a runner).", aliases: []string{"update runner", "edit runner", "pause runner", "resume runner", "modify runner"}, - related: []string{"gitlab_runner_get", "gitlab_runner_remove"}, + related: []string{actionRunnerGet, actionRunnerRemove}, description: "Update a runner's configuration (description, pause state, tags, locked, access level, timeout) by runner_id. Returns: the updated runner details. See also: gitlab_runner_get, gitlab_runner_remove.", }, "gitlab_runner_remove": { usage: "Remove (unregister) a runner by its numeric runner_id. Use runner.list or runner.list_project to obtain the runner_id first. Destructive: the runner is permanently deleted.", aliases: []string{"remove runner", "unregister runner", "delete runner", "drop runner"}, - related: []string{"gitlab_runner_get", toolRunnerDisableProject, toolRunnerDeleteRegistered}, + related: []string{actionRunnerGet, actionRunnerDisableProject, actionRunnerDeleteRegistered}, description: "Remove (unregister) a runner by numeric runner_id. Returns: a success confirmation. See also: gitlab_runner_disable_project, gitlab_runner_delete_registered, gitlab_runner_get.", }, "gitlab_runner_jobs": { usage: "List the CI jobs processed by a runner, identified by runner_id. Use status, order_by, and sort filters when the prompt asks which jobs a specific runner has executed.", aliases: []string{"runner jobs", "jobs processed by runner", "list jobs for runner", "show runner jobs"}, - related: []string{"gitlab_runner_get", "gitlab_runner_list"}, + related: []string{actionRunnerGet, actionRunnerList}, description: "List jobs processed by a runner, with status/order/sort filters and pagination. Returns: jobs with id, name, status, stage, ref, and duration, plus pagination metadata. See also: gitlab_runner_get, gitlab_job_get.", }, // Project and group assignment actions. toolRunnerListProject: { usage: "List the runners assigned to a project, identified by project_id. Use when the prompt scopes runners to one project. Use runner.enable_project or runner.disable_project to change those assignments.", aliases: []string{"list project runners", "runners for project", "show project runners", "browse project runners"}, - related: []string{toolRunnerEnableProject, toolRunnerDisableProject, "gitlab_runner_list"}, + related: []string{actionRunnerEnableProject, actionRunnerDisableProject, actionRunnerList}, description: "List runners assigned to a project, with type/status/tag/scope filters and pagination. Returns: runners with id, name, type, status, flags, plus pagination metadata. See also: gitlab_runner_enable_project, gitlab_runner_disable_project, gitlab_runner_list.", }, toolRunnerEnableProject: { usage: "Assign an existing runner to a project. Provide project_id for the target project and runner_id for the runner to enable. Use runner.list to find an available runner_id first.", aliases: []string{"enable project runner", "assign runner to project", "attach runner to project"}, - related: []string{toolRunnerDisableProject, toolRunnerListProject}, + related: []string{actionRunnerDisableProject, actionRunnerListProject}, description: "Assign an existing runner to a project by project_id and runner_id. Returns: the enabled runner. See also: gitlab_runner_disable_project, gitlab_runner_list_project.", }, toolRunnerDisableProject: { usage: "Remove a runner assignment from a project. Provide project_id for the project and runner_id for the runner to unassign. This detaches the runner without unregistering it (use runner.remove to delete the runner itself).", aliases: []string{"disable project runner", "unassign runner from project", "detach runner from project"}, - related: []string{toolRunnerEnableProject, toolRunnerListProject}, + related: []string{actionRunnerEnableProject, actionRunnerListProject}, description: "Remove a runner assignment from a project by project_id and runner_id. Returns: a success confirmation. See also: gitlab_runner_enable_project, gitlab_runner_list_project.", }, "gitlab_runner_list_group": { usage: "List the runners available to a group, identified by group_id, including group-specific and shared runners. Use type and status filters when the prompt scopes runners to a group or subgroup.", aliases: []string{"list group runners", "runners for group", "show group runners", "browse group runners"}, - related: []string{"gitlab_runner_list", toolRunnerListProject}, + related: []string{actionRunnerList, actionRunnerListProject}, description: "List runners available in a group (specific and shared), with type/status/tag filters and pagination. Returns: runners with id, name, type, status, flags, plus pagination metadata. See also: gitlab_runner_list, gitlab_runner_list_project.", }, // Runner registration actions. "gitlab_runner_register": { usage: "Register a new runner using a registration token. Provide the token plus optional description, tag_list, locked, run_untagged, and access_level. The response includes the runner's authentication token needed to configure the runner agent.", aliases: []string{"register runner", "create runner", "new runner", "provision runner"}, - related: []string{"gitlab_runner_verify", "gitlab_runner_list"}, + related: []string{actionRunnerVerify, actionRunnerList}, description: "Register a new runner with a registration token, optional info hashmap, tags, and configuration. Returns: the created runner including its authentication token. See also: gitlab_runner_verify, gitlab_runner_list.", }, toolRunnerDeleteRegistered: { usage: "Delete a registered runner by its numeric runner_id. Use this runner-registration-API variant when you have the runner_id. Prefer runner.delete_by_token when only the authentication token is known.", aliases: []string{"delete registered runner", "delete runner by id", "remove registered runner", "unregister runner by id"}, - related: []string{toolRunnerDeleteByToken, "gitlab_runner_remove"}, + related: []string{actionRunnerDeleteByToken, actionRunnerRemove}, description: "Delete a registered runner by its numeric runner_id. Returns: a success confirmation. See also: gitlab_runner_delete_by_token, gitlab_runner_remove.", }, toolRunnerDeleteByToken: { usage: "Delete a registered runner using its authentication token instead of a runner_id. Use when the runner agent's token is available but the numeric runner_id is not. Prefer runner.delete_registered when you have the runner_id.", aliases: []string{"delete runner by token", "unregister runner by token", "remove runner by token", "drop runner by token"}, - related: []string{toolRunnerDeleteRegistered, "gitlab_runner_remove"}, + related: []string{actionRunnerDeleteRegistered, actionRunnerRemove}, description: "Delete a registered runner by its authentication token. Returns: a success confirmation. See also: gitlab_runner_delete_registered, gitlab_runner_remove.", }, "gitlab_runner_verify": { usage: "Verify that a runner authentication token is valid without registering or modifying anything. Use to check a token before configuring a runner agent or after runner.reset_token.", aliases: []string{"verify runner token", "validate runner token", "check runner token", "test runner token"}, - related: []string{"gitlab_runner_register", "gitlab_runner_reset_token"}, + related: []string{actionRunnerRegister, actionRunnerResetToken}, description: "Verify that a runner authentication token is valid. Returns: a success confirmation when the token authenticates. See also: gitlab_runner_register, gitlab_runner_reset_token.", }, "gitlab_runner_reset_token": { usage: "Reset a runner's authentication token by runner_id, invalidating the previous token. Use when a runner token is compromised or must be rotated. The response returns the new token.", aliases: []string{"reset runner token", "rotate runner authentication token", "renew runner token", "regenerate runner token"}, - related: []string{"gitlab_runner_verify", "gitlab_runner_get"}, + related: []string{actionRunnerVerify, actionRunnerGet}, description: "Reset a runner's authentication token by runner_id. Returns: the new token and its expiry. See also: gitlab_runner_verify, gitlab_runner_get.", }, // Deprecated registration-token reset actions. toolRunnerResetInstanceReg: { usage: "Reset the instance-wide runner registration token (admin only, deprecated registration flow). Use only for legacy instance-level runner registration. Modern flows use runner.register with a created token.", aliases: []string{"reset instance registration token", "rotate instance runner registration token", "renew instance registration token", "regenerate instance runner token"}, - related: []string{toolRunnerResetGroupReg, toolRunnerResetProjectReg}, + related: []string{actionRunnerResetGroupReg, actionRunnerResetProjectReg}, description: "Reset the instance-level runner registration token (deprecated, admin only). Returns: the new registration token and its expiry. See also: gitlab_runner_reset_group_reg_token, gitlab_runner_reset_project_reg_token.", }, - toolRunnerResetGroupReg: resetScopedRegTokenEntry("group", "group_id", toolRunnerResetInstanceReg, toolRunnerResetProjectReg), - toolRunnerResetProjectReg: resetScopedRegTokenEntry("project", "project_id", toolRunnerResetInstanceReg, toolRunnerResetGroupReg), + toolRunnerResetGroupReg: resetScopedRegTokenEntry("group", "group_id", + actionRunnerResetInstanceReg, toolRunnerResetInstanceReg, + actionRunnerResetProjectReg, toolRunnerResetProjectReg), + toolRunnerResetProjectReg: resetScopedRegTokenEntry("project", "project_id", + actionRunnerResetInstanceReg, toolRunnerResetInstanceReg, + actionRunnerResetGroupReg, toolRunnerResetGroupReg), // Manager inspection. toolRunnerListManagers: { usage: "List the managers (individual runner processes/hosts) of a runner by runner_id. Use to see each manager's system id, version, platform, architecture, status, and contact IP for a single runner.", aliases: []string{"list runner managers", "runner managers", "show runner managers", "browse runner managers"}, - related: []string{"gitlab_runner_get", "gitlab_runner_list"}, + related: []string{actionRunnerGet, actionRunnerList}, description: "List the managers of a runner by runner_id. Returns: runner managers with system id, version, revision, platform, architecture, status, and IP. See also: gitlab_runner_get, gitlab_runner_list.", }, } diff --git a/internal/tools/runners/action_specs_test.go b/internal/tools/runners/action_specs_test.go index 64fb3b97e..c808ce267 100644 --- a/internal/tools/runners/action_specs_test.go +++ b/internal/tools/runners/action_specs_test.go @@ -98,18 +98,28 @@ func TestActionSpecs_EveryDecoratedTool_PublishesItsMetadataEntry(t *testing.T) } } -// TestRunnerActionMeta_RelatedActions_NameToolsThisPackageRegisters checks -// every cross-link against the tools the specs really register. Nothing in the -// repository validates these strings, so a misspelled one passes every gate and -// answers a model "unknown tool" the moment it follows the hint. -func TestRunnerActionMeta_RelatedActions_NameToolsThisPackageRegisters(t *testing.T) { - byTool := runnerSpecsByTool(t, ActionSpecs(testutil.NewTestClient(t, runnerActionHandler()))) +// TestRunnerActionMeta_RelatedActions_NameActionsThisPackageRegisters checks +// every cross-link against the canonical action IDs the specs really build. A +// misspelled one answers a model "unknown action" the moment it follows the +// hint. +// +// The IDs, not the individual tool names these entries are keyed by. Both +// resolve for gitlab_execute_action, since the decoration makes the tool name +// an alias, and only the canonical ID is published by the discovery tools that +// carry this field, so a tool name here is a cross-link a model can follow but +// cannot look up. cmd/audit_action_ids holds the whole tree to that rule. +func TestRunnerActionMeta_RelatedActions_NameActionsThisPackageRegisters(t *testing.T) { + specs := ActionSpecs(testutil.NewTestClient(t, runnerActionHandler())) + actions := make(map[string]struct{}, len(specs)) + for _, spec := range specs { + actions["runner."+spec.Name] = struct{}{} + } for tool, meta := range runnerActionMeta { t.Run(tool, func(t *testing.T) { for _, related := range meta.related { - if _, ok := byTool[related]; !ok { - t.Errorf("related action %q names no registered runner tool", related) + if _, ok := actions[related]; !ok { + t.Errorf("related action %q names no action this package registers", related) } } }) @@ -138,10 +148,21 @@ func TestMarkdownHints_ActionConstants_NameActionsTheCatalogHolds(t *testing.T) {actionRunnerGet, runnerActions}, {actionRunnerJobs, runnerActions}, {actionRunnerList, runnerActions}, + {actionRunnerListAll, runnerActions}, + {actionRunnerListProject, runnerActions}, + {actionRunnerListGroup, runnerActions}, {actionRunnerUpdate, runnerActions}, {actionRunnerRemove, runnerActions}, + {actionRunnerEnableProject, runnerActions}, + {actionRunnerDisableProject, runnerActions}, {actionRunnerRegister, runnerActions}, + {actionRunnerDeleteRegistered, runnerActions}, + {actionRunnerDeleteByToken, runnerActions}, {actionRunnerVerify, runnerActions}, + {actionRunnerResetToken, runnerActions}, + {actionRunnerResetInstanceReg, runnerActions}, + {actionRunnerResetGroupReg, runnerActions}, + {actionRunnerResetProjectReg, runnerActions}, {actionRunnerListManagers, runnerActions}, {actionJobGet, jobActions}, } diff --git a/internal/tools/runners/markdown.go b/internal/tools/runners/markdown.go index e2adfd371..f72aa0ede 100644 --- a/internal/tools/runners/markdown.go +++ b/internal/tools/runners/markdown.go @@ -8,17 +8,35 @@ import ( "github.com/jmrplens/gitlab-mcp-server/v3/internal/toolutil" ) -// Canonical action IDs the hints name, the one form every surface resolves. +// Canonical action IDs the hints and the RelatedActions cross-links name, the +// one form every surface resolves. +// +// The individual tool names in the const block of action_specs.go are not an +// alternative spelling for either. A tool name is what one surface calls the +// tool, and the discovery tools that publish these two fields hand out +// canonical IDs; a tool name written into a cross-link resolves through the +// alias table for execute and is findable under no name a listing shows. const ( - actionRunnerGet = "runner.get" - actionRunnerJobs = "runner.jobs" - actionRunnerList = "runner.list" - actionRunnerUpdate = "runner.update" - actionRunnerRemove = "runner.remove" - actionRunnerRegister = "runner.register" - actionRunnerVerify = "runner.verify" - actionRunnerListManagers = "runner.list_managers" - actionJobGet = "job.get" + actionRunnerGet = "runner.get" + actionRunnerJobs = "runner.jobs" + actionRunnerList = "runner.list" + actionRunnerListAll = "runner.list_all" + actionRunnerListProject = "runner.list_project" + actionRunnerListGroup = "runner.list_group" + actionRunnerUpdate = "runner.update" + actionRunnerRemove = "runner.remove" + actionRunnerEnableProject = "runner.enable_project" + actionRunnerDisableProject = "runner.disable_project" + actionRunnerRegister = "runner.register" + actionRunnerDeleteRegistered = "runner.delete_registered" + actionRunnerDeleteByToken = "runner.delete_by_token" + actionRunnerVerify = "runner.verify" + actionRunnerResetToken = "runner.reset_token" + actionRunnerResetInstanceReg = "runner.reset_instance_reg_token" + actionRunnerResetGroupReg = "runner.reset_group_reg_token" + actionRunnerResetProjectReg = "runner.reset_project_reg_token" + actionRunnerListManagers = "runner.list_managers" + actionJobGet = "job.get" ) // writeRunnerSummary writes the fields a runner carries wherever it is