cluade-reviewer-andresmgsl
634e7a3528
All checks were successful
CI / test (pull_request) Successful in 3m4s
CI / release-exercise (pull_request) Successful in 11s
CI / self-guards (pull_request) Successful in 6s
CI / action-exercise (pull_request) Successful in 6s
CI / docs-sync-exercise (pull_request) Successful in 6s
Refs guard / refs-not-closing (pull_request) Has been skipped
labels / labels (pull_request) Successful in 8s
@codex-reviewer-andresmgsl's five points. Three were correctness, and one of
them found that my must-fail cases could not fail.
1. THE OBJECT IS MANDATORY. UNVERIFIABLE-HERE is gone: a missing ref, an absent
object and a non-ancestor are three distinct refusals. The ref is now the
FULL 40-character SHA, and ci.yml fetches exactly that object before the
suite. "Runs offline" means the TEST reads local evidence; it never meant CI
may omit the evidence and pass.
2. THE SCAN COVERS WHAT THE INVENTORY CLAIMS. It walked shell under four globs
and never looked at workflows, .github/labels.conf or drills/ — three
categories the inventory governs. Widened, and it immediately found four
real blind spots on merged main: refs-not-closing's declaration,
labels.yml's inline forge decision, refs-guard.yml's GitHub-only scheduling
and release-exercise.yml's pinned CEREMONY_FORGE. All four are now inventory
entries with the issue that removes them, because a delta location with no
exit is indistinguishable from one nobody noticed. A file that DECLARES a
client is no longer exempt as a "consumer" — only files that merely CALL the
shim are.
3. THE TEETH NOW DRIVE THE GUARD. They asserted the predicates separately and
never invoked no_unlisted, so the guard could have been `return 0` and both
must-fail rows would still have passed. SCAN_ROOT is a parameter now and the
cases build a tree, add an unlisted decider — shell AND workflow, so
coverage cannot regress to the old glob — and assert the real top-level
check fails naming it. Replacing no_unlisted with `return 0` reds five.
4. PATH MATCHING, NOT PREFIX MATCHING. `drills/` accepted `drills-old/x` and
`lib/forge.sh` accepted `lib/forge.sh.backup`. Exact for files, `dir/` for
directories, with both negative boundaries covered.
5. THE IMMUTABLE SHA IS CAPTURED AT FETCH. The runbook now takes
upstream_sha=$(git rev-parse gh/main) once and merges and records that
value. This is not hypothetical: while this PR was in review upstream moved
from 8c3a4d1 to 08e2912, and re-reading gh/main at recording time would have
written a commit this tree does not contain. I caught that by walking into
it.
test/run.sh 29/29; upstream-delta 21/21; shellcheck 0.10.0, actionlint,
changelog-armed clean.
Refs #200
252 lines
12 KiB
Markdown
252 lines
12 KiB
Markdown
# Syncing this tree with upstream ceremony
|
|
|
|
`heavy-duty/ceremony` exists on two forges and they diverge in opposite
|
|
directions on purpose:
|
|
|
|
- **upstream** — `github.com/heavy-duty/ceremony`, where new ceremony features
|
|
are written. **Read-only from here.** No issue, PR, comment, review or
|
|
release is ever created there.
|
|
- **this tree** — `forgejo.heavyduty.builders/heavy-duty/ceremony`, which
|
|
carries upstream's content plus the forge-compatibility delta and never
|
|
writes back.
|
|
|
|
This document is the procedure for bringing upstream's work across. It is
|
|
written to be followed without prior context; where it states a resolution, the
|
|
resolution is standing and does not get re-decided each sync.
|
|
|
|
Worked example throughout: the `0.6.0` sync (#197, #198), which merged upstream
|
|
`8c3a4d1` onto `dad99dd` and took four heads to get green.
|
|
|
|
## The standing resolutions
|
|
|
|
These recur every sync. They are decided; re-deciding them is the cost this
|
|
list exists to remove.
|
|
|
|
| what | which side wins | decided by |
|
|
|---|---|---|
|
|
| `VERSION` | **upstream** — this tree tracks upstream's version numbers | #197 D2 |
|
|
| `CEREMONY_SELF_REF` (both carriers) | **upstream** | #197 D2 |
|
|
| `.github/labels.conf` | **this tree** — upstream's roster names identities that do not exist here | #195 |
|
|
| `drills/*.md` | **this tree** — a drill record is a record of a run *here* | #198 |
|
|
| `CHANGELOG.md` | **both**, upstream's new sections above this tree's | #198 |
|
|
| a section for a version **both** trees released | **this tree's** — ours is the published body of the tag that exists here | #198 |
|
|
|
|
Two consequences worth stating plainly:
|
|
|
|
- **Two trees answer to the same version number**, differing by the forge
|
|
delta. That is accepted, not accidental (#197 D2). The mitigation is
|
|
provenance in prose: `CHANGELOG.md`'s header names the upstream commit this
|
|
tree carries, and `.upstream-ref` records it in machine-readable form.
|
|
- **A tag that exists upstream may not exist here.** `CEREMONY_SELF_REF` takes
|
|
upstream's number, and both workflows carry the self-consumption bypass
|
|
(`if: github.repository != 'heavy-duty/ceremony'`), so ceremony's own CI is
|
|
unaffected. But **no consumer may bump its pin to that number until the
|
|
release ceremony cuts the tag here.**
|
|
|
|
## The procedure
|
|
|
|
### 1. Add the upstream remote, read-only, and confirm the merge base
|
|
|
|
```sh
|
|
git remote add gh https://github.com/heavy-duty/ceremony.git # if absent
|
|
git fetch gh
|
|
upstream_sha="$(git rev-parse gh/main)" # capture ONCE, in full
|
|
git merge-base main "$upstream_sha"
|
|
```
|
|
|
|
**Capture the full SHA immediately and use that value everywhere after** — the
|
|
merge, the provenance, the `.upstream-ref` write. `gh/main` is a moving
|
|
pointer: while this sync was being reviewed upstream advanced from `8c3a4d1`
|
|
to `08e2912`, and re-reading `gh/main` at recording time would have written a
|
|
commit this tree does not contain. The recorded ref is *what was merged*, never
|
|
*what upstream is now*.
|
|
|
|
**Confirm the merge base against `.upstream-ref` before merging anything.** If
|
|
it is not what the last sync recorded, something moved — stop and re-measure
|
|
rather than proceeding. A sync that starts from an unexpected base is a sync
|
|
whose conflict count means nothing.
|
|
|
|
### 2. Merge, never rebase
|
|
|
|
```sh
|
|
git merge "$upstream_sha"
|
|
```
|
|
|
|
One merge commit, conflicts resolved once (#197 D1). Rebasing the forge-only
|
|
commits onto upstream would rewrite every SHA, re-resolve the same conflicts
|
|
once per commit, and break any pin to them. A fresh re-import would discard the
|
|
provenance in this repo's issue comments, which is where its documentation
|
|
actually lives.
|
|
|
|
### 3. Resolve the conflicts
|
|
|
|
Apply the standing resolutions above. What is left is genuinely new and needs
|
|
judgement — in the `0.6.0` sync that was 5 hunks of 18.
|
|
|
|
### 4. Audit what the merge brought in that did NOT conflict
|
|
|
|
**This is the step the `0.6.0` sync nearly shipped without, and the one this
|
|
document exists for.**
|
|
|
|
`git merge` takes upstream's side wherever only upstream moved a region. So a
|
|
function upstream *added* to a file this tree already owns arrives with **no
|
|
conflict and no question asked**. Reviewing the conflict hunks cannot find
|
|
them: four reviewers read the same diff and each found a different subset.
|
|
|
|
In the `0.6.0` sync that was **eight** runtime `gh` call sites, in three files
|
|
and two file types, every one of which #188 had previously removed.
|
|
|
|
So, after resolving:
|
|
|
|
```sh
|
|
bash test/no-runtime-gh.test.sh
|
|
```
|
|
|
|
That guard is the mechanical form of #197's acceptance bar — no runtime `gh`
|
|
outside `lib/forge-github.sh` unless the file declares
|
|
`CEREMONY_FORGE_CLIENT=gh` **and** refuses when it cannot run. Do not satisfy
|
|
it by adding an exemption; a declaration without a refusal is a permission slip
|
|
for `gh: command not found`.
|
|
|
|
Then check the **variables** the same way, because the same mechanic applies to
|
|
state: if a conflicted region assigns something that auto-merged code consumes,
|
|
resolving it "to this tree's side" silently removes the producer. Every one of
|
|
those consumers degrades to empty rather than erroring, so nothing goes red.
|
|
The `0.6.0` sync had three such seams. Enumerate what each resolved region
|
|
assigns, and confirm each still has a producer.
|
|
|
|
### 5. Port or declare every new `gh` call site
|
|
|
|
Where a `forge_*` verb exists, port it in the merge itself. Where none does,
|
|
the file **declares** `CEREMONY_FORGE_CLIENT=gh` and refuses loudly, and the
|
|
port gets its own issue (#199 for `refs-not-closing`, #205 for the sweep
|
|
dispatch). "Never 'probably github'" applies to the sync as much as to a
|
|
runtime probe.
|
|
|
|
A workflow cannot call `forge_preflight`, so it declares in its `env:` block
|
|
and refuses inline — deciding the **forge** first and the **binary** second. A
|
|
guard that only asks whether `gh` is installed passes the moment a runner image
|
|
ships it.
|
|
|
|
### 6. Record the provenance
|
|
|
|
- `CHANGELOG.md`'s header: which upstream commit this tree now carries.
|
|
- `.upstream-ref`: the same **full 40-character** SHA, machine-readable,
|
|
checked by `test/upstream-delta.test.sh` — which refuses when the object is
|
|
absent or is not an ancestor, rather than reporting it unverifiable. `ci.yml`
|
|
fetches that exact object before the suite runs.
|
|
- A `changelog.d/` fragment for the sync issue.
|
|
|
|
### 7. Verify — and verify where it will actually run
|
|
|
|
`test/run.sh` green on your machine is the weakest of the checks below. The
|
|
`0.6.0` sync was "green locally" and red on the runner **three times, for three
|
|
different reasons**:
|
|
|
|
| what was green locally | why the runner disagreed |
|
|
|---|---|
|
|
| `shellcheck-all.sh` | it lints **tracked** files, and the new guard was untracked |
|
|
| the whole suite | CI pins **shellcheck 0.10.0**; a different local version reports differently |
|
|
| `issue_payload_valid` | `jq -e` on empty input exits **4** on jq 1.7 and **0** on jq 1.6 — and the runner image ships 1.6 |
|
|
|
|
That last one was not a test problem: on jq 1.6 the guard that refuses an
|
|
unreadable read was *accepting* one. **The distance between your environment
|
|
and the runner's is part of the sync's risk surface, not an inconvenience.**
|
|
|
|
So verify with the runner's own tooling:
|
|
|
|
```sh
|
|
git add -A # or shellcheck sees nothing new
|
|
CEREMONY_REQUIRE_NPM=1 CEREMONY_REQUIRE_YQ=1 bash test/run.sh
|
|
bash .github/scripts/shellcheck-all.sh # pinned 0.10.0, as ci.yml installs
|
|
bash .github/scripts/actionlint-all.sh
|
|
bash .github/scripts/self-ref-check.sh
|
|
bash .github/scripts/marker-check.sh
|
|
bash .github/scripts/vendored-check.sh
|
|
bash actions/changelog-armed/changelog-armed.sh
|
|
```
|
|
|
|
and run the suite once under the runner's `jq` as well as your own.
|
|
|
|
### Every branch that was open during the sync is now stale
|
|
|
|
Forgejo tests branch heads; it never tests what two branches produce together,
|
|
and it never re-tests an open PR when `main` moves under it. So after a sync
|
|
lands, **every PR that was open across it is green against a tree that no
|
|
longer exists** — its run did not contain the test files and rules the sync
|
|
introduced.
|
|
|
|
Both halves of that bit in this sync:
|
|
|
|
- `#206` and `#207` were cut from the pre-sync base. Their green suites had 22
|
|
test files; the merged tree has 28.
|
|
- `#206`'s changelog fragment was individually green and made the **combined**
|
|
tree red, because the terminal-citation rule (#262) arrives *with* the sync
|
|
and the fragment was written against a base without it.
|
|
|
|
So, for each PR still open:
|
|
|
|
```sh
|
|
git merge origin/main # in the branch — do not rewrite its commits
|
|
CEREMONY_REQUIRE_NPM=1 CEREMONY_REQUIRE_YQ=1 bash test/run.sh
|
|
```
|
|
|
|
or, if you are only checking rather than updating, merge them into a scratch
|
|
worktree together and run the full current suite and static guards there. A
|
|
prior approval is evidence about the tree it was given on; after a sync it is
|
|
not evidence about the tree the operator would merge.
|
|
|
|
### 8. After it merges — audit by executed steps, never by colour
|
|
|
|
The sync issue uses `Refs`, not `Closes`, and stays open until a real sweep on
|
|
the merged `main` is linked to it.
|
|
|
|
**A green run is not that evidence.** In this sync the first post-merge run was
|
|
green and had reconciled *nothing*: upstream's #209 restructure moved reconcile
|
|
out of the labels caller and behind a dispatch this forge cannot perform, so
|
|
the only job that ran was the refusal. Green, correct, and proof of the
|
|
refusal path only.
|
|
|
|
So before citing any run:
|
|
|
|
1. **Inventory what the sync changed about workflow triggers and jobs** — which
|
|
jobs exist now, which events fire them, and which of those this forge can
|
|
actually serve. A restructure upstream can move work between workflows
|
|
without touching a line of the code that does it.
|
|
2. **Read the run's executed steps**, not its status. Name the job that did the
|
|
thing, and quote the line that shows it did.
|
|
3. A green *skipped-or-refusing* path is valid evidence **for that path**, and
|
|
never evidence that the work happened.
|
|
|
|
Neither of these is caught by the no-runtime-`gh` scan in step 4: in this sync
|
|
both failures occurred with that guard green and CI green.
|
|
|
|
## Where the forge delta lives
|
|
|
|
Forge-specific behaviour is confined to the files below. Keeping it there is
|
|
what makes each sync cost 18 hunks instead of hundreds, and
|
|
`test/upstream-delta.test.sh` fails the PR that scatters it into a new file.
|
|
|
|
| file | what is forge-specific about it |
|
|
|---|---|
|
|
| `lib/forge.sh` | the selector: `forge_detect`, `forge_client`, `forge_preflight` |
|
|
| `lib/forge-github.sh` | the gh backend — the one file allowed to speak `gh` |
|
|
| `lib/forge-forgejo.sh` | the Forgejo backend, `/api/v1` over curl + jq |
|
|
| `lib/closes_references.sh` | the closing-keyword parser that replaced GraphQL |
|
|
| `.github/labels.conf` | this instance's roster |
|
|
| `drills/` | records of runs on this instance |
|
|
| `actions/refs-not-closing/run.sh` | declares `CEREMONY_FORGE_CLIENT=gh` — its gather is GraphQL, which Forgejo does not serve. #199 removes the declaration |
|
|
| `.github/workflows/labels.yml` | the sweep dispatch decides the forge inline and declares a client; a workflow has no shell to call `forge_preflight` from. #205 ports it |
|
|
| `.github/workflows/refs-guard.yml` | schedules its job on GitHub only, so an action that can only refuse here does not stand red. #199 removes the gate |
|
|
| `.github/workflows/release-exercise.yml` | pins `CEREMONY_FORGE: github` deliberately: the exercise drives the GitHub path |
|
|
|
|
Four of those are **temporary** and say which issue removes them. That is the
|
|
point of listing them rather than exempting them: a forge-delta location with
|
|
no exit is indistinguishable from one nobody noticed.
|
|
|
|
A file that merely **calls** the shim is not a delta location — every
|
|
reconciler and `release.yml` call `forge_select`, and that is what the shim is
|
|
for. A file that **decides** or **declares** is, and belongs here.
|
|
|
|
If a sync needs forge branching somewhere else, that is a design decision, not
|
|
a detail: add the file to the inventory in the same PR, with the reason.
|