docs(graphite-pr): reconcile vendored copy with runtime canonical#65
Merged
Conversation
Two-way drift had developed: runtime had `gt-merge-cascade.sh` + GraphQL review-thread pre-flight; vendored had Graphite/AI Reviews diagnostic + bottom-up `gh pr merge` cascade warning + SHA-record-before-sync tip. Merged section keeps both. Also splits the "track an untracked branch" quick-reference row into first-time vs. existing-stack — `gt trunk` errors when HEAD itself is untracked, so the original `$(gt trunk)` form fails the very first time you need to track a branch. Mirrored to ~/.agents/skills/graphite-pr/SKILL.md (canonical runtime shared with Codex; Claude reads it via the existing symlink). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a gt-merge-cascade script and updates the Graphite PR skill docs to gate merges on unresolved review threads via a GraphQL pre‑flight and dry‑run validation, replaces status-check rollup polling, refines single‑PR and branch‑tracking instructions, and emits JSON merge results with per‑PR latencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as gt-merge-cascade.sh
participant GT as gt (Graphite)
participant GH as GitHub GraphQL API
participant PR as Pull Request records
Dev->>Script: invoke cascade (optional interval/timeout)
Script->>GT: gt merge --dry-run
GT-->>Script: dry-run output (branch list / readiness)
Script->>GH: map branches -> open PR numbers (gh pr list)
Script->>GH: GraphQL query per-PR (reviewDecision, first 100 reviewThreads)
GH-->>Script: reviewDecision, reviewThreads, pagination info
alt any unresolved OR decision == "CHANGES_REQUESTED" OR truncated
Script->>Dev: abort with JSON {reason, detail}
else
Script->>GT: gt merge --no-interactive
GT-->>Script: merge initiated
loop poll until all merged or timeout
Script->>GH: gh pr view (PR state)
GH-->>Script: state (MERGED/CLOSED/OPEN/other)
alt state == MERGED
Script-->>Script: record merge timestamp for PR
else if state == CLOSED without MERGED
Script->>Dev: fail cascade with JSON {reason, detail}
end
end
Script->>Dev: output JSON success (total duration, PR count, per-PR latencies)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/5 reviews remaining, refill in 28 minutes and 19 seconds. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9efbd9c782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/graphite-pr/SKILL.md:
- Around line 94-95: Clarify that the merge gate condition ("unresolved == 0"
and "decision != \"CHANGES_REQUESTED\"") must be evaluated against the JSON
printed by the pre-flight command (the pre-flight/jq output that contains the
decision and unresolved fields), so update the wording to explicitly say
"compare the pre-flight JSON's decision and unresolved fields" (i.e., use the
JSON output from pre-flight as the source of truth) and note that a null
decision in that JSON is acceptable; keep the existing note that gt merge
ignores comment state so this JSON check is the safety net.
- Around line 88-92: The GraphQL query uses reviewThreads(first:50) which can
truncate results; update the query to request up to first:100 and add pageInfo {
hasNextPage endCursor } to detect truncation (or implement cursor-based
pagination) and loop using endCursor to fetch remaining reviewThreads until
hasNextPage is false; ensure you still extract reviewDecision and node fields
isResolved and isOutdated, and log/warn if more pages existed if you choose only
to warn instead of fully paginating.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d9674853-b132-4d47-b953-6d441df42ad9
📒 Files selected for processing (1)
.claude/skills/graphite-pr/SKILL.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".claude/skills/graphite-pr/SKILL.md">
<violation number="1" location=".claude/skills/graphite-pr/SKILL.md:82">
P1: `scripts/gt-merge-cascade.sh` is referenced as the primary stack-merge path but does not exist in the repository. Anyone (or any agent) following this instruction will hit "file not found" immediately. Either add the script at the documented path or rewrite this guidance to use an available command.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Agent as Claude / Codex
participant Skills as SKILL.md (Canonical & Vendored)
participant Local as Local Repo (git / gt CLI)
participant Script as scripts/gt-merge-cascade.sh
participant GH as GitHub API / CLI
participant GCloud as Graphite Cloud (App)
Note over Agent,Skills: Initialization: Agent loads instructions via symlink or config
Agent->>Skills: Read merge & tracking protocols
Note over Agent,Local: Flow A: Stack Management
alt NEW: First-time tracking (HEAD untracked)
Agent->>Local: gt track --parent <literal_trunk>
else Existing stack tracking
Agent->>Local: gt track --parent "$(gt trunk)"
end
Note over Agent,GH: Flow B: Single PR Merge (Manual)
Agent->>GH: NEW: GraphQL query (reviewThreads + reviewDecision)
GH-->>Agent: JSON (unresolved thread count)
alt Safety Check: unresolved == 0 AND decision != CHANGES_REQUESTED
Agent->>GH: gh pr merge --squash --delete-branch
else Unhappy Path: Unresolved threads or blocked
Agent->>Agent: Surface diagnostic to user
end
Note over Agent,Script: Flow C: Stack Merge (Automated)
Agent->>Script: CHANGED: Execute gt-merge-cascade.sh
activate Script
Script->>GH: NEW: GraphQL pre-flight (unresolved threads)
Script->>Local: gt merge (with 30s polling)
Script-->>Agent: Return execution JSON
deactivate Script
Note over Agent,GCloud: Flow D: Alternative (Cloud Queue)
Agent->>GCloud: Toggle "Merge when ready"
GCloud->>GH: Bottom-up merge + auto-restack
Note over Agent,Local: Post-Merge Cleanup
Agent->>Local: git rev-parse (Record SHA)
Agent->>Local: gt sync --no-interactive --force
Note right of Local: Sync deletes local tracking for closed branches
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…nate threads, clarify gate - Vendor `scripts/gt-merge-cascade.sh` (codex P2): the runtime canonical had it but the repo path was broken — agents reading the vendored SKILL.md and following `scripts/gt-merge-cascade.sh` would fail. - Bump GraphQL `reviewThreads` to `first:100` and add `pageInfo.hasNextPage` -> `truncated` (CodeRabbit Major): the previous `first:50` would silently miss unresolved threads on big PRs and let unsafe merges through. - Tie merge-gate condition explicitly to "the JSON printed above" (CodeRabbit Minor): removes ambiguity about where decision/unresolved come from. Add `truncated == false` to the merge precondition. Mirrored to ~/.agents/skills/graphite-pr/SKILL.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/graphite-pr/scripts/gt-merge-cascade.sh:
- Around line 36-40: The loop silently drops branches with no open PRs; instead,
change the logic in the branch iteration that uses BRANCHES and PR_LIST so it
fails fast when any branch has no mapped open PR: after calling gh pr list for a
given branch, if the result (variable pr) is empty, immediately call fail (the
existing fail function) with a clear error and include the branch name (e.g.,
"no-open-pr" and branch info) rather than continuing; continue to append valid
PR numbers to PR_LIST when present.
- Around line 49-55: The GraphQL query that computes n uses
reviewThreads(first:50) and misses pagination causing unresolved threads beyond
50 to be ignored; update the gh api/graphql call (the block that sets n and the
loop that aggregates UNRESOLVED_DETAIL) to page through reviewThreads using
pageInfo.hasNextPage and an after cursor, repeatedly requesting subsequent pages
until hasNextPage is false, accumulate the count (or unresolved nodes) across
pages instead of relying on a single first:50 response, and preserve the current
error handling that sets n="ERR" and appends to UNRESOLVED_DETAIL when
unresolved threads are found.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 774505de-ce4a-42cc-b5ca-013e3d713036
📒 Files selected for processing (2)
.claude/skills/graphite-pr/SKILL.md.claude/skills/graphite-pr/scripts/gt-merge-cascade.sh
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".claude/skills/graphite-pr/scripts/gt-merge-cascade.sh">
<violation number="1" location=".claude/skills/graphite-pr/scripts/gt-merge-cascade.sh:38">
P2: Branches with no open PR are silently dropped from `PR_LIST`. If only some stack branches have open PRs, the script proceeds with partial coverage — the review-thread gate and merge polling will only cover the mapped PRs, and the script can exit with `"result":"success"` even though unmapped branches were never checked or tracked. Fail explicitly when a branch cannot be mapped to an open PR.</violation>
<violation number="2" location=".claude/skills/graphite-pr/scripts/gt-merge-cascade.sh:49">
P2: The unresolved-thread merge gate only checks the first 50 review threads, so PRs with more threads can be merged even when unresolved threads still exist.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Palantir debate (Sauron + Saruman) found four real defects on top of the external-reviewer feedback already addressed in 3939cfc: 1. SKILL.md "load references/troubleshooting.md" pointed at a file that does not exist. Switched to references/recovery.md, which exists and covers cascade-recovery / split / conflicts / auth handoff. 2. Cascade script still queried reviewThreads(first:50) with no truncation guard — the SKILL.md inline pre-flight had been bumped to first:100 + truncated, but the *primary* hot path the doc tells you to use first was untouched. Bumped script to first:100, added pageInfo.hasNextPage, fail "review-threads-truncated" on >100. 3. Script gated only on unresolved threads; SKILL.md gated on that PLUS reviewDecision != CHANGES_REQUESTED. A PR a maintainer explicitly blocked with all threads resolved would sail through. Added reviewDecision to the GraphQL query and a "changes-requested" failure path so the script gate matches the documented gate. 4. Branch -> PR mapping used unquoted `for branch in $BRANCHES` (word split + glob expansion on branch names) and silently dropped any branch that had no open PR. Switched to `while IFS= read -r` and now fail loudly with "stack-branch-without-pr" listing the offenders, so the cascade does not merge a truncated subset of the dry-run-validated stack. Mirrored to ~/.agents/skills/graphite-pr/. bash -n syntax-clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/graphite-pr/scripts/gt-merge-cascade.sh:
- Around line 15-17: Validate the CLI numeric inputs before performing shell
arithmetic or calling sleep: ensure POLL_INTERVAL and TIMEOUT_MINUTES (the
variables set in the top-level assignment and used to compute TIMEOUT_SECONDS)
are positive integers using a numeric check (e.g. regex or integer-only test)
and reject non-numeric, zero or negative values by emitting the same structured
JSON failure (the JSON error pattern referenced in lines 7-11) and exiting; do
the same validation immediately before the later sleep/usage at the location
around line 116 so no arithmetic or sleep is attempted with invalid values.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d09f175f-c500-4bef-8819-3cd92337697a
📒 Files selected for processing (2)
.claude/skills/graphite-pr/SKILL.md.claude/skills/graphite-pr/scripts/gt-merge-cascade.sh
CodeRabbit Major (line 17): if either arg is non-numeric, the arithmetic on TIMEOUT_MINUTES and the later `sleep "$POLL_INTERVAL"` call would either error out or behave unpredictably, breaking the JSON failure contract the script promises. Moved json_str/fail above the arg parsing so we can fail loudly with a proper JSON envelope, then validate both args against `^[1-9][0-9]*$` (positive integer, no leading zero, no zero, no negative). Both argument positions tested: invalid string -> "invalid-poll-interval" JSON, zero -> same, valid args pass through to the gt merge stage as before. Mirrored to ~/.agents/skills/graphite-pr/scripts/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
.claude/skills/graphite-pr/SKILL.mdand the runtime canonical at~/.agents/skills/graphite-pr/SKILL.md. Each had improvements the other lacked.scripts/gt-merge-cascade.shfor stacks; GraphQL pre-flight (reviewThreads+reviewDecision) for single-PR safety net beforegh pr merge.Graphite / AI Reviewscheck doesn't appear; bottom-upgh pr mergealternative;--delete-branchcascade-close warning +--base $(gt trunk)workaround; "record SHA beforegt sync --force" tip.gt trunkerrors when HEAD itself is untracked) and existing-stack (the original$(gt trunk)form). This came from a real failure during a Nazgûl run earlier today.~/.agents/skills/graphite-pr/SKILL.mdmirrored to match — Claude continues to read it via the existing symlink at~/.claude/skills/graphite-pr; Codex picks it up via its[[skills.config]]entry.Test plan
diff -q ~/.agents/skills/graphite-pr/SKILL.md .claude/skills/graphite-pr/SKILL.mdreturns no output (sync verified locally; re-verify on merge)..markdownlint.json.🤖 Generated with Claude Code
Summary by cubic
Reconciled the vendored
./.claude/skills/graphite-pr/SKILL.mdwith the runtime canonical and hardened the merge flow with a vendored cascade script, stricter GraphQL gates, and clearer recovery guidance to prevent unsafe merges on large PRs. Also validated cascade script CLI args to preserve the JSON error contract.scripts/gt-merge-cascade.sh; script now validates CLI poll/timeout as positive integers, validates the stack, blocks on unresolved threads,CHANGES_REQUESTED, and >100-thread truncation, maps branches→PRs safely (fails if any branch lacks an open PR), runsgt merge, polls to completion, and emits JSON.reviewThreads(first:100)withpageInfo.hasNextPage→truncated; merge only whenunresolved == 0,decision != "CHANGES_REQUESTED", andtruncated == false.references/recovery.md; kept the Graphite/AI Reviews diagnostic, bottom-upgh pr mergealternative,--delete-branchwarning +--base $(gt trunk)workaround, SHA-before-sync tip; quick-reference splits first-time vs existing-stackgt track.Written for commit 73b7002. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Documentation