workspace: migrate multi-repo-development to plugin marketplace - #249
Conversation
Import the evolved claude-workspace plugin (fonta-rh/claude-workspace) into plugins/workspace/ and remove the old multi-repo-development/ folder it supersedes. The plugin ships 8 skills, hooks, scripts, and 3 bundled domains (tnf, lvm-operator, example). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
WalkthroughChangesWorkspace plugin migration
Estimated code review effort: 4 (Complex) | ~75 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
These were development artifacts from the upstream repo, not part of the plugin itself. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/workspace/domains/tnf/context/openshift-docs.md (2)
23-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the bare URLs with Markdown links. Per CONTRIBUTING.md, updated Markdown must pass markdownlint, and both of these lines violate MD034; add descriptive link text for
https://docs.openshift.comandhttps://clusterlabs.org/pacemaker/doc/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/context/openshift-docs.md` at line 23, Replace the bare URL in plugins/workspace/domains/tnf/context/openshift-docs.md:23-23 with a descriptive Markdown link, and make the same change for the bare https://clusterlabs.org/pacemaker/doc/ URL in plugins/workspace/domains/tnf/context/pacemaker.md:39-39; preserve the surrounding text while ensuring both links satisfy markdownlint MD034.Source: Path instructions
9-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines around these list blocks, and replace the bare URLs in the same files with Markdown links. Per CONTRIBUTING.md, updated Markdown must pass markdownlint (MD032/MD034).
plugins/workspace/domains/tnf/context/openshift-docs.md#L11-L12,#L19-L20plugins/workspace/domains/tnf/context/origin.md#L16-L17plugins/workspace/domains/tnf/context/pacemaker.md#L9-L10,#L16-L17,#L22-L23plugins/workspace/domains/tnf/context/release.md#L9-L10,#L20-L21,#L27-L28plugins/workspace/domains/tnf/context/resource-agents.md#L9-L10,#L17-L18,#L27-L28plugins/workspace/domains/tnf/context/two-node-toolbox.md#L79-L80plugins/workspace/domains/tnf/context/openshift-docs.md#L23andplugins/workspace/domains/tnf/context/pacemaker.md#L39: convert the bare URLs to Markdown links.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/context/openshift-docs.md` around lines 9 - 12, Update the listed Markdown files to satisfy MD032 and MD034: add blank lines immediately before and after each affected list block, and convert the bare URLs in plugins/workspace/domains/tnf/context/openshift-docs.md at line 23 and plugins/workspace/domains/tnf/context/pacemaker.md at line 39 into descriptive Markdown links. Apply the list-spacing changes in plugins/workspace/domains/tnf/context/openshift-docs.md at lines 9-12 and 19-20; origin.md at 16-17; pacemaker.md at 9-10, 16-17, and 22-23; release.md at 9-10, 20-21, and 27-28; resource-agents.md at 9-10, 17-18, and 27-28; and two-node-toolbox.md at 79-80, preserving the existing content.Source: Path instructions
plugins/workspace/domains/tnf/context/fence-agents.md (1)
27-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the blank line before the fenced block. Per CONTRIBUTING.md, Markdown changes must pass markdownlint, and this layout violates MD031.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/context/fence-agents.md` around lines 27 - 28, In the “Debugging a fence failure” section of fence-agents.md, restore a blank line between the section text and the fenced code block so the Markdown layout complies with MD031.Source: Coding guidelines
🟡 Minor comments (13)
plugins/workspace/templates/dev-env-self.yaml.template-24-28 (1)
24-28: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winQuote the substituted name so YAML-significant characters can't corrupt the rendered file.
setup.shonly escapes sed metacharacters (&,/,\) before substitution, so a name containing:or#renders invalid or misparsed YAML (name: a#bsilently truncates toa). Per CONTRIBUTING.md ("YAML: 2-space indent + quote special strings"; "include negative test/validation coverage when parsing/validation logic changes"), quote the value and extendtests/test_setup.shmetacharacter cases to include:/#.🛡️ Proposed template change
self: - name: __SELF_NAME__ + name: "__SELF_NAME__" summary: ""Note this requires
setup.shto also escape"in the name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/templates/dev-env-self.yaml.template` around lines 24 - 28, Quote the __SELF_NAME__ substitution in the YAML template so values containing YAML-significant characters such as : or # remain intact. Update setup.sh to escape embedded double quotes safely before substitution, and extend tests/test_setup.sh with : and # cases that validate the rendered YAML and preserve the full name.Source: Path instructions
plugins/workspace/tests/test_setup.sh-1-14 (1)
1-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShebang must be
#!/usr/bin/bash.Per CONTRIBUTING.md ("bash:
#!/usr/bin/bash,set -euo pipefail, shellcheck"), the shebang violates the repo shell standard. Omitting-eis defensible here — the harness intentionally continues past failures and exits via[[ $FAILED -eq 0 ]]at Line 535 — but worth a short inline comment so it doesn't read as an oversight.🔧 Proposed fix
-#!/bin/bash +#!/usr/bin/bash @@ -set -uo pipefail +# Note: `-e` is intentionally omitted; failures are tallied and reported at the end. +set -uo pipefail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/tests/test_setup.sh` around lines 1 - 14, Update the test_setup.sh shebang to #!/usr/bin/bash and add a brief inline comment explaining that set -e is intentionally omitted because the harness continues through failures and reports them via the final FAILED check. Preserve the existing set -uo pipefail behavior.Sources: Coding guidelines, Path instructions
plugins/workspace/scripts/consolidate-project.py-157-159 (1)
157-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrikethrough items are archived but also retained in CLAUDE.md.
build_archive_blockcopiessection.strikethroughinto the archive, whilebuild_replacementonly drops the archived checked lines, so strikethrough entries end up duplicated in both files. Either exclude them from the archive block or add theirline_idxtoto_archive_set.Also applies to: 177-182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/scripts/consolidate-project.py` around lines 157 - 159, The consolidation flow duplicates strikethrough entries by archiving them while retaining them in CLAUDE.md. Update build_replacement and its to_archive_set handling so each strikethrough item’s line_idx is treated as archived, while preserving the existing build_archive_block behavior.plugins/workspace/scripts/resume-project.py-308-308 (1)
308-308: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the ambiguous
lloop variable (ruff E741).Guidelines require Python to pass ruff; both comprehensions trip E741.
🔧 Proposed fix
- lines = [l for l in status_result.stdout.splitlines() if l.strip()] + lines = [ln for ln in status_result.stdout.splitlines() if ln.strip()]- return [l.strip() for l in result.stdout.splitlines() if l.strip()] + return [ln.strip() for ln in result.stdout.splitlines() if ln.strip()]Also applies to: 345-345
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/scripts/resume-project.py` at line 308, Rename the ambiguous `l` comprehension variable in both occurrences around the status-output processing to a descriptive name such as `line`, updating its references within each comprehension so ruff E741 is resolved without changing behavior.Sources: Coding guidelines, Linters/SAST tools
plugins/workspace/scripts/setup.sh-451-451 (1)
451-451: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQuote the expansion in the status printf.
$(echo -e $status)is unquoted (shellcheck SC2086) and word-splits; the column alignment also breaks because the ANSI escapes count toward%-30swidths. Guidelines require all variables quoted.🔧 Proposed fix
- printf "%-30s $(echo -e $status)%-1s %-20s %s\n" "$dir" "" "$branch_info" "$last_commit" + printf "%-30s %b%-12s%b %-20s %s\n" "$dir" "$status" "" "$NC" "$branch_info" "$last_commit"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/scripts/setup.sh` at line 451, Update the status output printf in the setup script to quote the status expansion passed through echo, preventing shell word splitting and preserving the status value as a single argument. Keep the existing formatting and output behavior unchanged.Sources: Coding guidelines, Linters/SAST tools
plugins/workspace/CLAUDE.md-10-14 (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the repository/plugin root distinction.
In this checkout,
plugins/workspace/is the plugin root, while the repository root contains.claude-plugin/marketplace.json. Saying “Repo root = plugin root” and listing both manifests under one.claude-plugindirectory is misleading. Document the two roots separately and point the marketplace entry to the repository-root catalog.Also applies to: 24-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/CLAUDE.md` around lines 10 - 14, Update the repository overview in CLAUDE.md to distinguish the repository root from the plugins/workspace plugin root. Describe the plugin’s .claude-plugin manifests separately, and identify the repository-root .claude-plugin/marketplace.json as the marketplace catalog referenced by the marketplace entry.plugins/workspace/domains/example/dev-env.yaml-9-9 (1)
9-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQuote the repository URL.
Per the YAML guideline, strings containing special characters must be quoted. Use
url: "https://github.com/github/gitignore.git".As per coding guidelines, YAML strings with special characters must be quoted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/example/dev-env.yaml` at line 9, Update the url value in the dev-env YAML configuration to quote the repository URL, preserving the existing URL exactly while using valid YAML string syntax.Source: Coding guidelines
plugins/workspace/domains/tnf/docs/testing.md-7-7 (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the repository’s
bare metalterminology.Change “Baremetal payloads” to “Bare metal payloads.”
Based on learnings, Markdown documentation in this repository uses “bare metal” as two words.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/docs/testing.md` at line 7, Update the “Stub extensions” documentation text to replace “Baremetal payloads” with “Bare metal payloads,” preserving the rest of the sentence unchanged.Source: Learnings
plugins/workspace/domains/tnf/docs/architecture.md-62-66 (1)
62-66: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender angle-bracket placeholders as literal documentation.
Both locations use placeholders that Markdown can interpret as HTML tags, hiding the patterns from readers. Wrap each complete pattern in backticks.
plugins/workspace/domains/tnf/docs/architecture.md#L62-L66: Quote the BMH, Machine, and Kubernetes node patterns with code spans.plugins/workspace/domains/lvm-operator/context/release.md#L40-L42: Quote the presubmit, periodic, and postsubmit job patterns with code spans.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/docs/architecture.md` around lines 62 - 66, Update the BMH, Machine, and K8s Node pattern entries in plugins/workspace/domains/tnf/docs/architecture.md lines 62-66 by wrapping each complete angle-bracket placeholder pattern in Markdown code spans. Apply the same change to the presubmit, periodic, and postsubmit job patterns in plugins/workspace/domains/lvm-operator/context/release.md lines 40-42; no other content changes are needed.Source: Coding guidelines
plugins/workspace/domains/tnf/docs/testing.md-6-6 (1)
6-6: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the required working directory explicit.
Step 1 says to run
make openshift-testsfrom the origin repository, while step 2 usesrepos/two-node-toolbox/..., which is workspace-root-relative. Running the steps from one shell directory can make the proxy command fail; document the requiredcd/workspace-root variable explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/docs/testing.md` at line 6, Update the testing instructions around the “Source proxy” step to explicitly establish the required working directory, either by adding the necessary cd command or defining and using a workspace-root variable. Ensure the documented commands consistently resolve the origin repository and the workspace-root-relative repos/two-node-toolbox/deploy/openshift-clusters/proxy.env path from the same shell context.plugins/workspace/domains/tnf/docs/debugging.md-31-37 (1)
31-37: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAvoid passing BMC passwords on the command line
fence_redfishsupports--password-script, so these examples should use that (or another secret-backed prompt/file) instead of--password=<pass>. Command-line passwords can leak via shell history and process listings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/docs/debugging.md` around lines 31 - 37, Update the fence_redfish examples in the debugging documentation to use --password-script or another secret-backed prompt/file instead of passing credentials through --password=<pass>. Apply this to each command that currently includes the password argument while preserving the existing actions and verbose options.plugins/workspace/domains/lvm-operator/docs/architecture.md-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to these fences
Per CONTRIBUTING.md, Markdown changes must pass markdownlint, so these unlabeled fenced blocks should use an info string such astextorini.
plugins/workspace/domains/lvm-operator/docs/architecture.md#L5plugins/workspace/domains/lvm-operator/docs/architecture.md#L46plugins/workspace/domains/lvm-operator/docs/architecture.md#L78plugins/workspace/domains/tnf/supplemental/fence-agents.md#L13plugins/workspace/domains/tnf/supplemental/fence-agents.md#L107🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/lvm-operator/docs/architecture.md` at line 5, Label every currently unlabeled Markdown fence with an appropriate info string, such as text or ini, to satisfy markdownlint: update plugins/workspace/domains/lvm-operator/docs/architecture.md at lines 5, 46, and 78, and plugins/workspace/domains/tnf/supplemental/fence-agents.md at lines 13 and 107. Preserve each block’s existing content.Sources: Coding guidelines, Path instructions
plugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md-3-3 (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a generated-doc-relative path here.
domains/tnf/supplemental/fence-agents.mdexists only in the plugin tree; aftersetup.shcopies these files, the doc lives at the repo root, so that reference won’t resolve. Same fix applies toplugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md,plugins/workspace/domains/tnf/supplemental/enhancements.md, andplugins/workspace/domains/tnf/supplemental/machine-config-operator.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md` at line 3, Update the example-reference comment in plugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md at lines 3-3, plugins/workspace/domains/tnf/supplemental/enhancements.md at lines 3-3, and plugins/workspace/domains/tnf/supplemental/machine-config-operator.md at lines 3-3 to use the generated-doc-relative path for fence-agents.md, ensuring the reference resolves after setup.sh copies the documents to the repository root.
🧹 Nitpick comments (3)
plugins/workspace/tests/test_setup.sh (1)
136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp dirs leak if the run aborts before Line 528.
PLUGINand the per-groupws/fixturedirs are only removed on the happy path; an abort underset -u(or aCtrl-C) leavesmktemp -dtrees behind. AnEXITtrap accumulating created dirs makes cleanup unconditional.♻️ Sketch
+_TMPDIRS=() +trap 'rm -rf "${_TMPDIRS[@]:-}"' EXIT + new_plugin() { local plugin plugin=$(mktemp -d) + _TMPDIRS+=("$plugin")with
new_workspace/fixtures registering their dirs the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/tests/test_setup.sh` around lines 136 - 137, Make temporary-directory cleanup unconditional in the test setup flow by adding an EXIT trap that tracks every directory created by new_plugin and the per-group new_workspace/fixture helpers. Register each created path immediately, and have the trap safely remove all registered paths under set -u, including when the script aborts or receives Ctrl-C.plugins/workspace/scripts/setup.sh (1)
561-561: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
ls | grepwith a glob loop (SC2010).- ls "$tmp_dir/pack/" 2>/dev/null | grep -v '^\.' | sed 's/^/ /' || true + for entry in "$tmp_dir"/pack/*/; do + [[ -d "$entry" ]] && echo " $(basename "$entry")" + done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/scripts/setup.sh` at line 561, Replace the ls, grep, and sed pipeline in the temporary package listing logic with a shell glob loop that iterates over non-hidden entries in "$tmp_dir/pack/" and prints each with the existing two-space prefix. Preserve the current suppression of errors and empty-directory behavior.Source: Linters/SAST tools
plugins/workspace/scripts/skills.py (1)
220-233: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
projectand frontmattersourceas single path components too.
is_safe_nameguards the skill name, butproject(from argv) and each skill'ssource(from project frontmatter) are joined intoroot / "projects" / projectandroot / "repos" / repowithout the same check, so../values read outside the workspace root. Reusingis_safe_namehere keeps all workspace path joins consistent.♻️ Proposed change
def read_project_skills(root: Path, project: str) -> list[dict]: """Normalized [{name, source}] from a project's frontmatter skills: list.""" + if not is_safe_name(project): + fail(f"invalid project name '{project}'") claude_md = root / "projects" / project / "CLAUDE.md"def find_skill_dir(root: Path, repo: str, name: str) -> Path | None: """Repo skill dir whose frontmatter name equals `name`.""" + if not is_safe_name(repo): + return None for skill_dir, fm in iter_repo_skills(root, repo):Also applies to: 236-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/workspace/scripts/skills.py` around lines 220 - 233, Update read_project_skills and the related source-path handling to validate the project argument and each frontmatter source with is_safe_name before using them in workspace path joins. Reject or fail unsafe values consistently with skill-name validation, while preserving existing behavior for valid single-component names.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/workspace/domains/lvm-operator/dev-env.yaml`:
- Around line 4-7: Populate the repos list in dev-env.yaml with the lvm-operator
repositories and their required per-repository context before bundling the
domain, following dev-env.yaml.template’s schema. If no repositories are
available, remove lvm-operator from the bundled catalog instead of shipping an
empty repos configuration.
In `@plugins/workspace/domains/tnf/docs/testing.md`:
- Line 8: Update the Hypervisor config example in the testing documentation so
privateKeyPath uses an expanded $HOME-based path or an absolute path instead of
a literal ~ inside the single-quoted JSON, ensuring SSH key lookup resolves
correctly.
In `@plugins/workspace/README.md`:
- Around line 18-19: Update the installation command in the workspace README to
use the migrated openshift-eng/edge-tooling marketplace, matching the source
documented in the root README, while keeping the workspace plugin installation
command unchanged.
In `@plugins/workspace/scripts/domain-info.py`:
- Around line 232-239: Harden the copy-on-write flow around shutil.copytree and
domain_name validation. Validate domain_name as a single safe path component
before using it in root / "domains" / domain_name or base / name, rejecting
absolute paths, separators, and traversal components. Catch copytree filesystem
errors and return the existing documented JSON error response while preserving
the always-exit-0 contract.
In `@plugins/workspace/scripts/resume-project.py`:
- Around line 300-326: Update the subprocess calls in get_recent_names and
resolve_worktree_status to pass the documented timeout value, and catch
subprocess.TimeoutExpired alongside the existing exceptions. Record the timeout
as an error using the existing entry error-handling path, ensuring a hung git
command cannot block processing indefinitely.
In `@plugins/workspace/scripts/setup.sh`:
- Line 1: Update the setup script shebang to #!/usr/bin/bash and enable strict
shell options with set -euo pipefail. Audit the script for unset-variable
compatibility, especially the set -- "${positionals[@]}" usage in the
positional-argument handling, and guard empty arrays with the
repository-compatible expansion or an equivalent Bash version guard.
- Around line 165-177: Replace direct interpolation of workspace paths into
Python source in the YAML parsing blocks, including parse_yaml_repos,
parse_yaml_domain, and get_domain_name_from_dir, with the existing
sys.argv-based argument-passing pattern used by record_domain_source. Pass each
path as a separate argument to python3 and read it from sys.argv inside the
script, preserving the current parsing and output behavior.
In `@plugins/workspace/skills/close-project/SKILL.md`:
- Around line 124-128: The PR checkout cleanup instructions currently
force-delete branches without protecting unmerged local commits. Update the PR
branch handling around “git branch -D pr/<number>” to require explicit user
confirmation before deletion, or attempt safe deletion with “-d” first and only
use “-D” after clearly warning what would be lost.
- Around line 1-5: Update the close-project frontmatter to make description
action-triggered with “Use when…” wording and add an allowed-tools declaration
that includes Bash plus only the capabilities needed for its file edits and user
questions. Preserve the existing name and argument-hint fields.
In `@plugins/workspace/skills/consolidate-project/SKILL.md`:
- Around line 1-5: Normalize the frontmatter in all seven listed skill files:
use the namespaced names workspace:consolidate-project, workspace:create-domain,
workspace:new-project, workspace:resume-project, workspace:setup-environment,
workspace:update-domain, and workspace:update-project respectively; add minimal
allowed-tools entries; and replace each description with an accurate
action-triggered “Use when…” description, following
plugins/docs/SKILL-GUIDELINES.md. Apply the corresponding metadata changes in
every listed file, not just the anchor.
In `@plugins/workspace/skills/new-project/SKILL.md`:
- Around line 17-30: Update “Step 0: Resolve the Workspace Root” to resolve $WS
in this order: WORKSPACE_ROOT, CLAUDE_PROJECT_DIR, then the nearest ancestor
containing dev-env.yaml; otherwise stop with an actionable setup or path error.
Keep the plugin root separate from the mutable workspace root, and continue
prefixing all projects/, repos/, and git -C paths with $WS.
- Around line 106-164: Update the worktree instructions around branch derivation
and analysis PR handling to validate all user- or repository-derived branch,
repository, and PR values with explicit allowlists, rejecting traversal,
separators, and malformed numbers before shell execution. Quote every validated
shell variable when used as an argument, and replace the current grep, echo,
sed, and git command workflow with the permitted Bash-only approach while
preserving default-branch detection and worktree creation behavior.
- Around line 169-229: The Repo Skill Linking workflow must not scan or expose
arbitrary repository skills. Replace the `skills.py scan` and selection flow in
“Repo Skill Linking” with a trusted explicit allowlist or reviewed manifest
containing only approved skill metadata, and link only entries from that source;
do not read or surface arbitrary `SKILL.md` content while preserving project
creation when no approved skills are available.
In `@plugins/workspace/skills/resume-project/SKILL.md`:
- Around line 12-15: Update the Step 1 command in the resume-project
instructions to extract only the first project token from $ARGUMENTS and pass
that token as a quoted argument to resume-project.py. Preserve tokens containing
spaces or special characters as intended, and include -- before the token if
resume-project.py supports option termination.
In `@plugins/workspace/skills/setup-environment/SKILL.md`:
- Around line 99-110: The Step 2 initialization command must preserve external
domain URLs containing fragments. Store the selected domain name or URL in a
quoted shell variable and pass that variable as "$domain_url" to setup.sh,
ensuring the `#subdir` portion is not interpreted as a shell comment.
In `@plugins/workspace/skills/update-project/SKILL.md`:
- Around line 12-16: Update the Scope Rules in SKILL.md to resolve the workspace
root as $WS before accessing project files, reusing the established workspace
resolution order. Change all relative projects/<name>/ paths in the skill to
$WS/projects/<name>/ absolute paths, while keeping workspace data separate from
the plugin root.
---
Outside diff comments:
In `@plugins/workspace/domains/tnf/context/fence-agents.md`:
- Around line 27-28: In the “Debugging a fence failure” section of
fence-agents.md, restore a blank line between the section text and the fenced
code block so the Markdown layout complies with MD031.
In `@plugins/workspace/domains/tnf/context/openshift-docs.md`:
- Line 23: Replace the bare URL in
plugins/workspace/domains/tnf/context/openshift-docs.md:23-23 with a descriptive
Markdown link, and make the same change for the bare
https://clusterlabs.org/pacemaker/doc/ URL in
plugins/workspace/domains/tnf/context/pacemaker.md:39-39; preserve the
surrounding text while ensuring both links satisfy markdownlint MD034.
- Around line 9-12: Update the listed Markdown files to satisfy MD032 and MD034:
add blank lines immediately before and after each affected list block, and
convert the bare URLs in plugins/workspace/domains/tnf/context/openshift-docs.md
at line 23 and plugins/workspace/domains/tnf/context/pacemaker.md at line 39
into descriptive Markdown links. Apply the list-spacing changes in
plugins/workspace/domains/tnf/context/openshift-docs.md at lines 9-12 and 19-20;
origin.md at 16-17; pacemaker.md at 9-10, 16-17, and 22-23; release.md at 9-10,
20-21, and 27-28; resource-agents.md at 9-10, 17-18, and 27-28; and
two-node-toolbox.md at 79-80, preserving the existing content.
---
Minor comments:
In `@plugins/workspace/CLAUDE.md`:
- Around line 10-14: Update the repository overview in CLAUDE.md to distinguish
the repository root from the plugins/workspace plugin root. Describe the
plugin’s .claude-plugin manifests separately, and identify the repository-root
.claude-plugin/marketplace.json as the marketplace catalog referenced by the
marketplace entry.
In `@plugins/workspace/domains/example/dev-env.yaml`:
- Line 9: Update the url value in the dev-env YAML configuration to quote the
repository URL, preserving the existing URL exactly while using valid YAML
string syntax.
In `@plugins/workspace/domains/lvm-operator/docs/architecture.md`:
- Line 5: Label every currently unlabeled Markdown fence with an appropriate
info string, such as text or ini, to satisfy markdownlint: update
plugins/workspace/domains/lvm-operator/docs/architecture.md at lines 5, 46, and
78, and plugins/workspace/domains/tnf/supplemental/fence-agents.md at lines 13
and 107. Preserve each block’s existing content.
In `@plugins/workspace/domains/tnf/docs/architecture.md`:
- Around line 62-66: Update the BMH, Machine, and K8s Node pattern entries in
plugins/workspace/domains/tnf/docs/architecture.md lines 62-66 by wrapping each
complete angle-bracket placeholder pattern in Markdown code spans. Apply the
same change to the presubmit, periodic, and postsubmit job patterns in
plugins/workspace/domains/lvm-operator/context/release.md lines 40-42; no other
content changes are needed.
In `@plugins/workspace/domains/tnf/docs/debugging.md`:
- Around line 31-37: Update the fence_redfish examples in the debugging
documentation to use --password-script or another secret-backed prompt/file
instead of passing credentials through --password=<pass>. Apply this to each
command that currently includes the password argument while preserving the
existing actions and verbose options.
In `@plugins/workspace/domains/tnf/docs/testing.md`:
- Line 7: Update the “Stub extensions” documentation text to replace “Baremetal
payloads” with “Bare metal payloads,” preserving the rest of the sentence
unchanged.
- Line 6: Update the testing instructions around the “Source proxy” step to
explicitly establish the required working directory, either by adding the
necessary cd command or defining and using a workspace-root variable. Ensure the
documented commands consistently resolve the origin repository and the
workspace-root-relative
repos/two-node-toolbox/deploy/openshift-clusters/proxy.env path from the same
shell context.
In `@plugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md`:
- Line 3: Update the example-reference comment in
plugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.md at lines
3-3, plugins/workspace/domains/tnf/supplemental/enhancements.md at lines 3-3,
and plugins/workspace/domains/tnf/supplemental/machine-config-operator.md at
lines 3-3 to use the generated-doc-relative path for fence-agents.md, ensuring
the reference resolves after setup.sh copies the documents to the repository
root.
In `@plugins/workspace/scripts/consolidate-project.py`:
- Around line 157-159: The consolidation flow duplicates strikethrough entries
by archiving them while retaining them in CLAUDE.md. Update build_replacement
and its to_archive_set handling so each strikethrough item’s line_idx is treated
as archived, while preserving the existing build_archive_block behavior.
In `@plugins/workspace/scripts/resume-project.py`:
- Line 308: Rename the ambiguous `l` comprehension variable in both occurrences
around the status-output processing to a descriptive name such as `line`,
updating its references within each comprehension so ruff E741 is resolved
without changing behavior.
In `@plugins/workspace/scripts/setup.sh`:
- Line 451: Update the status output printf in the setup script to quote the
status expansion passed through echo, preventing shell word splitting and
preserving the status value as a single argument. Keep the existing formatting
and output behavior unchanged.
In `@plugins/workspace/templates/dev-env-self.yaml.template`:
- Around line 24-28: Quote the __SELF_NAME__ substitution in the YAML template
so values containing YAML-significant characters such as : or # remain intact.
Update setup.sh to escape embedded double quotes safely before substitution, and
extend tests/test_setup.sh with : and # cases that validate the rendered YAML
and preserve the full name.
In `@plugins/workspace/tests/test_setup.sh`:
- Around line 1-14: Update the test_setup.sh shebang to #!/usr/bin/bash and add
a brief inline comment explaining that set -e is intentionally omitted because
the harness continues through failures and reports them via the final FAILED
check. Preserve the existing set -uo pipefail behavior.
---
Nitpick comments:
In `@plugins/workspace/scripts/setup.sh`:
- Line 561: Replace the ls, grep, and sed pipeline in the temporary package
listing logic with a shell glob loop that iterates over non-hidden entries in
"$tmp_dir/pack/" and prints each with the existing two-space prefix. Preserve
the current suppression of errors and empty-directory behavior.
In `@plugins/workspace/scripts/skills.py`:
- Around line 220-233: Update read_project_skills and the related source-path
handling to validate the project argument and each frontmatter source with
is_safe_name before using them in workspace path joins. Reject or fail unsafe
values consistently with skill-name validation, while preserving existing
behavior for valid single-component names.
In `@plugins/workspace/tests/test_setup.sh`:
- Around line 136-137: Make temporary-directory cleanup unconditional in the
test setup flow by adding an EXIT trap that tracks every directory created by
new_plugin and the per-group new_workspace/fixture helpers. Register each
created path immediately, and have the trap safely remove all registered paths
under set -u, including when the script aborts or receives Ctrl-C.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fa1e586c-8504-4c75-b045-887b26ff6daa
📒 Files selected for processing (84)
.claude-plugin/marketplace.jsonREADME.mdmulti-repo-development/.claude/commands/project/close.mdmulti-repo-development/.claude/commands/project/resume.mdmulti-repo-development/.claude/skills/dev-env-setup/SKILL.mdmulti-repo-development/.gitignoremulti-repo-development/CLAUDE.mdmulti-repo-development/README.mdmulti-repo-development/dev-env.yaml.templatemulti-repo-development/presets/lvm-operator/dev-env.yamlmulti-repo-development/presets/tnf/supplemental/resource-agents.mdmulti-repo-development/repos.txt.templatemulti-repo-development/setup.shplugins/workspace/.claude-plugin/plugin.jsonplugins/workspace/.gitignoreplugins/workspace/CLAUDE.mdplugins/workspace/README.mdplugins/workspace/domains/example/context/gitignore.mdplugins/workspace/domains/example/dev-env.yamlplugins/workspace/domains/example/domain.yamlplugins/workspace/domains/lvm-operator/context.mdplugins/workspace/domains/lvm-operator/context/konflux-release-data.mdplugins/workspace/domains/lvm-operator/context/lvm-operator.mdplugins/workspace/domains/lvm-operator/context/product-definitions.mdplugins/workspace/domains/lvm-operator/context/release.mdplugins/workspace/domains/lvm-operator/context/topolvm.mdplugins/workspace/domains/lvm-operator/dev-env.yamlplugins/workspace/domains/lvm-operator/docs/architecture.mdplugins/workspace/domains/lvm-operator/domain.yamlplugins/workspace/domains/lvm-operator/settings.local.json.tplplugins/workspace/domains/tnf/context.mdplugins/workspace/domains/tnf/context/api.mdplugins/workspace/domains/tnf/context/assisted-service.mdplugins/workspace/domains/tnf/context/cluster-baremetal-operator.mdplugins/workspace/domains/tnf/context/cluster-etcd-operator.mdplugins/workspace/domains/tnf/context/dev-scripts.mdplugins/workspace/domains/tnf/context/enhancements.mdplugins/workspace/domains/tnf/context/fence-agents.mdplugins/workspace/domains/tnf/context/installer.mdplugins/workspace/domains/tnf/context/machine-config-operator.mdplugins/workspace/domains/tnf/context/openshift-docs.mdplugins/workspace/domains/tnf/context/origin.mdplugins/workspace/domains/tnf/context/pacemaker.mdplugins/workspace/domains/tnf/context/release.mdplugins/workspace/domains/tnf/context/resource-agents.mdplugins/workspace/domains/tnf/context/two-node-toolbox.mdplugins/workspace/domains/tnf/dev-env.yamlplugins/workspace/domains/tnf/docs/architecture.mdplugins/workspace/domains/tnf/docs/ci.mdplugins/workspace/domains/tnf/docs/debugging.mdplugins/workspace/domains/tnf/docs/testing.mdplugins/workspace/domains/tnf/domain.yamlplugins/workspace/domains/tnf/settings.local.json.tplplugins/workspace/domains/tnf/supplemental/cluster-etcd-operator.mdplugins/workspace/domains/tnf/supplemental/enhancements.mdplugins/workspace/domains/tnf/supplemental/fence-agents.mdplugins/workspace/domains/tnf/supplemental/machine-config-operator.mdplugins/workspace/domains/tnf/supplemental/openshift-docs.mdplugins/workspace/domains/tnf/supplemental/origin.mdplugins/workspace/domains/tnf/supplemental/pacemaker.mdplugins/workspace/domains/tnf/supplemental/resource-agents.mdplugins/workspace/hooks/hooks.jsonplugins/workspace/scripts/consolidate-project.pyplugins/workspace/scripts/domain-info.pyplugins/workspace/scripts/recent-projects.pyplugins/workspace/scripts/resume-project.pyplugins/workspace/scripts/setup.shplugins/workspace/scripts/skills.pyplugins/workspace/scripts/workspace_lib.pyplugins/workspace/skills/close-project/SKILL.mdplugins/workspace/skills/consolidate-project/SKILL.mdplugins/workspace/skills/create-domain/SKILL.mdplugins/workspace/skills/create-domain/context-template.mdplugins/workspace/skills/new-project/SKILL.mdplugins/workspace/skills/resume-project/SKILL.mdplugins/workspace/skills/setup-environment/SKILL.mdplugins/workspace/skills/update-domain/SKILL.mdplugins/workspace/skills/update-project/SKILL.mdplugins/workspace/templates/dev-env-self.yaml.templateplugins/workspace/templates/dev-env.yaml.templateplugins/workspace/templates/settings.local.json.tplplugins/workspace/tests/test_domain_info.pyplugins/workspace/tests/test_setup.shplugins/workspace/tests/test_skills.py
💤 Files with no reviewable changes (11)
- multi-repo-development/CLAUDE.md
- multi-repo-development/dev-env.yaml.template
- multi-repo-development/presets/lvm-operator/dev-env.yaml
- multi-repo-development/.claude/skills/dev-env-setup/SKILL.md
- multi-repo-development/repos.txt.template
- multi-repo-development/.claude/commands/project/resume.md
- multi-repo-development/setup.sh
- multi-repo-development/README.md
- multi-repo-development/.claude/commands/project/close.md
- multi-repo-development/presets/tnf/supplemental/resource-agents.md
- multi-repo-development/.gitignore
| ## Scope Rules | ||
|
|
||
| **Update:** files under `projects/<name>/` in the workspace — CLAUDE.md | ||
| (index, checklists, progress) and detail files (investigation notes, test | ||
| results, plans, etc.). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve $WS before reading or editing project files.
This skill uses relative projects/<name>/... paths and never resolves the workspace root. Running it from a nested repository, worktree, or plugin directory can update the wrong project or fail to find the intended one. Reuse the workspace resolution order and operate on $WS/projects/<name>/... absolute paths.
Based on learnings, workspace data must remain under the resolved mutable workspace root, separate from the plugin root.
Also applies to: 32-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/workspace/skills/update-project/SKILL.md` around lines 12 - 16,
Update the Scope Rules in SKILL.md to resolve the workspace root as $WS before
accessing project files, reusing the established workspace resolution order.
Change all relative projects/<name>/ paths in the skill to $WS/projects/<name>/
absolute paths, while keeping workspace data separate from the plugin root.
Sources: Path instructions, Learnings
There was a problem hiding this comment.
Won't fix — the skill uses $WS-prefixed paths in its actual steps. The scope-rules section describes what the skill touches, not how paths are resolved.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
skill='plugins/workspace/skills/update-project/SKILL.md'
printf '%s\n' '--- update-project skill ---'
cat -n "$skill"
printf '%s\n' '--- comparable workspace-root resolution references ---'
rg -n -C 3 '\$WS|workspace root|resolve.*workspace' \
plugins/workspace/skills --glob 'SKILL.md'Length of output: 40836
@fonta-rh, I rechecked the current file. The scope statement is descriptive, but the operational workflow does not currently use $WS:
- Line 34:
Read projects/<name>/CLAUDE.md in full. - Line 45: references new files in
projects/<name>/.
There is no $WS assignment or workspace-root resolution anywhere in this skill. By contrast, new-project establishes $WS in Step 0 and requires absolute $WS/... paths. The finding therefore still applies.
You are interacting with an AI system.
Apply CodeRabbit suggestion from PR openshift-eng#249 Auto-applied: - plugins/workspace/README.md:18-19: install commands pointed to old fonta-rh/multi-repo-dev-env Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed CodeRabbit review-body findings:
|
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Nice work on this migration, Pablo! The plugin architecture is clean — the domain/workspace separation, JSON-over-stdout contract, and the skill symlink lifecycle are well-designed. The test suites have excellent isolation and good coverage of the external domain flow.
I found a few issues worth addressing. Two are high-priority (silent parse failures, deleted-then-read temp dir), and the rest are correctness and consistency fixes. Details in the inline comments.
| fi | ||
| "$callback" | ||
| done < <(parse_yaml_repos "$REPO_SOURCE") | ||
| } |
There was a problem hiding this comment.
Bug (high): iterate_repos() uses process substitution < <(parse_yaml_repos ...) to feed the while-read loop. In bash, exit codes from process substitution are not propagated — even under set -e. If parse_yaml_repos fails (malformed YAML, missing parser, wrong schema), the loop runs zero iterations and returns 0.
The callers clone_all() (line 382) and update_all() (line 398) then unconditionally print the green-checkmark "All repositories cloned!" message. The user sees success when literally nothing happened.
Suggestion — capture output into a variable and check the exit code before entering the loop:
iterate_repos() {
local callback="$1"
local repo_lines
repo_lines=$(parse_yaml_repos "$REPO_SOURCE") || {
log_error "Failed to parse repositories from $REPO_SOURCE"
return 1
}
if [[ -z "$repo_lines" ]]; then
log_warn "No repositories found in $REPO_SOURCE"
return 0
fi
while IFS='|' read -r url dir branch _name _cat _summary; do
...
done <<< "$repo_lines"
}There was a problem hiding this comment.
Good catch — fixed in 4c9123f. Switched to capturing output into a variable and checking the exit code before entering the loop.
Fix bugs and harden error handling based on Luca's review: - iterate_repos: capture parse output and check exit code instead of swallowing failures via process substitution - fetch_external_domain: show pack listing before deleting tmp dir - consolidate-project: exclude strikethrough items from consolidated output (they were duplicated in both archive and CLAUDE.md) - consolidate-project: use sys.exit(0) + "error" key to match the JSON-over-stdout convention used by all other scripts - setup.sh: pass file paths via sys.argv instead of splicing into Python source (parse_yaml_repos, parse_yaml_domain, get_domain_name_from_dir) - setup.sh: use git -C instead of cd to avoid cwd corruption under set -e (update_repo, show_status) - domain-info: wrap copytree in try/except to preserve JSON contract Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
coderabbitai
Bot
added
the
ready-for-human-review
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/workspace/scripts/consolidate-project.py`:
- Around line 171-175: The archived count reported in the JSON must match all
items added to to_archive_set, including strikethrough items. Update the final
sections[*].archived calculation in the consolidation flow to derive from the
complete archive set, or otherwise include strikethrough items consistently with
the pointer’s archived_count.
- Around line 346-355: Canonicalize the project directory derived in the
argument-handling flow around project_name and root, then verify the resolved
candidate remains within the resolved root / "projects" directory before any
is_dir() or file access. Reject absolute paths, traversal, and symlinked entries
that resolve outside the allowed directory, preserving the existing
error-and-exit behavior for invalid projects.
In `@plugins/workspace/scripts/domain-info.py`:
- Around line 237-244: Update the copy failure handler around shutil.copytree in
the domain-copy flow to remove dest when it was newly created, preventing
partially populated domains from persisting. Perform the cleanup in the except
block before returning, while preserving the existing error JSON output and
avoiding deletion of a pre-existing destination.
In `@plugins/workspace/scripts/setup.sh`:
- Around line 258-276: Update iterate_repos and its clone_all/update_all callers
so an empty repository list is distinguished from successful processing rather
than returning success; propagate a non-success or explicit no-op result when no
valid entries are processed, and ensure unconditional success messages are only
printed after at least one repository has been handled.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8eaf347b-e5fd-4b97-9d1d-fdf9eb53ce7d
📒 Files selected for processing (4)
plugins/workspace/TODO.mdplugins/workspace/scripts/consolidate-project.pyplugins/workspace/scripts/domain-info.pyplugins/workspace/scripts/setup.sh
| project_name = args[0] | ||
| root = workspace_lib.resolve_workspace_root() | ||
| if root is None: | ||
| print(json.dumps({ | ||
| "status": "error", | ||
| "error": "Could not determine the workspace root. Set WORKSPACE_ROOT " | ||
| "or run inside a workspace (a directory containing dev-env.yaml).", | ||
| })) | ||
| sys.exit(0) | ||
| project_dir = root / "projects" / project_name |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='plugins/workspace/scripts/consolidate-project.py'
echo '--- file outline ---'
python3 - <<'PY'
from pathlib import Path
p = Path("plugins/workspace/scripts/consolidate-project.py")
print(p, p.exists(), p.stat().st_size if p.exists() else None)
PY
echo '--- relevant lines around 320-390 ---'
nl -ba "$file" | sed -n '320,390p'
echo '--- search for project_dir / resolve / is_dir / symlink checks ---'
rg -n "project_dir|resolve\(|is_dir\(|symlink|CLAUDE\.md|archive" "$file"Repository: openshift-eng/edge-tooling
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='plugins/workspace/scripts/consolidate-project.py'
echo '--- relevant lines around 320-390 ---'
sed -n '320,390p' "$file" | cat -n
echo '--- function definitions / path logic ---'
python3 - <<'PY'
from pathlib import Path
import re
text = Path("plugins/workspace/scripts/consolidate-project.py").read_text()
for m in re.finditer(r"^def\s+([A-Za-z_]\w*)\(", text, re.M):
name = m.group(1)
start = m.start()
print(name)
PY
echo '--- path-related matches ---'
rg -n "project_dir|resolve\(|is_dir\(|symlink|CLAUDE\.md|archive|workspace_root" "$file"Repository: openshift-eng/edge-tooling
Length of output: 5281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='plugins/workspace/scripts/consolidate-project.py'
python3 - <<'PY'
from pathlib import Path
p = Path("plugins/workspace/scripts/consolidate-project.py")
lines = p.read_text().splitlines()
for i in range(330, 371):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: openshift-eng/edge-tooling
Length of output: 1477
Restrict project_name to a child of root / "projects".
root / "projects" / args[0] accepts absolute paths and .. segments, and a symlinked project entry can still resolve outside the workspace. Resolve the candidate, ensure it stays under root / "projects" before any is_dir()/file access, and reject anything else. Per CONTRIBUTING.md: trust-boundary paths must be canonicalized and kept within the allowed root; traversal must be rejected.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 348-352: use jsonify instead of json.dumps for JSON output
Context: json.dumps({
"status": "error",
"error": "Could not determine the workspace root. Set WORKSPACE_ROOT "
"or run inside a workspace (a directory containing dev-env.yaml).",
})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/workspace/scripts/consolidate-project.py` around lines 346 - 355,
Canonicalize the project directory derived in the argument-handling flow around
project_name and root, then verify the resolved candidate remains within the
resolved root / "projects" directory before any is_dir() or file access. Reject
absolute paths, traversal, and symlinked entries that resolve outside the
allowed directory, preserving the existing error-and-exit behavior for invalid
projects.
Source: Path instructions
There was a problem hiding this comment.
Won't fix — the script is invoked by Claude Code skills, not external users. Input comes from the LLM agent, not a trust boundary.
There was a problem hiding this comment.
@fonta-rh, acknowledged. Given the script’s intended skill-only invocation model, I’ll treat this as an accepted risk for this PR. If it is later exposed to user-controlled arguments or reused outside Claude Code, path containment should be added then.
✏️ Learnings added
Learnt from: fonta-rh
Repo: openshift-eng/edge-tooling PR: 249
File: plugins/workspace/scripts/consolidate-project.py:346-355
Timestamp: 2026-07-30T16:29:08.535Z
Learning: `plugins/workspace/scripts/consolidate-project.py` is intended to be invoked only through Claude Code workspace skills; maintainers currently do not treat the LLM-provided project-name argument as an external trust boundary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the fixes, Pablo — all 7 original findings are resolved cleanly in 4c9123f.
Second pass — two inline comments on remaining issues, plus a few lower-priority suggestions:
Lower priority
-
resume-project.py— removed bounds check: The oldif not names: return "no_projects"guard was removed. Numeric arg on empty workspace now shows confusing "out of range (1-0)" instead of the friendlier "No recent projects found." message. -
parse_frontmatter()duplicated: 6 copies acrossdomain-info.py,resume-project.py,skills.pywith subtle behavioral differences (e.g.skills.pycatchesUnicodeDecodeErrorbut doesn't coerce datetime; others do the opposite).workspace_lib.pyalready exists as a shared module. -
Non-atomic write in
consolidate-project.py:314:write_text()can truncate the project CLAUDE.md on crash. Low risk since it's a dev tool, but the file being modified is the project's primary context file.tempfile+os.replace()would make it atomic. -
No tests for
consolidate-project.pyandresume-project.py(3 of 5 scripts are covered).
Shellcheck: - SC2086: quote $status in printf (show_status) - SC2034: rename unused summary to _summary (list_repos) - SC2010: replace ls|grep with glob loop (fetch_external_domain) Markdownlint: - Add fenced-code-language to code blocks in CLAUDE.md and README.md - Ignore domains/ and skills/ in markdownlint config (pre-existing upstream content, not prose docs) Review findings (Luca, second pass): - merge_settings_template: check Python exit code to avoid reporting false success on crash - parse_yaml_domain: error when no YAML parser available instead of silently returning empty fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Re: second-pass lower-priority items (3–6):
These are captured in |
Add medium-priority items from Luca's second review pass: resume-project bounds check, non-atomic write, parse_frontmatter dedup, and missing test coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
coderabbitai
Bot
removed
the
ready-for-human-review
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.markdownlint-cli2.jsonc:
- Around line 16-17: Remove the broad plugins/workspace/domains/** and
plugins/workspace/skills/** exclusions from the markdownlint configuration, then
narrow them only to documented exceptions if needed. Ensure all other workspace
domain and skill Markdown files remain subject to markdownlint, fixing any
resulting violations rather than disabling linting for entire directories.
🪄 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), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5a73a02d-7e9f-4d76-afdb-4d7ded9e022a
📒 Files selected for processing (5)
.markdownlint-cli2.jsoncplugins/workspace/CLAUDE.mdplugins/workspace/README.mdplugins/workspace/TODO.mdplugins/workspace/scripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/workspace/TODO.md
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
LGTM — nice work on the migration, Pablo. The fix commit addressed all critical findings cleanly. Two remaining inline suggestions are minor hardening, not blockers.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh, lucaconsalvi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Addressed CodeRabbit review-body nitpick findings:
|
2 participants
Summary
plugins/workspace/, replacing the oldmulti-repo-development/folderTest plan
./marketplace validate workspacepasses./marketplace catalog-updatepicks up workspace (18 plugins)grep -r 'multi-repo-development' .returns no hits outside git historynpx markdownlint-cli2 README.md CLAUDE.mdpasses🤖 Generated with Claude Code
Summary by CodeRabbit