Skip to content

feat: add skills for issue verification and PR review#258

Open
generspooler wants to merge 5 commits into
LMCache:mainfrom
generspooler:feat/skills
Open

feat: add skills for issue verification and PR review#258
generspooler wants to merge 5 commits into
LMCache:mainfrom
generspooler:feat/skills

Conversation

@generspooler

Copy link
Copy Markdown
Contributor

Description:
This PR adds two AI-assisted development skills
that enhance the LMCache-Ascend development workflow:

Changes

  1. .gitignore: Un-ignore .claude/ directory so that skill files
    can be tracked in version control.

  2. .claude/skills/issue-verification-workflow/: An end-to-end workflow
    that automates issue-driven verification and fix pipelines — from
    fetching GitHub issues/PRs, locating root causes, reproducing,
    fixing, to outputting a structured verification report.

  3. .claude/skills/pr-review/: A structured PR review workflow that
    performs layered review (conventions, correctness, security,
    architecture, testing, documentation) and generates a review report
    with actionable findings.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces two automated workflows under .claude/skills/: an issue verification workflow and a PR review workflow, along with their associated scripts, templates, and rubrics. The .gitignore is also updated to track the .claude/ directory. Key feedback on these additions includes: in verify_issue.py, adding import torch_npu to properly initialize the NPU backend, dynamically resolving the repository root instead of hardcoding it, ensuring stream synchronization is called after asynchronous copy operations in benchmarks, and correcting the file paths in the usage docstring; in collect_pr_context.sh, fixing a potential script failure when gh pr list returns "null" for empty PR lists; in the PR review skill documentation, correcting the invalid --request flag to --request-changes for the gh pr review command; and in the issue verification skill documentation, adding a timeout to the health check loop to prevent infinite hangs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread .claude/skills/issue-verification-workflow/scripts/verify_issue.py
HAS_TORCH = False

# Base path for LMCache-Ascend source
DEFAULT_BASE = "/mnt/sdb/csy/p2p/LMCache-Ascend"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Hardcoding the absolute path /mnt/sdb/csy/p2p/LMCache-Ascend as DEFAULT_BASE makes the script non-portable and prone to failure on other machines or containers. It is better to dynamically resolve the repository root relative to the script's location.

Suggested change
DEFAULT_BASE = "/mnt/sdb/csy/p2p/LMCache-Ascend"
DEFAULT_BASE = str(Path(__file__).resolve().parents[4])

Comment on lines +76 to +80
torch.npu.synchronize()
t0 = time.perf_counter()
for _ in range(n_iters):
npu_p.copy_(pinned, non_blocking=True)
t_new = (time.perf_counter() - t0) / n_iters * 1e6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since non_blocking=True is used, the copy operation is asynchronous. Without calling torch.npu.synchronize() after the loop (before taking the end timestamp), the benchmark only measures the CPU launch overhead rather than the actual NPU transfer time, leading to incorrect speedup results.

Suggested change
torch.npu.synchronize()
t0 = time.perf_counter()
for _ in range(n_iters):
npu_p.copy_(pinned, non_blocking=True)
t_new = (time.perf_counter() - t0) / n_iters * 1e6
torch.npu.synchronize()
t0 = time.perf_counter()
for _ in range(n_iters):
npu_p.copy_(pinned, non_blocking=True)
torch.npu.synchronize()
t_new = (time.perf_counter() - t0) / n_iters * 1e6
References
  1. Do not remove explicit NPU stream synchronization if it is necessary for correctness, even if it limits host-NPU task overlap.

Comment thread .claude/skills/issue-verification-workflow/scripts/verify_issue.py Outdated
Comment on lines +130 to +132
PR_NUM="$(gh pr list --head "${CURRENT_BRANCH}" --state open \
--json number --jq '.[0].number' 2>/dev/null || true)"
if [ -n "${PR_NUM}" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If there are no open PRs for the current branch, gh pr list returns an empty array []. The jq expression .[0].number will output the string "null". Since "null" is a non-empty string, [ -n "${PR_NUM}" ] will evaluate to true, causing the script to attempt to run gh pr view "null", which will fail. To fix this, check that PR_NUM is not "null" or use // empty in the jq expression.

Suggested change
PR_NUM="$(gh pr list --head "${CURRENT_BRANCH}" --state open \
--json number --jq '.[0].number' 2>/dev/null || true)"
if [ -n "${PR_NUM}" ]; then
PR_NUM="$(gh pr list --head "${CURRENT_BRANCH}" --state open \
--json number --jq '.[0].number // empty' 2>/dev/null || true)"
if [ -n "${PR_NUM}" ] && [ "${PR_NUM}" != "null" ]; then

Comment thread .claude/skills/pr-review/SKILL.md
Comment thread .claude/skills/issue-verification-workflow/SKILL.md Outdated
generspooler and others added 4 commits June 26, 2026 17:01
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Yuanhao Luo <79679551+generspooler@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Yuanhao Luo <79679551+generspooler@users.noreply.github.com>
…e.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Yuanhao Luo <79679551+generspooler@users.noreply.github.com>
…e.py

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Yuanhao Luo <79679551+generspooler@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant