fix: report the PR source as unavailable when gh pr list fails, never as empty - #247
Conversation
… as empty
CodeRabbit, Major, found on cs-toolkit's copy of this workflow rather than here —
which is the shared-workflow design paying for itself on its first day: an adopter
converting its forked adapter to a pointer got the fork's reviewer aimed at the
kit's text.
The gather told you what to do when `gh pr list` TRUNCATES and said nothing about
what to do when it FAILS. A non-zero exit, a partial response, or an absent `gh`
all return nothing, and nothing is indistinguishable from a repo that genuinely
has no open PRs. The first case renders an all-clear 🔴 bucket off a command that
never ran — a briefing that is confidently wrong rather than visibly incomplete,
which is the same failure the truncation caveat two lines above exists to prevent.
What makes it clearly a defect rather than a design choice: the TRACKER source in
this same step already carries exactly this rule ("if the script/key fails, note
the gap and continue … never act on a partial payload"). The two sources differ
only in that one said so. The render template now has an explicit
`PRs: unavailable (<reason>)` alternative so the gap has somewhere to appear.
Deliberately narrow. This is not the search-route work: PR #244 tried to give the
remediation check a route when no parameter-passing search tool exists, drew two
HIGHs from both panel lenses, and was closed unmerged under a pre-declared bound
(#245 carries the findings). This touches a different paragraph, a different
failure mode, and nothing #245 covers.
VERIFIED: `make test` 721 passed; `python scripts/kit_doctor.py` 32 unchanged, 0
differ, 0 missing. Neither executes this prose. The finding itself came from a
reviewer reading it, and the fail-open it describes has not been reproduced live —
stated because "a failed gh pr list renders as all-clear" is a claim about agent
behaviour I have not executed.
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe session-start workflow now reports failed or unavailable sources explicitly, distinguishes failed PR queries from truncated results, and supports unavailable PR and CI/cron statuses with reasons. The manifest contains the updated workflow hash. ChangesSession-start reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
…the file's own bracket convention Panel round 1 on this PR — both lenses, no HIGH. Three findings, all acted on. 1. BOTH LENSES: the render template broke the file's own alternation convention. Every other either/or wraps BOTH sides in one bracket group (`<clean | N uncommitted/untracked>`, `<all green | N failed/skipped>`, `<delegate ⇒ … | inline ⇒ …>`). Mine put the `|` outside: `<N> open PRs | PRs: unavailable (<reason>)`. An agent following the template literally emits `0 open PRs | PRs: unavailable (gh: command not found)` — a fabricated zero in front of the caveat, which recreates the exact fail-open the paragraph exists to close. Now `<N open PRs | PRs unavailable: reason>`. Verified: `grep -n '<[^<>]*|[^<>]*>'` returns every alternation in the file and all of them are fully wrapped; no bare `|` survives outside a bracket. 2. BOTH LENSES, and the correctness one found the sharper version: my consistency argument was selective. I claimed the PR and tracker sources "differ only in that one said so" — while the CI/cron field IMMEDIATELY TO THE RIGHT in the same rendered line carries the identical fail-open with no handling at all. A cron health command that never ran renders as `all green`. So the rule is not a PR-source rule. It is now stated once for the whole gather, and CI/cron gets the `unavailable: reason` alternative it lacked. This is the same rule applied at its correct scope, not a new mechanism — the finding was that my scope was wrong, and narrowing the claim instead would have left a known fail-open in the field next to the one I was fixing. 3. ADVERSARIAL: "a partial response returns nothing" was wrong. A truncated but parseable response returns FEWER rows, not none — no exit code reveals it, and it is the adjacent full-page check's concern, not this one's. The two failure shapes are now distinguished explicitly instead of being collapsed. WHAT THE LENSES ALSO ESTABLISHED, worth carrying: both mutation-tested this change in isolated copies and both got `720 passed, 1 deselected` with the mutation SURVIVING. One reverted the whole paragraph, the other inverted its opening sentence to say the opposite. Nothing behavioural caught either. This file has no test coverage of its prose meaning at all, so the panel is the only check it gets — which is the argument for having run one on a nine-line diff. VERIFIED: `make test` 721 passed; `python scripts/kit_doctor.py` 32 unchanged, 0 differ, 0 missing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/agentic-dev-kit/workflows/session-start.md`:
- Around line 40-46: Update the config-drift rendering flow in the session-start
briefing to distinguish a failed or unavailable check from a genuine empty drift
result. When the drift command returns no usable result because of a non-zero
exit, missing binary, or absent credential, render “Config drift unavailable:
<reason>”; retain the existing all-clear rendering only for a successful check
with no outstanding drift.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 0dfcb405-5d34-46e7-ba61-c388a54797e7
📒 Files selected for processing (2)
docs/agentic-dev-kit/workflows/session-start.mdkit-manifest.json
| **A source that fails is reported as unavailable, never as its empty value.** A | ||
| non-zero exit, a missing binary, or an absent credential returns nothing — and | ||
| nothing is indistinguishable from a genuine zero: no open PRs, no failing jobs, no | ||
| drift. Rendering the empty value turns a command that never ran into an all-clear, | ||
| which is worse than a visible gap because nothing on the briefing says to look | ||
| again. This applies to every source below; the tracker bullet states it for itself | ||
| only because it was written first, not because it is the only one it covers. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="docs/agentic-dev-kit/workflows/session-start.md"
rg -n -C 4 'config-drift|unavailable|All clear|handoff|friction-log|Tracker|CI/cron|PRs' "$file"Repository: topij/agentic-dev-kit
Length of output: 10116
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="docs/agentic-dev-kit/workflows/session-start.md"
sed -n '34,72p;230,262p' "$file" | cat -nRepository: topij/agentic-dev-kit
Length of output: 5324
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
doc = Path("docs/agentic-dev-kit/workflows/session-start.md").read_text()
# Extract relevant sections by heading/index rather than line numbers.
gather = doc[doc.index("### 0 · Gather"):doc.index("### 1 · Classify")]
render = doc[doc.index("### 3 · Render the briefing"):doc.index("## Notes")]
unavailable_rule = re.search(
r"A source that fails is reported as unavailable.*This applies to every source below",
gather,
re.S,
).group(0)
print("Unavailable general rule present:", unavailable_rule.strip())
print("unavailable_rule_mentions_early": unavailable_rule.strip()[:80])
def has_unavailable_alternative(section, source):
if source in ("PRs", "open PRs"):
return "PRs unavailable" in section or "unavailable" in section
if source in ("CI/cron", "cron/CI"):
return "unavailable: reason" in section
if source == "Tracker":
return "if the script/key fails" in section or "gap" in section
return "unavailable" in section
for s in ["tracker", "PRs", "CI/cron", "config-drift"]:
print(s, "has_unavailable_alternative:", has_unavailable_alternative(render, s))
print("\nConfig drift instructions:")
patterns = [
"config-drift check only appears when your drift check reports something outstanding",
"Config drift unavailable",
"unavailable: reason",
]
for p in patterns:
print("contains", p, "=>", p in render)
PYRepository: topij/agentic-dev-kit
Length of output: 334
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
doc = Path("docs/agentic-dev-kit/workflows/session-start.md").read_text()
gather = doc[doc.index("### 0 · Gather"):doc.index("### 1 · Classify")]
render = doc[doc.index("### 3 · Render the briefing"):doc.index("## Notes")]
match = re.search(
r"A source that fails is reported as unavailable.*This applies to every source below",
gather,
re.S,
)
print("Unavailable general rule present:", bool(match))
print("Rule text:", match.group(0).strip() if match else "")
def has_unavailable_alternative(section, source):
if source in ("PRs", "open PRs"):
return "PRs unavailable" in section or "unavailable" in section
if source in ("CI/cron", "cron/CI"):
return "unavailable: reason" in section
if source == "Tracker":
return "if the script/key fails" in section or "gap" in section
return "unavailable" in section
for s in ["tracker", "PRs", "CI/cron", "config-drift"]:
print(s, "has_unavailable_alternative_on_render_page:", has_unavailable_alternative(render, s))
print("\nConfig drift instructions:")
for p in [
"config-drift line only appears when your drift check reports something outstanding",
"Config drift unavailable",
"unavailable: reason",
]:
print(repr(p), "=>", p in render)
PYRepository: topij/agentic-dev-kit
Length of output: 1070
Add an unavailable render path for config drift.
config-drift is included in the “every source” rule and can return nothing on a failed non-zero command, but the briefing template only renders it when outstanding drift is reported. A failed drift check can omit the line and still render ✅ All clear. Render Config drift unavailable: <reason> when the drift check fails.
🤖 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 `@docs/agentic-dev-kit/workflows/session-start.md` around lines 40 - 46, Update
the config-drift rendering flow in the session-start briefing to distinguish a
failed or unavailable check from a genuine empty drift result. When the drift
command returns no usable result because of a non-zero exit, missing binary, or
absent credential, render “Config drift unavailable: <reason>”; retain the
existing all-clear rendering only for a successful check with no outstanding
drift.
…ift render slot, and an accurate enumeration Delta pass, one MEDIUM regression, no HIGH. Declared before this round: it is the last fix round on this PR; anything surviving is logged rather than patched again. THE FINDING. Round 1's widened preamble claimed the rule "applies to every source below" and named "no drift" among its examples — while the config-drift render note was byte-identical to base and still binary: the line "only appears when your drift check reports something outstanding". So a drift check that could not run rendered exactly like one that found nothing. The paragraph named the failure shape it left unfixed, which is round 1's own finding recurring one level up: widening the claim created coverage I had not operationalised. Fixed by giving config-drift the slot the other two already had, and by replacing "every source below" with an enumeration that is checkable. AND THE ENUMERATION WAS WRONG ON ITS FIRST DRAFT — caught by verifying it rather than by a reviewer. The replacement sentence said all four named sources "each have a slot in Render the briefing". The tracker does not; it carries the rule inline and reports its gap in prose. Grepping for a slot per named source returned nothing for the tracker, before push. That is the third overclaim in three rounds on this paragraph, and the only one caught by its author. Every claim the paragraph now makes was re-checked mechanically: PR list has a render slot 1 CI/cron has a render slot 1 config-drift has a render slot 1 tracker states it inline 1 file reads excluded, with why 1 Also from the delta pass, NOT fixed and logged instead, per the declared bound: `<N open PRs | …>` puts its label inside the alternation while `CI/cron: <all green | …>` puts it outside. Both render sensibly; it is template-authoring inconsistency, not a defect, and a third round to restyle it would cost more than it returns. WHAT THREE ROUNDS ESTABLISHED ABOUT THIS FILE. All three lenses mutation-tested it independently and all three got the mutation SURVIVING at `720 passed, 1 deselected` — reverting the paragraph, inverting its opening sentence, and restoring the old render line respectively. There is no behavioural coverage of this document's meaning. The panel is the only check it gets, and on a change this size it found a real defect in every round. VERIFIED: `make test` 721 passed; `python scripts/kit_doctor.py` 32 unchanged, 0 differ, 0 missing.
|
Round 2 — your Major is already fixed, and it's a clean independent confirmation. The finding is real and staleYou reviewed Correct. It was fixed in
Verified by Two independent reviewers, same defectThe fallback panel's adversarial lens ran on That's worth recording rather than just noting: you were rate-limited across most of this PR, so the panel has been the substitute. Here the two overlapped on one head and converged on the same finding, which is the first direct evidence this session that the substitute is aimed at the same things the bot is. What the fix also exposed, caught before pushThe sentence replacing "every source below" claimed all four named sources have a render slot. The tracker doesn't — it carries the rule inline and reports its gap in prose. Found by grepping for a slot per named source before pushing, not by a reviewer. Third overclaim in three rounds on that one paragraph, and the only one I caught myself. The pattern is consistent: each time I widened the scope sentence, it outran what I'd actually implemented. What finally held was replacing the sweeping claim with an enumeration that can be checked mechanically — BoundDeclared before this round:
|
1 participant
Found by CodeRabbit reviewing cs-toolkit's copy of this file, not this repo's —
which is the shared-workflow design paying for itself on its first day. An adopter
converting its forked
/session-startadapter to a pointer put the fork's reviewerin front of the kit's text, and it found a bug here.
The defect
The gather says what to do when
gh pr listtruncates and nothing about whatto do when it fails.
A non-zero exit, a partial response, or an absent
ghall return nothing — andnothing is indistinguishable from a repo that genuinely has no open PRs. The first
case renders an all-clear 🔴 bucket off a command that never ran: a briefing
that is confidently wrong rather than visibly incomplete, which is precisely what
the truncation caveat two lines above exists to prevent.
Why it is a defect and not a design choice
The tracker source in this same step already carries exactly this rule:
Two sources in one step, one fail-closed and one not. They differ only in that one
said so.
The change
empty, and names the three ways it returns nothing.
PRs: unavailable (<reason>)alternative,so the gap has somewhere to appear instead of silently becoming
0 open PRs.Scope
Deliberately narrow, and deliberately not the search-route work. PR #244 tried
to give the Remediation check a route when no parameter-passing search tool
exists; both panel lenses returned HIGH on it and it was closed unmerged under a
bound declared before the second lens reported. #245 carries those findings and the
constraints for a next attempt.
This is a different paragraph, a different failure mode, and touches nothing #245
covers. It is separated because cs-toolkit's conversion PR is blocked on it and
should not wait on a design question.
Verification
make test→ 721 passed.python scripts/kit_doctor.py→ 32 unchanged, 0 differ,0 missing.
What that does not establish. Neither executes this prose. The finding came
from a reviewer reading it, and I have not reproduced the fail-open live — "a
failed
gh pr listrenders as all-clear" is a claim about agent behaviour, and theargument for the fix rests on the inconsistency with the tracker source rather than
on an observed briefing.
Summary by CodeRabbit