Repository navigation
ci(linter): check that every "step N" citation resolves - #36
Conversation
|
Warning Review limit reached
Next review available in: 103 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds a prompt-step citation linter, integrates it into the CI lint workflow, documents its checks, and adds generated-fixture self-tests for valid and invalid step references. ChangesPrompt-step linting
Sequence Diagram(s)sequenceDiagram
participant CI as CI lint workflow
participant Wrapper as lint-prompt-steps.sh
participant Validator as lint-prompt-steps.py
participant Repository as Repository files
CI->>Wrapper: run prompt-steps checker
Wrapper->>Validator: invoke whole-tree validation
Validator->>Repository: read prompt headings and citations
Repository-->>Validator: return step definitions and references
Validator-->>Wrapper: return validation status
Wrapper-->>CI: report checker result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
ci/PROMPT.md is 2100+ lines and is the specification nine sibling
modules are rolled out against. Its steps are addressed by number from
inside the file and from README.md, ci/feedback/, ci/tools/ and
ci/linter/ -- 118 citations in PROMPT.md alone. Nothing in the tree
could tell whether those numbers still resolved, so renumbering a step,
deleting one, or rewriting a block that cited one was a silent edit: the
reader following a dangling "see step 27" lands on the wrong instruction
with no error anywhere. The previous session rewrote ~120 lines of it and
had no way to check the citations survived.
Two directions, both silent on their own:
* a citation naming a step that does not exist -- renumber/deletion;
* a gap or duplicate in the step sequence itself, which is how a
half-finished renumber leaves the file and how two headings end up
answering to one number.
Whether a citation points at the RIGHT step is deliberately not checked;
that needs a human, and a check pretending to do it is false comfort.
No recognisable step heading is exit 2, not a clean pass and not a flood
of findings against an empty step set: rewrite the headings and a naive
checker reports every citation in the repo, which reads as noise and gets
the gate deleted.
The tree is currently clean at 42 steps -- which proves nothing on its
own, so selftest.sh gains five controls against a generated tree
(dangling, the passing-direction pair, gap, duplicate, headings gone).
Generated rather than committed: a fixture PROMPT.md is a 2100-line
near-duplicate nobody would remember to leave broken.
Also adds prompt-steps to lint.yml's LINT_ONLY -- the set-equality
control caught its absence, which is exactly the "runs locally and in the
hook but never on a PR" case that control exists for.
32f9823 to
db71a70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f1ea7a1-dd18-44f0-87ee-c03038473100
📒 Files selected for processing (5)
.github/workflows/lint.ymlci/linter/README.mdci/linter/lint-prompt-steps.pyci/linter/lint-prompt-steps.shci/linter/selftest.sh
| return [n for n in names if CITERS_RE.match(n)] | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the repository ruff configuration for PL rule selection
# and formatter quote style, and compare with a sibling Python file.
set -uo pipefail
fd -H -t f -g 'ruff.toml' -g '.ruff.toml' -g 'pyproject.toml' -g 'setup.cfg' . \
--exec sh -c 'echo "== $1"; cat -n "$1"' _ {}
echo "== quoting style used by sibling checker"
fd -t f 'workflow_policy.py' --exec sh -c 'rg -n -m 20 "^(import|from|[A-Z_]+ =)" "$1"' _ {}Repository: myguard-labs/nginx-skeleton-module
Length of output: 998
🏁 Script executed:
# Find ruff configuration files
fd -H -t f '(ruff\.toml|pyproject\.toml|setup\.cfg)$' . | head -20Repository: myguard-labs/nginx-skeleton-module
Length of output: 172
🏁 Script executed:
# Read the target file
cat -n ci/linter/lint-prompt-steps.pyRepository: myguard-labs/nginx-skeleton-module
Length of output: 8428
🏁 Script executed:
# Check lint-python.sh to see how ruff is configured
cat -n ci/linter/lint-python.shRepository: myguard-labs/nginx-skeleton-module
Length of output: 2075
🏁 Script executed:
# Search for pyproject.toml or any ruff config
find . -maxdepth 3 -name "pyproject.toml" -o -name "setup.cfg" -o -name "ruff.toml"Repository: myguard-labs/nginx-skeleton-module
Length of output: 172
🏁 Script executed:
# Check if ci/linter/lint-python.sh actually runs on this file
git ls-files '*.py' | grep -E '(ci/linter|ci/tools)' || echo "No Python files found in ci/linter or ci/tools"Repository: myguard-labs/nginx-skeleton-module
Length of output: 287
🏁 Script executed:
# Use Python to count branches in main() function according to PLR0912 logic
python3 - <<'PY'
import re
# Read the file
with open('ci/linter/lint-prompt-steps.py', 'r') as f:
content = f.read()
# Extract main() function
main_match = re.search(r'def main\(\):(.*?)(?=\ndef |\Z)', content, re.DOTALL)
if main_match:
main_body = main_match.group(1)
# Count branches (simplified PLR0912 logic)
# Branches include: if, elif, else, for, while, try, except, with, and operators
branch_count = 0
# Count each type of branch
if_count = len(re.findall(r'^\s+if\s+', main_body, re.MULTILINE))
elif_count = len(re.findall(r'^\s+elif\s+', main_body, re.MULTILINE))
else_count = len(re.findall(r'^\s+else\s*:', main_body, re.MULTILINE))
for_count = len(re.findall(r'^\s+for\s+\w+\s+in\s+', main_body, re.MULTILINE))
try_count = len(re.findall(r'^\s+try\s*:', main_body, re.MULTILINE))
except_count = len(re.findall(r'^\s+except\s+', main_body, re.MULTILINE))
print(f"if: {if_count}")
print(f"elif: {elif_count}")
print(f"else: {else_count}")
print(f"for: {for_count}")
print(f"try: {try_count}")
print(f"except: {except_count}")
print(f"Total: {if_count + elif_count + else_count + for_count + try_count + except_count}")
# Also check for ternary/conditional expressions
ternary_count = len(re.findall(r'\s+if\s+.*else', main_body))
print(f"Conditional expressions: {ternary_count}")
PYRepository: myguard-labs/nginx-skeleton-module
Length of output: 255
The repository ruff configuration does not explicitly set select, ignore, or quote-style options, so lint-python.sh will apply ruff defaults.
The main() function has 15 branches (8 if, 3 for, 2 try, 2 except statements), which exceeds the PLR0912 limit of 12. Since default ruff includes PL (Pylint) rules, the lint job will fail with PLR0912 unless the rule is disabled in configuration.
Additionally, this file uses single-quoted strings throughout, but ruff format by default rewrites single quotes to double quotes. The lint job will fail on the format check unless quote-style is set to single or preserve in the ruff configuration.
Either configure ruff in pyproject.toml (or equivalent) at the repository root to disable PLR0912 and set quote-style = "single", or refactor main() to reduce branching below the limit and reformat strings to double quotes before merge.
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 86-88: Command coming from incoming request
Context: subprocess.run(
['git', 'rev-parse', '--show-toplevel'],
check=True, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.1)
[warning] 86-86: Too many branches (13 > 12)
(PLR0912)
Source: Linters/SAST tools
| # A hole in the sequence -- how a half-finished renumber leaves the file. | ||
| printf '## 1 — First\n## 3 — Third\n' > "$promptroot/ci/PROMPT.md" | ||
| case_ 1 "prompt-steps: a gap in the step sequence is a finding" \ | ||
| env PROMPT_STEPS_ROOT="$promptroot" \ | ||
| python3 ci/linter/lint-prompt-steps.py |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the gap control from the dangling-citation path.
The fixture now defines steps 1 and 3, but README.md still carries See step 2 from line 316. Step 2 is undefined in this fixture, so the run exits 1 through the dangling-citation branch as well as the gap branch. Remove the gap check from the Python file and this control still reports ok. That is the one failure mode a negative control must not have, and the surrounding comments show you care about exactly this distinction.
Point the citation at a step that both fixtures define.
💚 Proposed fix so only the gap can trigger the finding
# A hole in the sequence -- how a half-finished renumber leaves the file.
+# The citation must name a step that stays defined, or the dangling-citation
+# path alone would carry this control.
+printf 'See step 1 for details.\n' > "$promptroot/README.md"
printf '## 1 — First\n## 3 — Third\n' > "$promptroot/ci/PROMPT.md"
case_ 1 "prompt-steps: a gap in the step sequence is a finding" \
env PROMPT_STEPS_ROOT="$promptroot" \
python3 ci/linter/lint-prompt-steps.py📝 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.
| # A hole in the sequence -- how a half-finished renumber leaves the file. | |
| printf '## 1 — First\n## 3 — Third\n' > "$promptroot/ci/PROMPT.md" | |
| case_ 1 "prompt-steps: a gap in the step sequence is a finding" \ | |
| env PROMPT_STEPS_ROOT="$promptroot" \ | |
| python3 ci/linter/lint-prompt-steps.py | |
| # A hole in the sequence -- how a half-finished renumber leaves the file. | |
| # The citation must name a step that stays defined, or the dangling-citation | |
| # path alone would carry this control. | |
| printf 'See step 1 for details.\n' > "$promptroot/README.md" | |
| printf '## 1 — First\n## 3 — Third\n' > "$promptroot/ci/PROMPT.md" | |
| case_ 1 "prompt-steps: a gap in the step sequence is a finding" \ | |
| env PROMPT_STEPS_ROOT="$promptroot" \ | |
| python3 ci/linter/lint-prompt-steps.py |
From CodeRabbit review on #36. Four confirmed, one refuted. The gap control was FALSE. Its fixture defines steps 1 and 3 while README.md still carried "See step 2" from the positive control above it, so the run exited 1 through the dangling-citation branch as well as the gap branch. Verified by mutation: deleting the gap check outright still printed ok. The citation is now pinned to step 1, which every fixture defines, so each sequence control can only go red through the branch it names -- and the same mutation now flips it to exit 0, failing the assertion as a control should. git rev-parse escaping as CalledProcessError printed a traceback and exited 1. The docstring promises 2 for "could not run", and 1 is the status that means "findings", so a missing git or a run from outside a checkout read as a defect in PROMPT.md. Now returns 2 with a message. GIT_DIR/GIT_WORK_TREE are exported by every git hook and win over cwd, so `git ls-files` answered for the checkout whatever directory it was pointed at -- the selftest's generated root would have been listed as the real repo, the os.walk fallback would never have run, and the controls would have asserted against the wrong tree while still printing ok. Stripped for both git calls. The trigger pattern was a character-for-character copy of CITERS_RE, which the module's docstring tells the next maintainer to extend. The wrapper now reads it back via --citers-re; a stale copy would leave the gate quiet under LINT_MODE=staged for exactly the path someone had just added it for. selftest.sh cleaned $promptroot on the happy path only, and the later `trap ... EXIT` for $badroot would have REPLACED any trap set for it rather than adding to it. One trap now covers both. Refuted: PLR0912 on main() and a single-quote format failure. The repo pins no ruff config, so ruff defaults apply -- and the default select is E4,E7,E9,F, which does not include PL. `ruff check` and `ruff format --check` both pass on the file, and the Lint job was green on the run that review read.
|
Five confirmed and fixed in The gap control was false — this is the one that mattered. The fixture defines steps 1 and 3 while
The duplicated pattern is now read back from the module via The trap was worse than described. Refuted: The premise is that "default ruff includes PL (Pylint) rules". It does not — ruff's default The repo pins no ruff config, so the defaults apply and
|
1 participant
TL;DR
The long instruction document at the heart of this repo refers to its own numbered steps constantly, and other files refer to them too. Until now nothing checked that those numbers still pointed at anything, so renaming or renumbering a step quietly broke every reference to it and no tool would notice. This adds a linter that does check.
Adds
ci/linter/lint-prompt-steps.{sh,py}: everystep Ncitation must name a stepci/PROMPT.mddefines, and the step sequence itself must have no gap or duplicate.Severity:
Low(documentation integrity — silent cross-reference rot in the adoption spec)Why this gap is invisible
ci/PROMPT.mdis 2134 lines and is the specification nine sibling modules are rolled out against. Its steps are addressed by number — 118 citations inside the file, plusREADME.md,ci/feedback/README.md,ci/tools/test_runtime.py,ci/linter/selftest.shandci/linter/fixture-selector.py.Renumbering a step, deleting one, or rewriting a block that cited one produces no error anywhere. The reader who follows a dangling
see step 27lands on the wrong instruction, or on nothing.What is checked
Two directions, both silent on their own:
Whether a citation points at the right step is deliberately not checked. That needs a human, and a check pretending to do it would be false comfort.
Missing step headings exit
2("could not run"), not0and not a flood of findings against an empty step set — rewrite the heading convention and a naive checker reports every citation in the repo, which reads as noise and gets the gate deleted.Testing
The tree is currently clean at 42 defined steps, all citations resolving. That proves nothing on its own — a checker that never finds anything and one that cannot find anything print the same line — so
selftest.shgains five controls against a generated tree: dangling citation, the passing-direction pair, sequence gap, duplicate number, and headings gone.Fixtures are generated rather than committed: a fixture
PROMPT.mdcarrying a deliberate defect is a 2100-line near-duplicate of the real spec that nobody would remember to leave broken.ci/linter/run-all.shandci/linter/selftest.shboth pass locally.One thing the checker found in its own PR
The reproduce recipe added to
ci/linter/README.mdcontains a literalstep 99, and the checker read it as a real dangling citation.ci/linter/README.mdwrites about citations rather than making them, so it joins the checker's own source in the skip list. The skip list stays scoped toci/linter/; a genuine dangling citation inREADME.md,ci/feedback/orci/tools/is exactly what this is for.Also
prompt-stepsis added tolint.yml'sLINT_ONLY. The existing set-equality control inselftest.shcaught its absence — the "runs locally and in the hook but never on a PR" case that control exists for.