Register test plan repo with test plan and test cases skills#3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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 `@registry.yaml`:
- Around line 142-146: The registry entry uses a mutable ref and allows
non-strict loading; change the source.ref from the branch name (main) to an
immutable commit SHA (pin to a specific revision) and set source.strict to true
so the loader enforces integrity; update the source block (fields: type, repo,
ref, strict) for the fege/test-plan entry to reference the commit SHA and
strict: true to prevent future unintended changes from being ingested.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 64be11b6-df28-4170-9a92-d466585d65c6
📒 Files selected for processing (3)
.claude-plugin/marketplace.jsoncatalog.mdregistry.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
scripts/validate_registry.py (1)
125-128: Consider adding timeout togit clone.
--depth 1mitigates slow clones, but network issues can still cause indefinite hangs. A generous timeout (e.g., 120s) would improve robustness.Proposed fix
result = subprocess.run( ["git", "clone", "--depth", "1", "--branch", ref, "--", clone_url, tmpdir], - capture_output=True, text=True, + capture_output=True, text=True, timeout=120, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate_registry.py` around lines 125 - 128, The subprocess.run call that performs the git clone (the call assigning to result) should include a timeout (e.g., timeout=120) to avoid hanging; update the subprocess.run invocation in validate_registry.py to pass timeout=120 and wrap the call in try/except catching subprocess.TimeoutExpired so you can log or handle the timeout (e.g., record the error and treat the clone as failed) instead of letting it hang. Ensure you reference the same variable (result) and preserve capture_output=True, text=True behavior while adding the timeout and proper error handling around subprocess.TimeoutExpired.scripts/check_versions.py (1)
109-120: Consider importingsync_marketplacedirectly instead of subprocess.Calling via subprocess works but adds overhead and complicates error handling. Since both scripts share the same structure, a direct import would be simpler.
Alternative approach
# At top of file from scripts.sync_marketplace import load_registry as _, generate_marketplace # Replace subprocess call from pathlib import Path import json marketplace = generate_marketplace(registry) output_path = Path(".claude-plugin/marketplace.json") output_path.parent.mkdir(parents=True, exist_ok=True) with open(output_path, "w") as f: json.dump(marketplace, f, indent=2, sort_keys=True) f.write("\n") print(f"Generated {output_path} with {len(marketplace['plugins'])} plugin(s)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check_versions.py` around lines 109 - 120, Replace the subprocess.run invocation that calls scripts/sync_marketplace.py with a direct import and call to the module's functions: import generate_marketplace (and load_registry if needed) from scripts.sync_marketplace, call generate_marketplace(registry) to produce the marketplace dict, ensure the output directory (.claude-plugin) exists, write the JSON to .claude-plugin/marketplace.json with json.dump(..., indent=2, sort_keys=True) and a trailing newline, then print a success message including len(marketplace["plugins"]); remove the subprocess.run block and instead handle exceptions from generate_marketplace to print errors and exit with nonzero status if needed.
🤖 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/generate_catalog.py`:
- Line 84: The assignment to repo can raise TypeError when plugin["source"] is
None; change the lookup to safely handle missing/None source by first retrieving
source = plugin.get("source") and if source is falsy use an empty dict (e.g.,
source = plugin.get("source") or {}) then read repo = source.get("repo", "") so
that plugin entries without a source don't crash the script; update the code
around the repo assignment (the repo variable retrieval) accordingly.
In `@scripts/sync_marketplace.py`:
- Around line 44-49: The code assumes plugin["source"] exists and will KeyError
if missing; update the mapping in scripts/sync_marketplace.py to safely fetch
the source (e.g., use plugin.get("source")), check for None, and handle it (log
a clear warning including plugin identifier and skip the entry or set a sensible
default) before accessing source["type"] and source["repo"]; also defensively
access source.get("type") and source.get("repo") so entry["source"] is only set
when the required fields are present.
In `@scripts/validate_registry.py`:
- Around line 96-99: The subprocess.run call that runs git show (the line
assigning result = subprocess.run([...])) lacks a timeout and can block
indefinitely; update that invocation in scripts/validate_registry.py (the result
= subprocess.run(...) call) to include timeout=30 and handle
subprocess.TimeoutExpired appropriately (catch the exception where this call is
made or let it propagate with a clear error message) so the process cannot hang.
- Around line 83-91: The subprocess.run call that invokes ["gh", "api",
f"repos/{repo}", "--silent"] can hang; add a timeout (e.g., timeout=10) to the
subprocess.run call and wrap it in a try/except to catch
subprocess.TimeoutExpired; on timeout append an error using the same pattern
(errors.append(f" Plugin '{plugin['name']}': repo '{repo}' not accessible
(timeout)")) and continue, and ensure any other subprocess.run usages in this
module (e.g., the later call around line 125) are updated the same way to
include timeouts and TimeoutExpired handling.
---
Nitpick comments:
In `@scripts/check_versions.py`:
- Around line 109-120: Replace the subprocess.run invocation that calls
scripts/sync_marketplace.py with a direct import and call to the module's
functions: import generate_marketplace (and load_registry if needed) from
scripts.sync_marketplace, call generate_marketplace(registry) to produce the
marketplace dict, ensure the output directory (.claude-plugin) exists, write the
JSON to .claude-plugin/marketplace.json with json.dump(..., indent=2,
sort_keys=True) and a trailing newline, then print a success message including
len(marketplace["plugins"]); remove the subprocess.run block and instead handle
exceptions from generate_marketplace to print errors and exit with nonzero
status if needed.
In `@scripts/validate_registry.py`:
- Around line 125-128: The subprocess.run call that performs the git clone (the
call assigning to result) should include a timeout (e.g., timeout=120) to avoid
hanging; update the subprocess.run invocation in validate_registry.py to pass
timeout=120 and wrap the call in try/except catching subprocess.TimeoutExpired
so you can log or handle the timeout (e.g., record the error and treat the clone
as failed) instead of letting it hang. Ensure you reference the same variable
(result) and preserve capture_output=True, text=True behavior while adding the
timeout and proper error handling around subprocess.TimeoutExpired.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: af74afc3-6b21-4522-a554-cb85ad623c21
📒 Files selected for processing (7)
.claude-plugin/marketplace.jsoncatalog.mdschema/registry.schema.jsonscripts/check_versions.pyscripts/generate_catalog.pyscripts/sync_marketplace.pyscripts/validate_registry.py
✅ Files skipped from review due to trivial changes (2)
- catalog.md
- schema/registry.schema.json
Summary
test-planplugin (fege/test-plan) in the skills registrytest-plan.create,test-cases.create) and 4 sub-agents (test-plan.analyze.endpoints,.risks,.infra,test-plan.review)registry.yaml,marketplace.json, andcatalog.mdSkills
test-plan.createtest-cases.createtest-plan.analyze.endpointstest-plan.analyze.riskstest-plan.analyze.infratest-plan.reviewInstall
/plugin install test-plan@opendatahub-skills
Summary by CodeRabbit