feat(skills): migrate issue scripts to standardized output envelope#1568
feat(skills): migrate issue scripts to standardized output envelope#1568rjmurillo-bot merged 8 commits intomainfrom
Conversation
Adds scripts/skill_registry.py that scans .claude/skills/, extracts YAML frontmatter metadata, determines last-modified dates from git history, and categorizes skills. Supports JSON and markdown output, with a --show-stale flag to surface underutilized skills. Fixes #1266 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add --session-message flag and format_session_message() to surface underutilized skills during sessions, satisfying AC-3 of issue #1266. Outputs "These skills haven't been used in 30+ days: X, Y, Z" for integration with session-init or reflect skills. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validate --project-root and --skills-dir for path traversal before use. Resolve paths after validation per gemini-code-assist review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use validate_safe_path() for CWE-22 containment instead of literal ".." component check, ensuring skills_dir stays within project_root - Fix off-by-one in filter_stale: use <= so skills modified exactly stale_days ago are considered stale - Return exit code 1 (not 2) for unexpected failures per ADR-035 - Narrow exception catch from bare Exception to (OSError, SubprocessError) to fix BLE001 - Update path traversal tests for containment-based validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Migrate get_issue_context.py, set_issue_assignee.py, and
set_issue_labels.py to use write_skill_output/write_skill_error
from github_core.output per ADR-051.
Scripts now emit the standard envelope format:
{"Success": bool, "Data": {...}, "Error": null, "Metadata": {...}}
This is the first batch of migrations toward issue #673
(standardize skill output format across all skills).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new skill_registry.py script for generating and managing skill metadata, including last-modified dates and categorization. It also refactors existing GitHub issue-related scripts (get_issue_context.py, set_issue_assignee.py, set_issue_labels.py) to adopt a standardized output and error handling mechanism using the github_core.output module. The review identifies two issues: get_issue_context.py has an incomplete migration to the standardized error handling, as write_skill_error is not imported or utilized, and the new skill_registry.py script will fail with a ModuleNotFoundError when executed directly because the scripts directory is not correctly added to the Python path.
📝 WalkthroughWalkthroughMoved GitHub issue scripts to a standardized skill output format (adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI
participant SkillRegistry as "skill_registry.py"
participant FS as "Filesystem (.claude/skills/)"
participant Git as "git"
participant Formatter as "JSON/Markdown output"
User->>CLI: invoke `skill_registry --skills-dir ...`
CLI->>SkillRegistry: parse args, validate paths (validate_safe_path)
SkillRegistry->>FS: scan skill dirs (skip symlinks)
FS-->>SkillRegistry: directory entries, `SKILL.md` contents
SkillRegistry->>SkillRegistry: parse frontmatter (explicit closing `---`)
SkillRegistry->>Git: query last-modified timestamps
Git-->>SkillRegistry: commit timestamps per skill
SkillRegistry->>SkillRegistry: categorize skills, filter stale (<= cutoff)
SkillRegistry->>Formatter: format output (JSON or Markdown + session message)
Formatter-->>User: emit formatted output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.claude/skills/github/scripts/issue/get_issue_context.py (2)
6-10:⚠️ Potential issue | 🟡 MinorFix stale exit-code documentation.
Line 9 documents exit code
2as “Not found”, which conflicts with the repository’s ADR-035 semantics and the config-error use at Line 34.Based on learnings: exit code 2 must be documented and used as config/environment error in this repository.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/github/scripts/issue/get_issue_context.py around lines 6 - 10, The exit-code documentation is stale: update the enumerated exit-code doc block so exit code 2 is documented as "Config/environment error" (per ADR-035) instead of "Not found", and audit references in this file (e.g., the config-error usage) to ensure any places that currently treat 2 as "Not found" are corrected to use 2 only for configuration/environment errors and not for missing resources; keep ADR-035 mentioned and make the doc string consistent with the code paths that raise or return config-error.
81-93:⚠️ Potential issue | 🟠 MajorUse the standardized error envelope on all failure paths and stop using exit code 2 for API/not-found failures.
Line 82 and Line 90 still call
error_and_exit(...), which bypasses the new envelope contract in JSON mode. Also, Line 84 maps API/not-found failure to exit code2, which should be reserved for config/environment failures.Proposed fix
from github_core.output import ( # noqa: E402 add_output_format_arg, get_output_format, + write_skill_error, write_skill_output, ) @@ if result.returncode != 0: - error_and_exit( - f"Issue #{args.issue} not found or API error (exit code {result.returncode})", - 2, - ) + data = {"issue": args.issue, "owner": owner, "repo": repo, "gh_exit_code": result.returncode} + write_skill_error( + f"Failed to fetch issue #{args.issue}", + 3, + error_type="ApiError", + output_format=fmt, + script_name="get_issue_context.py", + extra=data, + ) + raise SystemExit(3) @@ try: issue_data = json.loads(result.stdout) except json.JSONDecodeError: - error_and_exit("Failed to parse issue JSON", 3) + write_skill_error( + "Failed to parse issue JSON", + 3, + error_type="ParseError", + output_format=fmt, + script_name="get_issue_context.py", + extra={"issue": args.issue, "owner": owner, "repo": repo}, + ) + raise SystemExit(3) @@ if not issue_data: - error_and_exit("Failed to parse issue JSON", 3) + write_skill_error( + "Failed to parse issue JSON", + 3, + error_type="ParseError", + output_format=fmt, + script_name="get_issue_context.py", + extra={"issue": args.issue, "owner": owner, "repo": repo}, + ) + raise SystemExit(3)Based on learnings: per ADR-035 in this repository, exit code 2 is reserved for config/environment errors, not not-found/API failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/github/scripts/issue/get_issue_context.py around lines 81 - 93, Replace the raw error_and_exit calls with the repository's standardized JSON error envelope helper (use the project's error-envelope emitter function) on all failure paths in get_issue_context.py so errors are emitted in the agreed envelope format; specifically, when result.returncode != 0 (the API/not-found case referencing result.returncode and args.issue) do not call error_and_exit with exit code 2—emit the standard error envelope and exit with the proper non-config error code (choose a code other than 2 per ADR-035, e.g., 4); likewise, on JSON decode failures (json.JSONDecodeError and the empty issue_data path) emit the standard envelope and exit with the existing parse failure code (currently 3) instead of bypassing the envelope via error_and_exit..claude/skills/github/scripts/issue/set_issue_labels.py (1)
157-159:⚠️ Potential issue | 🟠 MajorNo-label path still bypasses standardized output format.
Line 158 writes plain stderr text and exits successfully without envelope output. In JSON mode, this breaks the “JSON-only output” contract for this migrated script.
Proposed fix
if not all_labels: - print("No labels to apply.", file=sys.stderr) + write_skill_output( + { + "issue": args.issue, + "applied": [], + "created": [], + "failed": [], + "total_applied": 0, + }, + output_format=fmt, + human_summary="No labels to apply.", + status="INFO", + script_name="set_issue_labels.py", + ) return 0As per coding guidelines: "Use identical syntax for all instances when migrating patterns to maintain consistency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/github/scripts/issue/set_issue_labels.py around lines 157 - 159, When all_labels is empty, replace the plain stderr print and immediate return (the lines referencing all_labels, print("No labels to apply.", file=sys.stderr), return 0) with the script's standard JSON-envelope output used elsewhere in this file: emit the identical envelope object/JSON structure the script emits in other success/no-op branches (e.g., include a status/result field and a message like "No labels to apply.") to stdout so JSON-only mode remains valid, then return the same exit code used by other no-op paths.
🧹 Nitpick comments (1)
tests/test_skill_registry.py (1)
331-354: Cover the remaining invalid path cases here.These tests stop at
..and out-of-root paths. The new boundary check should also be locked down against null bytes and control characters so path sanitization does not regress silently.As per coding guidelines, "Create unit test cases for path validation functions that cover path traversal attempts, null byte injection, control character injection, and absolute path rejection before deploying to production."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_skill_registry.py` around lines 331 - 354, Add unit tests to TestPathTraversal that cover remaining invalid inputs: create tests similar to test_rejects_skills_dir_outside_project_root and test_rejects_skills_dir_traversal but asserting main(...) returns error code 2 for (1) paths containing a null byte (e.g., str(project / "bad\0name")), (2) paths containing control characters (e.g., include '\n' or '\r' in the path string), and (3) absolute paths pointing outside the project root; reference the existing TestPathTraversal class and the main(...) call pattern so the new tests follow the same tmp_path setup and assert logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/skill_registry.py`:
- Around line 60-73: The current frontmatter parsing reads after the opening
'---' and accumulates key:value pairs even if no closing '---' exists; change
the logic in the block that iterates over lines[1:] (using variables lines,
stripped, frontmatter) to first verify a closing '---' exists (e.g., scan
lines[1:] for a stripped '---') and if it is not found, return {} immediately so
no body content is treated as metadata; only parse and populate frontmatter when
the closing delimiter is present.
- Around line 339-343: The --stale-days argument currently accepts negative
integers; validate it during parsing by replacing the plain int conversion with
a constrained check (e.g., a custom argparse type/validator) so values < 0 raise
an argparse.ArgumentTypeError and prevent proceeding; update the
parser.add_argument call for "--stale-days" (and any code that reads the parsed
value such as the stale_days variable) to enforce >= 0 and provide a clear error
message like "stale-days must be >= 0".
- Around line 221-226: The loop over skills_dir currently trusts every directory
from skills_dir.iterdir() and may follow symlinks outside the repo; update the
loop to validate each child before scanning by calling validate_safe_path(entry)
(or explicitly skip entries where entry.is_symlink() is True) and only then call
scan_skill(entry, project_root); modify the logic around skills_dir.iterdir()
and the call site of scan_skill to ensure unsafe paths are rejected, and add a
regression test that creates a symlink from .claude/skills/evil -> /etc to
assert it is skipped or rejected.
In `@tests/skills/github/test_issue_scripts.py`:
- Around line 491-493: Add an assertion that the captured return code variable
rc is zero to ensure the command succeeded; specifically, after rc is assigned
and before/after parsing captured output into output (the variable from
json.loads(capsys.readouterr().out)), add an assertion like assert rc == 0 (or
assert rc == 0, "non-zero return code") so a failing return code cannot be
masked by emitted output.
---
Outside diff comments:
In @.claude/skills/github/scripts/issue/get_issue_context.py:
- Around line 6-10: The exit-code documentation is stale: update the enumerated
exit-code doc block so exit code 2 is documented as "Config/environment error"
(per ADR-035) instead of "Not found", and audit references in this file (e.g.,
the config-error usage) to ensure any places that currently treat 2 as "Not
found" are corrected to use 2 only for configuration/environment errors and not
for missing resources; keep ADR-035 mentioned and make the doc string consistent
with the code paths that raise or return config-error.
- Around line 81-93: Replace the raw error_and_exit calls with the repository's
standardized JSON error envelope helper (use the project's error-envelope
emitter function) on all failure paths in get_issue_context.py so errors are
emitted in the agreed envelope format; specifically, when result.returncode != 0
(the API/not-found case referencing result.returncode and args.issue) do not
call error_and_exit with exit code 2—emit the standard error envelope and exit
with the proper non-config error code (choose a code other than 2 per ADR-035,
e.g., 4); likewise, on JSON decode failures (json.JSONDecodeError and the empty
issue_data path) emit the standard envelope and exit with the existing parse
failure code (currently 3) instead of bypassing the envelope via error_and_exit.
In @.claude/skills/github/scripts/issue/set_issue_labels.py:
- Around line 157-159: When all_labels is empty, replace the plain stderr print
and immediate return (the lines referencing all_labels, print("No labels to
apply.", file=sys.stderr), return 0) with the script's standard JSON-envelope
output used elsewhere in this file: emit the identical envelope object/JSON
structure the script emits in other success/no-op branches (e.g., include a
status/result field and a message like "No labels to apply.") to stdout so
JSON-only mode remains valid, then return the same exit code used by other no-op
paths.
---
Nitpick comments:
In `@tests/test_skill_registry.py`:
- Around line 331-354: Add unit tests to TestPathTraversal that cover remaining
invalid inputs: create tests similar to
test_rejects_skills_dir_outside_project_root and
test_rejects_skills_dir_traversal but asserting main(...) returns error code 2
for (1) paths containing a null byte (e.g., str(project / "bad\0name")), (2)
paths containing control characters (e.g., include '\n' or '\r' in the path
string), and (3) absolute paths pointing outside the project root; reference the
existing TestPathTraversal class and the main(...) call pattern so the new tests
follow the same tmp_path setup and assert logic.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7167baf0-40a1-45ac-8c39-bc67c3e290ab
📒 Files selected for processing (12)
.claude/skills/github/scripts/issue/get_issue_context.py.claude/skills/github/scripts/issue/set_issue_assignee.py.claude/skills/github/scripts/issue/set_issue_labels.pyscripts/skill_registry.pytests/skills/github/test_get_issue_context.pytests/skills/github/test_issue_scripts.pytests/skills/github/test_set_issue_assignee.pytests/skills/github/test_set_issue_labels.pytests/test_get_issue_context.pytests/test_set_issue_assignee.pytests/test_set_issue_labels.pytests/test_skill_registry.py
Review Triage RequiredNote Priority: NORMAL - Human approval required before bot responds Review Summary
Next Steps
Powered by PR Maintenance workflow - Add triage:approved label |
Resolve add/add conflicts in skill_registry.py and test_skill_registry.py by keeping the branch version which uses validate_safe_path utility for CWE-22 path traversal prevention. Main's inline checks are superseded by the shared utility approach from this branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Validation ReportNote ✅ Status: PASS Description Validation
PR Standards
QA Validation
⚡ Warnings
Powered by PR Validation workflow |
✅ Pass: Memory Validationadr-007-augmentation-research: no citations 📊 Validation Details
|
Spec-to-Implementation ValidationTip ✅ Final Verdict: PASS What is Spec Validation?This validation ensures your implementation matches the specifications:
Validation Summary
Spec References
Requirements Traceability DetailsBased on my analysis, I can now produce the requirements coverage matrix. Requirements Coverage Matrix
Summary
Gaps
Coverage Evidence
Implementation Completeness DetailsNow I have all the context needed. Let me verify the tests validate the envelope structure properly. Acceptance Criteria Checklist
Missing FunctionalityNone identified. The PR meets all 4 acceptance criteria from Issue #673. Edge Cases Covered
Edge Cases Not Covered
Implementation Quality
Run Details
Powered by AI Spec Validator workflow |
AI Quality Gate ReviewTip ✅ Final Verdict: PASS WalkthroughThis PR was reviewed by six AI agents in parallel, analyzing different aspects of the changes:
Review Summary
💡 Quick Access: Click on individual agent jobs (e.g., "🔒 security Review", "🧪 qa Review") in the workflow run to see detailed findings and step summaries. Security Review DetailsNow let me review the remaining test files to complete the security analysis: Now I have a complete picture of the PR. Let me analyze it for security vulnerabilities: Security Analysis: PR #1568PR Type Detection
This is a CODE PR requiring full OWASP review. Findings
Analysis SummaryScripts Reviewed:
Security Controls Verified:
RecommendationsNone required. The code follows security best practices. Verdict{
"verdict": "PASS",
"message": "No security vulnerabilities detected. Scripts use safe subprocess patterns, proper input validation, and authentication checks.",
"agent": "security",
"timestamp": "2026-04-10T21:39:23.362Z",
"findings": []
}QA Review DetailsNow I have a comprehensive view of the PR changes. Let me compile my QA review verdict. QA Review: PR #1568Test Coverage Assessment
Test Execution Results
Code Quality Assessment
Error Handling Verification
Output Envelope ComplianceAll 3 migrated scripts now emit the standard envelope format per ADR-051: {"Success": bool, "Data": {...}, "Error": null, "Metadata": {...}}Tests verify envelope structure explicitly via assertions like:
Quality Concerns
Regression Risk Assessment
VERDICT: PASS MESSAGE: Issue scripts successfully migrated to ADR-051 output envelope with comprehensive test coverage for happy paths, error handling, and edge cases. PR TYPE: CODE EVIDENCE:
{
"verdict": "PASS",
"message": "Issue scripts successfully migrated to ADR-051 output envelope with comprehensive test coverage for happy paths, error handling, and edge cases.",
"agent": "qa",
"timestamp": "2026-04-10T21:40:00Z",
"findings": [
{
"severity": "low",
"category": "code-quality",
"description": "Unused import 'error_and_exit' in set_issue_assignee.py and set_issue_labels.py",
"location": "set_issue_assignee.py:40, set_issue_labels.py:40",
"recommendation": "Remove unused import in follow-up cleanup PR"
}
]
}Analyst Review DetailsNow I have sufficient context to complete the analysis. PR #1568 Analysis: Migrate Issue Scripts to Standardized Output EnvelopeCode Quality Score
Overall: 5/5 Impact Assessment
Findings
Architectural AlignmentPositive observations:
Recommendations
Verdict{
"verdict": "PASS",
"message": "Clean migration to standardized output envelope per ADR-051. All 3 scripts and 6 test files follow consistent patterns.",
"agent": "analyst",
"timestamp": "2026-04-10T21:39:24.992Z",
"findings": [
{
"severity": "low",
"category": "consistency",
"description": "Unused import error_and_exit",
"location": "set_issue_assignee.py:40",
"recommendation": "Remove unused import to satisfy linters"
},
{
"severity": "low",
"category": "consistency",
"description": "Unused import error_and_exit",
"location": "set_issue_labels.py:40",
"recommendation": "Remove unused import to satisfy linters"
}
]
}Architect Review DetailsI have reviewed all the relevant files. Now let me provide my architectural assessment. Design Quality Assessment
Overall Design Score: 5/5 Architectural Concerns
Breaking Change Assessment
The breaking change is intentional and documented. ADR-051 Section "Consequences" acknowledges "Existing callers that parse raw JSON must adapt to the envelope (mitigated by phased migration)". The 3-script minimum criteria from Issue #673 is satisfied. Technical Debt Analysis
Standardizing output format reduces technical debt by eliminating inconsistent output handling. Future scripts follow one pattern. ADR Assessment
The migration correctly implements ADR-051. Scripts emit Recommendations
VerdictThe implementation demonstrates sound architectural practices:
{
"verdict": "PASS",
"message": "Implementation correctly follows ADR-051 envelope standardization with consistent patterns and comprehensive test coverage.",
"agent": "architect",
"timestamp": "2026-04-10T21:37:02Z",
"findings": [
{
"severity": "low",
"category": "tech-debt",
"description": "Unused import error_and_exit in set_issue_assignee.py",
"location": "set_issue_assignee.py:40",
"recommendation": "Remove unused import"
},
{
"severity": "low",
"category": "tech-debt",
"description": "Unused import error_and_exit in set_issue_labels.py",
"location": "set_issue_labels.py:41",
"recommendation": "Remove unused import"
}
]
}DevOps Review DetailsLet me check the local repository for these files: Let me also check the test files to complete the review: Now I have enough context to complete my DevOps review. DevOps Review: PR #1568PR Scope Detection
Scope: This PR modifies Python skill scripts and test files. No CI/CD workflows, GitHub Actions, or shell scripts are modified. Pipeline Impact Assessment
CI/CD Quality Checks
Script Quality Review (Python)
Findings
Template Assessment
Automation Opportunities
The migration follows existing patterns; no new automation needed. Recommendations
Verdict{
"verdict": "PASS",
"message": "Code-only PR migrating scripts to standardized output envelope. No CI/CD impact. ADR-035 exit codes maintained.",
"agent": "devops",
"timestamp": "2026-04-10T21:38:23Z",
"findings": [
{
"severity": "low",
"category": "shell-quality",
"description": "Unused import error_and_exit in migrated scripts",
"location": "set_issue_assignee.py:40, set_issue_labels.py:41",
"recommendation": "Remove unused import to reduce code noise"
}
]
}Roadmap Review DetailsI now have sufficient context to complete the roadmap review. This PR implements ADR-051 (Skill Output Format Standardization) by migrating 3 issue scripts to the standard envelope format. Roadmap Review: PR #1568Strategic Alignment Assessment
Feature Completeness
Impact Analysis
Concerns
Recommendations
Verdict{
"verdict": "PASS",
"message": "PR implements ADR-051 Phase 2 migration for 3 issue scripts with 100% test coverage; right-sized scope meets #673 acceptance criteria.",
"agent": "roadmap",
"timestamp": "2026-04-10T21:39:22.112Z",
"findings": [
{
"severity": "low",
"category": "scope",
"description": "Incremental migration leaves codebase in mixed-output state until all 30+ remaining scripts migrate",
"location": "ADR-051:66-67",
"recommendation": "Track completion target date in issue #673 or create follow-up tracking issue"
}
]
}Run Details
Powered by AI Quality Gate workflow |
- Complete get_issue_context.py migration to write_skill_error (ADR-051) - Add sys.path setup in skill_registry.py for direct execution - Guard parse_frontmatter against unterminated frontmatter blocks - Skip symlinks in build_registry to prevent path traversal - Reject negative --stale-days values - Assert return code in test_multiple_assignees Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/skill_registry.py (1)
229-236:⚠️ Potential issue | 🔴 CriticalFix nested symlink traversal in skill scanning.
Lines 230–236 skip only top-level symlinks.
scan_skill()still callsis_dir(),iterdir(), andread_text()on child paths without checking for symlinks. A symlinked child like.claude/skills/my-skill/tests/ -> /etc/allows iterdir() and read_text() to access content outside the skill directory.Add an
is_symlink()check inscan_skill()for each child path (SKILL.md,tests/,scripts/) before dereferencing it. Alternatively, validate child paths usingvalidate_safe_path()fromscripts/utils/path_validation.py. Add a regression test with a nested symlink.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/skill_registry.py` around lines 229 - 236, scan_skill() currently dereferences child entries (SKILL.md, tests/, scripts/, etc.) without checking symlinks, allowing nested symlink traversal; update scan_skill() to skip or reject any child path where Path.is_symlink() is True before calling is_dir(), iterdir(), or read_text(), or alternatively call validate_safe_path() from scripts/utils/path_validation.py on each child path to ensure it stays inside the skill directory; adjust logic in scan_skill() (references: scan_skill(), SKILL.md, tests/, scripts/, validate_safe_path()) and add a regression test that creates a nested symlink inside a skill (e.g., tests/ -> /etc/) to assert the scanner rejects or ignores the symlinked child.
🤖 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/github/scripts/issue/get_issue_context.py:
- Around line 91-101: In the except json.JSONDecodeError block in
get_issue_context.py, explicitly chain or suppress the original JSONDecodeError
when re-raising the SystemExit: capture the exception (e.g., except
json.JSONDecodeError as err) and then call write_skill_error as before and
re-raise with either "raise SystemExit(3) from err" to preserve the original
cause or "raise SystemExit(3) from None" to suppress it; update the except
clause and the raise accordingly around the json.loads/result handling and
write_skill_error call.
In `@scripts/skill_registry.py`:
- Around line 445-447: The except block that currently catches OSError and
subprocess.SubprocessError should return exit code 2 (config/environment error
per ADR-035) instead of 1; update the handler that prints "FATAL: {e}" to use
return 2 so callers can distinguish environment/config failures from logic bugs
(adjust the except for OSError and subprocess.SubprocessError in the same scope
as the existing return 2 path to keep behavior consistent).
---
Outside diff comments:
In `@scripts/skill_registry.py`:
- Around line 229-236: scan_skill() currently dereferences child entries
(SKILL.md, tests/, scripts/, etc.) without checking symlinks, allowing nested
symlink traversal; update scan_skill() to skip or reject any child path where
Path.is_symlink() is True before calling is_dir(), iterdir(), or read_text(), or
alternatively call validate_safe_path() from scripts/utils/path_validation.py on
each child path to ensure it stays inside the skill directory; adjust logic in
scan_skill() (references: scan_skill(), SKILL.md, tests/, scripts/,
validate_safe_path()) and add a regression test that creates a nested symlink
inside a skill (e.g., tests/ -> /etc/) to assert the scanner rejects or ignores
the symlinked child.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9995059a-6a4e-4a6b-9daf-7cbaec2cc3f9
📒 Files selected for processing (3)
.claude/skills/github/scripts/issue/get_issue_context.pyscripts/skill_registry.pytests/skills/github/test_issue_scripts.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
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 `@scripts/skill_registry.py`:
- Around line 385-388: Add a unit test that asserts filter_stale includes skills
whose last_modified equals the cutoff date (stale_days ago); create a skill with
last_modified set to (datetime.now(UTC) -
timedelta(days=30)).strftime("%Y-%m-%d") and call filter_stale(skills,
stale_days=30), then assert the result length is 1 and the returned
SkillMetadata.name equals the created skill (use the SkillMetadata constructor
used elsewhere to build the test input).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 59e19a8d-ceed-4b6b-bdd9-55aa3571e8e4
📒 Files selected for processing (2)
.claude/skills/github/scripts/issue/get_issue_context.pyscripts/skill_registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/github/scripts/issue/get_issue_context.py
rjmurillo
left a comment
rjmurillo
left a comment
There was a problem hiding this comment.
Reviewed and approved via autonomous PR monitoring session.
2 participants
Summary
get_issue_context.py,set_issue_assignee.py, andset_issue_labels.pyto usewrite_skill_output/write_skill_errorfromgithub_core.outputper ADR-051{"Success": bool, "Data": {...}, "Error": null, "Metadata": {...}}Fixes #673
Context
Only 3 of 33+ skill scripts used the standardized output helpers. This PR migrates the first batch of 3 issue scripts, meeting the minimum acceptance criteria from #673: at least 3 skills migrated to standard format.
Test plan
Generated with Claude Code