Repository navigation
ci: restructure PROMPT.md around the code seam, a depth pass and a safety phase - #15
Conversation
The three prompts (bring-up, sync, runner portability) plus a summary in README.md described one job four times. Bring-up and sync each carried their own copy of the ground rules, the port-band explanation, the runner section and the probe shapes, and the README repeated the sync prompt for a human reader. 1402 lines across three files, with the runner material triplicated after the last fold. One document now: eight checkpoints, one PR each, classification done inline at checkpoint 0 so nothing has to route. Forwarding one later change into an already-adopted module is a tail section rather than a peer document. The content changes, beyond deduplication, came from surveying the eight derived modules on 2026-08-03. The prompts described a fleet that does not exist: none has a ci.yml, every one has three to six workflows each carrying its own pull_request trigger, workflow_call appears nowhere, six have no ci/ and two have no src/. So the job is a merge into existing CI, not a greenfield install, and the demotion to a single entry point is its riskiest edit rather than two sentences. New material: - runner identity (checkpoint 2). builder02 is a myguard machine named in 15 selectors across 7 workflows plus actionlint.yaml and TRUST_SPLITS -- and nothing catches a copied label, since actionlint validates only literal runs-on and lint-ci-runners.sh compares against a TRUST_SPLITS that approves builder02 by construction. Adopters without their own pool get ubuntu-latest everywhere and no fork ternary. Both probes verified: the probe correctly exits 0 in this repo, and emptying TRUST_SPLITS first yields 16 findings rather than one, which is why the rewrite order is workflows first and the checker last. - orchestrator demotion as a three-step sequence, with the double-run as the proof the call graph is wired, plus the secrets-inherit and path-filter traps that only bite a called workflow. - no-src/ modules: everything scoped to src/ selects nothing and reports success -- linters, gcovr filter, CodeQL TU filter. - keep-what-the-target-has as a standing rule, with extra workflows decided explicitly rather than dropped. - badge label text pinned character for character; a derived module had the right order but wrote 'Build & Test' and 'Security scanners'. - the no-anchor case: the sync path claimed the standardisation PR merge was always available as a floor. No derived module has one. Corrected: the max-port note still told readers to distrust an ordering fixed in #14, and lint.yml's LINT_ONLY string was missing 'spelling'.
…ards Four changes to the adoption prompt, all from reviewing it against the machinery it describes: - The *_scan.c decision seam becomes checkpoint 1, ahead of the ci/ move. Checkpoints 5 and 6 link across it, so extracting it late meant the unit layer tested a reimplementation and the fuzz target drove one. Probes fall back to repo-root paths because src/ does not exist until checkpoint 2. - Checkpoint 10, a depth pass run after the others merge: does the ASan soak reach the handler, can the fuzz surface widen, is coverage measured over the module, is helgrind actually invoked, do the linters still bite. Each item answered with a measurement rather than a reading of the YAML. - Five phase headings grouping the checkpoints, which keep their numbers. - Phase -1: scope jail, clean-tree precondition, force-push and stash rules, a stop-condition table, and rollback. The file previously handed an agent with write access ten PRs against a live repo without saying what it may write to or when to stop. Checkpoint 4a step 3 gains an abort condition — it is the only step that can leave the repo with no PR gate at all.
WalkthroughThe PR consolidates module CI standardization and synchronization guidance into ChangesCI adoption guidance
Linter documentation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f81a001-d2cc-4744-b9d3-58716e8c1397
📒 Files selected for processing (5)
README.mdci/PROMPT-standardize-module.mdci/PROMPT-sync-module.mdci/PROMPT.mdci/linter/README.md
💤 Files with no reviewable changes (2)
- ci/PROMPT-standardize-module.md
- ci/PROMPT-sync-module.md
| | `workflow_policy.py` | — | the three repo-policy checks the `ci-*`/`docs-drift` wrappers call | | ||
| | `selftest.sh` | — | negative controls for the gate itself; run before the linters in `lint.yml` | | ||
| | `fixtures/policy/` | — | workflow trees the policy checks must go RED on, one per known bypass | | ||
| | `fixtures/policy/` | — | trees the policy checks must go RED on — the known bypasses, plus the runner-label and step-ordering cases; `clean/` and the `-ok` trees must stay GREEN | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Clarify the fixture-to-check mapping.
ci/linter/fixtures/README.md states that each fixture encodes one bypass. ci/linter/selftest.sh runs each fixture against a specific policy subcommand. This row can be read as requiring every fixture to make every policy check return RED.
Use wording that identifies the fixture/subcommand pairs. This will prevent contributors from adding incorrect self-test expectations.
Proposed wording
-| `fixtures/policy/` | — | trees the policy checks must go RED on — the known bypasses, plus the runner-label and step-ordering cases; `clean/` and the `-ok` trees must stay GREEN |
+| `fixtures/policy/` | — | fixture/subcommand pairs that must go RED — the known bypasses, plus the runner-label and step-ordering cases; `clean/` and the `-ok` trees must stay GREEN |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `fixtures/policy/` | — | trees the policy checks must go RED on — the known bypasses, plus the runner-label and step-ordering cases; `clean/` and the `-ok` trees must stay GREEN | | |
| | `fixtures/policy/` | — | fixture/subcommand pairs that must go RED — the known bypasses, plus the runner-label and step-ordering cases; `clean/` and the `-ok` trees must stay GREEN | |
| Ten checkpoints, one PR each, grouped into five phases: | ||
|
|
||
| | Phase | Checkpoints | What it is | | ||
| |---|---|---| | ||
| | −1 | — | preconditions, scope jail, stop conditions — read first | | ||
| | 0 | 0 | inventory and baseline, read-only | | ||
| | 1 | 1 | the decision seam — the one C refactor | | ||
| | 2 | 2–9 | adoption: layout, runners, entry point, tests, caching, lanes | | ||
| | 3 | 10 | depth pass — would any of it catch anything? | | ||
| | 4 | — | close out: docs, memory mirror, report | | ||
|
|
||
| Checkpoints are numbered continuously and referenced by number throughout; the | ||
| phases group them. Phase 3 runs only after every phase-2 checkpoint has merged. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize checkpoint numbering across the adoption documentation.
The unified prompt defines eleven numbered checkpoints, 0 through 10, but the two summaries use different totals and omit required checkpoint references.
ci/PROMPT.md#L19-L31: change “Ten checkpoints” to “Eleven checkpoints” and preserve checkpoints 0 through 10.README.md#L288-L306: update “Eight checkpoints,” include the decision seam and depth pass, and change runner identity to checkpoint 3.
📍 Affects 2 files
ci/PROMPT.md#L19-L31(this comment)README.md#L288-L306
| Two writes outside it are expected, and only these two: | ||
|
|
||
| - **The target's own memory mirror**, `memory/labs/<name>/` or | ||
| `memory/eilandert/<name>/` — where the inventory, issues and lessons go. | ||
| - **The superrepo gitlink**, once per merged checkpoint, if `<TARGET>` is a | ||
| myguard submodule. Submodule PR merges first, then the gitlink bump lands | ||
| signed on the superrepo's `master`. An external target has no gitlink and no | ||
| mirror; skip both and say so. | ||
|
|
||
| Rule 2's upstream PR (a gate the target has and the skeleton lacks) is a | ||
| **separate session** after the rollout, not a commit slipped in while here. | ||
| - A dirty submodule or unrelated change in another tree is left exactly as | ||
| found. Do not commit it, revert it, or `git checkout` over it. | ||
| - Writing to `memory/` for the target's own mirror is expected; writing to | ||
| another module's mirror is not. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the external memory-mirror rule.
Lines 87-92 say that an external target has no memory mirror and that both writes must be skipped. Lines 196-198 say to create a mirror when external work is ongoing. Choose one rule. Otherwise, an agent can write outside <TARGET> despite the scope jail.
Also applies to: 196-198
| ```sh | ||
| cd <TARGET> | ||
| git status --porcelain # -> MUST be empty | ||
| git rev-parse --abbrev-ref HEAD # note it; this is the base to branch from | ||
| git remote -v # confirm you are where you think you are | ||
| gh auth status # can you actually open a PR here? | ||
| ``` | ||
|
|
||
| - **Uncommitted changes → stop.** They are not yours; you do not know what they | ||
| were for. Ask. Never `git stash` to get a clean tree — a stash the user did not | ||
| ask for is data they cannot find later. | ||
| - **Not a git repo, or no push access → stop and say so.** Do not initialise one, | ||
| do not fork as a workaround. | ||
| - **The default branch is not a work surface.** Every checkpoint gets its own | ||
| branch off the current default, merged by PR. Nothing is committed straight to | ||
| `master`/`main`, including "trivial" doc fixes. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Determine the default branch explicitly.
Line 106 calls the current HEAD the base branch, but Lines 116-118 require every checkpoint branch to start from the current default branch. A clean tree can still be on an unrelated branch. Record the repository default branch explicitly, then branch each checkpoint from that ref.
| ## Rollback | ||
|
|
||
| Each checkpoint is one PR precisely so it can be reverted alone. If a merged | ||
| checkpoint turns out wrong, `git revert` the merge commit on its own branch and | ||
| PR that — do not force-push over history that others have pulled, and do not | ||
| "fix forward" by stacking a second broken change on the first. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make rollback instructions match the merge strategy.
The rollback section requires reverting a merge commit, while closeout requires squash-merging. A squash merge does not create a merge commit. Document separate commands for squash commits and merge commits, or select one merge strategy.
Also applies to: 1164-1169
| The extraction, when needed: | ||
|
|
||
| 1. `*_scan.c` / `*_scan.h` take bytes and return a verdict. No nginx request | ||
| types in the signature, no allocation from a request pool — pass a buffer in | ||
| or take an explicit allocator argument. | ||
| 2. `*_module.c` keeps the handler, directive parsing, config merging and every | ||
| `ngx_http_*` call, and calls into the seam. | ||
| 3. Wire both consumers: `ci/tests/unit/run.sh` and `ci/fuzz/build.sh` compile the | ||
| target's real `*_scan.c` — the same source, not a second copy. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep checkpoint 1 independent of the ci/ move.
Step 1.3 requires wiring ci/tests/unit/run.sh and ci/fuzz/build.sh. Later text assigns that wiring to checkpoint 2 and states that the target may not have ci/ yet. Make Step 1.3 conditional on existing consumers, or move the wiring requirement entirely to checkpoint 2.
Also applies to: 297-301
| ```sh | ||
| ls src/*_scan.c src/*_scan.h 2>/dev/null || ls *_scan.c *_scan.h | ||
| grep -n 'ngx_http_request_t\|r->\|ngx_http_' $(ls src/*_scan.c 2>/dev/null || ls *_scan.c) | ||
| grep -n '_scan\.c' ci/fuzz/build.sh ci/tests/unit/run.sh 2>/dev/null |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle the no-seam case without hanging.
When no *_scan.c file exists, the command substitution at Line 285 can expand to no filenames. grep then reads standard input instead of checking a file. This is a supported target state. Collect the files first and run grep only when the list is non-empty.
| - **No `src/`? Creating one is part of this checkpoint** — and the seam files | ||
| from checkpoint 1 move with the rest of the C. Two of eight derived modules | ||
| keep `ngx_http_<name>_module.c` (sometimes plus `<name>_core.c/.h`) at the | ||
| repo root. Everything downstream is scoped to `src/` — `lint-c.sh`, | ||
| `lint-nginx.sh`, the gcovr filter, the CodeQL TU filter — and every one | ||
| *passes* on an empty selection rather than failing. Move the C under `src/` | ||
| and update `config` in the same commit. Prove it: a `malloc`/`strcpy` probe | ||
| file where the module's real C lives must make `LINT_ONLY="c nginx"` exit 1. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stage the temporary C probe before trusting run-all.sh.
The checkpoint requires an untracked probe file to make LINT_ONLY="c nginx" fail. Later text states that run-all.sh reads git ls-files, so the probe is invisible while untracked. Stage the probe, or invoke the checker with an explicit path, before evaluating the result.
Also applies to: 1030-1031
| # 2. the checker actually rejects the reference's selector | ||
| cat > .github/workflows/_probe.yml <<'EOF' | ||
| name: probe | ||
| on: | ||
| schedule: | ||
| - cron: "0 4 * * 1" | ||
| jobs: | ||
| p: | ||
| runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || fromJSON('["self-hosted","builder02","lxc"]') }} | ||
| steps: | ||
| - run: echo probe | ||
| EOF | ||
| LINT_ONLY=ci-runners ci/linter/run-all.sh # MUST exit 1 in the target | ||
| rm .github/workflows/_probe.yml | ||
| ``` | ||
|
|
||
| **Probe 2 going green in the target is the bug this checkpoint exists for** — it | ||
| means `TRUST_SPLITS` was copied unedited. Fix `workflow_policy.py`; do not | ||
| delete the probe. Two things about it, both verified against the reference on |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify probe cleanup.
The command removes .github/workflows/_probe.yml at Line 442, but the prose says “do not delete the probe.” State whether the file is temporary and should be removed after verification, or whether it must remain as a fixture.
| Plus, whenever either applies: | ||
|
|
||
| 5. **Stopped** — which phase −1 condition fired, at which checkpoint, and what a | ||
| human has to decide. A job that stopped early is a legitimate outcome; one | ||
| reported as finished when it stopped is not. | ||
| 6. **Anything left disabled, skipped or unverified** — a workflow not enabled, a | ||
| soak skipped per 10e, a gate never seen red. Silence here reads as coverage | ||
| that does not exist. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the ordered-list markers.
markdownlint-cli2 reports MD029 for the explicit 5. and 6. prefixes. Use bullet items or the numbering style accepted by the repository configuration.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 1094-1094: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
[warning] 1097-1097: Ordered list item prefix
Expected: 2; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
Source: Linters/SAST tools
1 participant
TL;DR
ci/PROMPT.mdis the instruction sheet an agent follows to bring one of the derived nginx modules onto this repo's CI. It described ten checkpoints of work but left out two things that decide whether that work means anything: where the decision logic has to live before any test can reach it, and what the agent may do to the repository when a step goes wrong.Precisely: adds checkpoint 1 (the
*_scan.cseam) ahead of theci/move, adds checkpoint 10 (a post-merge depth pass that measures whether the green checks bite), groups the checkpoints under five# Phaseheadings, and adds a phase −1 covering scope, clean-tree preconditions, stop conditions, and rollback.The seam checkpoint
The rule it enforces: decision logic lives in
*_scan.ctaking(u_char *, size_t); onlyngx_http_request_tplumbing stays in*_module.c.That rule was previously a sentence inside the fuzz checkpoint saying "extract it first". Too late to be useful — the unit layer and the fuzz target both link across that seam. A module reaching checkpoint 5 without the extraction gets unit tests exercising a reimplementation, and checkpoint 6 fuzzing a second one. Both go green while proving nothing about the code that ships.
It is now checkpoint 1, with three outcomes off the checkpoint 0 probe: clean, nominal (
*_scan.cexists but reaches forr->— the tell is growth inci/fuzz/ngx_stubs.c), or absent.Two ordering bugs came out of placing it, both fixed here. The seam now runs before
ci/exists, so its acceptance criteria cannot referenceci/fuzz/build.shor the checkpoint-2 suite; it uses the checkpoint 0 baseline and defers consumer wiring to checkpoint 2. And its probes hardcodedsrc/*_scan.c, butsrc/is not created until checkpoint 2 and two of the eight derived modules keep their C at the repo root — as written it would have createdsrc/implicitly and split the same C across two commits. The probes now fall back to root paths with an explicit instruction not to createsrc/there.Checkpoint 10 — the depth pass
Runs only after the other checkpoints merge, and asks the question the others cannot: everything is green, would any of it catch anything. Eight items, each answered with a measurement in the PR body rather than a reading of the YAML — seam still clean, ASan soak reaches the handler, fuzz surface can widen past one target, coverage moves when a test is deleted, helgrind is actually invoked, ccache hit rate is real, linters still bite, lanes re-measured.
The helgrind item is conditional in one direction only: the 600s soaks skip when nothing moved, but the wiring and suppression-scope checks stay unconditional. A copied
ci-deep.ymlthat lost its helgrind job still shows a green CI Deep badge, and a dormant module is exactly where that survives longest. The skip clock issrc/+ci/+versions.env, not commits alone —bump.ymlbumps weekly andci-deep.ymlruns monthly, so a module with zero source commits can be running against a new nginx.Phase −1
The file handed an agent with write access ten PRs against a live repo and never named the repo it may write to, never required a clean tree, and never said what to do when a step fails. Measured before writing it: zero occurrences of force, stash, uncommitted, dirty, rollback, or sibling across all 986 lines.
Added: a scope jail naming one writable repo plus exactly two expected outside writes (the target's memory mirror and the superrepo gitlink); a clean-tree precondition that forbids
git stashas the way to reach it;--force-with-leaseonly, only on the agent's own unreviewed branch; nogit checkout ./reset --hard/clean -fdto undo its own mistake; a seven-row stop-condition table; an enumerated prohibition on disabling a failing check to make a PR mergeable; and rollback by reverting the merge commit rather than force-pushing or fixing forward.The first draft of that section declared the superrepo read-only, which contradicts the forwarding section's requirement to bump the superrepo gitlink. The jail was narrowed to permit the mirror and the gitlink; the requirement stayed.
Two further guards sit at the specific risky edits rather than in the preamble: checkpoint 4a step 3, the only step that can leave the repo with no PR gate at all, and checkpoint 2's
git mv, which must be verified withgit log --follow— a move recorded as delete+add loses history unrepairably once merged.Why the checkpoint numbers did not change
Five
# Phaseheadings group the checkpoints, which keep their continuous 0–10 numbering. Per-phase renumbering was the obvious alternative and was rejected: the file had already been renumbered twice and roughly forty cross-references say "checkpoint N". A third pass over those is where a stale pointer gets through unnoticed.Testing
Docs-only; there is no runtime behaviour to test.
ci/linter/run-all.shclean in both full and--stagedmodes (== all linters clean ==)ci/PROMPT.mdandREADME.mdresolveNot verified, and worth stating plainly:
ci/PROMPT.mdhas never been run end to end. It was written and twice restructured from a read-only survey of the eight derived modules. The first real target is also its first test, and the ordering bugs fixed above were all found by inspection — that is a decent sign the next run finds more of the same kind.