Skip to content

capabilities: make the embedded YAML matrix the source of the support tables - #98

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/cm2-capabilities-source
Sep 10, 2026
Merged

capabilities: make the embedded YAML matrix the source of the support tables#98
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/cm2-capabilities-source

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Make the embedded, validated YAML capability matrix the single source for the generated Markdown support tables.

Why

The support contract needs one typed representation that documentation and future machine consumers cannot drift away from. Until now docs/capabilities.md was hand-edited, so a row could say one thing while the engine's verdict said another, and nothing would notice.

What

  • Add the typed pkg/capabilities package and embedded capabilities.yaml matrix (53 rows across seven areas).
  • Validate closed vocabularies and cross-field invariants against the real verdict.Reasons(); every error names the row ID, the field, and the offending value (row 21 (index-build-on-a-partitioned-parent): engine_path "native_as_is_" is not one of [...]).
  • Refuse cells that cannot live in a Markdown table row (embedded newlines) and escape pipes at render time so a cell can never split or truncate the table.
  • Generate the marked matrix tables and the summary counts with make gen-capabilities; .DEFAULT_GOAL := build keeps bare make building the binary.
  • Guard the checked-in document with TestCheckedInMarkdownIsGenerated, and refuse malformed marker layouts (missing, duplicated, or reversed) with a distinct error for each before anything is written.
  • Partitioned-parent rows carry the engine's unsupported-partitioned-parent reason, pinned by test; the FK NOT VALID row matches the server-18 support that landed in main.

Before / after

Example: a reviewer wonders which operations the engine refuses with unsupported-partitioned-parent.

Before (hand-edited capabilities.md)

  docs/capabilities.md ── "| Index build on a partitioned parent | 🟡 | native, planned flow | Yes | ... |"
                            prose only: no machine-readable reason, nothing ties the row to
  pkg/plan/refusal.go  ── PartitionRefusal(cause) → ReasonUnsupportedPartitionedParent
                            so a stale or wrong row stays green forever

After (capabilities.yaml → validation → generator → capabilities.md)

  capabilities.yaml ── refusal_reason: unsupported-partitioned-parent
        │                 Validate: is it a verdict.Reason? present iff a front door is refused?
        │                 TestPartitionedParentRowsCarryTheEngineReason: partitioned-parent rows carry it
        ▼
  make gen-capabilities ── rewrites only the marked regions of docs/capabilities.md
        ▼
  TestCheckedInMarkdownIsGenerated ── CI fails if the checked-in doc and the YAML disagree
                                        (this is what caught the FK NOT VALID row after #97
                                         hand-edited the doc: 🟡 t2 in YAML vs ✅ t1 in the file)

A typo in the YAML shows the difference in the error too:

Before:  row 21: unknown enum value
After:   row 21 (index-build-on-a-partitioned-parent): engine_path "native_as_is_" is not one of
         [native_as_is native_safer_sequence native_planned_flow copy_and_swap none]

Refs: docs/capabilities-contract.md

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.

Kiran01bm and others added 3 commits September 10, 2026 14:35
… tables

docs/capabilities.md was hand-edited Markdown, so its tiers, marks, and
owner vocabulary could drift from the verdict reasons the engine emits
and from the contract in docs/capabilities-contract.md, and no machine
consumer could read it.

pkg/capabilities embeds capabilities.yaml (53 rows across 7 areas) and
exposes it as typed rows behind a small API. Loading validates the closed
vocabularies (tier, mark, backend, owner) and the cross-field rules
against the real verdict.Reasons(), so a row cannot name a reason the
engine does not have or carry an owner outside the tier that allows one.
A renderer emits the marked matrix tables and the summary counts between
markers in docs/capabilities.md; `make gen-capabilities` regenerates them
and a unit test fails when the checked-in document is stale or a marker
is malformed. Regeneration against the current document is a no-op and
the table cell text is unchanged.

This completes step 1 of the capabilities contract; the contract doc,
SAFETY.md periphery table, and go.mod (yaml.v3 promoted to a direct
dependency) are updated to match.
…d as the default goal, and make table cells safe by construction
@Kiran01bm
Kiran01bm marked this pull request as ready for review September 10, 2026 19:09
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 1/2 — correctness. Reviewed 721d7975..2bd70fdd (9 files, +1209/−3). The conversion is faithful: the seven tables are byte-identical to the pre-PR document, so the entire content diff is marker insertion plus the regenerated summary.

Mutation run: 18 mutants — 11 killed, 3 that cannot be written, 4 surviving make test-unit.

The golden test is genuinely load-bearing; it killed five mutants on its own, including the two label maps, where a missing key would otherwise render an empty cell:

mutant killed by
pathLabels loses copy_and_swap TestCheckedInMarkdownIsGenerated
areaHeadings loses the declarative-model heading TestCheckedInMarkdownIsGenerated
summary drops the other-tool class from out-of-scope TestCheckedInMarkdownIsGenerated
the safety detail is never appended / No — loses its owner TestCheckedInMarkdownIsGenerated
pipes unescaped TestRenderDocumentEscapesPipesInCells
duplicate markers use the first / end-before-begin allowed TestRenderDocumentRejectsBadMarkers
duplicate ids allowed, refusal reason need not be a verdict.Reason TestValidateRules

Three could not be written at all, which is worth naming because it is a stronger guarantee than a test: deleting the status_mark/tier agreement check, the refusal-reason-iff-refused check, or the newline check each leaves expected, refused, or value unused, and Go rejects the build. Those three predicates are compiler-pinned as long as nobody gives those variables a second reader.

One survivor is an equivalent mutant rather than a gap: counting the summary from counts[StatusSupported] instead of tiers[TierOne] passes everything, because the validator forces mark and tier to agree, so the two spellings are interchangeable by construction. That is the invariant working.

I also checked the summary correction independently. The generated line moves 17 supported / 20 planned to 18 / 19, and no table row's mark changed — so I counted the marks in the pre-PR file: 18 ✅ and 19 🟡. The old hand-written summary was stale by one in each direction and the tables were right all along, exactly as the description says. That is the strongest evidence for the whole change, and it is now arithmetic rather than a claim.


1. Nothing pins that every area is rendered — an omission is invisible to the golden test

Dropping one area from RenderDocument's list survives. So does dropping six of the seven, leaving only column changes: go test ./... stays green.

The asymmetry is what makes this worth fixing rather than noting. Every mutant that writes wrong content into a region dies immediately, because TestCheckedInMarkdownIsGenerated compares byte-for-byte. But an area absent from the list is never passed to replaceRegion at all, so its region is simply left alone — the render becomes a no-op there, input == output holds, and the test reports success. The failure mode is stale content, and the one test guarding the document cannot see it.

Two concrete ways this lands:

  • A new area is added to the Area constants, the checkEnum list, and the YAML, but not to the render list. Its rows never appear in the document. The only tripwire is assert.Len(t, rows, 53), which fires for the wrong reason — it says the count changed, not that the rows are unrendered — and the natural fix is to bump 53, after which the omission ships. The summary still counts those rows, so the document claims more operations than its tables show.
  • An area is dropped from the list during a refactor. Every subsequent YAML edit in that area is silently ignored, and CI stays green while the published table and the source of truth diverge — the precise drift this PR exists to end.

The repo already has the right pattern for this one invariant family up. RF-7's TestRefusalRegistryIsComplete walks the production closed sets, asserts every key is classified, and guards against a vacuous walk with require.NotEmpty on each set — so the harness cannot pass by finding nothing. Two cheap steps give the matrix the same property: hoist the ordered area list to one package-level areaOrder that both validateEnums and RenderDocument consume, and have RenderDocument count the rows it emitted and return an error when that is not len(rows). The second is the one that matters, since it makes an unrendered row a generator failure instead of a silent absence.

2. A bare carriage return reaches a cell

validateText correctly uses ContainsAny(value, "\r\n"), but weakening it to Contains(value, "\n") survives the suite: the only \r case in the tests is "one\r\ntwo", and CRLF still contains \n. So the bare-CR half of that guard is unpinned.

It is not academic — CommonMark treats a lone CR as a line ending, so a single \r in a cell ends the table row exactly like \n, which is the failure validateText was written to prevent, and a quoted YAML scalar can carry one. Changing that one case to "one\rtwo" (or adding it beside the existing one) closes it, and keeps the test honest about which characters the guard actually covers.

3. online_safety_detail is validated, then silently dropped, on a No row

answer := "Yes"
if !r.OnlineSafetyProblem {
    answer = "No — " + r.OwningToolClass
} else if r.OnlineSafetyDetail != "" {
    answer += " — " + r.OnlineSafetyDetail
}

On a No row the else if never runs, so a online_safety_detail set alongside online_safety_problem: false never reaches the document. Nothing refuses it: Validate constrains owning_tool_class in both directions but says nothing about online_safety_detail, and textCells newline-checks it unconditionally, which reads as a promise that it is always rendered.

No current row does this, so it is latent rather than live. But it is the same class as finding 1 — input the loader accepts and the renderer discards without a word — and it costs one line beside the two existing owning_tool_class rules: reject online_safety_detail on a No row, or append it to both branches.

4. KnownFields(true) is right and untested

Setting it to false survives. Most misspellings are caught downstream by accident — a typo'd reason_notes or online_safety_problem trips a required-field or cross-field rule — but an optional key fails silently, and online_safety_detail is exactly that key. One Load case with a bogus field name pins the guard that makes the YAML authoritative, and closes the last silent-acceptance path alongside finding 3.


Nothing here blocks. Finding 1 is the one I would take before the rollout's later steps add consumers, because it is the guard the single-source claim rests on; 2–4 are one line each.

This review was generated by Claude Code (claude-opus-5).

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 2/2 — invariants, docs, and the dependency change.

Invariants. This change classifies against docs/invariants.md as follows:

  • RF-7 — upheld, and extended to a documentation surface. pkg/capabilities never constructs a verdict.Refusal, so it cannot violate RF-7; what it adds is the adjacent check that a published refusal reason is a real one, via slices.Contains(verdict.Reasons(), row.RefusalReason). That is new ground — before this, a table cell could name a reason the engine never emits and nothing would notice. RF-7's *Enforced:* line names pkg/plan/refusal.go and pkg/migrate/refusal_registry.go and needs no change; the matrix is a consumer of that vocabulary, not a second home for it.
  • RF-6 — upheld. I checked the FK row against the invariant text rather than the description: RF-6 refuses a partitioned-parent foreign key NOT VALID "on PostgreSQL before 18", and the row reads "Supported on version 18 and later; refused on 14–17 with an environmental class". Those agree, and TestPartitionedParentRowsCarryTheEngineReason pins the two index-build rows to unsupported-partitioned-parent so the reason a consumer filters on is the one the engine emits.
  • No new entry is owed. The generator has no runtime safety role — a bug here produces a wrong document, never a wrong statement against a database — which is also why the ❌ periphery classification in SAFETY.md is the right call and why the invariant-IDs column is correctly . The new row sits with the other periphery packages and matches their wording.

I also verified the one row-level modelling question I had rather than raising it. 35 rows refuse at both doors while carrying a single refusal_reason, so I checked whether the two doors can disagree: for create-table-partition-of, whose notes say the imperative door refuses CREATE TABLE as a kind while the declarative path refuses the form at plan time, CreateShapeRefusal maps CreateShapePartitionOf to ReasonUnsupportedStatement — the same token the imperative side emits. The doors agree, and the single field is sufficient.

Forward-looking, for rollout step 2 rather than this PR. The row publishes refusal_reason but not the class and owner that RF-7 made the typed contract. Tier approximates the class today (T2 reads as a capability boundary, T3 as by-design or another tool class), which is fine for a Markdown table where the legend explains the marks. It gets sharper once pg-sprite capabilities --json exists, because adding a field to a published JSON surface later is a compatibility decision, while reserving it now is free. Worth deciding when step 2 is written, not here.

Dependency change is clean. gopkg.in/yaml.v3 v3.0.1 moves from the indirect block to the direct one at the same version, and go.sum is untouched — no version drift smuggled in behind a docs change, which is the thing worth checking when a docs PR touches go.mod.

The Makefile claim is true. The gen-capabilities comment says "CI fails if they drift", and it does — not through a docs job, but because the unit-tests job runs make test-unit, which runs TestCheckedInMarkdownIsGenerated. Worth being precise that CI detects drift rather than repairing it: make gen-capabilities is never run in CI, so a contributor who edits the YAML and forgets to regenerate gets a red test, not a fixed file. That is the right way round, and .DEFAULT_GOAL := build correctly keeps a bare make from rewriting a checked-in document — the comment above it explains exactly why, which is the kind of comment that survives.

Contract doc. The **Status:** step 1 is complete; steps 2–4 remain planned. line is accurate against what shipped: the package, YAML, validator, generator, and markers all landed together, so the repository is single-source on day one as step 1 specifies. docs/capabilities.md's new editing note points at the YAML and tells readers not to hand-edit the generated regions, which is the instruction that keeps step 1 true.

Leak check. Clean — the diff carries no internal hostnames, org names, or infrastructure detail, which matters here because the matrix is the most externally-read document in the repo.

This review was generated by Claude Code (claude-opus-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving. The conversion is faithful — the seven tables are byte-identical to the pre-PR document, so the whole content diff is marker insertion plus a regenerated summary whose correction I verified by counting marks in the old file (18 ✅ / 19 🟡; the hand-written 17 / 20 was stale, and the tables were right).

Mutation run: 18 mutants, 11 killed, 3 that cannot be written because Go rejects the unused variable, 4 surviving. The golden test earns its place. Four findings, none blocking: the render list's area coverage is unpinned because an omission leaves a region untouched and input == output still holds (findings and the RF-7-shaped fix are in 1/2), plus a bare-CR gap, a validated-then-discarded online_safety_detail on No rows, and an untested KnownFields.

RF-7 is upheld and extended to a documentation surface; RF-6 agrees with the FK row; no new entry is owed, and the periphery classification is right.

This stamp was left by Claude Code (claude-opus-5).

@Kiran01bm

Kiran01bm commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/98, follow-up commit

All four findings fixed: the partitioned-parent rows now carry the engine's own refusal reason, bare make builds again, table cells are escaped and single-line by construction, and every validation error names the row, field, and value.

# Finding Status Explanation
1 Partitioned-parent rows publish unsupported-statement instead of the engine's unsupported-partitioned-parent (blocking) Fixed index-build-on-a-partitioned-parent and add-constraint-using-index-on-a-partitioned-parent now say refusal_reason: unsupported-partitioned-parent, matching plan.PartitionRefusal, which maps every preflight.PartitionRefusalCause to verdict.ReasonUnsupportedPartitionedParent. The third row, add-foreign-key-not-valid-on-a-partitioned-parent, is no longer a refused row: main's #97 lifted it to ✅ t1 (supported on server 18+, environmental refusal on 14–17) and hand-edited docs/capabilities.md, which is why CI on this branch went red — the YAML still said 🟡 t2. The row now matches the merged doc line exactly. TestPartitionedParentRowsCarryTheEngineReason pins the two refused IDs and additionally asserts that any refused row whose operation mentions a partitioned parent carries that reason, so a future row cannot regress to a generic token. Regenerating the doc also corrects the summary sentence main left stale (17 supported / 20 planned → 18 / 19).
2 gen-capabilities placed above build: becomes make's default goal (blocking) Fixed gen-capabilities moved below lint:, and .DEFAULT_GOAL := build is declared above build: with a comment explaining why, so target order can no longer change what bare make does. make -n prints the go build line; make -n gen-capabilities prints the go run.
3 Free-text cells interpolated unescaped: a | splits the row, a block scalar's newline truncates the table (non-blocking) Fixed Two layers. Rendering: cell() prefixes every pipe in a cell with a backslash (GFM keeps it in the cell) for operation, the online-safety answer, and reason_notes; TestRenderDocumentEscapesPipesInCells puts a pipe in each and asserts the emitted row still parses to five cells with the escaped text in place. Validation: validateText refuses any \r/\n in operation, online_safety_detail, owning_tool_class, or reason_notes with <field> must be a single line (a table row cannot span lines); TestValidateRejectsMultilineCells and TestLoadRejectsBlockScalarCell cover the struct path and the YAML | block-scalar path. Mutation-checked: a block scalar on a real row makes go run ./internal/cmd/gen-capabilities exit 1 naming the row.
4 Dense || predicates collapse six enum causes into one opaque unknown enum value; marker error blames "missing, unbalanced, or duplicate" for any of them (suggestion) Fixed Validate now calls named helpers: validateEnums runs checkEnum[T ~string](field, value, allowed...) per field and reports engine_path "native_as_is_" is not one of [native_as_is native_safer_sequence native_planned_flow copy_and_swap none]; validateText owns the required-field and single-line rules; the error prefix is row N (<id>) so the in-scope ID is always present. replaceRegion delegates to markerIndex, which returns one of three distinct errors — capability marker %q is missing, appears more than once, or (from the caller) appears before %q. TestValidateNamesTheFieldAndValue and the rewritten table-driven TestRenderDocumentRejectsBadMarkers (empty, missing end, end-before-begin, duplicate begin, duplicate end) assert each message and that RenderDocument returns nil output.

Source: #98, scratch review scratch/code-reviews/pg-sprite-pr98-review.md at head d218543a; fixes in 2bd70fd

@Kiran01bm
Kiran01bm merged commit 7d8fb6b into main Sep 10, 2026
15 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/cm2-capabilities-source branch September 10, 2026 19:44
Kiran01bm added a commit that referenced this pull request Sep 10, 2026
…ities-ci-gate

* origin/main:
  capabilities: make the embedded YAML matrix the source of the support tables (#98)
  verdict: classify every refusal with a typed class and owner (#97)
  fix(dbconn): strip explicit pg_catalog from pooled search_path (#93)

# Conflicts:
#	Makefile
#	docs/capabilities-contract.md
#	docs/capabilities.md
#	pkg/capabilities/capabilities.go
#	pkg/capabilities/capabilities.yaml
#	pkg/capabilities/capabilities_test.go
Kiran01bm added a commit that referenced this pull request Sep 10, 2026
…ities-subcommand

* origin/main:
  capabilities: make the embedded YAML matrix the source of the support tables (#98)
  verdict: classify every refusal with a typed class and owner (#97)
  fix(dbconn): strip explicit pg_catalog from pooled search_path (#93)

# Conflicts:
#	Makefile
#	docs/capabilities-contract.md
#	docs/capabilities.md
#	pkg/capabilities/capabilities.go
#	pkg/capabilities/capabilities.yaml
#	pkg/capabilities/capabilities_test.go
Kiran01bm added a commit that referenced this pull request Sep 10, 2026
…class

* origin/main:
  capabilities: make the embedded YAML matrix the source of the support tables (#98)
  verdict: classify every refusal with a typed class and owner (#97)

# Conflicts:
#	docs/refusal-classes.md
#	pkg/statement/statement_test.go
#	pkg/verdict/verdict.go
#	pkg/verdict/verdict_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants