docs: apply comprehensive 2.0 copy review

[NikolayS]

Summary

Applies the comprehensive user-facing copy inventory from #157 after
rechecking every finding against current main and the queued final stamp in
#200.

The history is deliberately split:

1. docs: correct misleading 2.0 contracts applies every actively misleading
   finding, including the scheduler behavior needed to make documented
   cadences exact, the ROLLUP_DESIGN.md correction from 43,800 rows/year to
   8,760 rows/year (approximately 43,800 over five years), and the
   RAISE WARNING severity correction.
2. docs: normalize historical and catalog wording contains the lower-severity
   terminology and precision cleanups. It can be dropped independently.

Because #179 froze the beta payload in sql/, behavioral/catalog corrections
are carried as the valid 2.0-beta1 development installer overlay at
devel/sql/ash-install.sql; the workflow and upgrade-chain helpers consume
that overlay.

Tail-aware deduplication

The branch is based on 87c22a3, which already includes #192, #179, #198, and
#199. The recheck retained their fixes and removed copy-review duplicates:

• release: add behavioral feature-gate coverage #192's empty-minute rollup_minute() catalog contract remains untouched.
• Release-note restatements already covered more completely by release: prepare 2.0 stamp #200 were
  dropped: processed-grain returns, PostgreSQL 19beta2/release-gate coverage,
  default-reader regrant behavior, hourly-grain disclosure, retention
  geometry, logical/physical raw bounds, compacted attribution, and stale
  rollup fallback.
• ci: define and enforce prerelease payload identity #179's release-process correction is not duplicated.
• release: keep compacted query attribution honest #198 and release: keep stale rollups honest without losing fast path #199 wording is not reverted.

Coordination with draft #200

This PR should land before #200. Then #200 should be refreshed onto the
post-#157 main; it must not be merged first.

#200 wins on every identity surface it changes:

• README: current main contains 2.0 SQL, v1.5 is the previous stable release,
  and the status example reports 2.0.
• Release notes: final 2.0 heading, v2.0 tag/payload identity, beta-to-final
  upgrade wording, and final-release organization.
• SQL/SECURITY: all final 2.0 identity stamps and support labels.

After this PR lands, #200 must promote
devel/sql/ash-install.sql into sql/ash-install.sql, preserve all four final
2.0 identity sites, and remove the development overlay. A plain rebase that
leaves a final released payload plus the beta overlay is invalid under #179's
SQL-chain checks.

The overlapping RELEASE_NOTES.md content requires these explicit resolutions:

• Keep release: prepare 2.0 stamp #200's final structure, B1–B6 text, upgrade-convergence wording, PR
  credits, and release-engineering section.
• Keep this PR's qualified AAS-first/provenance wording.
• Fold this PR's cadence correction into release: prepare 2.0 stamp #200's sampler-interval bullet:
  whole-minute values must divide 60, whole-hour values must divide 24, and
  the maximum is 12 hours. Do not retain release: prepare 2.0 stamp #200's current “through 23 hours”
  wording.
• Keep this PR's status/sampling-evidence bullet.
• Keep release: prepare 2.0 stamp #200's chore: retire pre-2.0 benchmark suite #187 credit while retaining the exact retired path
  sql/ash--1.0.sql.
• Keep release: prepare 2.0 stamp #200's release: clarify AAS cadence and coverage limits #186 credit while retaining the wording that the
  ash.timeline() catalog comment describes “no stored observation”; the
  wording does not create heartbeat storage.

#178 remains independent and is not merged or marked ready by this PR.

Verification

• devel/scripts/release_gate.sh 17 all: PASS on all seven surfaces
  (fresh-install, upgrade-chain, features, degraded-no-cron,
  degraded-no-pgss, degraded-neither, and cron-path).
• devel/scripts/test_ci_step_script.py: 7/7 PASS.
• devel/scripts/test_release_stamp.py: 17/17 PASS.
• git diff --check origin/main...HEAD: PASS.
• Current release: prepare 2.0 stamp #200 head d1971fd was rechecked for overlap immediately before
  push; its identity wording remains authoritative as described above.

Refs #157.

Inventory:

• Findings 1–11
• Findings 12–23
• Findings 24–34 and tail notes

[NikolayS]

VERIFIED — ready to land.

• Hand rebase: clean/no-op onto current main 87c22a3; the reviewed head remains dac20cf.
• Commit split: 97f22ac contains the actively misleading contract fixes and the complete development installer overlay; dac20cf contains the independently droppable terminology/catalog normalization. I checked the changes against the owner's #157 inventory and found no wording hunk to drop.
• Red/green on PostgreSQL 17: current main accepted ash.start('59 minutes'), so the new uniform-cadence assertion failed with 59-minute calendar cadence was not rejected; this head rejects it, exposes sampling_config_enabled / sampling_evidence, removes the misleading legacy status labels, and uses WARNING severity as specified.
• devel/scripts/release_gate.sh 17 all: all seven surfaces PASS (fresh-install, upgrade-chain, features, three degraded modes, and cron-path).
• devel/scripts/test_ci_step_script.py: 7/7 PASS.
• devel/scripts/test_release_stamp.py: 17/17 PASS.
• git diff --check origin/main...HEAD: PASS.

No hunks dropped. (GitHub does not permit the repository owner to approve their own PR, so this is recorded as a verification review rather than an APPROVE event.)

[NikolayS]

VERIFIED on rewritten head a437807 — this supersedes my review on the pre-edit head.

The two-commit split is preserved (428a4a7 actively misleading contracts; a437807 lower-severity catalog/history normalization). I found and corrected five factual copy defects before landing:

1. made the threshold pseudocode internally consistent (count(*) for both total and threshold, matching the shipped one-row-per-appearance CTE);
2. made the one-bucket 1m example satisfy avg = peak = p99;
3. qualified raw → rollup_1m → rollup_1h ordering to sources whose retention reaches the requested start;
4. stated that samples() is raw-only without a source column and chart() identifies source only for a widening rollup_1h plan;
5. corrected RAISE LOG routing to account for both server and client thresholds and disclosed the changed enable-message text in the 2.0 notes.

I also scoped sql/ash--1.0.sql as absent from the current 2.0 tree rather than nonexistent. No complete wording hunk was dropped.

Discriminating PostgreSQL 17 red/green reproduced on the rewritten head: current main accepts ash.start('59 minutes'), so the new assertion fails with 59-minute calendar cadence was not rejected; a437807 rejects it and passes the new status/severity contract.

Final verification:

• devel/scripts/release_gate.sh 17 all: 7/7 surfaces PASS.
• devel/scripts/test_ci_step_script.py: 7/7 PASS.
• devel/scripts/test_release_stamp.py: 17/17 PASS.
• git diff --check origin/main...HEAD: PASS.
• Hand rebase onto 87c22a3: clean; PR reports MERGEABLE/CLEAN.

Ready for hosted CI and squash merge.

[samo-agent]

APPROVE on rebased head 77b196f6620b8017f699df3f9fd353017d87d6cd.

Verification reproduced the discriminators rather than relying only on copy inspection: base lacks the uniform-cadence guards and fails the focused probe; this head enforces a 12-hour maximum, accepts 30m/12h, rejects 59m/5h without changing jobs/config, corrects hourly storage to 8,760 rows/year (about 43,800 over five years), and emits a real RAISE WARNING. The complete PostgreSQL 17 release gate passed all seven surfaces; CI-step tests are 7/7, release-stamp tests 17/17, and the diff is clean.

The two commits remain split between actively misleading contracts and cosmetic normalization. The rebased tree is byte-identical to the reviewed pre-rebase tree. No hunks were dropped.

[samo-agent]

Reopening immediately to retrigger the missing pull_request check suite on the verified rebased head; no code or review state is changing.

[samo-agent]

Codex review (standing in for REV) — verification complete; merge blocked on hosted CI.

Reviewed exact head f7a7c8ef66db2a9339481cd678fe14a775b0228c after a clean hand rebase onto origin/main@87c22a3. git range-diff shows both commits patch-identical to the previously reviewed head and the final tree is byte-identical. Codex found no actionable regression; PostgreSQL 17 passed all seven release-gate surfaces, the CI-step tests passed 7/7, the release-stamp tests passed 17/17, and git diff --check passed. The two-commit misleading/cosmetic split remains intact; no wording hunk was dropped.

Hosted CI link: not available — GitHub allocated no workflow run or check suite for this fork head after the synchronize push. Per the landing rules, I will not merge until an upstream maintainer approves/runs the PR workflow and the exact head is green.

[NikolayS]

Automated review gate + reviewer notes

samorev Code Review Report

• PR: docs: apply comprehensive 2.0 copy review #201 - docs: apply comprehensive 2.0 copy review
• Author: @NikolayS
• AI-Assisted: Yes

Pipeline	Coverage
none	Not reported

---

No issues found. Reviewed for security, bugs, tests, guidelines, and documentation.

Result: PASSED

---

Summary

Area	Findings	Potential	Filtered
CI/Pipeline	0	0	0
Security	0	0	0
Bugs	0	0	0
Tests	0	0	0
Guidelines	0	0	0
Docs	0	0	0
Metadata	0	0	0

Note:

• Findings: High-confidence issues (8-10/10) - blocking or non-blocking per severity
• Potential: Medium-confidence issues (4-7/10) - review manually
• Filtered: Low-confidence issues (0-3/10) - excluded as likely false positives

Review metadata

provider=github
kind=pr
project=NikolayS/pg_ash
number=201
target=github:NikolayS/pg_ash#201
state=OPEN
draft=false
diff_lines=10354
diff_added=8736
diff_removed=381
diff_bytes=452590
comments_count=2
commits_count=2
ci_status=none
ci_summary=total=0 success=0 failure=0 pending=0 other=0
prompt=.claude/commands/review-mr.md
blocking=true
samorev_review=1
review_round=1/10
escalated=false
posted_by=local
no_comment=true
live_posting=not-run
defender_dropped=4

Contested / dropped

• [bugs] The CI's "dangling upgrade-chain" prose in devel/sql/ash-install.sql still instructs a fresh install via \i sql/ash-install.sql, but the overlay lives at devel/sql/ash-install.sql. Anyone following the header of the overlay installs the frozen beta payload instead of the corrected one, and the release: prepare 2.0 stamp #200 promotion step described in the PR has to remember to not carry this path through. — The header is intentionally a byte-identical copy destined for promotion into sql/ash-install.sql by release: prepare 2.0 stamp #200 (per PR description and ci: define and enforce prerelease payload identity #179's SQL-chain checks); CI consumes the overlay via ash_sql_chain.py, not the header prose, and the header is correct at its final path.
• [bugs] ANSI RGB codes were changed to zero-padded parameters (\033[38;2;080;250;123m). ECMA-48 parses decimal parameters so most terminals accept it, but some strict parsers and copy-paste into languages with octal-literal interpretation (e.g. 080 in JS/Python-era code) misbehave, and the RGB column next to it is unpadded, so the row is now internally inconsistent. — The doc change deliberately mirrors the shipped ash._wait_color() implementation, which zero-pads every escape to a uniform 19 chars for pspg alignment; the octal-literal concern is inapplicable to ANSI parameter strings, so reverting would make the docs diverge from the code.
• [tests] The CI workflow now asserts specific README/RELEASE_NOTES phrases but several new CI grep strings are split across line boundaries in the Markdown source (e.g. Top query IDs additionally / read raw samples for eligible extreme minutes.), so the check is fragile — any reflow of the README paragraph silently breaks CI without a behavior change, and the split also means the assertion no longer proves the two fragments belong to the same sentence. — The CI already greps line-fragments throughout this block by established convention (e.g. the pre-existing cron.database_name split), the fragments are unique enough to guard the documented contract, and reflow-sensitivity is an accepted LOW-severity tradeoff of the whole grep-based doc check rather than a defect introduced here.
• [docs] The PR merges documentation corrections with a behavioral change (scheduler cadence validation: whole-minute values must divide 60, whole-hour values divide 24, max 12 hours) and a renamed ash.status() metric (sampling_enabled → sampling_config_enabled, missed_samples → interrupted_sample_calls). Scripts that read the old metric names (as the demo scripts did before this PR) will silently get no rows. The diff shown does not include a release-note bullet for the metric rename (content trimmed), and the PR title docs: undersells a user-visible breaking change. — The diff already adds a RELEASE_NOTES.md bullet explicitly documenting the status metric rename (interrupted_sample_calls names the former missed_samples counter, sampling_config_enabled) and a separate bullet for the new cadence restrictions under a header now reading "This is a breaking release"; the PR description also states the behavioral change up front, so the only residual ask is retitling a PR, which is a non-actionable nitpick.

---

---

Reviewer notes

Reviewed at head f7a7c8ef. Do not trust the PASS above. It is a false negative and it is worth explaining why before anything else.

The gate passed for the wrong reason

The gate keys on CI status and draft state. This PR reports ci_status=none with total=0 — no CI has ever run on it — and "no checks at all" is not treated as a blocker, while draft=false clears the other condition. So the PASS means "nothing was checked", not "checks passed".

For contrast, the same gate raises a CRITICAL blocker on PRs whose full PostgreSQL 14–19 matrix is green, because two runs conclude skipped/neutral rather than success. Green CI fails the gate; absent CI passes it. Read the PASS accordingly.

The absent CI is not incidental here: this PR breaks a CI job, and no run exists to reveal it. See below.

Blocking: this PR breaks docs-lint

README.md:358 is reworded from

in raw samples or `ash.rollup_1h`, it returns SQL `NULL` and emits a NOTICE

to

`ash.report()` returns SQL `NULL` and emits a NOTICE naming that alternate

but the docs guard in .github/workflows/test.yml:119 still asserts the old wording verbatim:

grep -Fq 'it returns SQL `NULL` and emits a NOTICE' README.md

The step runs under set -euo pipefail with no if: condition, so grep -Fq returning 1 fails the Guard docs against stale privilege and reader names step and the whole docs-lint job. Verified both directions: the pattern matches on origin/main (guard passes today) and does not match on this head. The rewording is an improvement — naming ash.report() beats a dangling "it" — so the fix is to update the guard string alongside the prose, not to revert the sentence.

Every other guard grep in that step still matches.

Blocking: the title says docs, but the payload changes behavior

devel/sql/ash-install.sql is added here as a 7,924-line file that does not exist on origin/main at all (main's devel/sql/ holds only README.md). It is not a copy of the frozen installer — it carries real, user-visible behavior changes:

• ash.start() cadence contract narrowed. Maximum drops from 23h to 12h, and whole-minute/whole-hour intervals must now divide 60/24 — so 59 minutes and 5 hours are rejected where they were previously accepted. Confirmed against a live instance.
• ash.status() metric renames — breaking for anything scraping them: sampling_enabled → sampling_config_enabled, missed_samples → interrupted_sample_calls, last_sample_ts → last_activity_sample_ts, time_since_last_sample → time_since_last_activity_sample, plus a new sampling_evidence.
• Severity changes: the non-superuser warning goes NOTICE → WARNING; the rollup_minute gap message goes WARNING → NOTICE.

These may all be desirable. The objection is that a PR titled "apply comprehensive 2.0 copy review" is the wrong vehicle for a breaking rename of the observability surface, and reviewers reading the title will not look for them. They need explicit sign-off on their own merits.

This also has a knock-on effect the description should state: #204 adds the same devel/sql/ash-install.sql from scratch, so these two PRs are a guaranteed add/add conflict and their landing order is not arbitrary.

Prose that is still inaccurate

1. README.md:77 — "must use the de-prefixed 2.0 parameter names — for example, ash.start(every => ...), not p_every => ...". p_every never existed. The 1.x signature was ash.start(p_interval interval default '1 second'). blueprints/SPEC.md in this very PR gets it right and says p_interval. Separately, p_interval → every is a rename, not a de-prefixing, so the example contradicts the rule it illustrates.

2. Systemic, README.md:37 and :372 — the README tells users to install \i sql/ash-install.sql, then documents the divisor cadence rule, the 12h maximum, the new ash.status() metric names, and the new degraded-mode text. None of that is true of sql/ash-install.sql, which still has the 23h maximum, sampling_enabled, missed_samples, and the old wording. The same applies to the 2.0 highlights in RELEASE_NOTES.md:76-91, which present unreleased development behavior as shipped. A copy review that makes the docs describe a payload users cannot install trades one inaccuracy for another.

3. blueprints/SPEC.md:8, ROLLUP_DESIGN.md:8, PARTITIONED_QUERYMAP_DESIGN.md:8, CONFIGURABLE_PARTITIONS_AND_ROLLUP.md:8 — the new banners send readers to "the catalog comments in sql/ash-install.sql for current behavior". That file still contains the exact comments this PR identifies as wrong, including "RAISE LOG goes to server log only, never to the client". Point them at devel/sql/ or drop the file reference.

4. blueprints/AAS_EXAMPLES.md:27, AAS_USER_STORIES.md:162 (and the matching README/AAS_API wording) — "chart emits a source/effective-plan NOTICE only when rollup_1h widens its plan". The guard is just if v_source = 'rollup_1h' (devel/sql/ash-install.sql:6865-6871), so the NOTICE fires on every rollup_1h chart read, widening or not. The word "only" is the problem.

5. demos/ash_demo.cast:135-136 — the recording was hand-edited rather than re-recorded. Real psql renders │ metric │ (9 spaces left, 10 right); the cast now has │ metric │. It also still shows version | 2.0 while ash.config.version is 2.0-beta1. Per this repo's own demo rules this needs make -C demos record, not a manual patch — and hand-editing a cast is exactly the class of drift the demo harness exists to catch.

6. Nit, blueprints/AAS_EXAMPLES.md:64-72 — the 1w row was correctly changed rollup_1h → rollup_1m, but the 1mo row still shows rollup_1h with bucket 00:01:00/43200. With the default 30-day rollup_1m retention those two windows start at nearly the same boundary, so the row is only right exactly at the edge.

Merge conflicts

Narrow and tractable. Only three paths conflict, all modify/delete: demos/ash_demo.cast, demos/container-entrypoint.sh, demos/record.sh were deleted on main by the demo-harness rework and modified here. Everything else auto-merges. Given item 5, taking main's deletions is very likely the right resolution.

What is good

Credit where it is due — most of this review is real and lands. It corrects claims that were genuinely false: peak/p99 reported on a one-bucket window, rollup_1m cited where the planner reads raw, internally inconsistent backend_seconds/pct arithmetic, "report reads rollup_1m only", "100% test coverage", "CPU = green is universal", and the plainly wrong "RAISE LOG never reaches the client". The \gset and copy … to stdout \g file recipes now actually run; the previous multi-line \copy could not have.

No assertion weakening anywhere. I checked specifically. Every touched assertion is equal or stricter: devel/tests/degraded_no_cron.sql:10-15 moves from like '%schedule externally%' to exact cadence text; features_readers.sql:1308-1317 adds negative assertions that the old metric names are gone; features_lifecycle.sql keeps exact-string equality; new CI blocks assert that rejected cadences leave sample_interval and cron jobs untouched. Coverage of 59 minutes / 23 hours as accepted values disappears only because they are now rejected, and the rejection is asserted. The upgrade-chain split rewrite is sound: ash_sql_chain.py yields 8 lines and reapply-chain yields 1, so latest_upgrade_lines=2 correctly isolates ash-1.5-to-2.0.sql plus the devel overlay.

Verdict

Substantially accurate copy review, not mergeable as it stands.

Must fix before it can land:

• the test.yml:119 guard string, which this PR breaks;
• the p_every claim in README.md:77;
• the "only when rollup_1h widens" overclaim;
• the blueprint banners pointing at the stale frozen installer;
• the hand-edited asciicast — re-record it.

Needs an explicit decision, not a fix:

• whether the ash.start() cadence narrowing, the ash.status() metric renames, and the severity changes should ride along in this PR at all, or be split out where they can be reviewed as the breaking changes they are;
• the landing order against fix: make lifecycle job teardown atomic #204, which adds the same file.

I could not push these fixes myself — this head is on a fork with maintainer edits disabled.

[NikolayS]

Merge with current main: the conflict is small, the consequence is not

Follow-up to my review above. I attempted git merge origin/main into this branch locally (I cannot push it — this head is on a fork with maintainer edits disabled, so this is analysis, not a proposed commit).

The mechanical conflict

Only three paths conflict, all modify/delete:

CONFLICT (modify/delete): demos/ash_demo.cast deleted in origin/main and modified in HEAD.
CONFLICT (modify/delete): demos/container-entrypoint.sh deleted in origin/main and modified in HEAD.
CONFLICT (modify/delete): demos/record.sh deleted in origin/main and modified in HEAD.

Everything else auto-merges, including README.md. main deleted those three files in the demo-harness rework and replaced them with demos/{Makefile,bin,lib,render,scenes,theme,fixtures}.

Why "take main's deletion" is not the whole answer

This PR's changes to all three deleted files are exclusively the propagation of one rename it ships in the installer: sampling_enabled → sampling_config_enabled. For example, in demos/record.sh:

-  $PSQL -d demo -c "select metric, value from ash.status() where metric in ('version','sampling_enabled','pg_cron_available')"
+  $PSQL -d demo -c "select metric, value from ash.status() where metric in ('version','sampling_config_enabled','pg_cron_available')"

Discarding those files discards the propagation — but main's replacement harness still uses the old metric name, in four places that matter:

demos/scenes/scenes.tsv:35   select ... from ash.status() where metric in ('version','sampling_enabled', ...)
demos/fixtures/shape.tsv:1   labels=pg_cron_available,raw_retention,sample_interval,samples_total,sampling_enabled,version
demos/fixtures/status.ansi:1,8
demos/fixtures/expected/status.svg

So a naive resolution produces a tree that builds but is wrong: the status scene would query a metric that no longer exists and capture five rows where the frozen contract expects six.

Which gate catches it, and which does not

Worth being precise, because the two demo jobs check different things:

• demo-render re-renders the committed demos/fixtures/*.ansi and byte-compares against expected/*.svg. Frozen input on both sides, so it would still pass — it cannot see this.
• capture + reel (real PostgreSQL) seeds a real PostgreSQL, re-captures every scene, and runs make -C demos rot against fixtures/shape.tsv. That is the job that fails, because the fresh capture's label set no longer matches the recorded contract. It runs on every pull_request, so it will fire on this PR once CI runs at all.

That is exactly the drift the harness was built to catch, and it is catching a real one here.

Scope check on the rename

I checked whether the rename reaches the ash.config columns as well, since demos/lib/seed.sql:216,221 writes sampling_enabled and missed_samples directly. It does not — this PR renames only the ash.status() metric labels (devel/sql/ash-install.sql:2899, 2915); the config columns are unchanged (:265, :267). So demos/lib/seed.sql is unaffected and needs no edit.

What the resolution actually requires

1. Accept main's deletion of the three old demo files.
2. Propagate sampling_enabled → sampling_config_enabled into demos/scenes/scenes.tsv.
3. Re-record the fixtures with make -C demos record so shape.tsv, status.ansi and expected/status.svg match the new label set.

Step 3 is not optional and not hand-editable — which is the same point as finding 5 in my review above, where demos/ash_demo.cast had been hand-patched rather than re-recorded. After merging main the correct target is the new harness, not the retired cast.

I have not applied any of this. Per the conflict being non-trivial in consequence rather than in mechanics, I am reporting it instead of guessing at the intended resolution.