Skip to content

ci: run the unit tests and their docs guards on docs-only PRs - #96

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/ci-docs-guards
Sep 10, 2026
Merged

ci: run the unit tests and their docs guards on docs-only PRs#96
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/ci-docs-guards

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Runs the Docker-free unit suite on every PR, so the tests that pin prose to code fire on docs-only PRs instead of being skipped with the rest of CI.

Why

CI's changes job classifies **/*.md and docs/** as non-code and gates lint, build, the Docker test matrix and demo on code == 'true'. That skip is right for the matrix, but it also skipped the guards that pin prose to code: internal/safety/proof_types_test.go for the proof-type registries in SAFETY.md, .agents/checks/review.md and docs/tcb-model.md, and the seven docs_test.go files for the plan/suggest/execution-model report examples and CLI output examples. A docs-only PR could delete a registry row and merge green. Surfaced by the #88 review and scoped out of that PR as a CI change.

Two alternatives were considered: a docs-only leg running a hand-picked list of guard packages, or treating **/*.md as code for the test job. The first keeps a list that drifts exactly the way the prose does; the second runs the 5-version Docker matrix for a prose check. The unit suite runs in about 30 s locally and already contains every guard, so running it unconditionally costs one cheap redundant job on code PRs and needs no list.

What

  • New unit job: make test-unit (SKIP_INTEGRATION=1, no Docker), runs on every PR and push, not gated on the changes filter.
  • all-green requires it; its needs list is a strict superset of the old one.
  • docs/testing.md "matrix is a gate" bullet says docs-only PRs still pass the unit job and names which tests are the guards.

Before / after

A docs-only PR deletes the AbsentTarget row from the proof-type registry in docs/tcb-model.md. Before, the changes filter saw only markdown, every downstream job was skipped, and all-green passed with nothing having run. After, the same PR still skips lint, build, the matrix and demo, but the unit job runs and TestRegistriesNameEveryProofType fails on the missing row, so all-green fails. Code PRs are unchanged apart from one extra ~30 s job.

PR touches only docs/tcb-model.md (registry row "AbsentTarget" deleted)

Before                                          After
┌────────────────────────────┐                  ┌────────────────────────────┐
│ changes: code=false        │                  │ changes: code=false        │
└─────────────┬──────────────┘                  └─────────────┬──────────────┘
              │ every job is gated on code                    │ lint/build/test/demo gated on code;
              ▼                                               ▼ unit is not
┌────────────────────────────┐                  ┌────────────────────────────┐
│ lint   skipped             │                  │ lint   skipped             │
│ build  skipped             │                  │ build  skipped             │
│ test   skipped (matrix)    │                  │ test   skipped (matrix)    │
│ demo   skipped             │                  │ demo   skipped             │
│ (no unit job exists)       │                  │ unit   runs make test-unit │
└─────────────┬──────────────┘                  └─────────────┬──────────────┘
              │ nothing ran, so nothing failed                │ TestRegistriesNameEveryProofType fails:
              ▼                                               ▼ "tcb-model.md does not name AbsentTarget"
┌────────────────────────────┐                  ┌────────────────────────────┐
│ all-green: PASS            │                  │ all-green: FAIL            │
│ registry row lost, merged  │                  │ cannot merge until the row │
└────────────────────────────┘                  │ is restored                │
                                                └────────────────────────────┘

🤖 Created by Kiran's coding agent (Amp, Claude Opus 4.6)

The changes filter classifies *.md and docs/** as non-code and every
downstream job is gated on it, so the docs_test.go guards that pin prose
to code (the proof-type registries, the report and CLI output examples)
never ran on the one kind of PR they exist to catch. A docs-only PR could
drop a registry row and merge green; the guards passing locally was a
fact about the author's machine, not a property CI held.

The unit suite needs no Docker and finishes in well under a minute, so
it now runs unconditionally on every PR and push and joins all-green's
required set. Running it redundantly beside the version matrix on code
PRs is cheaper than maintaining a list of which packages are guards, and
that list would drift the same way the prose does.
Kiran01bm added a commit that referenced this pull request Sep 9, 2026
Add a machine-readable refusal class contract so consumers can route
every refusal without reinterpreting overloaded reason strings.

## Why

Existing refusal reasons explain the immediate cause but conflate
capability gaps, work owned by another tool class, permanent safety
decisions, run-site conditions, and pg-sprite's own defects.
`unsupported-statement` and `unsupported-partitioned-parent` each span
several of those categories, and the split runs one level below the
reason: along the typed cause (`PartitionRefusalCause`,
`CreateShapeCause`, admission sentinels) that the code already carries.

## What

- Define five additive classes: `capability-boundary`,
`no-online-safety-problem`, `by-design`, `environmental`, and
`invariant-violation` (the refusal-verdict face of the fail-closed
`ErrInvariantViolation` rule).
- Define structured owner values for `no-online-safety-problem` routing:
`data-change-runner`, `provisioning`, `direct-operator` (the ⚪ half of
T3), `declarative-front-door`.
- Key the map on the typed cause where one exists and on the refusal
site where none does: one table per cause set, plus the site-keyed table
for the rest. Name the parse-boundary work (`pkg/statement`
distinguishing the kinds behind the `KindOther` catch-all) that
classification requires.
- Pin the map to the code: `docs_test.go` guards in `pkg/verdict`,
`pkg/executor`, and `pkg/preflight` fail when a `Reason`,
`CreateShapeCause`, or `PartitionRefusalCause` exists without a row
(adds `preflight.PartitionRefusalCauses()`).
- Cite RF-5 (what `by-design` encodes) and RF-6 (which already separates
the not-implemented from the version-gated partitioned-parent case);
name the invariant rollout step 2 adds once the field ships.
- Bring `capabilities-contract.md`'s shared-vocabulary section into
agreement (fifth class, `direct-operator`), and add the design to the
documentation index.

Ordering: #95 (capabilities contract) has merged, so the cross-link here
is live. #96 (CI runs Go tests on docs-only PRs) is independent; this PR
touches Go test files, so full CI runs on it regardless.

## Before / after

Two refusals with the same `reason`, `unsupported-partitioned-parent`,
on PostgreSQL 16:

```text
Before: the consumer sees one reason and cannot tell the two apart

  CREATE INDEX ... ON parts_parent (col)           ALTER TABLE parts_parent
      │                                              ADD FOREIGN KEY ... NOT VALID
      ▼                                                  │
  refused, reason: unsupported-partitioned-parent        ▼
  (the partition-aware build is not implemented yet)  refused, reason: unsupported-partitioned-parent
      │                                              (PostgreSQL 16 cannot do this; 18 can)
      ▼                                                  │
  consumer: "unsupported" — wait? upgrade? give up?      ▼
                                                     consumer: the same guess, same reason string

After: the class is read off the typed cause, so each refusal names its own next step

  CREATE INDEX ... ON parts_parent (col)           ALTER TABLE parts_parent
      │                                              ADD FOREIGN KEY ... NOT VALID
      ▼                                                  │
  cause: parent-concurrent-index-build                   ▼
      │                                              cause: parent-not-valid-foreign-key
      ▼                                                  │
  reason: unsupported-partitioned-parent                 ▼
  class:  capability-boundary                        reason: unsupported-partitioned-parent
      │                                              class:  environmental
      ▼                                                  │
  consumer: wait for the pg-sprite release               ▼
            that adds the partition-aware flow       consumer: upgrade the server to 18; same
                                                               statement and same pg-sprite then run
```

The other cause sets follow the same rule: a `CREATE INDEX CONCURRENTLY`
on a table born this run is `by-design` (use the plain build), and a
`planRefusal` for a disposition this build does not know is
`invariant-violation` (report a pg-sprite bug, do not wait or retry).

🤖 Drafted with Amp (Claude Opus 4.6); reviewed and edited by the author.
@Kiran01bm
Kiran01bm marked this pull request as ready for review September 9, 2026 23:29
@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

🤖 Reviewed 61a8898a..72c0f67a (2 files, +32/−5). Comment 1 of 2 — does the new job actually gate, and does its premise hold. Comment 2 covers the two enumerations.

This is the right fix and the mechanism works. The two things I'd have expected to be wrong are both fine, and one of them is fine for a non-obvious reason worth recording.

The sentinel really does depend on the new job. all-green checks toJSON(needs) and greps for "result": "failure"|"cancelled" rather than enumerating needs.unit.result, needs.lint.result, … — so adding unit to needs at :219 is sufficient, with no second edit to the step body. That is the failure mode this PR could most easily have shipped: a job added to needs but not to a hand-written result check, which yields a green sentinel over a red job. It doesn't apply here, and the shape is why.

The premise is real, and I verified it by mutating the prose rather than the code. Two docs-only edits, each caught by the new job's oracle (SKIP_INTEGRATION=1 go test -count=1 ./...):

drop  | `unsupported-statement` |  from docs/cli-output-examples.md
  --- FAIL: TestDocListsEveryRefusalReason

drop  | `budget-lock-exceeded` |  from docs/execution-model.md
  --- FAIL: TestDocNamesEveryOutcomeCode
  --- FAIL: TestDocPermanentColumnMatchesCodePermanent

Under the old config both of those are docs-only PRs and merge with every Go test skipped. So the hole was live, not theoretical.

The guards genuinely run under the flag, rather than skipping. This is the thing that would hollow the job out silently, so I checked it directly instead of trusting the file names: all 24 tests across internal/safety/proof_types_test.go and the eight docs_test.go files RUN and PASS under SKIP_INTEGRATION=1, none SKIP. SKIP_INTEGRATION is only consulted in internal/testutil/{postgres,postgres_tls,ministack}.go, which no prose-pinning test reaches. make test-unit is green in ~15s locally and the job reports 1m30s in CI, so "cheap enough" at :73-74 is measured, not asserted.

The missing -count=1 is harmless, and not for the reason it looks like. make test-unit is SKIP_INTEGRATION=1 go test -race ./... while make test-db carries -count=1, and actions/setup-go restores the build cache by default — which reads like a way for a guard's PASS to be replayed on a docs-only PR that changed the very file the guard reads. It isn't: Go's test cache records the files a test opened, so touching the doc invalidates it. Confirmed on the branch — second run reports (cached), and after appending a line to docs/refusal-classes.md the same command re-executes in 0.181s. Worth a half-sentence somewhere near :85 precisely because the next reader will have the same doubt and cannot resolve it from the diff.

On invariants: docs/invariants.md has no CI family and shouldn't grow one — by the repo's own convention (docs/invariants.md's closing section) engineering rules live in AGENTS.md. The statement this change is accountable to is docs/testing.md:29-30, "The matrix is a gate, not advisory", and the effect of this PR is to move that from almost true to true: docs-only PRs previously skipped every Go test, guards included. :31-34 updating it in the same PR is the right disposition.

No blocking findings. Comment 2 has two small accuracy points, both in comments this PR writes.


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

@aparajon

Copy link
Copy Markdown
Collaborator

🤖 Comment 2 of 2 on 61a8898a..72c0f67a. Both points are about enumerations this PR adds, and both have the same fix: the predicate already said it.

1. The guard list is incomplete on the day it is written, in two places, and the job does not need one

ci.yml:68-70 names the guards as internal/safety/proof_types_test.go plus "the docs_test.go files for the verdict/plan/suggest report examples and CLI output examples", and docs/testing.md:32-34 repeats a slightly different version of the same list. There are eight docs_test.go files — internal/cli, internal/safety, pkg/executor, pkg/migrate, pkg/plan, pkg/preflight, pkg/suggest, pkg/verdict — so executor, migrate and preflight are unnamed. More to the point, two prose-pinning guards are not docs_test.go files at all:

  • pkg/schemadiff/managed_tables_test.goTestCapabilitiesDocShowsTheManagedTablesQuery, reads ../../docs/capabilities.md
  • internal/cli/dryrun_text_test.goTestDryRunCodesHaveDocAnchors, reads ../../docs/postgres-online-ddl-reference.md

Both run and pass under SKIP_INTEGRATION=1, so the behavior is right and nothing is uncovered — the job runs ./..., which includes every guard by construction, wherever it lives. The list is the only liability, and it is the kind that rots invisibly: the next guard added outside a docs_test.go file will be covered by the job and absent from both descriptions of it, with nothing failing.

The comment's own last sentence is the argument against writing it: running redundantly "costs less than maintaining a list of which packages are guards" (:73-74). That reasoning applies one level up. What the job actually promises is the whole module under SKIP_INTEGRATION=1, so every prose-pinning guard is included regardless of which package it lives in — shorter than the enumeration, strictly more accurate, and it stays true. Naming one or two examples parenthetically is fine; what I'd drop is the closed-set framing in both files. This is also the shape the repo applies to itself everywhere else — TestCodesEnumerateEveryDeclaredCode and friends exist precisely so a table is derived from a registry rather than maintained by hand.

2. aws-boundary is missing from the new skip list

:19-20 now reads:

PRs that touch only docs skip every job gated on code == 'true' below — lint, build, the Docker test matrix and demo

aws-boundary:194 is also if: needs.changes.outputs.code == 'true', so five jobs are gated and four are named. Same fix as above: the clause before the dash is already complete and checkable, and the list after it is what went stale — on the same commit, since aws-boundary predates this branch.

3. Smaller point

:213-214's "Succeeds when nothing failed" is now slightly less true than when it was written, because this PR is the one that makes the sentinel's set worth distinguishing from the job list: aws-boundary is deliberately outside needs with its rationale at :181-186. Since the sentence is being edited anyway, "succeeds when nothing in its required set failed" costs three words and pre-empts the reader who counts the jobs in the Checks tab and finds one that isn't a dependency.

Nothing here blocks. Approving — details in comment 1.


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 sentinel genuinely gates on the new job (all-green greps toJSON(needs) rather than enumerating results, so the one-line needs edit is sufficient), the guards run rather than skip under SKIP_INTEGRATION=1 (24/24 RUN, none SKIP), and I confirmed the hole was live by mutating the prose: dropping a required row from docs/cli-output-examples.md or docs/execution-model.md fails the unit job, and both edits are docs-only PRs that merged with every Go test skipped before this change. The two findings are stale enumerations in comments this PR writes — non-blocking. Details in the two comments above.

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

@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

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

Verdict: both findings addressed in 72c0f67; comment-only changes, the workflow graph is untouched.

# Finding Status Explanation
1 Non-blocking: "Docker-backed jobs" misdescribes the docs-only skip — lint and build are gated on changes too and use no Docker Fixed Both comments (the changes job and all-green) now say docs-only PRs skip every job gated on code == 'true' — lint, build, the Docker test matrix and demo — while the unit job runs regardless.
2 Suggestion: the proof-type registry guard is credited to docs_test.go, but proofTypeRegistries lives in internal/safety/proof_types_test.go Fixed The unit job comment and docs/testing.md attribute the registry guard to internal/safety/proof_types_test.go and the report/CLI example guards to the docs_test.go files.

Source: #96, scratch review pg-sprite-pr96-review.md at head be62d202

@Kiran01bm
Kiran01bm merged commit a166ebd into main Sep 10, 2026
15 checks passed
@Kiran01bm
Kiran01bm deleted the kiran01bm/ci-docs-guards branch September 10, 2026 01:39
Kiran01bm added a commit that referenced this pull request Sep 10, 2026
…g-first-search-path

* origin/main:
  ci: run the unit tests and their docs guards on docs-only PRs (#96)
  docs: define the refusal class routing contract (#94)
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