FSM design-doctor: route every noQuestion ai:design row back to ai:needs-work - #242
Conversation
…:needs-work (#241) An ai:design label with no trusted comment raising a question is a state no actor consumes: next_design withholds it, the producer and vetter both skip it, and no other command lists it — the row sits for ever. Ruling (thedavidmeister, 2026-08-09, #241): the daily FSM-conformance pass moves ALL noQuestion rows to ai:needs-work, automatically. - Detection is next_design's own classifier: nd_classify now carries the noQuestion bucket as typed addresses beside the counts it already stated, so the doctor acts on exactly the bucket the read presents — one enumeration, one classifier, no second detector. - The transition is the standard needs-work send-back: the trusted `Rework note @<head>` work order (naming the WHY and both exits: re-raise with flag-design, or proceed) plus the one-state label move, through the same human_rule_steps/human_rule_write machinery every ruling send-back uses — no hand-rolled label writes. Human decisions, live questions, drafts, unaddressable hits and archived repos are left alone; fetch errors fail the tick rather than shrinking it. Idempotent. - Wired as a flake runner (packages/apps.design-doctor, gh pinned in the closure) for the daily 04:00 UTC cron slot; CI closure gates extended. Closes #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repo ignores /* and allowlists each tracked file; without its entry the runner script never entered the commit and the flake's design-doctor output failed evaluation on a clean checkout (readFile of an absent path). Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 55 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)
WalkthroughAdds a ChangesDesign Doctor FSM
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Cron
participant design-doctor.sh
participant pr-review-report
participant GitHub
Cron->>design-doctor.sh: invoke daily at 04:00 UTC
design-doctor.sh->>design-doctor.sh: acquire flock and apply timeout
design-doctor.sh->>pr-review-report: run design-doctor
pr-review-report->>GitHub: classify and fetch current PR state
GitHub-->>pr-review-report: return labels, comments, and review fields
pr-review-report->>GitHub: post verdict and update labels for eligible rows
pr-review-report-->>design-doctor.sh: return exit status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…stionless rows Drafts and unaddressable hits also carry no live question; the counts line above already partitions them, so the empty-bucket line claims only what it checked. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Applied by CI's own pre-commit bundle (rainix rust-shell, pre-commit run --all-files) — the rs-static gate rejected the hand-wrapped lines. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rd (#241 review) A daily unattended pass that strips labels and posts trusted comments across three orgs was under-guarded relative to every sibling transition. Each fix follows the sibling that already had the shape: - The send-back is a VERDICT, not a human work order (draft_send_back_plan): `🤖 ai:vetter Reviewed <head>: needs-work` + a stated lens. That is the currency stamp (so the next vetter run cannot re-vet and strip the state this pass wrote) and it stops a machine decision wearing the human's `Rework note` marker. Comment first, then ONE label edit — the draft send-back's own reasoning, inverted because here the label is what keeps the row in this pass's re-enumeration. - The strip may not eat a co-resident modeled state (draft_send_back_strips_no_state, asked of classify_lane per label). - Fleet scoping ($PR_ASSIGNEE): both actors that consume ai:needs-work enumerate by author, so a third party's PR is withheld and NAMED rather than routed into a state nobody reads. - Settling window against the label-then-comment raise race (#147's rule: an absence is not licence until it is evidence). - Draft re-checked at write time; the search index lags. - Withheld classes are typed and named, and the three the pass can never drain (human-decided, not-our-fleet, no-anchor) print their consuming transition. NoAnchor is a classification, not a daily red cron. - Serial writes (retire_blocked_infra_mode), not a read-sized fan-out. - Runner honours the producer's DISABLED kill switch and carries a 30m timeout, as both model runners do. - The two mutation-proven gaps are closed: the archived-repo conformance gate now pins design_doctor_mode's read AND filter, and DESIGN_DOCTOR_ROUTE_FIELDS is pinned field-by-field (reviewDecision included) by the_issue_transition_fetches_what_its_guards_need. - README: restore the orphaned `### Vetting is a pure function` h3. - design-doctor.sh committed executable, as every other tracked script is. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stale exits prose - `design_doctor_route_from` is the seam `nd_classify`'s `fetch` parameter is: which classes write, which are reported and which red the tick were decisions only a live `gh` could reach, which is how "NoAnchor reds the cron daily" survived a green suite. Driven per class with an injected writer that panics if a withheld class writes anything. - The module header still described the retired `Rework note` design and the "otherwise proceed" exit; that prose was the only place the F5 mutant could land, so it survived by being a comment. Rewritten to the shipped design, and the mutant re-aimed at the note constant. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t name one The surviving mutant replaced "re-raise it with `flag-design`" with "mention it in a comment rather than `flag-design`" — still containing the command name, so a substring assertion on "flag-design" passed while the note had stopped naming a transition at all. That is finding 5's own defect class. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`cargo fmt` (rainix-rs-static) and the rainix pre-commit bundle's denofmt over README.md. No behaviour change. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-doctor.sh`:
- Around line 37-43: Require a non-empty PR_ASSIGNEE in the design-doctor setup
before invoking pr-review-report design-doctor. Validate the value after loading
cron.env and exporting ORGS, and fail closed with a nonzero exit when it is
unset or empty; preserve the existing behavior for valid assignee values.
- Line 12: Remove the global errexit disablement at the script’s strict-mode
setup and preserve set -euo pipefail. Update the final timeout/flock execution
to capture and explicitly accept only timeout status 124, while treating other
flock or command failures as errors; keep lock contention distinct from
lock-file creation and other flock failures.
In `@pr-review-report-rs/src/main.rs`:
- Around line 35950-35953: Update the FSM doctor doc comment to describe the
work order as only “re-flag,” removing the unsupported “or proceed” exit. Keep
the rest of the documented behavior unchanged and align the wording with
DESIGN_DOCTOR_NOTE and its associated test.
- Around line 28338-28364: Update the consuming_move documentation and match
behavior for DesignDoctorPlan::CoResidentState: either provide a Some(...)
message naming the human action that resolves the co-resident queue and clears
the retained ai:design state, or explicitly include CoResidentState in the None
rationale only if its transition is guaranteed to drain it. Ensure
ai:close-candidate and ai:blocked-on rows receive an actionable consuming
transition and do not recur indefinitely.
- Around line 28693-28712: Update design_doctor_mode to process only a bounded
number of q.no_question entries per run, using a clear per-run route cap.
Preserve existing failure handling for processed entries, and report the number
of unprocessed rows so the remaining backlog is visible and can drain on later
ticks.
In `@README.md`:
- Line 3102: Insert a blank line immediately after the closing cron code fence
and before the “Pause” list in the README, resolving the markdownlint MD031
requirement without changing the surrounding content.
- Line 3101: Update the documented cron entry so its schedule explicitly uses
UTC, rather than relying on the cron daemon or deployment host timezone;
preserve the existing 04:00 schedule and command.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5bc70b5a-dc04-424c-8db5-6d05315ae368
📒 Files selected for processing (6)
.github/workflows/rust.yml.gitignoreREADME.mddesign-doctor.shflake.nixpr-review-report-rs/src/main.rs
| # Packaged as a flake output (`packages.design-doctor`); `gh` and the binary come from the | ||
| # flake's locked nixpkgs. errexit is turned back off — writeShellApplication forces it, but this | ||
| # script reads the subcommand's exit status as data to log before passing it on. | ||
| set +o errexit |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching design-doctor.sh:\n'
fd -a 'design-doctor\.sh$' . || true
printf '\nGit status/stat:\n'
git diff --stat || true
for f in $(fd 'design-doctor\.sh$' .); do
printf '\n### %s\n' "$f"
wc -l "$f"
sed -n '1,120p' "$f" | cat -n
done
printf '\nSearch timeout usage and flock setup:\n'
rg -n "set \+o errexit|timeout |flock|PR_ASSIGNEE|design-doctor" .Repository: rainlanguage/issue-pr-cron
Length of output: 19258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path("design-doctor.sh")
src = path.read_text()
print("global_errexit_disabled=", bool(re.search(r"""set\s+[\da-z]*\+o\s+errexit""", src)))
print("errexit_reenabled=", bool(re.search(r"""set\s+[\da-z]*-o\s+errexit""", src)))
lock_line = next((i,l) for i,l in enumerate(src.splitlines(),1) if 'exec 9>"$DIR/.design-doctor.lock"' in l)
print("lock_line=%d" % lock_line[0], lock_line[1])
flock_line = next((i,l) for i,l in enumerate(src.splitlines(),1) if 'flock -n 9 ||' in l)
print("flock_line=%d" % flock_line[0], flock_line[1])
# Show whether no errexit leaves lock setup non-fatal when using || and then exiting.
print("flock_or_exit_is_always_0=", flock_line[1].strip().endswith('|| { log "skipped: a previous tick still holds the lock"; exit 0; }'))
print("exec_redirect_fails_without_errexit_and_reaches_flock_", True)
# Check for any remaining setup failures protected inside critical path:
for i,l in enumerate(src.splitlines(),1):
if l and not l.strip().startswith("#"):
if i < lock_line[0]:
if re.search(r"""(!.*&&|&&.*|or\(|&&|if\b|trap\b|exit\b|set\b)""", l):
print("pre-LOCK guard at %d: %s" % (i,l.strip()))
PYRepository: rainlanguage/issue-pr-cron
Length of output: 743
Keep strict failure handling enabled.
With global errexit disabled, lock-file creation failure can pass to flock -n 9, and the wrapper then treats flock errors as normal contention and exits 0. A failed cron.env source can also continue with partial configuration. Keep set -euo pipefail. Capture the expected timeout 124 status around the final command. Distinguish lock contention from other flock failures.
Suggested status handling
-set +o errexit
+set -euo pipefail
...
-timeout "$DOCTOR_MAXTIME" pr-review-report design-doctor "$@"
-rc=$?
+if timeout "$DOCTOR_MAXTIME" pr-review-report design-doctor "$@"; then
+ rc=0
+else
+ rc=$?
+fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-doctor.sh` at line 12, Remove the global errexit disablement at the
script’s strict-mode setup and preserve set -euo pipefail. Update the final
timeout/flock execution to capture and explicitly accept only timeout status
124, while treating other flock or command failures as errors; keep lock
contention distinct from lock-file creation and other flock failures.
Source: Linters/SAST tools
| # Org scope + fleet: single source is cron.env (same as the producer/vetter/refresher). PR_ASSIGNEE | ||
| # is what the pass calls "ours" — it withholds rows outside the fleet rather than routing them into | ||
| # a state no actor enumerates. | ||
| # shellcheck disable=SC1091 | ||
| [ -f cron.env ] && . ./cron.env | ||
| : "${ORGS:=rainlanguage cyclofinance S01-Issuer}"; export ORGS | ||
| export PR_ASSIGNEE |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 'PR_ASSIGNEE|design-doctor' pr-review-report-rs/src/main.rs design-doctor.shRepository: rainlanguage/issue-pr-cron
Length of output: 27853
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- relevant source slices ---\n'
sed -n '28149,28385p' pr-review-report-rs/src/main.rs
printf '\n--- pr_assignee usages in design doctor tests/logic ---\n'
rg -n 'pr_assignee\(|author|The fleet|fleet|NotOurFleet|not our' pr-review-report-rs/src/main.rs
printf '\n--- behavioral probe of Rust env-resolution and whitespace trim if present in helper context ---\n'
python3 - <<'PY'
# Read-only probe matching the Rust behavior described in source:
# std::env::var falls back only if the variable is absent, not if it is present and empty.
cases = {
'absent': None,
'empty': '',
'zero_length_quoted_like': '',
'single_whitespace': ' ',
'valid': 'thedavidmeister',
}
for name, value in cases.items():
pr_assignee = value if value is not None else 'thedavidmeister'
print(f'{name}: PR_ASSIGNEE={value!r} -> pr_assignee()={pr_assignee!r}')
PYRepository: rainlanguage/issue-pr-cron
Length of output: 255
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Check whether PR_ASSIGNEE is required/fails closed elsewhere in the shell wrapper or tests.
printf '--- PR_ASSIGNEE references ---\n'
rg -n -E 'PR_ASSIGNEE|export PR_ASSIGNEE|non-empty|require|missing|unset|cron.env' design-doctor.sh pr-review-report-rs/src/main.rs README.md crontab . 2>/dev/null | head -n 200Repository: rainlanguage/issue-pr-cron
Length of output: 255
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- relevant source slices ---'
sed -n '28149,28385p' pr-review-report-rs/src/main.rs
echo
echo '--- pr_assignee usages in design doctor tests/logic ---'
rg -n 'pr_assignee\(|author|The fleet|fleet|NotOurFleet|not our' pr-review-report-rs/src/main.rs
echo
echo '--- behavioral probe of Rust env-resolution and whitespace trim if present in helper context ---'
python3 - <<'PY'
# Read-only probe matching the Rust env-resolution semantics in source:
# std::env::var returns Err when the variable is absent and Some("") when it is present but empty.
# A ".trim().parse()" fallback then returns the default only for absent keys.
def pr_assignee(value):
if value is None:
return 'thedavidmeister'
return value
for name, value in [
('absent', None),
('empty', ''),
('single_whitespace', ' '),
('valid', 'thedavidmeister'),
('nonexistent_assigned', ''):
pass
]:
print(f'{name}: PR_ASSIGNEE={value!r} -> pr_assignee()={pr_assignee(value)!r}')
PYRepository: rainlanguage/issue-pr-cron
Length of output: 42554
Fail closed when PR_ASSIGNEE is empty.
design-doctor only uses PR_ASSIGNEE as the fleet author, so accepting an empty value gives the doctor a valid-looking author for route equality checks without defining a real fleet. Require PR_ASSIGNEE before calling pr-review-report design-doctor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-doctor.sh` around lines 37 - 43, Require a non-empty PR_ASSIGNEE in
the design-doctor setup before invoking pr-review-report design-doctor. Validate
the value after loading cron.env and exporting ORGS, and fail closed with a
nonzero exit when it is unset or empty; preserve the existing behavior for valid
assignee values.
| /// WHO drains this row, for the classes the doctor withholds — CLAUDE.md's "every state needs a | ||
| /// consuming transition", answered per class rather than left implied by an `Ok` line. | ||
| /// | ||
| /// `None` for the classes that need no consumer: the row either left the state already | ||
| /// (`Moot`, `LeftDesign`), is in the state it belongs in (`QuestionLive` — the human's queue | ||
| /// presents it), was just routed, or is withheld only until the next tick (`TooFresh`, | ||
| /// `Draft` — a draft's exit is its own author clearing the flag, which the vetter's draft | ||
| /// send-back already asks for). `Some` for the three the doctor can never drain by itself, so | ||
| /// the report NAMES the move a person has to make. | ||
| fn consuming_move(&self) -> Option<&'static str> { | ||
| match self { | ||
| DesignDoctorPlan::HumanSacred => Some( | ||
| "a human decision stands on this PR — the consuming transition is the human's own \ | ||
| `human-rule <owner/repo> <pr> needs-work|design \"…\" --rework \"…\"`, which \ | ||
| retires ai:design in the same call", | ||
| ), | ||
| DesignDoctorPlan::NotOurFleet { .. } => Some( | ||
| "outside the fleet ($PR_ASSIGNEE): no AI actor enumerates it, so the consuming \ | ||
| transition is a human's — rule on it (`human-rule …`) or drop the label by hand", | ||
| ), | ||
| DesignDoctorPlan::NoAnchor => Some( | ||
| "no head sha to pin a verdict to (a deleted fork head reads this way): the \ | ||
| consuming transition is closing the PR, or the author restoring the branch", | ||
| ), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Name the consumer for CoResidentState, or list it in the None rationale.
The doc lists the classes that need no consumer: Moot, LeftDesign, QuestionLive, routed, TooFresh, and Draft. It omits CoResidentState. That class returns None, so the report line for a ai:close-candidate or ai:blocked-on row prints no drain instruction. The row keeps ai:design and reappears on every tick until a person resolves the co-resident queue. Per the coding guideline, name that move.
♻️ Proposed change
fn consuming_move(&self) -> Option<&'static str> {
match self {
+ DesignDoctorPlan::CoResidentState { .. } => Some(
+ "another modeled state sits beside ai:design — the consuming transition is that \
+ state's own queue (rule on the close-candidate, or clear the blocked-on), after \
+ which the next tick routes this row",
+ ),
DesignDoctorPlan::HumanSacred => Some(Update the None rationale in the doc comment to match whichever way you resolve this.
As per coding guidelines: "A guard that identifies a defect must have an executable transition that can clear that defect; avoid deadlocked states."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review-report-rs/src/main.rs` around lines 28338 - 28364, Update the
consuming_move documentation and match behavior for
DesignDoctorPlan::CoResidentState: either provide a Some(...) message naming the
human action that resolves the co-resident queue and clears the retained
ai:design state, or explicitly include CoResidentState in the None rationale
only if its transition is guaranteed to drain it. Ensure ai:close-candidate and
ai:blocked-on rows receive an actionable consuming transition and do not recur
indefinitely.
Source: Coding guidelines
| if q.no_question.is_empty() { | ||
| println!("design-doctor: no noQuestion rows — nothing to route"); | ||
| return if c.fetch_errors > 0 { 1 } else { 0 }; | ||
| } | ||
| let mut failed = c.fetch_errors > 0; | ||
| for (slug, num) in &q.no_question { | ||
| match design_doctor_route(slug, *num, dry_run) { | ||
| Ok(line) => println!("{line}"), | ||
| Err((_, msg)) => { | ||
| eprintln!("{msg}"); | ||
| failed = true; | ||
| } | ||
| } | ||
| } | ||
| if failed { | ||
| 1 | ||
| } else { | ||
| 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the design-doctor write loop with the sibling bulk-write pass and gh_run.
set -euo pipefail
f=$(fd -t f 'main.rs' pr-review-report-rs | head -1)
echo "== retire_blocked_infra_mode =="
ast-grep run --pattern 'fn retire_blocked_infra_mode($$$) { $$$ }' --lang rust "$f" || true
echo "== gh_run definition =="
ast-grep run --pattern 'fn gh_run($$$) { $$$ }' --lang rust "$f" || true
echo "== retrying_rate_limit call sites =="
rg -nP -C3 '\bretrying_rate_limit\s*[(<]' "$f" || true
echo "== existing per-run batch caps =="
rg -nP -C2 '\.take\(|MAX_(ROUTES|BATCH|PER_RUN)|_LIMIT:\s*usize' "$f" || trueRepository: rainlanguage/issue-pr-cron
Length of output: 9523
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=$(fd -t f 'main.rs' pr-review-report-rs | head -1)
echo "== file size =="
wc -l "$f"
echo "== locate relevant symbols =="
rg -n "design_doctor_mode|design_doctor_route|design-doctor|retire_blocked_infra_mode|fn gh_run|gh_retrying|GH_RATE_LIMIT|retrying_rate_limit" "$f" || true
echo "== gh_run area =="
sed -n '425,480p' "$f"
echo "== design_doctor_mode area =="
sed -n '28620,28725p' "$f"
echo "== design_doctor_route definition and call sites =="
rg -n -C3 "fn design_doctor_route|design_doctor_route\(" "$f" || trueRepository: rainlanguage/issue-pr-cron
Length of output: 13783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=$(fd -t f 'main.rs' pr-review-report-rs | head -1)
echo "== gh_run full definition =="
sed -n '1130,1165p' "$f"
echo "== write and plan argv functions =="
rg -n -C5 "fn write|fn design_doctor_plan_argv|plan_argv|verdict_comment|state_comment|human_rule_comment|rework_note_comment" "$f" | sed -n '1,220p'
echo "== retire_blocked_infra_mode outline/body =="
sed -n '39040,39330p' "$f"
echo "== rate limit references outside test setup =="
rg -n -C2 "gh_retrying|GH_RATE_LIMIT|rate.limit|retrying_rate_limit|RATE_LIMIT" "$f" | sed -n '1,220p'
echo "== tests around design_doctor_mode routing loop size =="
rg -n -C4 "no_question|routes this tick|second run|drain|MAX_|batch|limit|route" "$f" | sed -n '28720,29450p'Repository: rainlanguage/issue-pr-cron
Length of output: 38304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=$(fd -t f 'main.rs' pr-review-report-rs | head -1)
echo "== design_doctor_route_from full =="
sed -n '28608,28675p' "$f"
echo "== gh_write references =="
rg -n -C2 'gh_write|state_comment|human_rule_comment|rework_note_comment|GH_WRITE|write\(' "$f" | sed -n '1,240p'
echo "== no cap in design_doctor_mode write loop =="
python3 - <<'PY'
from pathlib import Path
p=Path("pr-review-report-rs/src/main.rs")
text=p.read_text()
start=text.index("fn design_doctor_mode(dry_run: bool) -> i32")
end=text.index("\n#[cfg(test)]\nmod design_doctor_tests", start)
body=text[start:end]
print(body[body.index("if q.no_question.is_empty()"):body.index("if failed {")])
print("contains take:", ".take(" in body[body.index("if q.no_question.is_empty()"):body.index("if failed {")])
PY
echo "== retire_blocked_infra write loop =="
sed -n '39082,39120p' "$f"Repository: rainlanguage/issue-pr-cron
Length of output: 15588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=$(fd -t f 'main.rs' pr-review-report-rs | head -1)
echo "== gh_result and output reporting =="
rg -n -C8 "fn gh_result|fn gh_output_report|fn gh_cli|Command::new|gh_api_result" "$f" | sed -n '1100,1235p'
echo "== design_doctor_plan_argv definition =="
rg -n -C10 "fn design_doctor_plan_argv|fn label.*edit|fn state.*edit|remove-label|add-label|status.*edit" "$f" | sed -n '1,220p'
echo "== design_doctor tests around routing and failures =="
sed -n '29360,29435p' "$f"
echo "== precise counts/symbols in design_doctor_mode =="
python3 - <<'PY'
from pathlib import Path
text=Path("pr-review-report-rs/src/main.rs").read_text()
start=text.index("fn design_doctor_mode(dry_run: bool) -> i32")
body=text[start:]
end=body.index("\n#[cfg(test)]\nmod design_doctor_tests")
loop=body[body.index("for (slug, num) in &q.no_question"):body.index("if failed {")]
print("loop body:", loop)
print("contains .take:", ".take(" in loop)
print("contains cap constant reference:", any(s in loop for s in ["DESIGN_DOCTOR_MAX_ROUTES", "MAX_ROUTES", "MAX_ROUTE"]))
PYRepository: rainlanguage/issue-pr-cron
Length of output: 13654
Add a per-run route cap.
design_doctor_mode routes every q.no_question row sequentially. A route can make two gh write calls, and the write path reports rate-limit refusals as failures without retry. A large backlog can fail part-way and leave remaining rows unprocessed in this tick. Cap q.no_question and surface the remaining count so the backlog drains over safe ticks.
♻️ Proposed change
+/// The most rows ONE tick routes. Each route is up to two content-creating `gh` calls through
+/// [`gh_run`], which has no rate-limit retry, so an unbounded first pass over a large backlog
+/// trips the secondary limit and fails every row after it. The queue drains over several ticks.
+const DESIGN_DOCTOR_MAX_ROUTES: usize = 20;
+
fn design_doctor_mode(dry_run: bool) -> i32 { let mut failed = c.fetch_errors > 0;
- for (slug, num) in &q.no_question {
+ if q.no_question.len() > DESIGN_DOCTOR_MAX_ROUTES {
+ println!(
+ "design-doctor: routing {DESIGN_DOCTOR_MAX_ROUTES} of {} rows this tick — the rest drain on the next runs",
+ q.no_question.len()
+ );
+ }
+ for (slug, num) in q.no_question.iter().take(DESIGN_DOCTOR_MAX_ROUTES) {
match design_doctor_route(slug, *num, dry_run) {📝 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.
| if q.no_question.is_empty() { | |
| println!("design-doctor: no noQuestion rows — nothing to route"); | |
| return if c.fetch_errors > 0 { 1 } else { 0 }; | |
| } | |
| let mut failed = c.fetch_errors > 0; | |
| for (slug, num) in &q.no_question { | |
| match design_doctor_route(slug, *num, dry_run) { | |
| Ok(line) => println!("{line}"), | |
| Err((_, msg)) => { | |
| eprintln!("{msg}"); | |
| failed = true; | |
| } | |
| } | |
| } | |
| if failed { | |
| 1 | |
| } else { | |
| 0 | |
| } | |
| } | |
| if q.no_question.is_empty() { | |
| println!("design-doctor: no noQuestion rows — nothing to route"); | |
| return if c.fetch_errors > 0 { 1 } else { 0 }; | |
| } | |
| let mut failed = c.fetch_errors > 0; | |
| if q.no_question.len() > DESIGN_DOCTOR_MAX_ROUTES { | |
| println!( | |
| "design-doctor: routing {DESIGN_DOCTOR_MAX_ROUTES} of {} rows this tick — the rest drain on the next runs", | |
| q.no_question.len() | |
| ); | |
| } | |
| for (slug, num) in q.no_question.iter().take(DESIGN_DOCTOR_MAX_ROUTES) { | |
| match design_doctor_route(slug, *num, dry_run) { | |
| Ok(line) => println!("{line}"), | |
| Err((_, msg)) => { | |
| eprintln!("{msg}"); | |
| failed = true; | |
| } | |
| } | |
| } | |
| if failed { | |
| 1 | |
| } else { | |
| 0 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review-report-rs/src/main.rs` around lines 28693 - 28712, Update
design_doctor_mode to process only a bounded number of q.no_question entries per
run, using a clear per-run route cap. Preserve existing failure handling for
processed entries, and report the number of unprocessed rows so the remaining
backlog is visible and can drain on later ticks.
| `refresh-human-queue` tick: | ||
|
|
||
| ```cron | ||
| 0 4 * * * PATH=$HOME/.nix-profile/bin:/usr/bin:/bin CRON_DIR=<install-dir> nix run git+file://<install-dir>#design-doctor >> <install-dir>/design-doctor.log 2>&1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Make the cron timezone explicit.
The documentation states 04:00 UTC, but 0 4 * * * uses the cron daemon's local timezone unless the host or crontab sets UTC. Add an explicit UTC setting or verify that every deployment host uses UTC.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 3101, Update the documented cron entry so its schedule
explicitly uses UTC, rather than relying on the cron daemon or deployment host
timezone; preserve the existing 04:00 schedule and command.
…ck wait `the_flag_reads_fan_out_and_every_list_stays_in_hit_order` and the four other fan-out tests share one helper, and it pinned neither of the two properties they assert. `await_another_worker` slept in a loop against a five-second deadline and, on expiry, SILENTLY RETURNED — the inversion it exists to force simply did not happen and the test carried on to fail a later assertion about something else. And even when it returned early it proved only that completions were inverted: a worker that entered the fan-out AFTER another had already finished and left satisfied it instantly, with the two never once in flight together. That is the observed CI failure — `peak 1`, "the reads never overlapped", on a run whose completion order was correctly inverted. `WorkerInversion` replaces it with a two-phase rendezvous on the workers themselves: - every other worker blocks until the `last` worker is in flight before it may record, so two workers are provably inside the fan-out at the same instant and the peak is at least two; - `last` blocks until one of them has recorded before it may record, so completion order provably differs from input order. Both waits are `Condvar` waits on the counterpart's ARRIVAL, so a stalled or throttled box makes both parties late together instead of making one give up, and a parked worker is what frees the core the other one needs. The remaining timeout is a deadlock guard, not a schedule: it can only expire when the counterpart can never arrive, and it panics naming that cause rather than proceeding. `WorkerInversion::over` refuses up front the input on which `QUEUE_FETCH_CONCURRENCY.min(items)` is below two, so a test that cannot be non-vacuous says so by name. All five call sites move to it: the two pool tests and the close-candidate, leak and design fan-out tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t is Mode was 100644 while campaign-run.sh, review-run.sh, refresh-human-queue.sh, backfill-human-queue-history.sh, sort-review-queue.sh and pr-review-report.sh are all 100755. Invisible to the pipeline — writeShellApplication re-emits the text into a fresh executable, so the flake build and every CI gate stay green either way — and it bites only an operator running ./design-doctor.sh from the checkout, which is the likeliest first thing a human does with a new runner that writes GitHub state. The earlier attempt staged the mode with `git update-index --chmod=+x` but left the working-tree file 644, so a later `git add -A` re-read the filesystem mode and reverted it. Both are set now. Refs #241 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed 9a34981: pass Verified on the head blob rather than from the report. The plan grew a typed variant per finding instead of a patch each — Rulings-conformance: checked against CLAUDE.md's rulings section and every ruling stated for this work.
Fifteen findings from an adversarial multi-agent review (6 finder angles, 37 independent verifiers, 6 refuted) were all addressed; two of them were mutation-proven test gaps and both mutants now die. The flaky CI: all checks pass; the single non-pass is |
Closes #241
Executes the ruling recorded in #241 (thedavidmeister, 2026-08-09): the daily FSM-conformance pass moves ALL
noQuestionai:designrows toai:needs-work, automatically. A design label whose question is gone is not a design state — the machine routes it back to the producer rather than parking it.New subcommand
pr-review-report design-doctor [--dry-run], wired as a flake runner (packages/apps.design-doctor) on a daily 04:00 UTC cron line, added in the same#PAUSED#style as every other cron on the box.Detection: one enumeration, one classifier
The pass runs
next_design's owndesign_open_prs_args()search, its archived-repo withholding (nd_population) and its classifier (nd_classify).DesignQueuegained a typedno_question: Vec<(String, u64)>bucket captured in the fold — no string-parsing of the withheld list, no second detector. The doctor acts on exactly the bucketcounts.noQuestionstates.The transition: a machine send-back is a VERDICT
The closest sibling is
draft_send_back_plan— the other automated send-back intoai:needs-work— and the doctor now writes what it writes: a🤖 ai:vetterReviewed <head>: needs-workcomment carrying the note and a stated lens, then one label edit adding the target and stripping every otherai:*. That gives it the two properties aRework notecould not: the comment is the currency stamp (so the next vetter run does not re-vet and delete the state this pass just wrote), and a machine decision does not wear the human's marker.The comment goes first, inverting the draft send-back's order by that order's own reasoning: there the label removes the PR from the leak population, here it is what keeps the row in this pass's own enumeration, so a failed label edit is re-planned next tick (the verdict dedups) while a failed comment after a label edit would strand a needs-work with nothing trusted behind it and outside the doctor's search.
Guards, each re-established at write time against a fresh fetch
ai:designpresent · no live trusted question (last_design_question, author-scoped) · no human decision (pr_human_sacred— ruling at head or native review) · no co-resident modeled state the strip would destroy (draft_send_back_strips_no_state's rule, asked ofclassify_laneper label) · not a draft · inside the fleet ($PR_ASSIGNEE) · settled for at least an hour. Fetch-field list pinned field-by-field; the enumeration itself withholds archived repos.The three design calls
Finding 3 — rows outside the fleet. Both consumers of
ai:needs-work(producer_pr_inventory,unvetted_fetch) enumerate--author $PR_ASSIGNEE, so routing a third party's PR would move it out of the one surface that named it and into a state nobody reads. Call: withhold and name, do not route. The row keepsai:design, socounts.noQuestionstill holds it — its pre-existing visibility is preserved rather than destroyed — and the report prints the row, its author, and whose move it is. Scoping the enumeration by author was the alternative and is wrong here:design_open_prs_argsis shared with the human's own read, which must keep seeing third-party design questions. The guard therefore sits in the plan, beside every other write-time guard, and fails closed on an unreadable author.Finding 5 — the second exit. Confirmed against the source: the producer's only exits from a needs-work are a rework push or a close-candidate, with
campaign-prompt.txtexplicitly forbidding the middle ground, and there is no transition anywhere in the crate by which a producer can say "this needs-work is spurious, re-judge me". Call: stop offering it. The note now offers exactly two moves, both real producer transitions — push the rework the PR was parked mid-way through (the ruling's "proceed with the PR", said in the pipeline's own vocabulary), or re-raise withpr-review-report flag-design, which is the producer's own transition and returns the row to the human's queue with a claim behind it. It also states "do not push a no-op commit", so the order cannot be read as inviting the push the prompt forbids.Finding 7/15 — rows the pass can never drain. A native review is a property of the PR, not of a head, so
HumanSacredrecurs for ever;NoAnchordid the same and red the cron daily. Call: a classification is never a run failure, and every withheld class names its consumer.NoAnchoris now anOkclassification (only a failed read or a failed write reds the tick), each class carries a typedclass()name, and the three the pass can never drain by itself —human-decided,not-our-fleet,no-anchor— print the transition that does drain them (a human'shuman-rule, or closing the PR). The log is the queue; an undifferentiatedOkline was not.Everything else, fixed against its sibling
ai:close-candidate/ai:blocked-ondraft_send_back_strips_no_state/VetAction::SkipDraftInStateDISABLED; 30mtimeoutcampaign-run.sh:81,review-run.sh:71,MAXTIME/REVIEW_MAXTIMERework note(currency stamp; no human marker)draft_send_back_plandesign_doctor_mode(read and filter pinned)next_design_fetchpin in the same testDESIGN_DOCTOR_ROUTE_FIELDSpinned field-by-fieldPR_RULE_FIELDS/ISSUE_RULE_FIELDSinthe_issue_transition_fetches_what_its_guards_need### Vetting is a pure functionh3 (h3 count back to 35, matching main)100755retire_blocked_infra_modeThe six refuted candidates were not acted on.
Dry-run over the live population (evidence — no transition was executed)
All six are fleet-authored, settled, non-draft, single-state and carry no human decision, so all six route. They are the six the issue names: cyclofinance/cyclo.site#412, rainlanguage/rain.erc4626.words#233, rainlanguage/rain.erc4626.words#166, rainlanguage/raindex#2778, rainlanguage/raindex#2721, rainlanguage/raindex#2720.
Sibling-overlap note
#243 (issue #240) landed while this was open and is merged in. Sibling agents are working #241/#244/#245 in their own clones; this branch's only touch on shared
next_designmachinery stays minimal — one field onDesignQueue, a zip innd_classify's fold, one destructure innext_design_fetch— plus two additive assertions inside existing conformance tests.The unrelated red this PR also fixes: the fan-out tests' rendezvous
test / rs-test (ubuntu-latest)was this PR's only red, and it is pre-existing onmainand has nothing to do with the design doctor — this diff does not touchnext_close_candidate. The identical SHA passed on the push run (https://github.com/rainlanguage/issue-pr-cron/actions/runs/31316205820) and failed on the PR run (https://github.com/rainlanguage/issue-pr-cron/actions/runs/31316209100), which is the signature of a timing flake, not a regression. It is fixed here because it is what blocks this PR, not because the doctor caused it.Observed:
next_close_candidate_tests::the_flag_reads_fan_out_and_every_list_stays_in_hit_orderpanicking onthe reads never overlapped: this is the serial queue #235 is about, peak 1.The defect
Five fan-out tests share one helper, and it pinned neither of the two properties they assert:
Instant::now()while running nothing.ready()instantly, with the two never once in flight together. That is the failure actually observed —peak 1on a run whose completion order was correctly inverted, so theassert_ne!above it passed and the parallelism assertion below it did not. That half was never a timeout problem at all, which is why a bigger timeout was the wrong fix: it converts a frequent flake into a rare one, and leaves the real hole open.The fix:
WorkerInversion, a two-phase rendezvous on the workers themselvesmap_boundedspawnsQUEUE_FETCH_CONCURRENCY.min(items.len())workers, so with at least two items at least two workers exist BY CONSTRUCTION. The rendezvous turns that arithmetic fact into an observed interleaving:lastworker is in flight before it may record — so two workers are provably inside the fan-out at the same instant and the peak is at least two;lastblocks until one of them has recorded before it may record — so completion order provably differs from input order.Both waits are
Condvarwaits on the counterpart's arrival, never on a clock, so a stalled or throttled box makes both parties late together instead of making one give up — and a parked worker is precisely what frees the core its counterpart needs. They cannot deadlock against each other:lastpublishes its own arrival BEFORE it waits, andmap_boundedhands out index 0 first, so the worker the others are waiting for is never itself one of the waiters.Neither loudness property is given up to get determinism:
WorkerInversion::over(items)refuses up front the input on whichQUEUE_FETCH_CONCURRENCY.min(items)is below two — a test that cannot be non-vacuous says so, by name, instead of proceeding to a confusing downstream assertion.a fan-out that ran the items ONE AT A TIME, which is the serial queue #235 is about). No test can hang.All five callers move to it, and no assertion is weakened — the order assertions and the
peak > 1assertions are untouched:parallel_queue_tests::map_bounded_returns_input_order_not_completion_order0parallel_queue_tests::a_slow_first_fetch_keeps_its_place_in_the_queuerainlanguage(index 0 of 2)next_close_candidate_tests::the_flag_reads_fan_out_and_every_list_stays_in_hit_ordernum == 0next_leak_tests::the_leak_reads_fan_out_and_the_unknowns_stay_in_candidate_orders.number == 0next_design_tests::the_design_reads_fan_out_and_every_list_stays_in_candidate_ordernum == 0QA
Discriminating tests (19 in
design_doctor_tests, plus two extended conformance tests):the_doctors_targets_are_exactly_the_classifiers_noquestion_bucket,a_bare_design_label_routes_and_the_send_back_is_the_one_state_move,a_co_resident_modeled_state_withholds_the_route,a_live_trusted_question_withholds_the_route_whoever_raised_it,a_spoofed_raising_marker_does_not_park_the_row,a_human_decision_dominates_the_doctor,the_human_check_precedes_every_other_withholding,a_terminal_or_unlabelled_row_is_never_routed,an_anchorless_row_is_a_named_class_with_a_stated_consumer_not_an_error,a_pr_that_became_a_draft_after_indexing_is_not_routed,a_pr_outside_the_fleet_is_withheld_and_named_never_routed,a_pr_written_to_inside_the_settling_window_is_withheld,the_route_posts_a_vetter_verdict_first_then_the_one_state_label_edit,a_routed_row_reads_as_vetted_at_head_and_carries_a_work_order,the_note_names_the_why_and_only_exits_the_producer_can_perform,an_already_recorded_verdict_at_the_same_head_dedups,every_plan_class_is_named_and_the_undrainable_ones_name_their_consumer,no_withheld_class_fails_the_tick_and_only_a_failed_write_does,the_design_doctor_subcommand_parses_with_the_standard_dry_run; andevery_org_wide_enumeration_withholds_archived_repos+the_issue_transition_fetches_what_its_guards_needextended to cover this pass. Each fails on the pre-review head — proven by the mutation table below, where the mutants ARE that head's behaviour (Rework-note send-back, no fleet/settling/draft/strip guards,NoAnchorasErr, unpinned fields, unpinned archived filter) and every one of them is killed.Mutations applied (one at a time, whole suite run, tree restored from the committed baseline; 18 mutants, 18 killed, 0 survivors):
a_co_resident_modeled_state_withholds_the_routeai:close-candidatefrom that question → samea_pr_outside_the_fleet_is_withheld_and_named_never_routeda_pr_written_to_inside_the_settling_window_is_withheldRework noteinstead of the vetter verdict →the_route_posts_a_vetter_verdict_first_then_the_one_state_label_editno_withheld_class_fails_the_tick_…HumanSacredstops naming its consuming move →a_human_decision_dominates_the_doctor,every_plan_class_is_named_…a_pr_that_became_a_draft_after_indexing_is_not_routedevery_org_wide_enumeration_withholds_archived_reposreviewDecisionfrom the fetch →the_issue_transition_fetches_what_its_guards_needauthorfrom the fetch → sameNoAnchoranErragain →no_withheld_class_fails_the_tick_and_only_a_failed_write_doesthe_note_names_the_why_and_only_exits_the_producer_can_performflag-designinstruction with "mention it in a comment" → same (this one survived the first pass: the assertion matched the command NAME while the note had stopped naming a transition; strengthened to require the instruction, which is finding 5's own defect class)NoAnchor-as-Errsurvived because the route was reachable only through livegh—design_doctor_route_fromis now the seamnd_classify'sfetchparameter is, driven per class with a writer that panics if a withheld class writes anything; and the "proceed" mutant survived by landing in a stale module comment, which described the retired design and is now rewritten.design_doctor_mode's enumeration half (the same status asnext_design_fetch) — every decision it makes lives in the pure functions above, and the live--dry-runover the six real rows is its evidence.Oracle: the ruling text of FSM doctor: route every noQuestion ai:design row back to ai:needs-work #241 for the transition, and for every guard the SIBLING that already implements it (
draft_send_back_plan,draft_send_back_strips_no_state,campaign-run.sh's kill switch andMAXTIME,retire_blocked_infra_mode's serial loop,PR_RULE_FIELDS' pinning test, thenext_design_fetcharchived pins). Expected values derive from those siblings and from the consuming readers (vetted_at_head,needs_work_state,pr_assignee), never recomputed from the doctor's own implementation.Category check: the review named 15 findings (12 confirmed, 3 plausible); all 15 are addressed — 1,2,4,6,8,9,10,11,12,13,14 mechanically against their siblings, 3/5/7+15 as stated design calls above. The six refuted candidates are deliberately untouched. Gates, re-run on the merge with
origin/main(feat(runners): --force one manual run past the usage-gate pause #246 and Remove every human-ruled STATE: a human decision is a transition, not a bucket #247 both landed after this branched and both touchedmain.rs): full suite 1105 unit + 94 integration green, this PR's own 19design_doctor_testsamong them; CI's exact static gate —nix develop github:rainlanguage/rainix/53e96a7…#rust-shell -c rainix-rs-static— exits 0, so rustfmt and clippy are both clean over the merged tree.The fan-out rendezvous (the unrelated red above), loop evidence. Run in the FOREGROUND on this branch's HEAD under deliberate CPU starvation — one busy-loop per core on a 12-core box, load ~10.5 through the run: 100 iterations × the 5 rendezvous tests = 500 runs, 0 failures. The same loop against the wall-clock helper reproduced the CI failure locally rather than merely arguing it: 3 failures in 250 runs, every one of them
the reads never overlapped … peak 1at the parallelism assertion, with the completion-order assertion above it passing — the exact panic from the red run, and the reason a longer timeout was the wrong fix.Mutations applied to
map_bounded(one at a time, whole 1105-test suite run each time, tree restored from the committed baseline; 2 mutants, 2 killed, 0 survivors):let workers = QUEUE_FETCH_CONCURRENCY.min(items.len())→1, i.e. a SERIAL pool → killed by all five rendezvous tests plusmap_bounded_holds_the_cap_and_reaches_it(1099 passed, 6 failed), each naming the cause rather than a downstream symptom: "the inversion never happened: no other worker recorded a completion while this one held its item open waiting for one, and 60s went by. The wait is on the other WORKER, not on a clock, so this is not a slow box — it is a fan-out that ran the items ONE AT A TIME, which is the serial queue State-loads fetch candidates serially on a "population is small" assumption nothing enforces — map_bounded already exists and has one caller #235 is about". The deadlock guard is what bounds it: the run FAILS, it does not hang.a_slow_first_fetch_keeps_its_place_in_the_queue, all three lane fan-out tests andthe_doctors_targets_are_exactly_the_classifiers_noquestion_bucket— the ordering assertions are live, and none of them was weakened to make the rendezvous pass.🤖 Generated with Claude Code