ci(requirements-sync): fetch board deterministically with a projects-scoped token - #899
Closed
SarahLittlejohn wants to merge 1 commit into
Closed
ci(requirements-sync): fetch board deterministically with a projects-scoped token#899SarahLittlejohn wants to merge 1 commit into
SarahLittlejohn wants to merge 1 commit into
Conversation
…scoped token The nightly sync has been reporting "no drift" while the requirements DB fell 39 gated issues behind, plus #438 stale at approved vs implemented on the board. Root cause: the job authenticates as the public Claude App (claude[bot], bot_id 41898282), whose declared permissions are actions, checks, contents, discussions, issues, members, metadata, pull_requests, repository_hooks, statuses, workflows. There is no organization_projects or repository_projects among them, and Project #43 is org-owned by hmcts, so the board read in prompt step 2 could never succeed. This is not an unchecked toggle on the installation — the App does not declare the permission, so there is nothing to grant and no workflow-only fix. When that query failed mid-prompt, Claude improvised a fallback nobody specified: closed issue + merged closing PR as a proxy for Done=verified, recorded only in SQL comments in migrations 008, 009 and 010. That proxy structurally cannot see the 20 "Refined Tickets" items, which have no PR by definition, so it found nothing and the job faithfully reported no drift. The integrity gate passed throughout, because a DB missing 39 rows is still internally consistent. Fixed by moving the board read out of the prompt: - New requirements/scripts/fetch_board.sh pages Project #43, maps board columns to requirement statuses, filters to gated issues and emits JSON Lines. It exits non-zero if the GraphQL call errors, if the project node comes back null (how a token without projects access actually presents), or if zero items are returned — an unreadable board now stops the run instead of silently degrading it. - A "Fetch board state" step runs it BEFORE Claude, using PROJECTS_READ_TOKEN, and fails with an actionable message if that secret is absent. Claude reads the resulting file instead of querying GitHub, so it cannot substitute a proxy for data it no longer fetches. - Prompt step 2 rewritten to read BOARD_STATE_FILE and forbid inferring status from issue state or merged PRs; step 3's now-redundant column mapping removed so the two cannot drift apart. Bash(gh:*) narrowed to Bash(gh pr:*) so the board is not reachable even if the prompt is misread. STATUS_FIELD_ID dropped — the script queries the Status field by name. PROJECTS_READ_TOKEN is a fine-grained PAT with organization Projects: Read. Being user-owned, it runs as that person's identity, expires within a year, and dies on offboarding — an org-owned App declaring organization_projects: read is the durable replacement and needs no change here beyond the secret reference. Verified against the live board: the script reads 281 items, 191 gated, no duplicate issue numbers, and reproduces the known drift exactly (39 new, #438 mismatch). Both failure paths were exercised — invalid project id and invalid token each exit 1 with no partial output. Does not backfill the drift. Migrations 008-010 were all written under the proxy, so their contents are suspect too; that needs a reviewed migration of its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe workflow now fetches Project ChangesRequirements synchronisation
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant fetch_board.sh
participant GitHub GraphQL API
participant Claude
GitHub Actions->>fetch_board.sh: Provide PROJECTS_READ_TOKEN and PROJECT_ID
fetch_board.sh->>GitHub GraphQL API: Fetch paginated Project `#43` data
GitHub GraphQL API-->>fetch_board.sh: Return board items and merged PR metadata
fetch_board.sh-->>GitHub Actions: Write BOARD_STATE_FILE
GitHub Actions->>Claude: Provide BOARD_STATE_FILE
Claude-->>GitHub Actions: Compute requirement delta from JSONL
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Contributor
🎭 Playwright E2E Test Results82 tests 52 ✅ 3m 59s ⏱️ Results for commit 2929c53. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 182144af-a8cf-4913-adca-eb6840d26866
📒 Files selected for processing (2)
.github/workflows/requirements-sync.ymlrequirements/scripts/fetch_board.sh
Comment on lines
+59
to
+66
| closedByPullRequestsReferences(first: 20, includeClosedPrs: true) { | ||
| nodes { | ||
| number | ||
| state | ||
| mergeCommit { oid } | ||
| files(first: 100) { nodes { path } } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Emitted JSON leaks the raw boardStatus, and mergedPrs has no timestamp to determine "latest".
Two related contract gaps for the downstream consumer (the Claude prompt in requirements-sync.yml):
- Line 101 merges
statusonto the object built at lines 105‑118, but doesn't drop the originalboardStatuskey first. The final JSONL line therefore contains both the raw column name and the mappedstatus. The prompt explicitly forbids inferring status from anything butstatus— leaving the raw value in the file undermines the very guarantee this PR exists to provide. - The GraphQL query (lines 59‑66) fetches
number,state,mergeCommit { oid }andfilesfor each merged PR but no merge timestamp (e.g.mergedAt). The prompt instructs Claude to pick "the latest merged closing PR's mergeCommitOid from mergedPrs" (requirements-sync.yml line 159), but without a timestamp there's no reliable signal for "latest" — Claude is left guessing from array order or PR number.
♻️ Proposed fix
closedByPullRequestsReferences(first: 20, includeClosedPrs: true) {
nodes {
number
state
+ mergedAt
mergeCommit { oid }
files(first: 100) { nodes { path } }
}
} mergedPrs: [
.content.closedByPullRequestsReferences.nodes[]
| select(.state == "MERGED")
- | {number, mergeCommitOid: .mergeCommit.oid, paths: [.files.nodes[].path]}
+ | {number, mergedAt, mergeCommitOid: .mergeCommit.oid, paths: [.files.nodes[].path]}
]
}' <<<"$resp")
done < <(...)
jq -c --arg status "$mapped" '. + {status: $status}' <<<"$item"And drop boardStatus from the final emitted object (e.g. del(.boardStatus) after the merge, or omit it from the object construction and pass board_status to map_status via --argjson/an intermediate variable instead).
Also applies to: 102-118
Comment on lines
+81
to
+90
| if ! resp=$(read_page "$cursor" 2>&1); then | ||
| echo "::error::Cannot read Project board — GraphQL query failed. The token needs organization Projects: Read on hmcts. Response: ${resp}" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # A token without projects access gets a null node rather than an HTTP error. | ||
| if [ "$(jq -r '.data.node // "null"' <<<"$resp")" = "null" ]; then | ||
| echo "::error::Project board returned no node for PROJECT_ID=${PROJECT_ID}. The token almost certainly lacks organization Projects: Read on hmcts." >&2 | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stray stderr merges into $resp even on success, corrupting downstream jq parsing.
resp=$(read_page "$cursor" 2>&1) redirects stderr into the captured value unconditionally, not just on failure. If gh ever writes anything to stderr on a successful call (rate-limit notices, deprecation warnings, etc.), $resp stops being valid JSON and every subsequent jq call (lines 87, 92, 102, 118, 120‑121) silently fails inside a command substitution — which set -e does not catch — rather than triggering the loud, explicit failure this script is designed to guarantee.
🛡️ Proposed fix: capture stderr separately, only surface it on failure
- if ! resp=$(read_page "$cursor" 2>&1); then
- echo "::error::Cannot read Project board — GraphQL query failed. The token needs organization Projects: Read on hmcts. Response: ${resp}" >&2
+ err_file=$(mktemp)
+ if ! resp=$(read_page "$cursor" 2>"$err_file"); then
+ echo "::error::Cannot read Project board — GraphQL query failed. The token needs organization Projects: Read on hmcts. Response: $(cat "$err_file")" >&2
+ rm -f "$err_file"
exit 1
fi
+ rm -f "$err_file"📝 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.
Suggested change
| if ! resp=$(read_page "$cursor" 2>&1); then | |
| echo "::error::Cannot read Project board — GraphQL query failed. The token needs organization Projects: Read on hmcts. Response: ${resp}" >&2 | |
| exit 1 | |
| fi | |
| # A token without projects access gets a null node rather than an HTTP error. | |
| if [ "$(jq -r '.data.node // "null"' <<<"$resp")" = "null" ]; then | |
| echo "::error::Project board returned no node for PROJECT_ID=${PROJECT_ID}. The token almost certainly lacks organization Projects: Read on hmcts." >&2 | |
| exit 1 | |
| fi | |
| err_file=$(mktemp) | |
| if ! resp=$(read_page "$cursor" 2>"$err_file"); then | |
| echo "::error::Cannot read Project board — GraphQL query failed. The token needs organization Projects: Read on hmcts. Response: $(cat "$err_file")" >&2 | |
| rm -f "$err_file" | |
| exit 1 | |
| fi | |
| rm -f "$err_file" | |
| # A token without projects access gets a null node rather than an HTTP error. | |
| if [ "$(jq -r '.data.node // "null"' <<<"$resp")" = "null" ]; then | |
| echo "::error::Project board returned no node for PROJECT_ID=${PROJECT_ID}. The token almost certainly lacks organization Projects: Read on hmcts." >&2 | |
| exit 1 | |
| fi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The nightly sync has been reporting "no drift" while the requirements DB fell 39 gated issues behind, plus #438 stale at
approvedvsimplementedon the board. Last night's run succeeded at 45 turns and $3.33 having seen nothing.Root cause: the job authenticates as the public Claude App (
claude[bot], bot_id41898282). Its declared permissions areactions, checks, contents, discussions, issues, members, metadata, pull_requests, repository_hooks, statuses, workflows— noorganization_projectsorrepository_projects. Project #43 is org-owned byhmcts, so the board read in prompt step 2 could never succeed.This is not an unchecked toggle on the installation. The App doesn't declare the permission, so there's nothing to grant and no workflow-only fix.
When that query failed mid-prompt, Claude improvised a fallback nobody specified: closed issue + merged closing PR as a proxy for
Done=verified, recorded only in SQL comments in migrations 008, 009 and 010. That proxy structurally cannot see the 20Refined Ticketsitems — they have no PR by definition — so it found nothing. The integrity gate passed throughout, because a DB missing 39 rows is still internally consistent.What changed
requirements/scripts/fetch_board.sh— pages Project VIBE-159 Add confirm upload details page #43, maps board columns to requirement statuses, filters to gated issues, emits JSON Lines. Exits non-zero if the GraphQL call errors, if the project node comes backnull(how a token without projects access actually presents), or if zero items are returned. An unreadable board now stops the run instead of silently degrading it.PROJECTS_READ_TOKEN, failing with an actionable message if the secret is absent. Claude reads the resulting file instead of querying GitHub, so it can't substitute a proxy for data it no longer fetches.BOARD_STATE_FILEand forbid inferring status from issue state or merged PRs. Step 3's now-redundant column mapping removed so the two can't drift apart.Bash(gh:*)narrowed toBash(gh pr:*)so the board isn't reachable even if the prompt is misread.STATUS_FIELD_IDdropped — the script queries the Status field by name.Create secret
PROJECTS_READ_TOKEN: a fine-grained PAT with organization Projects: Read onhmcts. Without it the job fails fast with a clear error rather than silently going blind.Being user-owned, this PAT runs as that person's identity, expires within a year, and dies on offboarding. An org-owned App declaring
organization_projects: readis the durable replacement and needs no change here beyond the secret reference.Verification
Against the live board: 281 items read, 191 gated, no duplicate issue numbers, and reproduces the known drift exactly (39 new, #438 mismatch). Both failure paths exercised — invalid project id and invalid token each exit 1 with no partial output.
Out of scope
Does not backfill the drift. Migrations 008–010 were all written under the proxy, so their contents are suspect too — that needs a reviewed migration of its own.
Supersedes the direct-push approach in #884 (closed unmerged); this fixes detection, which that PR assumed was working.
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Reliability