fix(project-board): refuse truncated board reads instead of trusting them - #2699
Conversation
…them The card told a run to "pass an explicit high limit" to gh project item-list. Measured 2026-08-06, that is not sufficient: --limit 3000 returned exactly 3000 items against a true totalCount of 4988, with no warning, no error and exit 0 -- so ~40% of the board was silently absent and every health check below would have reported clean. The same pass also left the GraphQL budget at 73/5000. That budget is attached to the user, so all three lanes share it, and the surveyor's paginated reviewThreads queries are what the pentad's unresolved-thread count depends on -- a starved pentad reads clean, failing open on the promotion gate. Prefer per-issue projectItems membership (~1 point, and it distinguishes "on no project" from "on project 5"). Where the whole board is genuinely needed, pair the enumeration with a truncation guard that treats returned == limit or returned < totalCount as a hard failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…easured reason
Self-review flagged the cond && { ...; exit 1; } form. The first
justification written for it was wrong and testing caught it: set -e does
NOT abort mid-script there, because it exempts a non-final component of
an AND-OR list.
The real hazard, measured: on the healthy path that form evaluates to 1,
so as the last command of a script or function it returns a spurious
failure -- and as a function's last line it took a set -e caller down
(outer rc=1). The if-form has no such edge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review done at this head before requesting: 54-line docs-only change to one skill file, the Requesting review now rather than after CI because every check on this head is @coderabbitai review |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughUpdated project-board health-check guidance to avoid broad item enumeration for coverage checks. Per-issue 🚥 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 |
![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 @.claude/skills/products/project-board/SKILL.md:
- Around line 50-54: Update the item-list retrieval around gh project item-list
so its command status is checked with if !, preserving the command’s output and
stderr while rejecting failures. Validate both n and total after extraction as
non-empty integer counts, including null and non-numeric values, before
performing numeric comparisons; only run the existing truncation and
completeness checks after validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e74dd6f6-1795-467d-8833-944e7c80d2c9
📒 Files selected for processing (1)
.claude/skills/products/project-board/SKILL.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-07T16:28:59.865Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2077
File: .claude/adr/0001-kagent-agent-fleet-substrate.md:35-35
Timestamp: 2026-07-07T16:28:59.865Z
Learning: In this repo, markdownlint CI only runs for `docs/**`, so Markdown files under `.claude/` (e.g., ADRs like `.claude/adr/*.md`) won’t be automatically linted. During code review, treat `.claude/**/*.md` as needing the same markdownlint-clean style as the docs—e.g., avoid MD022 issues like missing/incorrect blank lines around headings—and flag markdownlint violations even though CI won’t catch them.
Applied to files:
.claude/skills/products/project-board/SKILL.md
🪛 SkillSpector (2.5.1)
.claude/skills/products/project-board/SKILL.md
[error] 29: [AR2] Anti-Refusal Statement: Skill instructs the agent to omit warnings, disclaimers, or ethical commentary. Stripping safety caveats hides risk from the user and is a common jailbreak preamble.
Remediation: Remove instructions that suppress warnings, disclaimers, or ethical commentary. Let the agent surface safety-relevant caveats to the user.
(Anti-Refusal (AR2))
[warning] 29: [OH3] Unbounded Output: Output size or generation rate is not bounded. Unbounded output enables denial-of-service through resource exhaustion, log flooding, or context-window stuffing.
Remediation: Set explicit limits on output length, generation count, and rate. Use max_tokens and truncation to prevent unbounded output.
(Output Handling (OH3))
[error] 3: [YR4] YARA rule 'agent_skill_mcp_tool_poisoning_metadata': MCP/tool metadata poisoning indicators in tool schemas or skill manifests [agent_skills]: YARA rule matched a hack tool or exploit indicator (offensive tools, reconnaissance, privilege escalation, or exploit frameworks).
Remediation: Remove offensive tool references and exploit code. Legitimate agent skills should not contain penetration testing tools, exploit frameworks, or reconnaissance utilities.
(YARA Match (YR4))
…rors The guard read its count through a single pipeline, so a non-zero gh left the count empty. Both integer comparisons then errored, and a failing [ inside an if condition is exempt from set -e, so the guard reached its success path and exited 0 having counted nothing. Each call is now checked on its own line and both counts are asserted numeric.
Re-requesting at the new head. @coderabbitai review |
|
|
The 20:22Z request was rate limited and returned an empty reply container rather than a review, so the current head still carries no external review. Re-requesting now that the window has passed. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.claude/skills/products/project-board/SKILL.md:
- Around line 42-44: Update the project-item lookup described in the project
board instructions to resolve the expected organization project by owner and
compare its globally unique project.id, rather than matching project.number
alone. Ensure the query or comparison includes owner identity so projects with
the same number under different owners cannot match.
- Around line 42-44: Update the project membership query in the project-board
instructions to paginate projectItems using pageInfo.hasNextPage and
pageInfo.endCursor, continuing until all pages are checked before declaring an
issue missing; alternatively enforce a documented repository-specific maximum
that safely covers all memberships.
- Around line 42-44: Update the Issue.projectItems query in the project-board
skill to pass includeArchived: false, ensuring coverage reads exclude archived
project items while preserving the existing project number lookup.
- Around line 41-44: Update the per-issue projectItems verification around the
gh api graphql read to fail closed: preserve stderr, check the command exit
status, and report the membership result as unverified when the read fails. Only
interpret an empty projectItems result as “not on a project” after a successful
query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ee1ffc3-5997-4cb4-9a51-4e8a8a0728c6
📒 Files selected for processing (1)
.claude/skills/products/project-board/SKILL.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-07T16:28:59.865Z
Learnt from: devantler
Repo: devantler-tech/monorepo PR: 2077
File: .claude/adr/0001-kagent-agent-fleet-substrate.md:35-35
Timestamp: 2026-07-07T16:28:59.865Z
Learning: In this repo, markdownlint CI only runs for `docs/**`, so Markdown files under `.claude/` (e.g., ADRs like `.claude/adr/*.md`) won’t be automatically linted. During code review, treat `.claude/**/*.md` as needing the same markdownlint-clean style as the docs—e.g., avoid MD022 issues like missing/incorrect blank lines around headings—and flag markdownlint violations even though CI won’t catch them.
Applied to files:
.claude/skills/products/project-board/SKILL.md
🪛 SkillSpector (2.5.1)
.claude/skills/products/project-board/SKILL.md
[error] 29: [AR2] Anti-Refusal Statement: Skill instructs the agent to omit warnings, disclaimers, or ethical commentary. Stripping safety caveats hides risk from the user and is a common jailbreak preamble.
Remediation: Remove instructions that suppress warnings, disclaimers, or ethical commentary. Let the agent surface safety-relevant caveats to the user.
(Anti-Refusal (AR2))
[warning] 29: [OH3] Unbounded Output: Output size or generation rate is not bounded. Unbounded output enables denial-of-service through resource exhaustion, log flooding, or context-window stuffing.
Remediation: Set explicit limits on output length, generation count, and rate. Use max_tokens and truncation to prevent unbounded output.
(Output Handling (OH3))
[error] 3: [YR4] YARA rule 'agent_skill_mcp_tool_poisoning_metadata': MCP/tool metadata poisoning indicators in tool schemas or skill manifests [agent_skills]: YARA rule matched a hack tool or exploit indicator (offensive tools, reconnaissance, privilege escalation, or exploit frameworks).
Remediation: Remove offensive tool references and exploit code. Legitimate agent skills should not contain penetration testing tools, exploit frameworks, or reconnaissance utilities.
(YARA Match (YR4))
🔇 Additional comments (1)
.claude/skills/products/project-board/SKILL.md (1)
26-40: LGTM!Also applies to: 46-80
…-closed bar The per-issue query replaced enumeration but inherited its traps. It matched the board by project number, which is unique only within one owner, so a repository-level or other-owner project of the same number satisfied it and reported an unboarded issue as covered. It took the first 20 items with no pagination, which truncates exactly as the enumeration does and answers not on the board for an issue that is. And it could not tell an empty result from a failed query, so a read error read as verified absence. The read now resolves the board by id, paginates to exhaustion, excludes archived items explicitly, and returns three distinct outcomes: on the board, verified absent, and unverified. Verified against live data: a boarded issue returns 0, platform#1 returns 1, and an unresolvable repository returns 2.
CodeRabbit reviewed @codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CodeRabbit is rate limited (51 min) and Codex reports its code-review usage limit is reached, both at this head. Advancing to the third lane. |
|
@cursor review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_9098fbef-88af-4764-97f5-f31624632071) |
…bles The helper assigns in the caller's scope, so its page and cursor variables are now prefixed rather than taking the common names page and after from whatever copies it. Its structural probe no longer discards stderr, matching the rule this same card states two paragraphs later: a suppressed error is indistinguishable from an empty result. The two behaviours a reader is most likely to mistake for bugs — the typed null on the first page and index returning a truthy zero — are recorded next to the code so neither gets corrected into a fail-open.
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 4440d8684aed3c6fe7ae75470f9b9b0b63a37261
All three lanes were requested in priority order at this PR's heads today and each returned a provider quota state, not a verdict. None states a usable retry window inside this run:
- CodeRabbit — delivered a real review at
b73a142(4 actionable findings, all fixed in5e669e9), then rate limited: "Next review available in: 51 minutes" (01:15Z). - Codex — "You have reached your Codex usage limits for code reviews" (01:26Z).
- Cursor Bugbot — check-run
neutralwithoutput.title: Error, comment "Bugbot couldn't run - usage limit reached" (01:29Z). TheErrortitle means the run never started, as distinct from a review that found nothing.
What I checked, behaviourally
This PR is a procedure card, so the thing that can be wrong is the recipe. I extracted the on_board helper verbatim from the rendered card and ran it against live data, asserting all three outcomes rather than only the happy one:
| case | expected | result |
|---|---|---|
| an issue known to be on the board | 0 |
0 |
platform#1 (the card's own negative control) |
1 |
1 |
| an unresolvable repository | 2 |
2 |
The third is the one that matters: before this change a failed read and an empty result were the same answer downstream, which is the fail-open the PR exists to close. Re-ran after the variable rename, so the verification is against the shipped text and not an earlier draft.
Findings I raised on my own diff, and fixed
- The helper assigned
pageandafterin the caller's scope — common enough names that a copy of this snippet could clobber them. Prefixed. - Its structural probe discarded stderr, two paragraphs above the card's own rule that a suppressed error is indistinguishable from an empty result. Removed.
- The explanatory note landed inside the fence rather than after it. Fence balance re-checked (6, even).
Reviewed for the failure this card is about
Every claim in the changed text is one I can point at evidence for: the --limit 3000 → 3000-of-4988 measurement, the shared-budget figure, and the set -e behaviour of the && form (which the card correctly warns against and does not use). The two lines most likely to be "corrected" into a fail-open — the typed null cursor and index() returning a truthy 0 — now carry their reasoning inline.
Verdict: no P0/P1 findings
Readiness at
|
1 participant
Caution
PARKED — CI is wedged GitHub-side on this branch. Do not promote; do not merge.
Measured 2026-08-06T23:2xZ. This branch's
CIrun (31125693200, created 18:18:41Z during theActions
major_outage) is a zombie:gh run viewreportsstatus: queuedwhilegh run cancelrefuses with "Cannot cancel a workflow run that is completed". The two APIsdisagree and neither state can be acted on.
Consequently no CI has run at the current head
b73a1420e7— the only check-runs present aretwo CANCELLED CodeQL analyses from the previous head
ef543c2a41.close+reopen(which firespull_request: reopened, a default type for this workflow) produced no new run.Positive control, so this is not a repo-wide CI failure:
monorepoCI ran green on a differentbranch (
claude/cred-table-blob-run-evidence-2522) at 22:56:59Z. The wedge is specific to thisbranch's concurrency group
ci-CI-refs/pull/2699/merge.Unblocks when GitHub reaps the zombie run, or the next real push to this branch dispatches a
fresh event. Nothing in the diff is implicated — no code fix is owed here.
Why
The board's health checks are how we notice the board drifting. Right now they can't be trusted: the card tells a run to read the board with "an explicit high limit", and that quietly returns a partial board with no sign anything was missing — so a check comes back "100% clean" off ~60% of the data. A prior run nearly filed a fabricated finding off exactly this.
It also has a side effect worth caring about: one board read consumes nearly the whole hourly GitHub query budget, which all three agent lanes share. When that budget is gone, the PR review-thread checks come back empty too — and empty reads as "nothing unresolved", which is the wrong way for a safety check to fail.
What
Stops treating a partial board read as an answer. Coverage is now checked from the issue's side, which is cheap and exact; where the whole board really is needed, the read has to prove it got all of it or refuse. Both failure modes now stop the check instead of silently passing it.
Fixes #2698
Blocked — GitHub Actions outage (not a defect in this change)
Parked pending GitHub recovery. GitHub reported Actions:
major_outage(verified live on githubstatus.com at 2026-08-06T22:03Z). At this head noCIrun dispatched at all; at the previous head thechangesjob andCI - Required Checkswere cancelled by the platform with every downstream jobskipped— an infrastructure cancellation, not a test failure. Re-running is wasted spend until Actions recovers. TheCodeRabbit / success — Review rate limitedstatus is provider quota, not a verdict.