Add REVIEW.md and lint shell scripts, Dockerfiles and workflows in CI

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,17 +1,41 @@
 ## Summary
 
-<!-- What does this change and why? -->
+<!-- What does this change and why? Link the issue that it resolves. -->
+
+Closes #
 
 ## Checklist
 
-- [ ] `./run-ci-local.sh` passes (gofmt, golangci-lint, build, pg_regress,
-  journey)
-- [ ] Tests added/updated (test-first)
-- [ ] Docs updated where applicable (README / USAGE / INSTALL / ARCHITECTURE)
-- [ ] Bakery / mesh / distributed changes: TLA+ model updated and TLC
-  re-checked (`docs/formal/`)
-- [ ] Commit messages are short and imperative
+- [ ] The pull request links a tracking issue.
+- [ ] `./run-ci-local.sh` passes (gofmt, golangci-lint, shellcheck, hadolint,
+  actionlint, build, pg_regress, journey).
+- [ ] The change includes unit tests, written before the code, or the Testing
+  section explains why it has none.
+- [ ] The change includes regression tests in `extension/coldfront/test/`, or
+  the Testing section explains why it has none.
+- [ ] The Testing section notes any impact on the integration tests
+  (`ci/journey.sh` and the CI matrix).
+- [ ] The change updates `README.md` and `docs/`, or the Notes for Reviewers
+  explain why it needs no update.
+- [ ] The change adds no secrets, and the Risk section covers its security
+  impact.
+- [ ] A bakery, mesh, or distributed change updates the TLA+ model and re-runs
+  TLC (`docs/formal/`).
+- [ ] Author's AI review completed and findings addressed.
+- [ ] Commit messages are short and imperative.
 
-## Related issues
+## Testing
 
-Closes #
+<!-- How was the change tested? -->
+
+## Risk
+
+<!-- What could go wrong, and what would the automated checks not catch? -->
+
+## How to Revert
+
+<!-- How can the change be undone if needed? -->
+
+## Notes for Reviewers
+
+<!-- Known limitations, design rationale, or follow-up tasks. -->

.github/workflows/ci-walkthrough.yml

@@ -1,6 +1,7 @@
 name: CI - Walkthrough
 on:
   push:
+    branches: [main]
     paths: ['examples/walkthrough/**', '.devcontainer/**', 'docs/walkthrough*.md', '.github/workflows/ci-walkthrough.yml']
   pull_request:
     paths: ['examples/walkthrough/**', '.devcontainer/**', 'docs/walkthrough*.md', '.github/workflows/ci-walkthrough.yml']
@@ -20,8 +21,6 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v7
-      - name: Run ShellCheck
-        run: shellcheck -x --source-path=examples/walkthrough examples/walkthrough/*.sh
       - name: Validate Docker Compose
         run: |
           docker compose -f examples/walkthrough/docker-compose.yml config --quiet

.github/workflows/ci.yml

@@ -4,7 +4,8 @@ name: CI
 # steps never diverge from local. Two jobs:
 #
 #   quick — every push + PR. ci/matrix.sh --quick = preflight (gofmt/vet/lint/
-#           unit tests/build) + ONE hermetic cell (PG18 · vanilla · tiered ·
+#           shellcheck/hadolint/actionlint/unit tests/build) + ONE hermetic
+#           cell (PG18 · vanilla · tiered ·
 #           primary · s3) on SeaweedFS + the pg_regress unit layer. No cloud
 #           creds, no real cloud calls. This is the fast gate and is exactly
 #           what run-ci-local.sh runs.

.github/workflows/release.yml

@@ -25,7 +25,8 @@
 #   3. Push RPMs to dnf.pgedge.com and DEBs to apt.pgedge.com, back up to S3,
 #      and publish a promotion manifest + Slack notification.
 #
-# common-functions.sh is supplied by pgedge-builder-action at build time.
+# pgedge-builder-action runs common/build.sh, which sources the committed copy
+# of the shared build helpers in common/common-functions.sh.
 # -------------------------------------------------------------------------
 
 name: Release

.hadolint.yaml

@@ -0,0 +1,10 @@
+# Rules the current Dockerfiles break. docker/Dockerfile.duckdb15-base is the
+# base image recipe, so any edit to it, an inline ignore comment included,
+# needs a base image republish.
+failure-threshold: warning
+ignored:
+  - DL3002  # Build stages run as root, and each final stage switches to postgres.
+  - DL3003  # A build step changes into a cloned source tree within one RUN.
+  - DL3006  # The manylinux stage aliases pull untagged images.
+  - DL3029  # The manylinux stage aliases pick one image per architecture.
+  - DL3041  # dnf installs packages without a pinned version.

CLAUDE.md

@@ -16,7 +16,7 @@ Every change keeps all five. A change that cannot is raised before it is made, n
 - Every public function should have a unit test
 - Never rig a test: never change its timing, ordering or inputs so it stops meeting the case where it fails. A failing test gets a product fix. An assertion is weakened only to a guarantee agreed beforehand, and the change says it is weaker. Before changing a test alongside a fix, confirm the changed test still fails on the unfixed code in every interleaving and input it can meet.
 - Run `make test` after every change
-- MUST run `./run-ci-local.sh` before every commit — it runs gofmt, golangci-lint, tests, build, Docker integration tests
+- MUST run `./run-ci-local.sh` before every commit — it runs gofmt, golangci-lint, shellcheck (`-S warning`), hadolint, actionlint, tests, build, Docker integration tests
 - GitHub Actions CI must always be identical in steps to `run-ci-local.sh` — never let them diverge
 - MUST verify any mesh/distributed/bakery change against the TLA+ model before committing it. Scope (the cold-write serialization protocol): the bakery functions in `extension/coldfront/coldfront--1.0.sql` (`_claim_iceberg_lock`, `_insert_claim`, `_take_iceberg_claim`, `_exec_iceberg_with_claim`, `_enqueue_release`, `_on_claim_apply`, `_on_claim_release`, `ensure_replicated`), the C `XactCallback` in `extension/coldfront/src/coldfront.c` (`coldfront_xact_callback`, which releases the claim at `XACT_EVENT_COMMIT`, after pg_duckdb has committed the Iceberg transaction at `XACT_EVENT_PRE_COMMIT`), the `coldfront.iceberg_async_parquet` ordering, and the spock/`synchronous_*` GUCs gating claim replication. The one question: does the change alter the modeled protocol (claimant set, claim/conditional-update/release ordering, async-vs-stock parquet ordering, defer/drain steps)? If yes, model it first: update `docs/formal/Bakery.tla`, re-translate (`pcal.trans`), and re-run every TLC config (safe configs report "No error has been found"; `Bakery_race.cfg` still violates `NoLakekeeperConflict`, and `Bakery_adopt_race.cfg` still violates `NoDoubleRegistration`). If no (a pure refactor whose emitted claim/lock/release sequence is byte-identical, leaving the `.tla`/`.cfg` untouched), that equivalence argument IS the proof: do NOT re-run TLC, since an unchanged model only reprints the same verdicts. See `docs/formal/README.md`.
 - Update README.md as you implement functionality; update ARCHITECTURE.md in the same commit as the structural change it describes

CONTRIBUTING.md

@@ -14,9 +14,9 @@ Thanks for your interest in improving ColdFront.
 - **Test-driven.** Write the test first, then the implementation. Prioritise
   smoke / integration / endpoint tests over unit tests.
 - **Run the gate before every commit:** `./run-ci-local.sh` — gofmt,
-  golangci-lint, unit tests, build, the pg_regress layer, and the canonical
-  user journey. GitHub Actions runs the identical `ci/matrix.sh` harness, so
-  local and CI never diverge.
+  golangci-lint, shellcheck, hadolint, actionlint, unit tests, build, the
+  pg_regress layer, and the canonical user journey. GitHub Actions runs the
+  identical `ci/matrix.sh` harness, so local and CI never diverge.
 - **Bakery / mesh / distributed changes** must be modelled and verified in the
   TLA+ spec **first** ([docs/formal/](docs/formal/)) before the code change
   lands.
@@ -30,6 +30,40 @@ Thanks for your interest in improving ColdFront.
 - Open the PR against `main` and complete the
   [pull-request checklist](.github/PULL_REQUEST_TEMPLATE.md).
 
+## Reviewing Pull Requests
+
+Every change to `main` goes through a pull request. A pull request merges into
+`main` when the pull request meets these requirements:
+
+- The description links the GitHub issue that the change resolves.
+- CI passes, including the tests and the linters described in
+  [Checking Code Style](#checking-code-style).
+- CodeRabbit and Codacy have reviewed the pull request, and the author has
+  fixed each finding or answered each finding on the pull request with a
+  reason.
+- A code owner listed in [.github/CODEOWNERS](.github/CODEOWNERS), other than
+  the author, has approved the latest commit.
+
+[REVIEW.md](REVIEW.md) describes what a defect looks like in ColdFront and
+what the reviewer checks.
+
+A code owner may merge their own pull request without a second approval when
+the change alters no code or behaviour, such as a version bump, a changelog
+cleanup, or a typo fix in a comment or a document. In the CI workflows under
+`.github/workflows/` and in the release and packaging scripts, such a pull
+request may change only version numbers. CI and the automated reviews must
+still pass, and the author adds a comment to the pull request that explains why
+the pull request merged without review.
+
+## Checking Code Style
+
+CI checks Go code with gofmt, go vet, and golangci-lint, the shell scripts
+with shellcheck, the Dockerfiles with hadolint, and the GitHub workflows with
+actionlint. CI also builds the documentation with `mkdocs build --strict` and
+scans the commit history for secrets with gitleaks. On each pull request,
+CodeRabbit runs shellcheck, hadolint, yamllint, markdownlint, and semgrep, and
+Codacy runs its static analysis.
+
 ## Versioning
 
 ColdFront uses two independent version numbers. Release tags follow three-part

REVIEW.md

@@ -0,0 +1,135 @@
+# Reviewing Changes to ColdFront
+
+This file describes what a defect looks like in ColdFront, which paths the
+automated review tools skip, and what a reviewer checks in every pull request.
+The author's AI review, CodeRabbit, and the approving reviewer all work from
+this file.
+
+## Recognizing a Failure
+
+ColdFront runs inside the PostgreSQL server process and moves data between
+PostgreSQL and Apache Iceberg, so a defect can lose data or crash the server.
+A change fails review when the change can cause any of the following:
+
+- a row that the archiver, a tiered `INSERT`, or the partitioner loses,
+  duplicates, or alters as the row moves between partitions or between the hot
+  and cold tiers.
+- a cold read that returns rows or values that differ from what PostgreSQL
+  returns for the same data.
+- two cold writes that commit to one Iceberg table out of turn, or two nodes
+  that register the same Iceberg table.
+- a crash or memory error in `extension/coldfront/src/coldfront.c`, which
+  terminates the backend and makes PostgreSQL end every other session while
+  the server recovers from the crash.
+- a storage credential or catalog token that appears in a log, an error
+  message, a query text, or a committed file.
+- a compaction or a cold `DROP` that deletes a data file that a live snapshot
+  or an open transaction still reads.
+
+## Skipping Paths in Automated Review
+
+CodeRabbit reviews every tracked file. Codacy skips files that do not ship or
+that another check covers, as the `.codacy.yaml` file lists, and a change to
+that file updates this section to match.
+
+The following table describes the paths that Codacy skips:
+
+| Path | Reason |
+|---|---|
+| `.claude/**` and `CLAUDE.md` | These files instruct coding agents and do not ship. |
+| `config.example.yaml` | This file is an example configuration. |
+| `THIRD_PARTY_NOTICES.md` | This file is generated license text. |
+| `docker/*.patch` | These patches change duckdb-iceberg, and CodeRabbit and the approving reviewer read them. |
+| `docs/formal/**` | The TLC model checker checks the TLA+ model in this directory. |
+| `extension/coldfront/test/**` | These files are pg_regress tests and their expected output. |
+| `**/*_test.go` | These files are Go tests, which do not ship. |
+
+## Checking a Change
+
+A reviewer checks the following points in addition to the checklist in the
+pull request template. The linters cover formatting, so the review omits
+findings about formatting alone.
+
+### Checking Cold Writes and the Bakery
+
+The bakery protocol serializes every cold write through the
+`coldfront.claims` table. A change to cold writes follows these rules:
+
+- A change to the bakery functions in the extension SQL, the commit callback
+  in `coldfront.c`, the `coldfront.iceberg_async_parquet` ordering, or the
+  Spock settings that replicate claims updates `docs/formal/Bakery.tla` and
+  re-runs every TLC configuration. A pure refactor instead explains in the
+  pull request why the claim, commit, and release sequence is unchanged.
+  [docs/formal/README.md](docs/formal/README.md) describes the model and every
+  configuration.
+- A cold write runs outside any subtransaction, such as a `SAVEPOINT` or a
+  PL/pgSQL `EXCEPTION` block, because pg_duckdb refuses every cold write inside
+  one.
+- A cold write commits to the catalog without a retry, because the bakery
+  prevents commit conflicts.
+
+### Checking Generated SQL
+
+The query rewrite emits SQL that DuckDB or a second session runs, and that SQL
+must read the same in every session. Generated SQL follows these rules:
+
+- Every relation, function, and operator reference is schema-qualified, so
+  the code does not rely on `search_path`.
+- Every value that leaves the session as text goes through
+  `coldfront._canonical_text`.
+- A change to the shape of a rendered qual or `INSERT` regenerates every
+  pg_regress expected file that echoes the statement, including the `_1.out`
+  variants.
+
+### Checking Component Boundaries
+
+Each component keeps a narrow set of dependencies. A change respects these
+boundaries:
+
+- SQL and the C extension make HTTP calls only through the catalog client in
+  duckdb-iceberg, and only the Go programs call the Lakekeeper management API.
+- The archiver orchestrates plain parameterized SQL through pgx and uses no
+  DuckDB, Iceberg, or Arrow Go libraries.
+- The compactor in `cmd/compactor/` stays a separate Go module, so its
+  dependencies stay out of the archiver.
+
+### Checking Tests and Dependencies
+
+Tests prove the behaviour that a change claims, and a dependency workaround
+proves that it is still needed. A change meets these conditions:
+
+- A bug fix includes a test that fails on the unfixed code.
+- A test changed alongside a fix still fails on the unfixed code in every
+  interleaving and input that the test can meet.
+- A workaround for a dependency names the dependency and the pinned version
+  that shows the defect, and includes a test that runs the dependency without
+  the workaround and fails once the defect is gone.
+- A change to the base image recipe (`docker/Dockerfile.duckdb15-base` and the
+  files that it copies) passes CI only after `base-image.yml` publishes the
+  base image under the tag that `ci/base-ref.sh` prints.
+
+### Checking Documentation
+
+The documentation describes the current behaviour. A change keeps the
+documentation consistent in these ways:
+
+- `README.md` and `docs/index.md` change together where they share the
+  introduction.
+- A new, renamed, or removed guide in `docs/` updates the `mkdocs.yml`
+  navigation and the `README.md` table of contents in the same commit.
+- `mkdocs build --strict` passes with no warnings.
+
+## Reporting Findings
+
+Every reviewer reports findings by these rules, so that the reviews of a pull
+request converge:
+
+- Label each finding as Critical, Major, or Minor. A Critical finding can cause
+  a failure listed in [Recognizing a Failure](#recognizing-a-failure). A Major
+  finding causes an error or a wrong result on a narrower path. A Minor finding
+  is any other defect.
+- State whether the finding concerns code that the pull request changes or code
+  that existed before the pull request.
+- Omit findings that concern style alone.
+- On a later review of the same pull request, review only the new commits, and
+  do not repeat an earlier finding.

ci/journey.sh

@@ -1849,8 +1849,7 @@ EOSQL
     # Two vector columns on one table. Each gets its own cluster column and its own
     # assignment; only the first gets the file sort order, so only its probe prunes.
     # Its own table, so nothing above depends on the second column existing.
-    local i2
-    for i2 in 1 2 3 4 5; do
+    for _ in 1 2 3 4 5; do
         q_may "$HOST" "SELECT coldfront.create_iceberg_table('public','icetwo','[{\"name\":\"id\",\"type\":\"bigint\"},{\"name\":\"ts\",\"type\":\"timestamptz\"},{\"name\":\"embedding\",\"type\":\"vector(3)\"},{\"name\":\"summary\",\"type\":\"vector(2)\"}]'::jsonb);" >/dev/null 2>&1
         [ "$(q "$HOST" "SELECT count(*) FROM pg_class WHERE relname='icetwo' AND relkind='v';")" = "1" ] && break
         sleep 2
@@ -8227,9 +8226,9 @@ story_vector_probe_pruning() {
     # probed read has two ICEBERG_SCAN nodes, the probe arm first and the
     # unassigned arm second, each read on its own (strict jsonpath: lax .**
     # yields every array element twice).
-    local counters s1 t1 s2 t2
+    local counters s1 t1 t2
     counters=$(q "$HOST" "SELECT duckdb.raw_query('SET custom_profiling_settings = ''{\"OPERATOR_TYPE\": \"true\", \"OPERATOR_ROW_GROUPS_SCANNED\": \"true\", \"OPERATOR_TOTAL_ROW_GROUPS_TO_SCAN\": \"true\"}'''); SELECT duckdb.raw_query('SET enable_profiling = ''json'''); SELECT duckdb.raw_query('SET profiling_output = ''/tmp/tc193.json'''); SELECT id FROM public.tcvp ORDER BY embedding <=> ARRAY[100,100,100]::real[] LIMIT 1; SELECT duckdb.raw_query('SET enable_profiling = ''no_output'''); SELECT string_agg((j->>'operator_row_groups_scanned') || ' ' || (j->>'operator_total_row_groups_to_scan'), ' ' ORDER BY o) FROM jsonb_path_query(pg_read_file('/tmp/tc193.json')::jsonb, 'strict \$.** ? (@.operator_name == \"ICEBERG_SCAN\")') WITH ORDINALITY AS t(j, o);" | tail -1)
-    read -r s1 t1 s2 t2 <<< "$counters"
+    read -r s1 t1 _ t2 <<< "$counters"
     assert_gt "TC-193: the file holds more than one row group" 1 "$t1"
     assert_gt "TC-193: the probe arm read at least one row group" 0 "$s1"
     assert_gt "TC-193: the probe arm skipped the row groups of the cluster it did not visit" "$s1" "$t1"