Gate the data quality tools behind a --data-quality capability flag

[betty-pr-factory-app]

🎯 What does this PR do?

Gates the 18 data quality tools behind a new top-level boolean capability flag. They registered
unconditionally on main; several write to Collibra and two (dq_delete_job, dq_delete_job_run)
delete irreversibly, so a deployment now opts the capability in explicitly.

⚠️ BREAKING CHANGE — anyone running without the flag loses the data quality tools. They no
longer appear in tools/list and cannot be called until --data-quality (or
COLLIBRA_MCP_DATA_QUALITY, or mcp.data-quality) is set. Verified against a built binary: 48
tools with the flag, 30 without, the difference being exactly the 18 below.

The flag

--data-quality / COLLIBRA_MCP_DATA_QUALITY / mcp.data-quality, default false, modelled on
--enable-debug-tools (cmd/chip/config.go, McpConfig, cmd/chip/main.go) with a matching
DataQuality bool on chip.ServerToolConfig. Precedence is viper's usual flag > env > file.

It is not an experimental feature and is deliberately absent from knownExperimentalFeatures.
The two axes stay separate: Experimental is a list of opt-in names with no stability promise;
DataQuality is a capability that is either on or off. All 18 tools are generally available.
--experimental=data-quality logs the usual unknown-feature warning and registers nothing.

Gated as one block

pkg/tools/register.go wraps the registrations per docs/TOOL_CONTRIBUTION_STANDARDS.md 3.2,
registration order unchanged: create_data_quality_job, create_data_quality_rule,
get_data_quality_rule, get_data_quality_rule_results, validate_data_quality_rule,
list_data_quality_rule_templates, get_data_quality_rule_template,
deploy_data_quality_rule_template, generate_data_quality_rule_sql, find_data_quality_rules,
dq_cancel_job_run, dq_delete_job_run, dq_delete_job, dq_update_job, dq_get_job,
dq_get_job_run, dq_search_jobs, dq_search_job_runs.

search_catalog_columns sat inside that run of registrations but is a Knowledge Graph search over
catalog Column assets with nothing to do with DQ jobs or rules, so it moves out of the block and
stays ungated. No tool is added, removed, renamed or re-described, and no schema or behaviour
changes — only reachability.

Because the block gates registration, --enabled-tools cannot override it: a tool inside a
closed gate never reaches toolRegister, so IsToolEnabled never sees it. The allow-list is a
filter within the enabled capability set, not an escape hatch. Documented in the README and in
standards 3.6 rather than worked around.

No preview machinery was added. The rule for later (standards 3.2): a DQ tool that ships as preview
nests its own experimental check inside the DQ block, so it needs both flags, and graduating it
means deleting the inner check and nothing else.

The DQ skills follow the flag

With --experimental=skills and the DQ flag off, an agent could previously load a guide telling it
to call tools that were never registered. pkg/skills/frontmatter.go now parses a requires: key;
collibra/dq-rules and collibra/dq-rule-workbench declare requires: data-quality, and
pkg/skills/catalog.go filters them at catalog load. Declaring the gate in the skill rather than
hardcoding names in Go lets an externally supplied skill from --skills-dir gate itself and stops
a rename silently un-gating a skill. An unrecognized requires: value fails catalog load with a
message naming the value.

collibra/index no longer names the two skills in its routing table or related: header — filtering
the catalog does not rewrite markdown, so the navigator would have kept routing to skills that no
longer load. It carries a short note instead that data quality skills exist and are served when the
capability is on; they stay discoverable through list_collibra_skills. This is the one judgement
call worth flagging: standards 10.2 asks every skill to be registered in the index, and 10.2 is now
qualified for capability-gated skills.

Documentation

• README: the eight annotations that read "Experimental (data-quality feature flag)" — a flag
  nothing implemented — now name the real boolean, and the nine other gated DQ tools are marked the
  same way so the docs match reachability. Added the missing create_data_quality_job entry (text
  taken from the tool's own Description) and a "Data quality tools" configuration section.
• docs/CONFIG.md: the flag in both the environment-variable and mcp field lists, the example
  YAML and the env-var mapping. Also fixed two pre-existing staleness bugs there: the known
  experimental features were listed as just skills, omitting context-specifications (lines 28
  and 88), and the precedence preamble omitted command-line flags, which --help documents as
  highest.
• docs/TOOL_CONTRIBUTION_STANDARDS.md section 3 rewritten: 3.1 said "New tools go behind an
  experimental feature flag, off by default", which is wrong for a GA domain and would be followed
  literally. Section 3 now separates the two gating axes, covers the nested-preview rule, skill
  self-gating, and the enabled-tools consequence.
• SKILLS.md: the requires: frontmatter key and capability-gated skills.

Tests

go test ./... passes (run without -race: this sandbox has CGO_ENABLED=0 and no C compiler, so
-race cannot run here — CI runs go test -race -v ./...).

• pkg/tools/register_test.go — all 18 absent by name with a default config, all 18 present with
  the flag; the non-DQ surface asserted by name membership in both directions so the wrapper cannot
  have swallowed a neighbour; search_catalog_columns present in both states; --enabled-tools
  naming DQ tools with the flag off registers none; data-quality as an experimental feature name
  registers none. The annotation test now enables the flag so the DQ tools stay covered.
• cmd/chip/config_test.go — the flag through YAML, env var and command-line flag, asserting flag

  env > file.

• cmd/chip/experimental_test.go — data-quality absent from knownExperimentalFeatures.
• pkg/skills/capability_test.go — exact served-skill sets in both flag states, unknown requires:
  failing load (embedded and external), an external skill gating itself, and a cross-reference check
  that every collibra/* name in a served skill's body or related: header resolves to a skill that
  is also served, in both states.

Impact analysis

Reachability only. Deployments that rely on the DQ tools must add the flag — that is the breaking
change. Non-DQ tools, all schemas and all tool behaviour are untouched. skills.RegisterAll and
skills.Load/LoadWith take the server tool config now (internal signatures, updated at all call
sites). Two open PRs declare a conflicting DQ flag of their own (#134 data-quality as an
experimental feature, #140 data-quality-experimental for dq_run_job); this lands first and both
rebase onto it. Nothing from either branch is referenced here.

Note for reviewers: AC-8 of the task describes nine non-DQ and eleven total skills; main embeds
seven non-DQ and nine total. The skills tests therefore assert the exact skill-name sets rather than
counts, which is stricter and does not depend on the total.

✅ Checklist

• My code follows the style guidelines of this project.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation (if needed).
• My commit messages follow the Conventional Commits standard.

[betty-pr-factory-app]

Review — gating the data quality tools behind --data-quality

Verdict: the implementation is correct and the gate does exactly what it claims. I verified
the reachability change empirically rather than by reading the diff (details below). What is left
is one commit-message convention fix and a short list of nits; nothing here changes behaviour.

What I verified, and how

I built the branch and diffed the actual registered tool surface against main by listing the
tools over an in-memory MCP session in each configuration.

• Only the intended 18 tools lost reachability. With a default config the surface goes from
  48 tools on main to 30 on this branch, and the set difference is exactly:
  create_data_quality_job, create_data_quality_rule, get_data_quality_rule,
  get_data_quality_rule_results, validate_data_quality_rule,
  list_data_quality_rule_templates, get_data_quality_rule_template,
  deploy_data_quality_rule_template, generate_data_quality_rule_sql,
  find_data_quality_rules, dq_cancel_job_run, dq_delete_job_run, dq_delete_job,
  dq_update_job, dq_get_job, dq_get_job_run, dq_search_jobs, dq_search_job_runs.
  No neighbouring registration was swallowed. pkg/tools/ contains exactly 18 *_dq_* tool
  packages, so no data quality tool was left outside the gate either.
• search_catalog_columns stays ungated — present in the tool list with the flag both off and
  on, and covered by TestRegisterAll_SearchCatalogColumnsIsNotGated.
• No tool schema, description or annotation changed. With every gate on
  (EnableDebugTools + DataQuality + both experimental features), the marshalled
  tools/list payload is byte-identical to main (256,424 bytes each, empty diff). pkg/tools/
  has no changes outside register.go and register_test.go.
• The reordering is inert. Moving search_catalog_columns out of the block is the only
  reordering, and it cannot be observed by a client: the SDK returns tools/list sorted by name
  (confirmed against the unsorted call). Relative order inside the block is unchanged.
• The flag is a plain boolean and is not an experimental feature. DataQuality bool on
  chip.ServerToolConfig and McpConfig; cmd/chip/experimental.go is untouched, so
  knownExperimentalFeatures still has exactly skills and context-specifications, and
  TestDataQualityIsNotAnExperimentalFeature plus
  TestRegisterAll_DataQualityAsExperimentalFeatureRegistersNothing pin that down from both ends.
• The gating tests assert name membership, not a count. TestRegisterAll_DataQualityTools*
  check every one of the 18 by name in both directions, and
  TestRegisterAll_DataQualityFlagOnlyMovesDataQualityTools asserts the non-DQ surface is
  identical in both states by membership in both directions; the count assertion is an extra
  guard after that, not a substitute for it. This is the right shape.
• The branch's relationship to its base is clean. One non-merge commit whose parent is
  cfe51c9, the current tip of main; git merge-base main HEAD equals that same commit. No
  merge of main, and nothing from feature/DEV-205663 or feat/DEV-211807 (no DQ tool source,
  no dq_run_job).
• Checks I could run here: go build ./..., go vet ./... (compiles every package's tests),
  gofmt -l ./cmd ./pkg (clean), golangci-lint run over ./cmd/... ./pkg/skills/... ./pkg/tools ./pkg/chip/... → 0 issues, and go test on ./pkg/skills ./pkg/tools ./cmd/chip ./pkg/chip → all ok. I did not run the repo-wide suite or -race (no C
  compiler and a memory-capped sandbox); CI's go test -race -v ./... is still the deciding run.

The design choices are the right ones and the comments in pkg/chip/server.go and
pkg/tools/register.go explain why the capability axis is separate from --experimental, which
is the part a future contributor would otherwise get wrong. Gating the two DQ skills via
requires: frontmatter rather than a hardcoded list in Go is a better answer than the story
strictly needed: an external --skills-dir skill can gate itself, and a rename cannot silently
un-gate one. TestEmbeddedCatalog_crossReferencesResolveInEveryState is the test I would have
asked for — it is what keeps collibra/index from advertising a skill the configuration filtered
out. I also checked the references/ and _shared/ markdown: no served skill mentions a DQ skill
or a DQ tool name.

Please change

1. The commit message carries two conventional-commit headers. Below the Co-authored-by:
   trailer of the first message there is a second header, docs(dq): document the --data-quality capability flag DEV-219055. The branch squash-merges under the first subject, so the second
   header lands as body noise and — more importantly — BREAKING CHANGE: is no longer the last
   footer, which CONTRIBUTING.md §3 requires ("at the very bottom of the commit"). That section
   also asks for a ! after the type for a breaking change, and AGENTS.md asks for a short
   one-liner. Please amend to a single header — feat(dq)!: gate data quality tools behind --data-quality DEV-219055 — with the docs rationale folded into the body and
   BREAKING CHANGE: … as the final footer.

2. docs/TOOL_CONTRIBUTION_STANDARDS.md:59 contradicts §3.2. "A domain uses one of the
   two, not both" reads as a prohibition, and twenty lines later the nested-preview example is
   introduced with "so it needs both flags". Reword to something like "one domain gate, not two;
   a preview tool inside a generally available domain may nest an additional experimental check".
   (More generally, the §3 rewrite goes well beyond what this change needs — §3.5, §3.6 and the
   nested-preview machinery that no code implements yet. The 3.1 correction was clearly warranted,
   so I am not asking for a revert; just flagging that whoever owns that doc should get a look.)

3. README: seven of the 18 **Gated:** markers are missing the sentence-ending period
   (lines 21–27, e.g. "…active/suppressed state Gated: the data-quality capability"). The
   eleven write-tool bullets and the existing **Requires:** markers all follow a full stop, so
   this is inconsistent inside the PR itself.

Nits, take or leave

4. pkg/skills/capability_test.go:123 — skillRefPattern matches any collibra/<slug> substring
   in a skill body, so a future served skill that quotes a path such as
   pkg/skills/files/collibra/dq-rules/SKILL.md will fail the cross-reference test with a
   misleading message. Restricting the match to backticked references, or a comment recording the
   limitation, would age better.
5. pkg/skills/capability.go:36 — the capability-name → config-field mapping sits in pkg/skills
   while DataQualityCapabilityName sits in pkg/chip, so a second capability has to be wired in
   two packages. A method on chip.ServerToolConfig (CapabilityEnabled(name string) (bool, error)) with requirementsMet left in skills would keep the switch next to the fields it
   reads.
6. cmd/chip/config_test.go:52 leaves --data-quality=true set on the global
   pflag.CommandLine after the test (only viper.Reset is cleaned up). A
   t.Cleanup(func() { _ = pflag.CommandLine.Set("data-quality", "false") }) removes a landmine
   for the next test added to package main. Relatedly, the test re-creates Init()'s viper
   wiring (config name, search paths, env prefix) rather than exercising it, so a change to the
   env prefix or config name in Init() would not be caught — worth a comment saying so.
7. skills.Load, skills.LoadWith and skills.RegisterAll are exported and changed signature.
   Harmless for a binary-only module, but it is a breaking Go API change that the
   BREAKING CHANGE: footer does not mention.
8. Naming a gated tool in enabled-tools with the capability off is silently a no-op — deliberate
   and documented, but validateConfigFile could warn ("enabled-tools names
   create_data_quality_rule, which requires --data-quality") and save an operator a debugging
   session. Follow-up, not this PR.

One judgement call I agree with

Dropping the two DQ rows from collibra/index (and qualifying standards 10.2) trades a routing
entry for the guarantee that the navigator never advertises a skill the configuration filtered
out. Given that filtering the catalog cannot rewrite markdown, that is the right trade, and the
replacement note pointing at list_collibra_skills keeps the skills discoverable when the
capability is on.

The PR description's note about AC-8 (nine non-DQ / eleven total skills vs. the seven and nine
actually embedded) is worth a maintainer's eye, but asserting exact skill-name sets instead of
counts is the stricter choice and does not depend on the discrepancy being resolved.

[betty-pr-factory-app]

Review — gating the data quality tools behind --data-quality

Reviewed against docs/TOOL_CONTRIBUTION_STANDARDS.md as it stands on this branch (i.e. with
section 3 as rewritten here), plus CONTRIBUTING.md and AGENTS.md. Every finding cites a section
number.

Because this PR edits the standard it is measured against, the two questions are kept apart:

• Part 1 — does the code obey the rules as the branch now states them? Yes. Section 3.1–3.6 and
  10.2 are satisfied, including 3.2's single-block requirement. No blocking code finding.
• Part 2 — are the rewritten rules correct and followable? The rewrite fixes the real defect
  (old 3.1 told a contributor to put a generally-available tool behind --experimental), and it
  does state the two things asked of it: a preview tool nests its experimental check inside the
  capability gate so both are required, and graduating it deletes the inner check. But section 3
  now contains two internal contradictions and one incomplete wiring checklist, and it has lost
  the old 3.2's "across PRs and teams" wording, which is the one sentence that closed the per-tool /
  per-PR flag loophole. Those are worth fixing in this PR, while the section is open.

What was verified here, verbatim

$ go build ./...                                            # (after module download) BUILD_OK
$ gofmt -l cmd pkg                                          # (no output)
$ go test ./pkg/skills/... ./pkg/tools ./cmd/chip
ok  github.com/collibra/chip/pkg/skills   ...
ok  github.com/collibra/chip/pkg/tools    ...
ok  github.com/collibra/chip/cmd/chip     ...
$ go vet ./pkg/skills/... ./pkg/tools ./cmd/chip ./pkg/chip # (no output)
$ golangci-lint run --concurrency=2 ./pkg/skills/... ./pkg/tools ./cmd/chip ./pkg/chip
0 issues.

CONTRIBUTING.md asks for go test -race ./.... Not verified: this review environment has no C
compiler, so -race cannot run, and a repo-wide run is out of budget here. The packages this change
touches were run without -race; CI (.github/workflows/build.yaml:35) runs go test -race -v ./...
and is the authority on that.

Also not verified: the PR body's claim of "48 tools with the flag, 30 without" against a built
binary. The equivalent property is asserted in-process by
TestRegisterAll_DataQualityFlagOnlyMovesDataQualityTools, which passes.

---

Part 1 — code compliance with section 3 as rewritten

Section	Requirement	Result
§3 preamble, §3.1	pick the right axis; a GA domain that writes to Collibra gets a capability flag, not --experimental	Pass — --data-quality is a bool, and pkg/chip/server.go documents why it is not an experimental feature
§3.2	all of a domain's registrations gated as one block, no flag per tool or per PR	Pass — a single if toolConfig.DataQuality at pkg/tools/register.go:112 wraps all 18 registrations; one flag, one block, registration order unchanged
§3.2	only tools of the domain inside the block	Pass — search_catalog_columns is moved out to pkg/tools/register.go:104 and stays ungated, with TestRegisterAll_SearchCatalogColumnsIsNotGated pinning it in both states
§3.2	bool on chip.ServerToolConfig wired to flag, env var and YAML field	Pass — cmd/chip/config.go:97-100, McpConfig.DataQuality, cmd/chip/main.go:37, ServerToolConfig.DataQuality; cmd/chip/config_test.go proves flag > env > file
§3.3	a capability flag must not be in knownExperimentalFeatures	Pass — absent, and TestDataQualityIsNotAnExperimentalFeature plus TestRegisterAll_DataQualityAsExperimentalFeatureRegistersNothing keep it that way
§3.4	assert both directions by tool name, surrounding surface identical	Pass — all 18 by name in both directions, non-DQ surface compared by membership both ways and by count
§3.5	a skill for a gated domain declares its own gate; remove it from collibra/index	Pass — requires: data-quality on both DQ skills, filtered at catalog load, external --skills-dir skills gate themselves, index routing rows and related: header removed, and TestEmbeddedCatalog_crossReferencesResolveInEveryState guards the markdown
§3.6	enabled-tools is a filter, not an escape hatch; document it	Pass — TestRegisterAll_EnabledToolsCannotReopenDataQualityGate, plus README and docs/CONFIG.md
§5.3	annotations enforced on every tool behind every gate	Pass — DataQuality: true added to the annotation test's config, so the 18 stay covered
§10.2	skill registered in collibra/index, exception for capability-gated skills	Pass — the exception is written into 10.2 in the same change, so the index note is compliant rather than a deviation

One extra check, beyond what §3.5 asks for: no served (non-DQ) skill names a gated DQ tool
anywhere in its body or references, so the "agent handed a guide for tools it cannot call" hazard is
closed in substance and not just for skill-to-skill links. Grep over
pkg/skills/files/collibra/*/SKILL.md (excluding dq-*) and their references/ returned nothing.

Code findings

F1 (medium) — an unrecognized requires: value is fatal to server startup, which contradicts the
repo's own stale-config principle. pkg/skills/capability.go:40-45 returns an error, which
skills.RegisterAll → tools.RegisterAll → cmd/chip/main.go:57-59 turns into os.Exit(1).
Meanwhile an unknown --experimental name only warns, deliberately: cmd/chip/experimental.go
("Stale configs from retired or renamed features should not break server startup"), and both
README.md and docs/CONFIG.md advertise that. So the two axes now behave oppositely on the same
class of operator error. The consequence lands on exactly the case §3.5 is proud of supporting: an
operator with --skills-dir pointing at their own skills gets a server that will not start after a
chip upgrade renames or retires a capability.

The code comment justifying the hard failure says an unknown value "would otherwise be served
unconditionally" — but that is a false choice. Treating an unrecognized capability as not met
(skip the skill, log a warning) is both fail-safe and non-fatal. Suggest: keep the hard error for
embedded skills if you want it caught at build time (a test already does that), and warn-and-skip
for --skills-dir skills. Whatever you choose, §3.5 should state the operational consequence,
since it currently reads as a neutral fact.

F2 (low) — pkg/skills exported API changed shape. Load, LoadWith (pkg/skills/catalog.go:50,61)
and RegisterAll (pkg/skills/register.go:65) all take *chip.ServerToolConfig now. The PR body
calls these "internal signatures", but they are exported from a published Go module, so any external
importer breaks at compile time. Not necessarily worth avoiding — just say so in the BREAKING CHANGE: footer rather than describing it as internal.

F3 (low) — capabilityEnabled's error message will go stale. pkg/skills/capability.go:41-44
hardcodes chip.DataQualityCapabilityName as "known capabilities", so a second capability flag makes
the message wrong while the code is right. A map[string]func(*chip.ServerToolConfig) bool (or
map[string]bool built from the config) would make the switch, the message, and the registry §3.5
needs to point at (see R6) one thing.

F4 (low, pre-existing) — dangling prepare_create_data_quality_job. README.md:23 and
README.md:27 — both lines edited by this PR to add the Gated: marker — still tell the reader to
get edgeSiteId/connectionId "from prepare_create_data_quality_job", a tool that no longer
exists (folded into create_data_quality_job; see pkg/tools/create_dq_job/discover.go:1). The same
dead name appears in four live tool descriptions (validate_dq_rule/tool.go:32,33,55,
generate_dq_rule_sql/tool.go:32,33,54) and in both DQ skills. §6.2 ("where code and documentation
disagree, either implement the claim or delete it") and §7.3 (each description must stand alone) both
bite, and the tool-description instances are LLM-facing, so an agent will try to call a tool that is
not registered in any configuration. Pre-existing and larger than this PR — but this PR's stated goal
is "the documented reachability matches the code", so the two README lines are cheap to fix here and
the rest is worth a follow-up ticket.

Informational — §4.1 and the dq_* tool names. Eight of the eighteen (dq_cancel_job_run,
dq_delete_job_run, dq_delete_job, dq_update_job, dq_get_job, dq_get_job_run,
dq_search_jobs, dq_search_job_runs) do not spell out the abbreviation, which §4.1 requires of the
MCP Name. Pre-existing on main, untouched here, and renaming them would be a second breaking
change — noted only so it is not mistaken for something this PR introduced.

---

Part 2 — are the rewritten rules followable?

The rewrite does what was needed: the old 3.1 ("New tools go behind an experimental feature flag,
off by default") is gone, the two axes are named and distinguished, 3.3 now forbids putting a
capability flag in knownExperimentalFeatures, and the nested-preview rule is stated with both the
"needs both flags" consequence and the "graduating means deleting the inner check" consequence. The
findings below are about the seams.

R1 (high) — §3 preamble contradicts §3.2 on "both". Line 59 says "A domain uses one of the
two, not both." Line 102-103 says a preview tool inside a GA domain "nests its own experimental check
inside the domain block, so it needs both flags". Both sentences are bold, and they are 40
lines apart in the same section. The intended reading is presumably "the domain's block is gated by
one axis; an individual preview tool inside it may add a nested check" — but a contributor who reads
the preamble and stops has been told not to do the thing 3.2 mandates, and a reviewer citing "3,
not both" against a nested check would be citing the document correctly. Fix the preamble, e.g.:
"A domain's registration block is gated by one of the two, never both. (A single preview tool
inside a generally available domain is the one exception — 3.2.) Do not add a flag per PR or per
tool."

R2 (high) — the section never answers "I am adding one new tool to the data quality domain".
That is the most common thing the next contributor will do, and it is the case the task set for this
rewrite. Today they must synthesise the answer from two places, and §3.1's wording points the wrong
way: "it is mandatory for admin/write tools (create / edit / delete) that are not yet generally
available" (lines 64-65). "Generally available" is left undefined, so a new delete_data_quality_rule
supports two readings — (a) it is a brand-new tool, therefore not yet GA, therefore it needs a nested
preview flag; (b) it joins a GA domain and inherits its status, therefore it goes straight into the
existing if toolConfig.DataQuality block with no new flag. Both are defensible from the text, and
they produce different PRs. This is a genuine ambiguity in the rewritten rule, not something I can
resolve by reading the code — the code has no preview DQ tool to imitate (the PR body confirms "No
preview machinery was added"). Suggest an explicit paragraph in 3.2, in the contributor's own words:

Adding a tool to a domain that is already gated: put the registration inside the existing block
and add no flag of any kind. The domain's gate already covers it. Add a nested experimental check
(below) only if the tool's own shape is unsettled and you intend to change it without a deprecation
cycle — not merely because the tool is new, and not because it writes.

R3 (high) — §3.2 vs §3.3 disagree about what graduating a preview tool requires. Line 114 says
graduating "means deleting the inner check and nothing else". §3.3 (lines 122-124) says "a name
in that map that nothing gates on is worse than no flag at all". If the graduated tool was the last
user of that preview feature name, deleting only the inner check leaves precisely such an orphan in
knownExperimentalFeatures — the document tells you to create the state it elsewhere calls worse
than no flag. Fix line 114: "…means deleting the inner check — and, if it was the last tool gated on
that preview name, the knownExperimentalFeatures entry too (3.3). Nothing else changes." §3.1's
GA-migration paragraph (lines 67-69) has the same omission for a whole tool set graduating off
--experimental.

R4 (high) — §3.2's capability-flag wiring list is incomplete, and the omission fails silently.
Lines 87-89 name "a bool field on chip.ServerToolConfig wired to a flag, an env var and a YAML
field in cmd/chip/config.go". Following exactly that and no more produces a flag that parses,
appears in --help, and never turns anything on, because the assignment that carries it from
config.Mcp into ServerToolConfig lives in cmd/chip/main.go:37 and is not mentioned. There is no
compile error: the struct field simply stays false. Two further hand-maintained spots are also
unnamed — the ENVIRONMENT VARIABLES and CONFIGURATION FILE EXAMPLE blocks inside
cmd/chip/config.go (this PR updates both, at lines 136 and 171). The asymmetry with §3.3 makes it
worse: 3.3 explicitly reassures the reader that for an experimental name "nothing else needs to
change", so a reader carries that expectation into 3.2, where four things do. Suggest listing the
five touch points, or simply: "wire it end to end exactly as DataQuality is wired — pflag +
BindEnv + BindPFlag + SetDefault and both --help blocks in cmd/chip/config.go, the
McpConfig field, the assignment in cmd/chip/main.go, and the ServerToolConfig field. Forgetting
the main.go assignment compiles and silently does nothing."

R5 (high) — the per-tool / per-PR loophole has been widened, not closed. The old 3.2 read "All of
a domain's tools share a single feature name, across PRs and teams. Do not add a flag per PR or
per tool." The rewrite keeps the prohibition (line 59) but drops "across PRs and teams", which was
the part that told a later contributor to reuse what already exists rather than to reason afresh.
What replaces it is a nested example whose placeholder is YourPreviewFeatureName (line 108) and a
closing sentence — "Do not add the nested block, or a preview feature name, before there is a preview
tool to put in it" (lines 114-115) — that reads naturally as one preview name per preview tool.
Nothing in the section says the preview name is itself one-per-domain and shared. So the next two
contributors each add their own preview name inside the DQ block, both citing 3.2, and the domain ends
up with a per-tool flag set — the exact outcome line 59 forbids. This is not hypothetical: the PR body
records two open PRs that each invented a DQ flag of their own (#134 data-quality as an experimental
feature, #140 data-quality-experimental). Suggest restoring the dropped clause and making the
preview name singular:

A domain has at most two gates ever: its domain gate, and — while some tool in it is in preview —
one shared preview feature name (e.g. data-quality-preview), used by every preview tool in
the domain. This holds across PRs and teams: if the domain already has a gate, reuse it; opening a
second one is a review finding, whoever opens it.

R6 (medium) — §3.5 omits the Go-side registry, so following 3.2 + 3.5 alone produces a server that
will not start. 3.5 tells the contributor to declare requires: <capability> in the skill's
frontmatter and says an unrecognized value fails catalog load. It does not say where a capability
name becomes recognized: the switch in pkg/skills/capability.go:36-45, keyed on a constant
declared in pkg/chip/server.go (DataQualityCapabilityName). A contributor who adds a second
capability flag per 3.2 and a requires: line per 3.5 gets "unknown requires value" — and, per F1,
os.Exit(1). One sentence fixes it: "Declare the capability name as a constant next to its
ServerToolConfig field (see chip.DataQualityCapabilityName) and add a case for it in
pkg/skills/capability.go, or no skill can require it."

R7 (medium) — §3 preamble overstates "off by default" and presents a false dichotomy. Line 49:
"New tools are off by default. There are two separate axes for that." §3.1 lines 67-68 then offer a
third outcome — a GA tool set "needs no gate at all" — and §3.2 line 98-100 cites a shipped ungated
tool (search_catalog_columns) approvingly. A contributor reading only the preamble concludes every
new tool must be gated somehow, which is not what the repo does or what 3.1 says. Reword to "Most new
tools ship gated. Decide first whether the tool set needs a gate at all (3.1); if it does, these are
the two axes, and picking the wrong one is a review finding."

R8 (medium) — no rule for retro-gating a domain that already shipped, which is what this PR
does. Section 3 is written entirely about new tools, yet the change it accompanies removes 18
generally-available tools from the default surface. The next team to do this has no guidance on the
obligation that follows: CONTRIBUTING.md ("Footer & Breaking Changes") requires a BREAKING CHANGE:
footer and/or !, and operators need a migration line. This PR does it right — footer present in the
first commit, the README section spells out the consequence — so the rule can simply be written down
from what was done here: "Gating a domain that already shipped ungated removes tools from every
existing deployment. That is a breaking change: signal it per CONTRIBUTING.md, and state in the
README how an operator restores the tools."

R9 (low) — §3.5 covers skill-to-skill references but not gated tool names in a served skill's
body. The rule is about the index and related: headers; the hazard it names ("an agent cannot be
handed a guide that calls unregistered tools") is equally triggered by an ungated skill that mentions
dq_delete_job in prose. TestEmbeddedCatalog_crossReferencesResolveInEveryState only matches
collibra/* skill names, so nothing catches the tool-name case. I checked and no such reference
exists today, so this is prevention, not a defect: extend the sentence to "…or a gated tool's name",
and consider extending that test to scan served skill bodies for the names of tools not registered in
the same configuration.

R10 (low) — no naming convention for the capability triple. §4 governs tool names but nothing
governs flag names, and the two shipped examples disagree: --enable-debug-tools /
COLLIBRA_MCP_ENABLE_DEBUG_TOOLS versus --data-quality / COLLIBRA_MCP_DATA_QUALITY. §3.2 cites
both as models to follow, so the next flag is a coin flip. State the intended form (--<domain>,
COLLIBRA_MCP_<DOMAIN>, mcp.<domain>) and note that --enable-debug-tools predates it.

R11 (low) — §3.4's "both states" will not scale past one capability. With a second capability
flag, "both states" becomes four, and the interesting cases are the mixed ones. The same applies to
the test this PR adds: TestEmbeddedCatalog_crossReferencesResolveInEveryState loops over
[]bool{false, true} on DataQuality alone. Correct today, quietly incomplete the day a second flag
lands; worth a note in 3.4 ("every combination of the gates a skill or tool can depend on") so the
next contributor extends it rather than copying it.

---

CONTRIBUTING.md and AGENTS.md

• Conventional Commits — both commits conform: feat(dq): gate data quality tools behind --data-quality DEV-219055 and docs(dq): document the --data-quality capability flag DEV-219055;
  imperative mood, scoped, descriptions well under 72 characters, bodies explaining why. The
  breaking change is signalled by a BREAKING CHANGE: footer, which CONTRIBUTING.md accepts
  ("AND/OR"). Non-blocking nit: the footer sits above the Co-authored-by: trailer rather than "at
  the very bottom"; releases here are cut manually by tag (.github/workflows/release.yaml), so
  nothing parses it automatically and the version bump is a human decision — flagging it only so the
  MAJOR expectation is conscious.
• Tests and lint — CONTRIBUTING.md asks for go test -race ./... and golangci-lint run. Lint
  is clean on the touched packages (0 issues.). -race could not run here; see the verification
  block above.
• AGENTS.md stepdown rule — followed. pkg/skills/capability.go reads requirementsMet then
  capabilityEnabled, and catalog.go keeps Load → LoadWith → loadFromFS → walk →
  loadSkill descending. New code sits next to the abstraction that calls it.
• AGENTS.md PR template — informational, no action for this PR. AGENTS.md prescribes
  "Description of your changes / Impact Analysis / Checklist (3 items)", while
  .github/pull_request_template.md has "🎯 What does this PR do? / ✅ Checklist (5 items)". The PR
  body follows the .github template with every box ticked, and folds an "Impact analysis" subsection
  in, which satisfies AGENTS.md's intent. The two templates diverging is a repo-level inconsistency
  worth reconciling separately. AGENTS.md also asks that the description avoid words like
  "comprehensive" — it does.

---

Summary

Nothing here blocks the code. The single block at pkg/tools/register.go:112, the skill self-gating
via requires:, the enabled-tools clarification and the test coverage are all what section 3
asks for, and the choice of a capability flag over an experimental name is correct and well
argued in both pkg/chip/server.go and the commit message.

The work left is in the document. R1–R5 are worth fixing before merge, because section 3 is
open in this PR and each of them is a sentence: the one of the two / both flags contradiction
(R1), the missing "adding one tool to an already-gated domain" rule (R2), the graduation instruction
that contradicts 3.3 (R3), the wiring list that omits main.go and fails silently (R4), and the
restoration of "one shared preview name per domain, across PRs and teams" (R5). R5 in particular is
the rule that stops the two competing DQ flags already open in #134 and #140 from becoming the
precedent.