WIP: Add node-team plugin#457
Draft
harche wants to merge 2 commits into
Draft
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harche The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
WalkthroughIntroduces a new "node-team" Claude plugin registered in the marketplace with a skill definition and comprehensive reference documentation covering repository setup, binary deployment procedures, development guidance for kubelet/MCO/CRI-O/crun/conmon-rs/Kueue, Jira workflows, Red Hat Support APIs, Kubernetes/OpenShift platform documentation, and Prometheus query patterns. ChangesNode Team Plugin & Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
plugins/node-team/skills/node/references/deployment/debug-binary.md-93-93 (1)
93-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken link:
layered-image.mdreference.Line 93 also references
[layered images](layered-image.md)which is not present in this PR.🤖 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/node-team/skills/node/references/deployment/debug-binary.md` at line 93, The markdown in debug-binary.md references a missing doc by linking `[layered images](layered-image.md)` causing a broken link; fix it by either adding the referenced file `layered-image.md` with the intended content, or update the link target in the sentence starting "Use bind mounts for quick single-node testing..." to point to the correct existing document or remove the link text "layered images" if no doc exists, ensuring the link target matches a committed file name.plugins/node-team/skills/node/references/deployment/debug-binary/cross-compile.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBroken link:
layered-image.mdnot included in this PR.Line 17 references
[layered images](layered-image.md)but this file is not present in the changed files. Users clicking this link will get a 404.Possible fixes
- If
layered-image.mdwill be added in a future PR, add a TODO comment here- If it's in a different directory, fix the relative path
- If it's not planned, remove the reference
🤖 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/node-team/skills/node/references/deployment/debug-binary/cross-compile.md` at line 17, The markdown contains a broken reference to [layered images](layered-image.md) (appears next to the "Check `go.mod` in the source directory:" line); fix it by either adding the missing layered-image.md content in this PR, correcting the relative path to the actual file location, or removing the link and/or adding a TODO comment that explains the file will be added in a subsequent PR—update the "[layered images](layered-image.md)" link accordingly so it no longer 404s.plugins/node-team/skills/node/references/deployment/debug-binary/cross-compile.md-79-87 (1)
79-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify: conmon-rs is Rust, not Go.
Line 81 states "For Go binaries (CRI-O, conmon-rs components)" but
conmon-rsis a Rust binary (as documented in lines 89-117 below). This is confusing.🔧 Suggested fix
-For Go binaries (CRI-O, conmon-rs components): +For Go binaries (CRI-O):🤖 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/node-team/skills/node/references/deployment/debug-binary/cross-compile.md` around lines 79 - 87, The heading and sentence wrongly group "conmon-rs" with Go binaries; update the text so GOOS/GOARCH/CGO_ENABLED applies only to Go binaries (e.g., CRI-O) and explicitly call out that conmon-rs is a Rust binary — either remove it from the Go list or add a separate note/section for conmon-rs explaining its Rust build/cross-compile requirements; reference the phrase "For Go binaries (CRI-O, conmon-rs components)" and the token "conmon-rs" when making this change.plugins/node-team/skills/node/references/platform-docs.md-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the official OpenShift documentation repository instead of a personal fork.
Line 15 references
harche/openshift-docs-md, a personal repository. Official OpenShift documentation is maintained inopenshift/openshift-docs. Update the reference to use the official repository to ensure reliability, proper versioning, and long-term maintenance.🤖 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/node-team/skills/node/references/platform-docs.md` at line 15, Update the repository reference in plugins/node-team/skills/node/references/platform-docs.md by replacing the personal fork string "harche/openshift-docs-md" with the official repository "openshift/openshift-docs" (the line that currently reads `Repo: 'harche/openshift-docs-md', path: 'docs/{version}/'`). Ensure the path and version placeholder remain unchanged and commit the updated reference so downstream links point to the official OpenShift docs repository.plugins/node-team/scripts/worktree.sh-142-149 (1)
142-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
if [ $? -ne 0 ]aftersubmodule foreachis unreachable underset -euo pipefail.When
git submodule foreach '… exit 1'returns non-zero,set -eaborts the script before reaching the[ $? -ne 0 ]block, so the user never sees the helpful "Resolve them, then run …" message. Either run the foreach with explicit error handling, or check exit status inline usingif ! git -C "$ROOT" submodule foreach …; then.🛠️ Suggested pattern (applies to both pull and merge)
- git -C "$ROOT" submodule foreach --quiet ' - … - ' - - if [ $? -ne 0 ]; then + if ! git -C "$ROOT" submodule foreach --quiet ' + … + '; then echo "" echo "Pull stopped due to conflicts. Resolve them, then run:" … exit 1 fiAlso applies to: 249-256
🤖 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/node-team/scripts/worktree.sh` around lines 142 - 149, The post-submodule check using "if [ $? -ne 0 ]" is unreachable under "set -euo pipefail"; change the submodule invocation to perform explicit error handling, e.g. run "git -C \"$ROOT\" submodule foreach '...'" inside an if-not construct ("if ! git -C \"$ROOT\" submodule foreach ...; then") or capture the command exit status with "|| rc=$?; if [ "${rc:-0}" -ne 0 ]; then" so the helpful message and "exit 1" in worktree.sh (the pull and merge submodule blocks that currently use "git submodule foreach '… exit 1'" and then "if [ $? -ne 0 ]") are executed when submodule foreach fails.plugins/node-team/scripts/worktree_test.sh-33-50 (1)
33-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin the initial branch to
mainfor portability.
git init --quiethonorsinit.defaultBranchfrom the user's config — if that's set tomaster/trunk(or unset on older git), the seed repos and workspace get a non-maindefault whileworktree.shand assertions hardcodemain. Tests then fail for environment-specific reasons. Pass--initial-branch=mainexplicitly.🛠️ Suggested change
- git init --bare "${TEST_DIR}/remotes/${repo}.git" --quiet + git init --bare --initial-branch=main "${TEST_DIR}/remotes/${repo}.git" --quiet … - git -C "${seed}" init --quiet + git -C "${seed}" init --initial-branch=main --quiet … - git -C "${ws}" init --quiet + git -C "${ws}" init --initial-branch=main --quiet🤖 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/node-team/scripts/worktree_test.sh` around lines 33 - 50, The tests assume the initial branch is "main" but git init can use a different default; update the git init invocations to explicitly set the initial branch to main by adding --initial-branch=main to the bare repo creation and the non-bare inits (the git init --bare "${TEST_DIR}/remotes/${repo}.git" and git -C "${seed}" init and git -C "${ws}" init) so seed repos and workspace always use main regardless of the user's init.defaultBranch.plugins/node-team/scripts/lib/composite/team-activity.sh-60-64 (1)
60-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't detect comment-fetch failures with a raw
"error"substring check.This drops the entire comment set for an issue whenever any valid comment body contains the word
error, which is pretty likely for debugging tickets. Parse the response shape/status instead of substring-matching the JSON payload.🤖 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/node-team/scripts/lib/composite/team-activity.sh` around lines 60 - 64, The current check uses a substring match on variable c (from parallel_get) which wrongly rejects valid comments containing the word "error"; instead parse the JSON response in c and inspect its status/error fields (e.g. use jq or shell JSON parsing) to determine whether the fetch failed. Update the block that reads c (variable c, function parallel_get, and the assembly into comments_combined) to first validate that c is valid JSON and that it does not contain an error indicator (e.g. no .error field and .status != "error" or an explicit success flag) before appending; only treat the fetch as failed when the parsed shape indicates an error, not when the raw string contains the substring "error".plugins/node-team/skills/node/references/development/worktrees.md-18-27 (1)
18-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to this fenced example.
This block is missing a fence language, which will keep
markdownlintcomplaining with MD040. Mark it asbashso the docs lint stays clean.Suggested fix
-``` +```bash ./scripts/worktree.sh create my-feature cd .worktrees/my-feature/ # work across repos... # optionally pull in upstream changes: ./scripts/worktree.sh pull my-feature # done — merge back and clean up: ./scripts/worktree.sh merge my-feature ./scripts/worktree.sh remove my-feature</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/development/worktrees.mdaround
lines 18 - 27, The fenced code block containing usage of ./scripts/worktree.sh
(create/pull/merge/remove) is missing a language tag which triggers markdownlint
MD040; update the triple-backtick fence that surrounds the commands in the
worktrees.md example to specify bash (```bash) so the block is treated as shell
script and the linter stops complaining.</details> </blockquote></details> <details> <summary>plugins/node-team/scripts/jira.sh-33-78 (1)</summary><blockquote> `33-78`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Help text is missing 5 composite commands.** The help documentation (lines 59-67) doesn't include `my-board-data`, `my-bugs-data`, `my-standup-data`, `epic-progress`, or `pickup-data`, but these commands are implemented and routed in the dispatcher (lines 114-118). Users won't discover these commands through `jira.sh --help`. <details> <summary>📝 Suggested addition to help text</summary> Add these commands to the "High-level composite commands" section: ```diff High-level composite commands: sprint-dashboard <team> Sprint info + issues by status + workload + blockers standup-data <team> Dashboard + recent updates + new bugs + comments bug-overview <team> Bug triage data (untriaged, unassigned, blockers, new) carryover-report <team> Not-done items with carryover context planning-data <team> Full planning package (carryovers + backlog + bugs) issue-deep-dive <KEY> Full issue + comments (ADF converted) + linked issues release-data <team> [version] Release readiness (blockers, bugs, epics) team-activity <team> Per-member sprint items + comment counts roster-sync [--force] Download team rosters from Jira attachments + my-board-data <team> Personal sprint dashboard (items assigned to you) + my-bugs-data <team> Your assigned bugs with escalation/blocker categorization + my-standup-data <team> Your standup data (done/in-progress/blocked/comments) + epic-progress <team> Epics you're contributing to with progress metrics + pickup-data <team> Unassigned sprint items and bugs available for pickup ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/jira.sh` around lines 33 - 78, The cmd_help() function's help text is missing five implemented composite commands; update the "High-level composite commands" section inside cmd_help to list my-board-data, my-bugs-data, my-standup-data, epic-progress, and pickup-data so users see them in jira.sh --help; locate the cmd_help() definition and add those command names (with brief one-line descriptions consistent with the surrounding entries) to the block printed by the heredoc. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/bug-overview.sh-32-47 (1)</summary><blockquote> `32-47`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Stale comment: only 3 queries are dispatched.** The banner says "4 queries: all_open + new_this_week + team_no_component + escalation-labeled", but the `escalation-labeled` query is not actually run. Update or remove the comment to avoid misleading readers. <details> <summary>📝 Proposed fix</summary> ```diff - # ── 4 queries: all_open + new_this_week + team_no_component + escalation-labeled + # ── 3 queries: all_open + new_this_week + team_no_component ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/bug-overview.sh` around lines 32 - 47, The comment/header is stale: it claims "4 queries: all_open + new_this_week + team_no_component + escalation-labeled" but only three parallel_run calls (all_open, new_this_week, team_no_component) are executed; either add the missing escalation-labeled parallel_run (using parallel_run "escalation-labeled" cmd_search "<JIRA JQL...>" ...) or update/remove the banner to accurately list the three dispatched queries; change the comment near parallel_init/parallel_run (references: parallel_init, parallel_run calls for "all_open", "new_this_week", "team_no_component", and parallel_wait_all) so it no longer mentions escalation-labeled unless you add the corresponding query invocation. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/api/issue.sh-134-146 (1)</summary><blockquote> `134-146`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fragile HTTP code parsing and broken empty-body fallback.** Two issues here: 1. `grep "HTTP_"` to extract the status code (and `grep -v "HTTP_"` for the body) breaks if the response body legitimately contains the literal `HTTP_`. Since the sentinel is appended on its own line via `\n`, prefer `tail -n1` / `sed '$d'` or use `--write-out` to a separate file/var. 2. `${body:-\"{}\"}` substitutes the seven-character string `"{}"` when the body is empty, producing `"response":"{}"` — a JSON *string*, not the empty *object* the surrounding shape suggests. Use `${body:-{}}` (no embedded quotes) so consumers can `.response.<field>` reliably. <details> <summary>🐛 Proposed fix</summary> ```diff local result result=$(_curl -X PUT "${JIRA_BASE}/rest/api/3/issue/${key}/assignee" \ -d "{\"accountId\": \"${account_id}\"}" -w "\nHTTP_%{http_code}") - local code - code=$(echo "$result" | grep "HTTP_" | sed 's/HTTP_//') + local code body + code=$(printf '%s' "$result" | tail -n1 | sed 's/^HTTP_//') + body=$(printf '%s' "$result" | sed '$d') if [[ "$code" == "204" ]]; then echo "{\"key\":\"${key}\",\"assignee\":\"${assignee}\",\"accountId\":\"${account_id}\",\"status\":\"ok\"}" else - local body - body=$(echo "$result" | grep -v "HTTP_") - echo "{\"key\":\"${key}\",\"status\":\"error\",\"code\":\"${code}\",\"response\":${body:-\"{}\"}}" >&2 + echo "{\"key\":\"${key}\",\"status\":\"error\",\"code\":\"${code}\",\"response\":${body:-{}}}" >&2 return 1 fi ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/issue.sh` around lines 134 - 146, The current HTTP parsing uses grep "HTTP_" which fails if the body contains that literal and the empty-body fallback produces a JSON string instead of an object; update the logic that reads the _curl output (variable result) to extract the status code with tail -n1 (or similar) and compute the body by removing the last line (e.g., sed '$d') instead of grep, and change the empty-body fallback from the quoted form to an actual empty object (use ${body:-{}}) so "response" is an object, not a JSON string; adjust variables result, code, and body accordingly where they are used. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/standup-data.sh-68-83 (1)</summary><blockquote> `68-83`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Fragile substring check drops any comment containing the literal word "error".** `[[ -n "$c" && "$c" != *"error"* ]]` is too broad — a comment whose body legitimately contains the word "error" (a common case on a triage/debug-oriented team) will cause the entire issue's comment payload to be silently dropped from `comments_combined`. Concatenating raw responses into a hand-built JSON string is also brittle. Prefer a structural check (e.g., `jq -e 'has("comments")'` or a small Python step) and, ideally, build `comments_combined` with `jq -n` / Python rather than string concatenation so a single malformed response doesn't corrupt the merged JSON. <details> <summary>🛡️ Sketch of a more robust merge</summary> ```diff - local comments_combined="{" - local first=true - for key in $issue_keys; do - local c - c=$(parallel_get "comments_${key}" 2>/dev/null) - if [[ -n "$c" && "$c" != *"error"* ]]; then - if [[ "$first" == "true" ]]; then - first=false - else - comments_combined+="," - fi - comments_combined+="\"${key}\":${c}" - fi - done - comments_combined+="}" + local comments_combined + comments_combined=$( + for key in $issue_keys; do + local c + c=$(parallel_get "comments_${key}" 2>/dev/null) + [[ -z "$c" ]] && continue + printf '%s\0%s\0' "$key" "$c" + done | python3 -c ' +import json, sys +raw = sys.stdin.buffer.read().split(b"\0") +out = {} +for i in range(0, len(raw) - 1, 2): + key = raw[i].decode() + try: + body = json.loads(raw[i+1]) + except json.JSONDecodeError: + continue + if isinstance(body, dict) and "comments" in body: + out[key] = body +print(json.dumps(out)) +' + ) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/standup-data.sh` around lines 68 - 83, The current loop that builds comments_combined uses a fragile substring test ([[ -n "$c" && "$c" != *"error"* ]]) and manual string concatenation, which drops any payload containing the literal "error" and risks producing invalid JSON; replace that logic around parallel_get "comments_${key}" by validating each response structurally (e.g., use jq -e 'has("comments")' on the variable c) and then merge entries using jq -n/--argjson (or a short Python snippet) to construct the combined JSON object instead of hand-concatenating into comments_combined, keeping references to the loop (for key in $issue_keys), parallel_get, and the comments_combined variable so you locate and update the exact block. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (22)</summary><blockquote> <details> <summary>plugins/node-team/skills/node/references/deployment/debug-binary/deploy.md (1)</summary><blockquote> `58-61`: _💤 Low value_ **Add language specifier to code block.** The code block showing the SELinux error message should have a language specifier for proper syntax highlighting. As per static analysis, fenced code blocks should specify a language. <details> <summary>Suggested fix</summary> ```diff -``` +```text Failed to locate executable <path>: Permission denied ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/deployment/debug-binary/deploy.md
around lines 58 - 61, The fenced code block that displays the SELinux/systemd
error message (the block containing "Failed to locate executable :
Permission denied") lacks a language specifier; update that fenced code block in
deploy.md to include a language tag (e.g., text) so the block becomestext ...for proper syntax highlighting and to satisfy the static analysis rule.</details> </blockquote></details> <details> <summary>plugins/node-team/skills/node/references/deployment/debug-binary.md (2)</summary><blockquote> `13-15`: _💤 Low value_ **Add language specifier to code block.** The bind mount example should have a language specifier. As per static analysis, fenced code blocks should specify a language. <details> <summary>Suggested fix</summary> ```diff -``` +```bash mount --bind /home/core/crio /usr/bin/crio ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/deployment/debug-binary.mdaround
lines 13 - 15, Add a language specifier to the fenced code block containing the
mount command so the example uses proper syntax highlighting; locate the
markdown block that contains the text "mount --bind /home/core/crio
/usr/bin/crio" and change the opening fence fromtobash (i.e., add
"bash" after the triple backticks).</details> --- `97-113`: _💤 Low value_ **Add language specifier to workflow diagram.** The ASCII workflow diagram should have a language specifier for proper rendering. As per static analysis, fenced code blocks should specify a language. <details> <summary>Suggested fix</summary> ```diff -``` +```text Local Machine RHCOS Worker Node ───────────── ───────────────── ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/deployment/debug-binary.mdaround
lines 97 - 113, The fenced ASCII workflow diagram in debug-binary.md is missing
a language specifier; update the opening fence for the diagram block (the triple
backticks that start the Local Machine / RHCOS Worker Node ASCII art) to include
a language like "text" (e.g., changetotext) so the diagram renders
correctly as plain text.</details> </blockquote></details> <details> <summary>plugins/node-team/skills/node/references/deployment/debug-binary/crio.md (2)</summary><blockquote> `34-40`: _💤 Low value_ **Add language specifier to code block.** The shared library list should have a language specifier for consistency and proper rendering. As per static analysis, fenced code blocks should specify a language. <details> <summary>Suggested fix</summary> ```diff -``` +```text libseccomp.so.2 libgpgme.so.11 libassuan.so.0 libgpg-error.so.0 libc.so.6 ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/deployment/debug-binary/crio.md
around lines 34 - 40, The fenced code block containing the shared library list
(lines with libseccomp.so.2, libgpgme.so.11, libassuan.so.0, libgpg-error.so.0,
libc.so.6) should include a language specifier; update the opening fence from
totext (or another appropriate language) so the block is rendered
consistently and satisfies the static analysis rule about fenced code blocks
specifying a language.</details> --- `50-56`: _💤 Low value_ **Add language specifier to code block.** The build tags list should have a language specifier for consistency. As per static analysis, fenced code blocks should specify a language. <details> <summary>Suggested fix</summary> ```diff -``` +```text containers_image_ostree_stub exclude_graphdriver_btrfs btrfs_noversion seccomp selinux ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/node-team/skills/node/references/deployment/debug-binary/crio.md
around lines 50 - 56, The fenced code block containing the build tags (the list:
containers_image_ostree_stub, exclude_graphdriver_btrfs, btrfs_noversion,
seccomp, selinux) needs a language specifier; change the opening fence fromtotext so the block reads with a language tag (e.g., ```text) to satisfy the
static analysis requirement for fenced code blocks.</details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/core.sh (1)</summary><blockquote> `67-74`: _⚡ Quick win_ **Consider using an array for AUTH to improve robustness.** The current pattern stores curl arguments in a string variable and expands it unquoted, which works for typical API tokens and email addresses but could break if these values ever contain spaces or special characters. <details> <summary>♻️ More robust pattern using Bash array</summary> ```diff - AUTH="-u ${JIRA_USER}:${JIRA_API_TOKEN}" + AUTH=(-u "${JIRA_USER}:${JIRA_API_TOKEN}") ``` ```diff _curl() { _init_auth - curl -s $AUTH -H "Content-Type: application/json" "$@" + curl -s "${AUTH[@]}" -H "Content-Type: application/json" "$@" } ``` This eliminates word-splitting risk and satisfies Shellcheck SC2086. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/core.sh` around lines 67 - 74, AUTH is currently a string expanded unquoted in _curl which risks word-splitting; change AUTH to a bash array (e.g., AUTH=( -u "${JIRA_USER}:${JIRA_API_TOKEN}" )) in _init_auth and update _curl to use the array expansion ("${AUTH[@]}") when calling curl. Locate the _init_auth function that sets AUTH and the _curl function and replace the string assignment and unquoted expansion with the array form to avoid SC2086-style issues. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/util/cache.sh (1)</summary><blockquote> `43-63`: _⚡ Quick win_ **Use `printf` to round-trip arbitrary JSON.** `echo "$value"` is bash-builtin-dependent (xpg_echo, leading `-e/-n/-E`, embedded backslashes in JSON strings) and command substitution will already strip the trailing newline on read. Persist with `printf '%s' "$value"` and emit cached content the same way to guarantee byte-for-byte fidelity. <details> <summary>♻️ Suggested change</summary> ```diff cache_set() { _cache_init local key="$1" local value="$2" - echo "$value" > "${_CACHE_DIR}/${key}" + printf '%s' "$value" > "${_CACHE_DIR}/${key}" } # Cache-through wrapper for sprint discovery cached_sprints() { local state="${1:-active}" local cache_key="sprints_${state}" local cached if cached=$(cache_get "$cache_key" 2>/dev/null); then - echo "$cached" + printf '%s\n' "$cached" return 0 fi local result result=$(cmd_sprints "$state") cache_set "$cache_key" "$result" - echo "$result" + printf '%s\n' "$result" } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/util/cache.sh` around lines 43 - 63, The cache write/read uses echo which can mangle arbitrary JSON; in cache_set replace echo "$value" with printf '%s' "$value" so bytes are preserved, and in cached_sprints replace both echo "$cached" and echo "$result" with printf '%s' "$cached" and printf '%s' "$result" respectively; keep the existing use of cache_get and cache_set and no other logic changes. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/api/sprint.sh (1)</summary><blockquote> `8-24`: _⚡ Quick win_ **Embedded filter strings and 50-result cap will bite for closed sprints.** `maxResults=50` with no pagination silently truncates when callers pass `state=closed` (boards routinely accumulate hundreds of historic sprints) — the "Node/Kueue" filter would then drop matches that exist past the first page. The substring filter itself is also baked into the API helper, which makes the module non-reusable for any team beyond the two named here. Lift the filter to a constant (or env var like `JIRA_SPRINT_FILTER`) and either page until exhausted or expose the limit as a parameter. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/sprint.sh` around lines 8 - 24, The cmd_sprints function currently hardcodes a 50-result page and an embedded "Node/Kueue" substring filter causing truncated or non-reusable results; change it to accept/observe a configurable filter variable (e.g., JIRA_SPRINT_FILTER env var or module-level constant) instead of hardcoding "Node" and "Kueue", and replace the single-request call with paginated fetching (looping on startAt/maxResults until all sprints are retrieved) or expose the maxResults/startAt as parameters so callers can control pagination; update the embedded python filter invocation to read the filter from that variable rather than hardcoded strings and ensure cmd_sprints accepts an optional limit parameter when full pagination is not desired. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/issue-deep-dive.sh (1)</summary><blockquote> `13-33`: _⚡ Quick win_ **Swallowed parallel failures + argv-passed JSON can crash on big issues / macOS.** `parallel_wait_all || true` discards any fetch error, so a failed `cmd_get` produces empty/non-JSON content that immediately raises a `json.JSONDecodeError` deeper in the Python block — surfacing as a confusing traceback rather than a structured error. Separately, passing 4 JSON blobs (`issue`, `comments`, `transitions`, plus `adf_py`) as `argv` will hit `ARG_MAX` for issues with long comment threads on macOS (256 KB) and can stress Linux. Pipe the JSON via stdin and validate before parsing. <details> <summary>♻️ Sketch (pipe blobs via stdin)</summary> ```bash python3 - "$adf_py" <<PYEOF <<<"$(jq -n --argjson i "$issue_json" --argjson c "$comments_json" --argjson t "$transitions_json" '{issue:$i,comments:$c,transitions:$t}')" import json, sys, importlib.util adf_py_path = sys.argv[1] data = json.load(sys.stdin) issue, comments_data, transitions_data = data["issue"], data["comments"], data["transitions"] # … rest unchanged PYEOF ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/issue-deep-dive.sh` around lines 13 - 33, The script currently swallows fetch failures with "parallel_wait_all || true" and passes large JSON blobs as argv to the python3 call (risking ARG_MAX); change to fail fast by removing the "|| true" and check the return value of parallel_wait_all and, if non-zero, log and exit non‑zero so cmd_get/cmd_comments/cmd_transitions errors are visible; validate that parallel_get("issue"/"comments"/"transitions") returned non-empty/valid JSON (use jq or a quick test) before invoking Python; and change the python3 invocation to take only adf_py as argv and stream the combined JSON (issue/comments/transitions) to python's stdin, then update the Python block to read json.load(sys.stdin) and pull “issue”, “comments”, “transitions” from that object. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/api/fields.sh (2)</summary><blockquote> `30-42`: _⚡ Quick win_ **Error envelope can itself be invalid JSON.** If Jira returns a non-JSON body (HTML 502, plain-text 401 page from a proxy, etc.), `${body:-\"{}\"}` is interpolated unescaped into the `"response"` slot — the emitted "error JSON" is then itself unparseable, breaking any caller that tries to `jq` the stderr. Building this envelope through `python3 -c "import json,...; print(json.dumps({...}))"` (or `jq -n`) keeps the contract intact. Same pattern in `cmd_link` (lines 64–76). <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/fields.sh` around lines 30 - 42, The error envelope writes unescaped `${body}` into the "response" field which can break the emitted JSON if Jira returned non-JSON; update the error path in this function (the block using `result`, `code`, `body`, and variables `key`/`field`) to construct the envelope via a JSON serializer (e.g., call out to `python3 -c` or `jq -n`) so the `response` becomes a proper JSON string or object (use raw body as a string when it isn’t valid JSON), emit that serialized JSON to stderr and still `return 1`; apply the same fix to the similar `cmd_link` error block at lines referenced (around 64–76). ``` </details> --- `8-13`: _⚡ Quick win_ **Validate `points` is numeric before interpolating into JSON.** `${points}` is dropped raw into the request body, so any non-numeric value (e.g., `""`, `"abc"`, `"1; whoops"`) produces an invalid JSON payload that Jira rejects with an opaque 400. A lightweight regex check (or routing through `cmd_set_field`, which already JSON-parses values) would catch this at the source. <details> <summary>♻️ Suggested change</summary> ```diff cmd_set_points() { local key="$1" local points="$2" + if ! [[ "$points" =~ ^-?[0-9]+(\.[0-9]+)?$ ]]; then + echo "{\"key\":\"${key}\",\"status\":\"error\",\"reason\":\"non-numeric points: ${points}\"}" >&2 + return 1 + fi _curl -X PUT "${JIRA_BASE}/rest/api/3/issue/${key}" \ -d "{\"fields\": {\"${CF_STORY_POINTS}\": ${points}}}" } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/fields.sh` around lines 8 - 13, The cmd_set_points function inserts ${points} raw into JSON which breaks if non-numeric; add validation at the top of cmd_set_points to ensure points matches a numeric regex (e.g., integer or decimal as your convention) and return a clear error if not numeric, or alternatively call the existing cmd_set_field helper that already JSON-parses values instead of interpolating directly; keep the JSON payload numeric (no quotes) when valid. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/api/comment.sh (1)</summary><blockquote> `31-39`: _⚡ Quick win_ **Anchor the HTTP status parsing and avoid `2>&1` contaminating the captured response.** `grep "HTTP_"` is unanchored and will match anywhere in the body if a Jira response or curl stderr happens to contain "HTTP_". Combined with `2>&1`, transient curl diagnostics get folded into `$result` and parsed alongside the body. With `-w "\nHTTP_%{http_code}"`, the marker is always the last line — match it that way and keep stderr separate so a real failure surfaces a usable response body. This same pattern is duplicated in `transition.sh` and `fields.sh` (and would benefit from a small helper in `core.sh`). <details> <summary>♻️ Suggested change</summary> ```diff - result=$(_curl -X POST "${JIRA_BASE}/rest/api/3/issue/${key}/comment" -d "$payload" -w "\nHTTP_%{http_code}" 2>&1) - local code - code=$(echo "$result" | grep "HTTP_" | sed 's/HTTP_//') + result=$(_curl -X POST "${JIRA_BASE}/rest/api/3/issue/${key}/comment" -d "$payload" -w "\nHTTP_%{http_code}") + local code + code=$(printf '%s\n' "$result" | tail -n1 | sed -n 's/^HTTP_//p') ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/comment.sh` around lines 31 - 39, The HTTP status parsing can be contaminated by stderr and by unanchored grep; instead, stop merging stderr into the body and anchor the marker lookup to the last line. Call _curl without 2>&1, use -w "\nHTTP_%{http_code}" and capture its output, then extract the last line (e.g., via tail -n1) and match '^HTTP_' to get the code, leaving the rest as the body; update the logic in comment.sh (and the duplicated patterns in transition.sh and fields.sh) and consider factoring into a helper in core.sh that returns the body and status code separately so parsing uses the anchored '^HTTP_' marker and stderr is preserved. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/worktree.sh (1)</summary><blockquote> `79-86`: _⚡ Quick win_ **Inject vars into `submodule foreach` via env, not single-quote-then-double-quote concat.** The `'"$name"'` / `'"$ws"'` pattern is fragile — any character that needs quoting in `$name` (space, `$`, `'`, `"`) breaks the embedded shell with no error and silently constructs the wrong git command. Bash exposes a clean alternative: prefix the foreach with the variables. ```bash name="$name" ws="$ws" branch="wt/$name" \ git -C "$ROOT" submodule foreach --quiet ' [ -d "$ws/$sm_path" ] || { echo " $sm_path: not in workspace"; exit 0; } cur=$(git -C "$ws/$sm_path" branch --show-current) … ' ``` This also clears the SC2016 noise from shellcheck for the rest of the file. Also applies to: 112-142, 202-249, 308-316, 341-347 <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/worktree.sh` around lines 79 - 86, The embedded single-quote-then-double-quote injection of variables into the git submodule foreach string (using '"$name"'/'"$ws"' and building branch="wt/'"$name"') is fragile and should be replaced by exporting/prefixing the foreach with the variables so the inner single-quoted script can use $name, $ws, $branch safely; update each foreach invocation to pass name="$name" ws="$ws" branch="wt/$name" before git -C "$ROOT" submodule foreach --quiet '...', and remove the concatenated quoting inside the single-quoted block (e.g., use $ws/$sm_path, $sm_path, etc. directly), applying the same change to the other foreach blocks flagged in the comment. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/api/transition.sh (1)</summary><blockquote> `44-49`: _💤 Low value_ **Implicit dependency on `comment.sh`.** `cmd_close` calls `cmd_comment` but this module's load guard only covers itself. If `jira.sh` ever changes source order or someone sources `transition.sh` standalone, this breaks at runtime. Consider sourcing `comment.sh` here defensively (the `_API_COMMENT_LOADED` guard makes it idempotent), or guard the call. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/api/transition.sh` around lines 44 - 49, The loop in transition.sh calls cmd_comment (and cmd_transition) but doesn't ensure comment.sh is loaded, creating a runtime dependency; fix by defensively sourcing comment.sh at the top of this module using the same idempotent load guard (_API_COMMENT_LOADED) before any usage of cmd_comment (or alternatively check for the existence of the cmd_comment function before calling and only call if present); update references in this file (where cmd_comment is invoked in the for loop and any cmd_close handlers) to rely on the guarded source so the call never fails if transition.sh is sourced standalone or order changes. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/skills/node/references/SETUP.md (1)</summary><blockquote> `14-21`: _⚡ Quick win_ **Prefer the wrapper script here, not raw `git worktree`.** The companion worktree docs make `scripts/worktree.sh` the supported path because it also handles sync/submodule setup. These manual commands only create the top-level worktree, so the setup guide now diverges from the workflow you're documenting elsewhere. The same drift applies to the PR/Jira examples below. <details> <summary>Suggested fix</summary> ```diff ## Worktree for Feature Work Never work directly on the default branch. Create a worktree: ```bash -git worktree add .worktrees/<name> -b wt/<name> +./scripts/worktree.sh create <name> cd .worktrees/<name> ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/skills/node/references/SETUP.md` around lines 14 - 21, Replace the raw git worktree command with the repository wrapper and update related examples: change the manual "git worktree add .worktrees/<name> -b wt/<name>" usage to use the wrapper "./scripts/worktree.sh create <name>" and adjust the subsequent "cd .worktrees/<name>" guidance as needed; also scan the PR/Jira example sections and replace any other direct git worktree instructions with the corresponding scripts/worktree.sh commands so the doc aligns with the supported workflow. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/pickup-data.sh (1)</summary><blockquote> `59-61`: _⚡ Quick win_ **Consider externalizing bot account names to configuration.** The bot account name is hardcoded in the Python block (`BOT_ACCOUNTS = {"Node Team Bot Account"}`). If the bot account name changes or additional bot accounts are introduced, this code will require modification. Consider sourcing bot account names from the team configuration (via `team_config`) or an environment variable to improve maintainability. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/pickup-data.sh` around lines 59 - 61, The BOT_ACCOUNTS set is hardcoded; change the code that builds BOT_ACCOUNTS and the unassigned_sprint filter to load bot account names from configuration (e.g., team_config["bot_accounts"] or an environment variable), normalizing into a set before use; update references to BOT_ACCOUNTS, unassigned_sprint, and the extract(issues_data) filter so the check uses the configured set with a sensible default fallback (empty set) if no config is provided. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/my-standup-data.sh (2)</summary><blockquote> `54-64`: _⚡ Quick win_ **JSON construction via string concatenation is fragile.** Building the `comments_combined` JSON object through string concatenation (lines 54-64) can produce malformed JSON if any `parallel_get` result contains invalid JSON. The validation on line 59 only checks for non-empty values and the absence of the substring "error", but doesn't validate JSON structure. Consider using `jq` to construct the JSON object or add stricter validation of the comment data before concatenation. <details> <summary>Alternative approach using jq</summary> ```bash # Build comments JSON using jq for safer construction local comments_json_args=() for key in $my_keys; do local c c=$(parallel_get "comments_${key}" 2>/dev/null) if [[ -n "$c" ]] && jq -e . >/dev/null 2>&1 <<< "$c"; then comments_json_args+=("--arg" "key_${key}" "$c") fi done # Construct with jq (requires building the jq expression dynamically) # Or use Python to build the combined object from validated inputs ``` Alternatively, pass individual comment results to Python and let Python handle the JSON construction. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/my-standup-data.sh` around lines 54 - 64, The current loop builds comments_combined by string concatenation (variables: comments_combined, my_keys, parallel_get) which can produce malformed JSON; change the logic to validate each fetched comment with a JSON validator (e.g., jq -e) before adding it, or instead accumulate validated key/value pairs and use jq (or a small Python snippet) to construct the final JSON object atomically rather than concatenating strings; ensure you only append entries whose parallel_get output passes jq -e . and escape or pass raw JSON values to jq to build the final comments_combined. ``` </details> --- `66-67`: _⚡ Quick win_ **Validate adf.py existence before use.** The script derives the path to `adf.py` but doesn't verify the file exists before passing it to Python (line 77). If the file is missing, the dynamic import will fail with a less clear error message. <details> <summary>Add existence check</summary> ```diff local adf_py adf_py="$(cd "$(dirname "${BASH_SOURCE[0]}")/../util" && pwd)/adf.py" +if [[ ! -f "$adf_py" ]]; then + echo "ERROR: adf.py not found at $adf_py" >&2 + return 1 +fi ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/my-standup-data.sh` around lines 66 - 67, The script sets adf_py but never verifies it exists before invoking Python; add a file existence check after computing adf_py (use the adf_py variable in my-standup-data.sh) such as testing [ -f "$adf_py" ] and if missing emit a clear error to stderr and exit non-zero (e.g., echo "adf.py not found at $adf_py" >&2; exit 1) so the subsequent Python invocation that imports/executes adf.py won't fail with an unclear dynamic-import error. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/bug-overview.sh (1)</summary><blockquote> `21-27`: _💤 Low value_ **Roster display names are interpolated into JQL with raw quoting.** `f'assignee = \"{n}\"'` will produce malformed JQL if any roster name contains a `"` or `\`. The roster is currently maintained internally so this is unlikely to be exploited, but a single bad entry in the JSON will silently break every bug-overview run for the team. Consider escaping at minimum: ```python def _q(n: str) -> str: return n.replace('\\', '\\\\').replace('"', '\\"') clauses = ' OR '.join(f'assignee = "{_q(n)}"' for n in names) ``` Even better, resolve `accountId`s once and use those (`assignee in (...)`), which is also more robust to display-name changes in Jira. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/bug-overview.sh` around lines 21 - 27, The JQL-building Python fragment (assigning assignee_emails via the inline script using f'assignee = \"{n}\"') injects raw display names and will break if a name contains " or \; change the code to escape backslashes and double-quotes in each name before formatting (e.g., define a small _q(name) sanitizer that replaces \ with \\ and " with \") and use f'assignee = "{_q(n)}"' when joining clauses; alternatively, replace the display-name approach by resolving each roster entry to its Jira accountId and generate a single clause using assignee in (<comma-separated-accountIds>) for robustness. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/sprint-dashboard.sh (1)</summary><blockquote> `12-19`: _💤 Low value_ **`--stream` is parsed but never used.** Same pattern as `standup-data.sh` — `stream` is assigned and dropped (Shellcheck SC2034). Wire it through or remove the flag so the CLI surface doesn't promise streaming it can't deliver. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/sprint-dashboard.sh` around lines 12 - 19, The --stream flag is parsed into the local variable "stream" in sprint-dashboard.sh but never used; either remove the flag parsing to avoid a misleading CLI or wire "stream" through to the code that produces output. Fix by deleting the --stream case and the "stream" variable if streaming is unsupported, or propagate the boolean "stream" into the dashboard generation path (e.g., pass it into the function that renders or outputs the sprint dashboard such as the main renderer/generator function invoked after argument parsing) and branch on it to enable streaming behavior. Ensure the CLI help/usage is updated to match the change. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/composite/standup-data.sh (1)</summary><blockquote> `13-20`: _💤 Low value_ **`--stream` is parsed but never used.** `stream` is set on line 16 and never referenced again (also flagged by Shellcheck SC2034). Either wire it into the downstream Python (e.g., to gate streaming output) or drop the flag so callers don't think they're getting streaming behavior. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/composite/standup-data.sh` around lines 13 - 20, The --stream flag is parsed into the local variable "stream" but never used; either remove the flag handling from the parsing block (the while/case using "stream") or thread the flag into the downstream consumer by passing it when invoking the Python program (e.g., add "--stream" to the Python invocation when the "stream" variable is true). Update the parsing block (where "local stream=false" and the case for --stream sets stream=true) and the code that calls the Python script (or remove the case entirely) so the flag has effect and ShellCheck SC2034 is resolved. ``` </details> </blockquote></details> <details> <summary>plugins/node-team/scripts/lib/team.sh (1)</summary><blockquote> `47-86`: _⚡ Quick win_ **Deduplicate `ALL_NODE_COMPONENTS` and Node Core's `TEAM_BUG_COMPONENTS`.** The two strings are identical today, and the "all" branch already reuses `ALL_NODE_COMPONENTS`. Reusing the variable for the Core branch as well removes a future drift hazard (e.g., a new component added to one list and not the other). <details> <summary>♻️ Proposed refactor</summary> ```diff - TEAM_BUG_COMPONENTS='"Node", "Node / CRI-O", "Node / Kubelet", "Node / CPU manager", "Node / Memory manager", "Node / Topology manager", "Node / Numa aware Scheduling", "Node / Device Manager", "Node / Pod resource API", "Node / Node Problem Detector", "Node / Kueue", "Node / Instaslice-operator"' + TEAM_BUG_COMPONENTS="$ALL_NODE_COMPONENTS" TEAM_BACKLOG_KEYWORDS="" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/node-team/scripts/lib/team.sh` around lines 47 - 86, Replace the duplicated literal list in the "Node Core" case by reusing the ALL_NODE_COMPONENTS variable: inside the case branch for "Node Core"|"Core"|"core" change the TEAM_BUG_COMPONENTS assignment from the long literal to TEAM_BUG_COMPONENTS="$ALL_NODE_COMPONENTS" (preserving existing quoting/expansion style) so Core and the "all" branch share a single source of truth. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `b7a451e5-bee9-48e3-b60c-edf90dd9e5ae` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0948119635915e5deecfed07dd58d5a6d5e3c293 and 24fb5df803246a00c6d03d00d0aeee37dd1e55f8. </details> <details> <summary>📒 Files selected for processing (52)</summary> * `plugins/node-team/.claude-plugin/plugin.json` * `plugins/node-team/config/team-roster-core.example.json` * `plugins/node-team/config/team-roster-dra.example.json` * `plugins/node-team/scripts/jira.sh` * `plugins/node-team/scripts/lib/api/comment.sh` * `plugins/node-team/scripts/lib/api/fields.sh` * `plugins/node-team/scripts/lib/api/health.sh` * `plugins/node-team/scripts/lib/api/issue.sh` * `plugins/node-team/scripts/lib/api/sprint.sh` * `plugins/node-team/scripts/lib/api/transition.sh` * `plugins/node-team/scripts/lib/composite/bug-overview.sh` * `plugins/node-team/scripts/lib/composite/carryover-report.sh` * `plugins/node-team/scripts/lib/composite/epic-progress.sh` * `plugins/node-team/scripts/lib/composite/issue-deep-dive.sh` * `plugins/node-team/scripts/lib/composite/my-board-data.sh` * `plugins/node-team/scripts/lib/composite/my-bugs-data.sh` * `plugins/node-team/scripts/lib/composite/my-standup-data.sh` * `plugins/node-team/scripts/lib/composite/pickup-data.sh` * `plugins/node-team/scripts/lib/composite/planning-data.sh` * `plugins/node-team/scripts/lib/composite/release-data.sh` * `plugins/node-team/scripts/lib/composite/roster-sync.sh` * `plugins/node-team/scripts/lib/composite/sprint-dashboard.sh` * `plugins/node-team/scripts/lib/composite/standup-data.sh` * `plugins/node-team/scripts/lib/composite/team-activity.sh` * `plugins/node-team/scripts/lib/core.sh` * `plugins/node-team/scripts/lib/team.sh` * `plugins/node-team/scripts/lib/util/adf.py` * `plugins/node-team/scripts/lib/util/cache.sh` * `plugins/node-team/scripts/lib/util/parallel.sh` * `plugins/node-team/scripts/lib/util/retry.sh` * `plugins/node-team/scripts/ocp-install.sh` * `plugins/node-team/scripts/worktree.sh` * `plugins/node-team/scripts/worktree_test.sh` * `plugins/node-team/skills/node/SKILL.md` * `plugins/node-team/skills/node/references/INDEX.md` * `plugins/node-team/skills/node/references/SETUP.md` * `plugins/node-team/skills/node/references/deployment/debug-binary.md` * `plugins/node-team/skills/node/references/deployment/debug-binary/crio.md` * `plugins/node-team/skills/node/references/deployment/debug-binary/cross-compile.md` * `plugins/node-team/skills/node/references/deployment/debug-binary/deploy.md` * `plugins/node-team/skills/node/references/deployment/debug-binary/rollback.md` * `plugins/node-team/skills/node/references/deployment/debug-binary/ssh-bastion.md` * `plugins/node-team/skills/node/references/development/crio-dev.md` * `plugins/node-team/skills/node/references/development/crun-conmon.md` * `plugins/node-team/skills/node/references/development/kubelet-dev.md` * `plugins/node-team/skills/node/references/development/kueue-operator-dev.md` * `plugins/node-team/skills/node/references/development/mco-dev.md` * `plugins/node-team/skills/node/references/development/worktrees.md` * `plugins/node-team/skills/node/references/jira.md` * `plugins/node-team/skills/node/references/platform-docs.md` * `plugins/node-team/skills/node/references/prometheus.md` * `plugins/node-team/skills/node/references/support.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
OpenShift Node team assistant plugin for development, deployment, debugging, and workflow tasks across kubelet, MCO, CRI-O, crun, conmonrs, Kueue operator, Jira, and platform docs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
harche
force-pushed
the
add-node-team-plugin
branch
from
75787a5 to
f6b3d6a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
plugins/node-team/scripts/lib/team.sh (1)
47-72: ⚡ Quick winDuplicate component list —
Node Core'sTEAM_BUG_COMPONENTSis identical toALL_NODE_COMPONENTS.The string at Line 72 is character-for-character the same as
ALL_NODE_COMPONENTSdefined at Line 48. Reuse the variable to keep the two in sync if Node components change.♻️ Proposed refactor
- TEAM_BUG_COMPONENTS='"Node", "Node / CRI-O", "Node / Kubelet", "Node / CPU manager", "Node / Memory manager", "Node / Topology manager", "Node / Numa aware Scheduling", "Node / Device Manager", "Node / Pod resource API", "Node / Node Problem Detector", "Node / Kueue", "Node / Instaslice-operator"' + TEAM_BUG_COMPONENTS="$ALL_NODE_COMPONENTS"🤖 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/node-team/scripts/lib/team.sh` around lines 47 - 72, Replace the duplicated literal list in the "Node Core" case by reusing the ALL_NODE_COMPONENTS variable: set TEAM_BUG_COMPONENTS to the value of ALL_NODE_COMPONENTS (i.e., TEAM_BUG_COMPONENTS="${ALL_NODE_COMPONENTS}") in the case branch for "Node Core"/"Core"/"core" instead of repeating the long literal string so both stay in sync when ALL_NODE_COMPONENTS changes.plugins/node-team/scripts/lib/composite/sprint-dashboard.sh (1)
13-19: ⚡ Quick winUnknown CLI flags are silently dropped.
The
*) shift ;;arm makes typos like--sprintt fooconsume--sprinttandfoowithout warning, leaving the user wondering why the value was ignored. Print an error and return non-zero on unknown flags.♻️ Proposed refactor
while [[ $# -gt 0 ]]; do case "$1" in --stream) stream=true; shift ;; --sprint) sprint_ref="${2:?--sprint requires a value}"; shift 2 ;; - *) shift ;; + *) echo "Unknown argument: $1" >&2; return 2 ;; esac 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/node-team/scripts/lib/composite/sprint-dashboard.sh` around lines 13 - 19, The argument parser's default case currently swallows unknown flags (the "*) shift ;;" arm), so change that to print an error to stderr including the offending token (e.g., "$1") and exit non-zero instead of silently shifting; update the case in the while loop that handles --stream and --sprint (the case block around --stream/--sprint and the "*)") to report "Unknown option: $1" (or similar) to >&2 and exit 1 so typos like --sprintt foo produce an error and stop processing.plugins/node-team/scripts/lib/util/cache.sh (1)
46-51: ⚡ Quick win
cache_setwrites are non-atomic; concurrent writers (e.g., viaparallel.sh) can leave partial files.If two background tasks compute and store the same key simultaneously (which is plausible given the
parallel_runpattern used elsewhere in this PR), one writer can read a half-written file viacache_get. Write to a temp file in the same directory andmvinto place for atomic replacement.♻️ Proposed refactor
cache_set() { _cache_init local key="$1" local value="$2" - echo "$value" > "${_CACHE_DIR}/${key}" + local tmp + tmp=$(mktemp "${_CACHE_DIR}/.${key}.XXXXXX") || return 1 + printf '%s\n' "$value" > "$tmp" && mv -f "$tmp" "${_CACHE_DIR}/${key}" }This also avoids
echo's portability footguns when$valuestarts with-e/-n/-E.🤖 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/node-team/scripts/lib/util/cache.sh` around lines 46 - 51, The cache_set function currently writes directly and non-atomically to "${_CACHE_DIR}/${key}"; change it to initialize the cache (call _cache_init), write the value to a temporary file in the same directory (use a safe tmp name like "${_CACHE_DIR}/${key}.tmp.$$" or mktemp -p "$_CACHE_DIR"), write the content with a portable method (use printf '%s' rather than echo to avoid flags), fsync/close if available, then atomically mv the temp file into "${_CACHE_DIR}/${key}" so concurrent writers cannot leave partial files; keep references to cache_set, _cache_init, _CACHE_DIR and consider any cache_get readers expecting the final file name.
🤖 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/node-team/scripts/lib/api/transition.sh`:
- Around line 13-29: cmd_transition currently always returns success even if
some transitions fail; modify it to detect failures and return a non-zero status
so callers can react. Inside cmd_transition (function name) introduce a flag
(e.g., overall_exit or had_error) initialized to 0, set it to 1 whenever the
HTTP code parsed from result (variable code) is not "204" or when code is empty,
and after the for loop return the flag (return 1 on any failure, return 0
otherwise). Ensure you still print the per-key success/error JSON to
stdout/stderr as before but propagate the failure via the function return value.
- Around line 44-69: The loop in cmd_close currently ignores failures from
cmd_comment and treats per-key errors (failed transitions lookup or missing
transition and failed cmd_transition) as non-fatal by simply continuing; modify
cmd_close to track an overall failure status (e.g., a local failures=0 flag),
check the exit status of cmd_comment and increment failures on non-zero, also
increment failures when cmd_transitions fails (the existing continue path) or
when no close transition is found, and check the exit status of cmd_transition
and increment failures on non-zero; after the loop, return a non-zero exit code
if failures>0 so the command fails when any key fails.
In `@plugins/node-team/scripts/lib/composite/sprint-dashboard.sh`:
- Around line 51-57: The timing block can throw ValueError when
sprint["startDate"] or sprint["endDate"] is empty; guard the conversion before
calling datetime.fromisoformat by reading start_raw = sprint.get("startDate") or
"" and end_raw = sprint.get("endDate") or "" and only parse/compute start, end,
now, total_days, elapsed_days and days_remaining when both start_raw and end_raw
are non-empty; otherwise set total_days, elapsed_days and days_remaining to 0 so
sprint-dashboard.sh (the variables start, end, total_days, elapsed_days,
days_remaining) never calls datetime.fromisoformat on an empty string.
- Around line 32-49: parallel_wait_all currently swallows failures which later
cause JSONDecodeError in the Python block; change the script to check the exit
status of parallel_wait_all (call parallel_wait_all into a variable like rc=$?
or if ! parallel_wait_all; then log which tasks failed and exit 1) so the script
fails fast on worker errors, and additionally normalize possibly-empty captures
from parallel_get by defaulting sprint_json and roster_json to '{}' and
issues_json to '{"issues":[]}' before invoking python3 (check variables
sprint_json, roster_json, issues_json after calling parallel_get and set them to
those defaults if they are empty) so the Python json.loads calls in the python3
block (which reads sys.argv[1..3]) never receive empty strings.
In `@plugins/node-team/scripts/lib/team.sh`:
- Around line 75-81: The "all" team sets TEAM_SPRINT_FILTER="" which makes
substring checks always true and returns the wrong sprint; update sprint
resolution to reject an empty/blank TEAM_SPRINT_FILTER for the "all"/"All Node"
config instead of treating it as a wildcard: change team_sprint and
resolve_sprint to detect TEAM_SPRINT_FILTER=="" or TEAM_NAME=="All Node" and
return a clear error/exit explaining that "all" requires an explicit sprint or a
real filter, and update callers (e.g., cmd_sprint_dashboard /
sprint-dashboard.sh) to require an explicit --team or --sprint when using "all"
(or alternatively set TEAM_SPRINT_FILTER to a sentinel non-matching value and
handle that sentinel as an error) so sprint matching is deterministic.
- Around line 268-294: The fallback path in team_sprint_fallback uses the
variable closed_sprints (populated from cached_sprints or cmd_sprints) directly
into python3 -c with json.loads, which will raise JSONDecodeError if
closed_sprints is empty; before invoking python, check if closed_sprints is
empty or unset and if so set it to a safe default '{"values": []}' (or otherwise
short-circuit with an error), then pass that variable to the python invocation;
update references to closed_sprints and keep the same TEAM_SPRINT_FILTER usage
so behavior is unchanged when data exists.
In `@plugins/node-team/scripts/lib/util/cache.sh`:
- Around line 54-66: cached_sprints currently caches failed/empty outputs;
change it so cache_get only returns when the cached value is non-empty and only
call cache_set when cmd_sprints exits successfully. Specifically, in
cached_sprints (function cached_sprints) after cache_get "$cache_key" check that
"$cached" is non-empty before echoing/returning; then run result=$(cmd_sprints
"$state") capturing its exit status ($?) and only call cache_set "$cache_key"
"$result" and echo "$result" if cmd_sprints exited with 0 (and optionally result
is non-empty); if cmd_sprints failed, do not call cache_set and propagate the
non-zero exit status (return that error) so callers can see failures and retries
can occur.
In `@plugins/node-team/scripts/lib/util/retry.sh`:
- Around line 49-55: The parsed Retry-After value in retry.sh (variable
retry_after) may contain trailing CR or non-numeric data which can make sleep
fail; normalize and validate retry_after after it's parsed: strip whitespace and
any trailing \r, check it is a positive integer (e.g. via a numeric regex or
integer conversion), and if validation fails fallback to the existing delay
value; then use the sanitized retry_after for the _log and sleep calls (keep
existing logic around attempt, _RETRY_MAX and delay doubling intact).
In `@plugins/node-team/scripts/worktree_test.sh`:
- Around line 26-57: The test setup in setup() leaves repo defaults to the
caller's Git config causing non-main initial branches and missing user identity;
ensure each temporary repository (the bare repos created under
TEST_DIR/remotes/*, each seed directory referenced by local seed, and the
workspace at ws/WORKSPACE) is initialized with init.defaultBranch=main and
configured with a local user.name and user.email before the first git commit and
before pushing; update the git -C "${seed}" init and git -C "${ws}" init steps
to set init.defaultBranch=main (or run git config user.name/user.email in those
directories) so commits and pushes in the functions that call git -C ... commit
and git -C ... push succeed deterministically.
In `@plugins/node-team/scripts/worktree.sh`:
- Around line 174-181: The current root merge uses the current checked-out
branch (root_main=$(git -C "$ROOT" rev-parse --abbrev-ref HEAD)) which can be a
feature branch; change it to explicitly resolve the repository's intended main
branch and use that as the merge target: compute root_main by detecting origin's
HEAD (e.g., git symbolic-ref --quiet --short refs/remotes/origin/HEAD and strip
the "origin/" prefix) with a fallback of "main" if that fails, then use that
root_main to calculate root_ahead and perform the merge; additionally, if you
prefer the stricter option, validate that the working tree is on that resolved
root_main before merging and bail with an explanatory message if not.
- Around line 23-25: The root repo is fetched but its worktree branches
(wt/<name>) are never advanced; update cmd_sync() and cmd_pull() to
fast-forward/merge the root tracked branch into each root worktree branch just
like submodules are advanced: after git -C "$ROOT" fetch --quiet, determine the
tracked branch (the same branch you use for submodules) and run a fast-forward
merge into the corresponding wt/<name> branch (e.g., git -C "$ROOT" merge
--ff-only origin/<tracked-branch> or git -C "$ROOT" update-ref/checkout of
wt/<name> to that commit) so the root worktree actually reflects upstream
commits; ensure this logic lives alongside the existing submodule fast-forward
code in cmd_sync() and that cmd_pull() performs the same merge/ff-only update
before updating submodule pointers (instead of only git add/commit) so root
worktrees do not remain on isolated wt/<name> branches.
---
Nitpick comments:
In `@plugins/node-team/scripts/lib/composite/sprint-dashboard.sh`:
- Around line 13-19: The argument parser's default case currently swallows
unknown flags (the "*) shift ;;" arm), so change that to print an error to
stderr including the offending token (e.g., "$1") and exit non-zero instead of
silently shifting; update the case in the while loop that handles --stream and
--sprint (the case block around --stream/--sprint and the "*)") to report
"Unknown option: $1" (or similar) to >&2 and exit 1 so typos like --sprintt foo
produce an error and stop processing.
In `@plugins/node-team/scripts/lib/team.sh`:
- Around line 47-72: Replace the duplicated literal list in the "Node Core" case
by reusing the ALL_NODE_COMPONENTS variable: set TEAM_BUG_COMPONENTS to the
value of ALL_NODE_COMPONENTS (i.e.,
TEAM_BUG_COMPONENTS="${ALL_NODE_COMPONENTS}") in the case branch for "Node
Core"/"Core"/"core" instead of repeating the long literal string so both stay in
sync when ALL_NODE_COMPONENTS changes.
In `@plugins/node-team/scripts/lib/util/cache.sh`:
- Around line 46-51: The cache_set function currently writes directly and
non-atomically to "${_CACHE_DIR}/${key}"; change it to initialize the cache
(call _cache_init), write the value to a temporary file in the same directory
(use a safe tmp name like "${_CACHE_DIR}/${key}.tmp.$$" or mktemp -p
"$_CACHE_DIR"), write the content with a portable method (use printf '%s' rather
than echo to avoid flags), fsync/close if available, then atomically mv the temp
file into "${_CACHE_DIR}/${key}" so concurrent writers cannot leave partial
files; keep references to cache_set, _cache_init, _CACHE_DIR and consider any
cache_get readers expecting the final file name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4015fa74-004c-47fb-83f2-9e5608caa20c
📒 Files selected for processing (13)
.claude-plugin/marketplace.jsonplugins/node-team/scripts/lib/api/issue.shplugins/node-team/scripts/lib/api/transition.shplugins/node-team/scripts/lib/composite/bug-overview.shplugins/node-team/scripts/lib/composite/roster-sync.shplugins/node-team/scripts/lib/composite/sprint-dashboard.shplugins/node-team/scripts/lib/team.shplugins/node-team/scripts/lib/util/cache.shplugins/node-team/scripts/lib/util/retry.shplugins/node-team/scripts/ocp-install.shplugins/node-team/scripts/worktree.shplugins/node-team/scripts/worktree_test.shplugins/node-team/skills/node/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- .claude-plugin/marketplace.json
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/node-team/scripts/lib/composite/roster-sync.sh
- plugins/node-team/skills/node/SKILL.md
- plugins/node-team/scripts/lib/api/issue.sh
- plugins/node-team/scripts/lib/composite/bug-overview.sh
- plugins/node-team/scripts/ocp-install.sh
Comment on lines
+13
to
+29
| cmd_transition() { | ||
| local transition_id="$1" | ||
| shift | ||
| local keys=("$@") | ||
| for key in "${keys[@]}"; do | ||
| local result | ||
| result=$(_curl -X POST "${JIRA_BASE}/rest/api/3/issue/${key}/transitions" \ | ||
| -d "{\"transition\":{\"id\":\"${transition_id}\"}}" -w "\nHTTP_%{http_code}" 2>&1) | ||
| local code | ||
| code=$(echo "$result" | grep "HTTP_" | sed 's/HTTP_//') | ||
| if [[ "$code" == "204" ]]; then | ||
| echo "{\"key\":\"${key}\",\"status\":\"ok\"}" | ||
| else | ||
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"code\":\"${code}\"}" >&2 | ||
| fi | ||
| done | ||
| } |
Contributor
There was a problem hiding this comment.
cmd_transition always exits successfully, even when transitions fail.
Line 26 reports errors, but the function still returns 0. Callers can’t detect failed Jira transitions.
Proposed fix
cmd_transition() {
local transition_id="$1"
shift
local keys=("$@")
+ local failed=0
for key in "${keys[@]}"; do
local result
result=$(_curl -X POST "${JIRA_BASE}/rest/api/3/issue/${key}/transitions" \
-d "{\"transition\":{\"id\":\"${transition_id}\"}}" -w "\nHTTP_%{http_code}" 2>&1)
local code
- code=$(echo "$result" | grep "HTTP_" | sed 's/HTTP_//')
+ code=$(echo "$result" | sed -n 's/^HTTP_//p' | tail -n1)
if [[ "$code" == "204" ]]; then
echo "{\"key\":\"${key}\",\"status\":\"ok\"}"
else
echo "{\"key\":\"${key}\",\"status\":\"error\",\"code\":\"${code}\"}" >&2
+ failed=1
fi
done
+ return "$failed"
}🤖 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/node-team/scripts/lib/api/transition.sh` around lines 13 - 29,
cmd_transition currently always returns success even if some transitions fail;
modify it to detect failures and return a non-zero status so callers can react.
Inside cmd_transition (function name) introduce a flag (e.g., overall_exit or
had_error) initialized to 0, set it to 1 whenever the HTTP code parsed from
result (variable code) is not "204" or when code is empty, and after the for
loop return the flag (return 1 on any failure, return 0 otherwise). Ensure you
still print the per-key success/error JSON to stdout/stderr as before but
propagate the failure via the function return value.
Comment on lines
+44
to
+69
| for key in "${keys[@]}"; do | ||
| if [[ -n "$comment" ]]; then | ||
| cmd_comment "$comment" "$key" > /dev/null | ||
| fi | ||
| # Resolve close transition dynamically per issue | ||
| local close_id="${JIRA_CLOSE_TRANSITION_ID:-}" | ||
| if [[ -z "$close_id" ]]; then | ||
| local transitions_json | ||
| transitions_json=$(cmd_transitions "$key") || { echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to fetch transitions\"}" >&2; continue; } | ||
| close_id=$(echo "$transitions_json" | python3 -c " | ||
| import json, sys | ||
| data = json.load(sys.stdin) | ||
| for name in ['Closed', 'Close', 'Done']: | ||
| for t in data.get('transitions', []): | ||
| if t.get('name', '').lower() == name.lower(): | ||
| print(t['id']) | ||
| sys.exit(0) | ||
| print('') | ||
| " 2>/dev/null) | ||
| if [[ -z "$close_id" ]]; then | ||
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"no Close/Closed/Done transition found\"}" >&2 | ||
| continue | ||
| fi | ||
| fi | ||
| cmd_transition "$close_id" "$key" | ||
| done |
Contributor
There was a problem hiding this comment.
cmd_close masks per-key failures and still returns success.
Line 46 ignores cmd_comment failures, and Lines 52/64 continue without tracking failure. The command should return non-zero when any key fails.
Proposed fix
cmd_close() {
local comment=""
local keys=()
+ local failed=0
@@
for key in "${keys[@]}"; do
if [[ -n "$comment" ]]; then
- cmd_comment "$comment" "$key" > /dev/null
+ if ! cmd_comment "$comment" "$key" > /dev/null; then
+ echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to add comment\"}" >&2
+ failed=1
+ continue
+ fi
fi
@@
local transitions_json
- transitions_json=$(cmd_transitions "$key") || { echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to fetch transitions\"}" >&2; continue; }
+ transitions_json=$(cmd_transitions "$key") || {
+ echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to fetch transitions\"}" >&2
+ failed=1
+ continue
+ }
@@
if [[ -z "$close_id" ]]; then
echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"no Close/Closed/Done transition found\"}" >&2
+ failed=1
continue
fi
fi
- cmd_transition "$close_id" "$key"
+ cmd_transition "$close_id" "$key" || failed=1
done
+ return "$failed"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for key in "${keys[@]}"; do | |
| if [[ -n "$comment" ]]; then | |
| cmd_comment "$comment" "$key" > /dev/null | |
| fi | |
| # Resolve close transition dynamically per issue | |
| local close_id="${JIRA_CLOSE_TRANSITION_ID:-}" | |
| if [[ -z "$close_id" ]]; then | |
| local transitions_json | |
| transitions_json=$(cmd_transitions "$key") || { echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to fetch transitions\"}" >&2; continue; } | |
| close_id=$(echo "$transitions_json" | python3 -c " | |
| import json, sys | |
| data = json.load(sys.stdin) | |
| for name in ['Closed', 'Close', 'Done']: | |
| for t in data.get('transitions', []): | |
| if t.get('name', '').lower() == name.lower(): | |
| print(t['id']) | |
| sys.exit(0) | |
| print('') | |
| " 2>/dev/null) | |
| if [[ -z "$close_id" ]]; then | |
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"no Close/Closed/Done transition found\"}" >&2 | |
| continue | |
| fi | |
| fi | |
| cmd_transition "$close_id" "$key" | |
| done | |
| cmd_close() { | |
| local comment="" | |
| local keys=() | |
| local failed=0 | |
| for key in "${keys[@]}"; do | |
| if [[ -n "$comment" ]]; then | |
| if ! cmd_comment "$comment" "$key" > /dev/null; then | |
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to add comment\"}" >&2 | |
| failed=1 | |
| continue | |
| fi | |
| fi | |
| # Resolve close transition dynamically per issue | |
| local close_id="${JIRA_CLOSE_TRANSITION_ID:-}" | |
| if [[ -z "$close_id" ]]; then | |
| local transitions_json | |
| transitions_json=$(cmd_transitions "$key") || { | |
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"failed to fetch transitions\"}" >&2 | |
| failed=1 | |
| continue | |
| } | |
| close_id=$(echo "$transitions_json" | python3 -c " | |
| import json, sys | |
| data = json.load(sys.stdin) | |
| for name in ['Closed', 'Close', 'Done']: | |
| for t in data.get('transitions', []): | |
| if t.get('name', '').lower() == name.lower(): | |
| print(t['id']) | |
| sys.exit(0) | |
| print('') | |
| " 2>/dev/null) | |
| if [[ -z "$close_id" ]]; then | |
| echo "{\"key\":\"${key}\",\"status\":\"error\",\"cause\":\"no Close/Closed/Done transition found\"}" >&2 | |
| failed=1 | |
| continue | |
| fi | |
| fi | |
| cmd_transition "$close_id" "$key" || failed=1 | |
| done | |
| return "$failed" | |
| } |
🤖 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/node-team/scripts/lib/api/transition.sh` around lines 44 - 69, The
loop in cmd_close currently ignores failures from cmd_comment and treats per-key
errors (failed transitions lookup or missing transition and failed
cmd_transition) as non-fatal by simply continuing; modify cmd_close to track an
overall failure status (e.g., a local failures=0 flag), check the exit status of
cmd_comment and increment failures on non-zero, also increment failures when
cmd_transitions fails (the existing continue path) or when no close transition
is found, and check the exit status of cmd_transition and increment failures on
non-zero; after the loop, return a non-zero exit code if failures>0 so the
command fails when any key fails.
Comment on lines
+32
to
+49
| parallel_init | ||
| parallel_run "issues" cmd_sprint_issues "$sprint_id" | ||
| parallel_run "roster" team_roster "$team" | ||
| parallel_wait_all || true | ||
|
|
||
| local issues_json roster_json | ||
| issues_json=$(parallel_get "issues") | ||
| roster_json=$(parallel_get "roster") | ||
|
|
||
| # ── Process everything in Python for speed ─────────────────────────────── | ||
| python3 - "$sprint_json" "$roster_json" "$issues_json" <<'PYEOF' | ||
| import json, sys | ||
| from datetime import datetime, timezone | ||
|
|
||
| sprint = json.loads(sys.argv[1]) | ||
| roster = json.loads(sys.argv[2]) | ||
| data = json.loads(sys.argv[3]) | ||
| issues = data.get("issues", []) |
Contributor
There was a problem hiding this comment.
Failures from parallel_run are swallowed and propagate as JSON parse errors downstream.
parallel_wait_all || true discards the exit status, and parallel_get returns whatever (possibly empty) string was captured. The Python block then calls json.loads(sys.argv[2]) / json.loads(sys.argv[3]) (Lines 47–48), which raises JSONDecodeError on empty input — the user sees a Python traceback instead of an actionable error. Either fail fast on a non-zero parallel_wait_all, or normalize empty results before passing to Python.
🛠️ Proposed fix
parallel_init
parallel_run "issues" cmd_sprint_issues "$sprint_id"
parallel_run "roster" team_roster "$team"
- parallel_wait_all || true
+ parallel_wait_all || {
+ echo '{"error":"Failed to fetch sprint issues or roster"}' >&2
+ return 1
+ }
local issues_json roster_json
issues_json=$(parallel_get "issues")
roster_json=$(parallel_get "roster")
+ [[ -z "$issues_json" ]] && issues_json='{"issues":[]}'
+ [[ -z "$roster_json" ]] && roster_json='[]'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parallel_init | |
| parallel_run "issues" cmd_sprint_issues "$sprint_id" | |
| parallel_run "roster" team_roster "$team" | |
| parallel_wait_all || true | |
| local issues_json roster_json | |
| issues_json=$(parallel_get "issues") | |
| roster_json=$(parallel_get "roster") | |
| # ── Process everything in Python for speed ─────────────────────────────── | |
| python3 - "$sprint_json" "$roster_json" "$issues_json" <<'PYEOF' | |
| import json, sys | |
| from datetime import datetime, timezone | |
| sprint = json.loads(sys.argv[1]) | |
| roster = json.loads(sys.argv[2]) | |
| data = json.loads(sys.argv[3]) | |
| issues = data.get("issues", []) | |
| parallel_init | |
| parallel_run "issues" cmd_sprint_issues "$sprint_id" | |
| parallel_run "roster" team_roster "$team" | |
| parallel_wait_all || { | |
| echo '{"error":"Failed to fetch sprint issues or roster"}' >&2 | |
| return 1 | |
| } | |
| local issues_json roster_json | |
| issues_json=$(parallel_get "issues") | |
| roster_json=$(parallel_get "roster") | |
| [[ -z "$issues_json" ]] && issues_json='{"issues":[]}' | |
| [[ -z "$roster_json" ]] && roster_json='[]' | |
| # ── Process everything in Python for speed ─────────────────────────────── | |
| python3 - "$sprint_json" "$roster_json" "$issues_json" <<'PYEOF' | |
| import json, sys | |
| from datetime import datetime, timezone | |
| sprint = json.loads(sys.argv[1]) | |
| roster = json.loads(sys.argv[2]) | |
| data = json.loads(sys.argv[3]) | |
| issues = data.get("issues", []) |
🤖 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/node-team/scripts/lib/composite/sprint-dashboard.sh` around lines 32
- 49, parallel_wait_all currently swallows failures which later cause
JSONDecodeError in the Python block; change the script to check the exit status
of parallel_wait_all (call parallel_wait_all into a variable like rc=$? or if !
parallel_wait_all; then log which tasks failed and exit 1) so the script fails
fast on worker errors, and additionally normalize possibly-empty captures from
parallel_get by defaulting sprint_json and roster_json to '{}' and issues_json
to '{"issues":[]}' before invoking python3 (check variables sprint_json,
roster_json, issues_json after calling parallel_get and set them to those
defaults if they are empty) so the Python json.loads calls in the python3 block
(which reads sys.argv[1..3]) never receive empty strings.
Comment on lines
+51
to
+57
| # Sprint progress | ||
| start = datetime.fromisoformat(sprint["startDate"].replace("Z", "+00:00")) | ||
| end = datetime.fromisoformat(sprint["endDate"].replace("Z", "+00:00")) | ||
| now = datetime.now(timezone.utc) | ||
| total_days = max((end - start).days, 1) | ||
| elapsed_days = min(max((now - start).days, 0), total_days) | ||
| days_remaining = max(total_days - elapsed_days, 0) |
Contributor
There was a problem hiding this comment.
datetime.fromisoformat will crash for sprints with empty startDate/endDate.
team_sprint/resolve_sprint populate these fields with s.get('startDate', '') (see team.sh Lines 170–171, 232, 239, 286–287), so future sprints (which often have null start/end in Jira) propagate as "". datetime.fromisoformat("".replace("Z","+00:00")) raises ValueError. With --sprint <future sprint name> the dashboard breaks instead of producing usable output.
Consider guarding the timing block, e.g.:
start_raw = sprint.get("startDate") or ""
end_raw = sprint.get("endDate") or ""
if start_raw and end_raw:
start = datetime.fromisoformat(start_raw.replace("Z", "+00:00"))
end = datetime.fromisoformat(end_raw.replace("Z", "+00:00"))
now = datetime.now(timezone.utc)
total_days = max((end - start).days, 1)
elapsed_days = min(max((now - start).days, 0), total_days)
days_remaining = max(total_days - elapsed_days, 0)
else:
total_days = elapsed_days = days_remaining = 0🤖 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/node-team/scripts/lib/composite/sprint-dashboard.sh` around lines 51
- 57, The timing block can throw ValueError when sprint["startDate"] or
sprint["endDate"] is empty; guard the conversion before calling
datetime.fromisoformat by reading start_raw = sprint.get("startDate") or "" and
end_raw = sprint.get("endDate") or "" and only parse/compute start, end, now,
total_days, elapsed_days and days_remaining when both start_raw and end_raw are
non-empty; otherwise set total_days, elapsed_days and days_remaining to 0 so
sprint-dashboard.sh (the variables start, end, total_days, elapsed_days,
days_remaining) never calls datetime.fromisoformat on an empty string.
Comment on lines
+75
to
+81
| "all"|"All"|"All Node") | ||
| TEAM_NAME="All Node" | ||
| TEAM_SPRINT_FILTER="" | ||
| TEAM_ROSTER_FILE="" | ||
| TEAM_BUG_COMPONENTS="$ALL_NODE_COMPONENTS" | ||
| TEAM_BACKLOG_KEYWORDS="" | ||
| ;; |
Contributor
There was a problem hiding this comment.
all team produces an empty TEAM_SPRINT_FILTER, causing sprint matching to be non-deterministic.
For team_config "all", TEAM_SPRINT_FILTER is set to "". Both team_sprint (Line 166) and resolve_sprint (via the substring match path) test team_filter in s.get('name', '') — and Python's "" in "anything" is always True, so the very first sprint encountered is returned regardless of which team it belongs to. That makes cmd_sprint_dashboard "all" (and any --sprint lookup against all) silently return the wrong sprint.
Either reject team="all" in team_sprint/resolve_sprint with a clear error, or make the consumer (e.g., sprint-dashboard.sh) require an explicit team for sprint resolution.
Also applies to: 148-178, 185-198
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 76-76: TEAM_NAME appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 79-79: TEAM_BUG_COMPONENTS appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 80-80: TEAM_BACKLOG_KEYWORDS appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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/node-team/scripts/lib/team.sh` around lines 75 - 81, The "all" team
sets TEAM_SPRINT_FILTER="" which makes substring checks always true and returns
the wrong sprint; update sprint resolution to reject an empty/blank
TEAM_SPRINT_FILTER for the "all"/"All Node" config instead of treating it as a
wildcard: change team_sprint and resolve_sprint to detect TEAM_SPRINT_FILTER==""
or TEAM_NAME=="All Node" and return a clear error/exit explaining that "all"
requires an explicit sprint or a real filter, and update callers (e.g.,
cmd_sprint_dashboard / sprint-dashboard.sh) to require an explicit --team or
--sprint when using "all" (or alternatively set TEAM_SPRINT_FILTER to a sentinel
non-matching value and handle that sentinel as an error) so sprint matching is
deterministic.
Comment on lines
+54
to
+66
| cached_sprints() { | ||
| local state="${1:-active}" | ||
| local cache_key="sprints_${state}" | ||
| local cached | ||
| if cached=$(cache_get "$cache_key" 2>/dev/null); then | ||
| echo "$cached" | ||
| return 0 | ||
| fi | ||
| local result | ||
| result=$(cmd_sprints "$state") | ||
| cache_set "$cache_key" "$result" | ||
| echo "$result" | ||
| } |
Contributor
There was a problem hiding this comment.
cached_sprints caches failures, masking errors for the TTL window.
If cmd_sprints "$state" fails (network issue, auth failure, etc.), result becomes an empty string (or partial output), and cache_set happily stores it. Subsequent calls within _CACHE_TTL (default 300s) hit the cache and return the empty/bad output without ever retrying — and since cache_get succeeds on an empty file, callers won't even fall through to cmd_sprints. Check the exit status before caching.
🛠️ Proposed fix
cached_sprints() {
local state="${1:-active}"
local cache_key="sprints_${state}"
local cached
if cached=$(cache_get "$cache_key" 2>/dev/null); then
echo "$cached"
return 0
fi
local result
- result=$(cmd_sprints "$state")
- cache_set "$cache_key" "$result"
- echo "$result"
+ if ! result=$(cmd_sprints "$state"); then
+ return 1
+ fi
+ [[ -n "$result" ]] && cache_set "$cache_key" "$result"
+ echo "$result"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cached_sprints() { | |
| local state="${1:-active}" | |
| local cache_key="sprints_${state}" | |
| local cached | |
| if cached=$(cache_get "$cache_key" 2>/dev/null); then | |
| echo "$cached" | |
| return 0 | |
| fi | |
| local result | |
| result=$(cmd_sprints "$state") | |
| cache_set "$cache_key" "$result" | |
| echo "$result" | |
| } | |
| cached_sprints() { | |
| local state="${1:-active}" | |
| local cache_key="sprints_${state}" | |
| local cached | |
| if cached=$(cache_get "$cache_key" 2>/dev/null); then | |
| echo "$cached" | |
| return 0 | |
| fi | |
| local result | |
| if ! result=$(cmd_sprints "$state"); then | |
| return 1 | |
| fi | |
| [[ -n "$result" ]] && cache_set "$cache_key" "$result" | |
| echo "$result" | |
| } |
🤖 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/node-team/scripts/lib/util/cache.sh` around lines 54 - 66,
cached_sprints currently caches failed/empty outputs; change it so cache_get
only returns when the cached value is non-empty and only call cache_set when
cmd_sprints exits successfully. Specifically, in cached_sprints (function
cached_sprints) after cache_get "$cache_key" check that "$cached" is non-empty
before echoing/returning; then run result=$(cmd_sprints "$state") capturing its
exit status ($?) and only call cache_set "$cache_key" "$result" and echo
"$result" if cmd_sprints exited with 0 (and optionally result is non-empty); if
cmd_sprints failed, do not call cache_set and propagate the non-zero exit status
(return that error) so callers can see failures and retries can occur.
Comment on lines
+49
to
+55
| local retry_after | ||
| retry_after=$(grep -i "^retry-after:" "$hdrfile" 2>/dev/null | awk '{print $2}' || echo "$delay") | ||
| retry_after=${retry_after:-$delay} | ||
| if (( attempt < _RETRY_MAX )); then | ||
| _log "WARN" "Rate limited (429), retrying in ${retry_after}s (attempt ${attempt}/${_RETRY_MAX})..." | ||
| sleep "$retry_after" | ||
| delay=$((delay * 2)) |
Contributor
There was a problem hiding this comment.
Normalize and validate Retry-After before sleeping.
At Line 50, the parsed header can include a trailing \r (and sometimes non-numeric values), which can make Line 54 (sleep "$retry_after") fail and cause immediate/incorrect retries.
🔧 Proposed fix
429)
# Rate limited — respect Retry-After if present
local retry_after
- retry_after=$(grep -i "^retry-after:" "$hdrfile" 2>/dev/null | awk '{print $2}' || echo "$delay")
- retry_after=${retry_after:-$delay}
+ retry_after=$(awk 'BEGIN{IGNORECASE=1} /^retry-after:/ {print $2; exit}' "$hdrfile" 2>/dev/null | tr -d '\r')
+ [[ "$retry_after" =~ ^[0-9]+$ ]] || retry_after="$delay"
if (( attempt < _RETRY_MAX )); then
_log "WARN" "Rate limited (429), retrying in ${retry_after}s (attempt ${attempt}/${_RETRY_MAX})..."
sleep "$retry_after"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local retry_after | |
| retry_after=$(grep -i "^retry-after:" "$hdrfile" 2>/dev/null | awk '{print $2}' || echo "$delay") | |
| retry_after=${retry_after:-$delay} | |
| if (( attempt < _RETRY_MAX )); then | |
| _log "WARN" "Rate limited (429), retrying in ${retry_after}s (attempt ${attempt}/${_RETRY_MAX})..." | |
| sleep "$retry_after" | |
| delay=$((delay * 2)) | |
| local retry_after | |
| retry_after=$(awk 'BEGIN{IGNORECASE=1} /^retry-after:/ {print $2; exit}' "$hdrfile" 2>/dev/null | tr -d '\r') | |
| [[ "$retry_after" =~ ^[0-9]+$ ]] || retry_after="$delay" | |
| if (( attempt < _RETRY_MAX )); then | |
| _log "WARN" "Rate limited (429), retrying in ${retry_after}s (attempt ${attempt}/${_RETRY_MAX})..." | |
| sleep "$retry_after" | |
| delay=$((delay * 2)) |
🤖 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/node-team/scripts/lib/util/retry.sh` around lines 49 - 55, The parsed
Retry-After value in retry.sh (variable retry_after) may contain trailing CR or
non-numeric data which can make sleep fail; normalize and validate retry_after
after it's parsed: strip whitespace and any trailing \r, check it is a positive
integer (e.g. via a numeric regex or integer conversion), and if validation
fails fallback to the existing delay value; then use the sanitized retry_after
for the _log and sleep calls (keep existing logic around attempt, _RETRY_MAX and
delay doubling intact).
Comment on lines
+26
to
+57
| setup() { | ||
| TEST_DIR="$(mktemp -d)" | ||
| echo "Test directory: ${TEST_DIR}" | ||
| echo "" | ||
|
|
||
| export GIT_CONFIG_COUNT=1 | ||
| export GIT_CONFIG_KEY_0=protocol.file.allow | ||
| export GIT_CONFIG_VALUE_0=always | ||
|
|
||
| for repo in repo-a repo-b; do | ||
| git init --bare "${TEST_DIR}/remotes/${repo}.git" --quiet | ||
| local seed="${TEST_DIR}/seed-${repo}" | ||
| mkdir -p "${seed}" | ||
| git -C "${seed}" init --quiet | ||
| echo "# ${repo}" > "${seed}/README.md" | ||
| git -C "${seed}" add README.md | ||
| git -C "${seed}" commit -m "initial commit" --quiet | ||
| git -C "${seed}" remote add origin "${TEST_DIR}/remotes/${repo}.git" | ||
| git -C "${seed}" push --quiet origin main 2>/dev/null | ||
| done | ||
|
|
||
| local ws="${TEST_DIR}/workspace" | ||
| mkdir -p "${ws}" | ||
| git -C "${ws}" init --quiet | ||
| git -C "${ws}" submodule add "${TEST_DIR}/remotes/repo-a.git" repo-a 2>/dev/null | ||
| git -C "${ws}" submodule add "${TEST_DIR}/remotes/repo-b.git" repo-b 2>/dev/null | ||
| git -C "${ws}" commit -m "add submodules" --quiet | ||
|
|
||
| cp "${WORKTREE_SH}" "${ws}/worktree.sh" | ||
| chmod +x "${ws}/worktree.sh" | ||
| WORKSPACE="${ws}" | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the test harness pins the default branch and commit identity.
sed -n '26,57p' plugins/node-team/scripts/worktree_test.sh
echo "----"
sed -n '72,81p' plugins/node-team/scripts/worktree_test.sh
echo "----"
sed -n '149,159p' plugins/node-team/scripts/worktree_test.sh
echo "----"
rg -n 'init --(initial-branch|-b) main|branch -M main|config user\.(name|email)' plugins/node-team/scripts/worktree_test.shRepository: openshift-eng/ai-helpers
Length of output: 2101
🏁 Script executed:
# Check if worktree.sh uses/references the main branch
grep -n 'main\|master' plugins/node-team/scripts/worktree_test.sh | head -20
echo "---"
# Check the full file for any git config setup outside of setup()
rg -n 'GIT_CONFIG|user\.(name|email)' plugins/node-team/scripts/worktree_test.sh
echo "---"
# Look for any test framework setup or bats directives
head -30 plugins/node-team/scripts/worktree_test.shRepository: openshift-eng/ai-helpers
Length of output: 1861
Test setup still depends on the caller's Git defaults.
These temp repos inherit init.defaultBranch, user.name, and user.email from the environment. Since the suite explicitly asserts commits on the main branch (lines 87, 91, 132-133, 165-167, 191, 197-198) but doesn't pin the initial branch or configure user identity, it will fail on systems where init.defaultBranch is not main or where no global Git user config is set. Explicitly set the default branch to main in each temporary repo and configure local user.name and user.email before the first 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/node-team/scripts/worktree_test.sh` around lines 26 - 57, The test
setup in setup() leaves repo defaults to the caller's Git config causing
non-main initial branches and missing user identity; ensure each temporary
repository (the bare repos created under TEST_DIR/remotes/*, each seed directory
referenced by local seed, and the workspace at ws/WORKSPACE) is initialized with
init.defaultBranch=main and configured with a local user.name and user.email
before the first git commit and before pushing; update the git -C "${seed}" init
and git -C "${ws}" init steps to set init.defaultBranch=main (or run git config
user.name/user.email in those directories) so commits and pushes in the
functions that call git -C ... commit and git -C ... push succeed
deterministically.
Comment on lines
+23
to
+25
| # Fetch + pull root repo | ||
| echo " root: fetching..." | ||
| git -C "$ROOT" fetch --quiet |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that root sync/pull never advance or merge the root branch/worktree.
sed -n '20,40p' plugins/node-team/scripts/worktree.sh
echo "----"
sed -n '92,156p' plugins/node-team/scripts/worktree.sh
echo "----"
rg -n 'root.*pull|origin/main|root worktree' plugins/node-team/scripts/worktree_test.shRepository: openshift-eng/ai-helpers
Length of output: 3061
🏁 Script executed:
# Search for help text and design comments
head -30 plugins/node-team/scripts/worktree.sh
echo "---"
# Check the test file to understand expected behavior
sed -n '175,195p' plugins/node-team/scripts/worktree_test.shRepository: openshift-eng/ai-helpers
Length of output: 2017
🏁 Script executed:
# Check how the root worktree is created and what branch it's on
sed -n '55,91p' plugins/node-team/scripts/worktree.shRepository: openshift-eng/ai-helpers
Length of output: 1249
Root repo is never advanced or merged into the root worktree during sync/pull.
The cmd_sync() function fetches the root repo but never advances it; it only fast-forwards submodules. The cmd_pull() function merges tracked branches into each submodule's worktree branch but never merges the root repo's branch into the root worktree—it only updates root submodule pointers via git add/commit. This leaves root worktrees on their isolated wt/<name> branch without absorbing any commits to the root repo's tracked branch until the final merge command. If root-level changes accumulate on main, worktrees will not reflect them even after pull.
🤖 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/node-team/scripts/worktree.sh` around lines 23 - 25, The root repo is
fetched but its worktree branches (wt/<name>) are never advanced; update
cmd_sync() and cmd_pull() to fast-forward/merge the root tracked branch into
each root worktree branch just like submodules are advanced: after git -C
"$ROOT" fetch --quiet, determine the tracked branch (the same branch you use for
submodules) and run a fast-forward merge into the corresponding wt/<name> branch
(e.g., git -C "$ROOT" merge --ff-only origin/<tracked-branch> or git -C "$ROOT"
update-ref/checkout of wt/<name> to that commit) so the root worktree actually
reflects upstream commits; ensure this logic lives alongside the existing
submodule fast-forward code in cmd_sync() and that cmd_pull() performs the same
merge/ff-only update before updating submodule pointers (instead of only git
add/commit) so root worktrees do not remain on isolated wt/<name> branches.
Comment on lines
+174
to
+181
| if git -C "$ROOT" rev-parse --verify "$branch" >/dev/null 2>&1; then | ||
| local root_main | ||
| root_main=$(git -C "$ROOT" rev-parse --abbrev-ref HEAD) | ||
| local root_ahead | ||
| root_ahead=$(git -C "$ROOT" rev-list --count "$root_main..$branch" 2>/dev/null || echo "0") | ||
| if [ "$root_ahead" != "0" ]; then | ||
| echo " root: merging $branch ($root_ahead commit(s)) into $root_main..." | ||
| if git -C "$ROOT" merge --ff-only "$branch" --quiet 2>/dev/null; then |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify merge target selection vs the user-facing contract.
sed -n '170,186p' plugins/node-team/scripts/worktree.sh
echo "----"
sed -n '342,349p' plugins/node-team/scripts/worktree.shRepository: openshift-eng/ai-helpers
Length of output: 1398
🏁 Script executed:
# Search for references to "main" and merge-related logic
rg -n "main|merge" plugins/node-team/scripts/worktree.sh | head -30Repository: openshift-eng/ai-helpers
Length of output: 2143
🏁 Script executed:
# Check if there are comments near the problematic code explaining intent
sed -n '160,190p' plugins/node-team/scripts/worktree.shRepository: openshift-eng/ai-helpers
Length of output: 1180
🏁 Script executed:
# Look for any other branch references in the merge function
sed -n '155,220p' plugins/node-team/scripts/worktree.shRepository: openshift-eng/ai-helpers
Length of output: 2623
Root merge targets the current HEAD branch instead of the documented main branch.
The help text (line 342-349) states merge <name> will "merge worktree branches into main + update root", but the code at lines 175-176 derives the merge target from git rev-parse --abbrev-ref HEAD — the repo's currently checked-out branch. If the root repo is on a feature branch or another wt/* branch when this command runs, it will silently merge into that branch instead of main, contradicting the CLI contract and risking unintended branch updates.
The submodule merge logic (line 194+) correctly uses tracked branch configuration from .gitmodules (defaulting to main), but the root repo merge does not follow this pattern. Resolve the root target branch explicitly to match documented behavior — either by reading main from configuration (e.g., git symbolic-ref --quiet --short refs/remotes/origin/HEAD | sed 's#^origin/##' with "main" fallback) or by validating the root repo is already on the main branch before proceeding.
🤖 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/node-team/scripts/worktree.sh` around lines 174 - 181, The current
root merge uses the current checked-out branch (root_main=$(git -C "$ROOT"
rev-parse --abbrev-ref HEAD)) which can be a feature branch; change it to
explicitly resolve the repository's intended main branch and use that as the
merge target: compute root_main by detecting origin's HEAD (e.g., git
symbolic-ref --quiet --short refs/remotes/origin/HEAD and strip the "origin/"
prefix) with a fallback of "main" if that fails, then use that root_main to
calculate root_ahead and perform the merge; additionally, if you prefer the
stricter option, validate that the working tree is on that resolved root_main
before merging and bail with an explanatory message if not.
Remove ~4,900 lines of bash/Python scripts and replace with self-contained reference documentation that teaches Claude how to interact with Jira REST API, manage worktrees, and understand team structure directly — no wrapper scripts needed. - Delete all scripts/ (jira.sh, worktree.sh, ocp-install.sh, all API/composite/utility modules) - Rewrite jira.md as complete API reference (auth, endpoints, ADF format, custom fields, JQL recipes, triage definitions) - Update worktrees.md with native git worktree commands - Update SKILL.md to remove scripts tool permission - Bump version to 0.12.0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/node-team/skills/node/references/development/kueue-operator-dev.md (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stray
18artifact from the markdown file.Line 18 appears to contain an accidental literal
18, which will render as noise in the reference doc.✂️ Proposed fix
-- 18🤖 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/node-team/skills/node/references/development/kueue-operator-dev.md` at line 18, Remove the stray literal "18" from the markdown document (the accidental artifact currently present as a standalone token) so the reference doc renders correctly; edit the file containing the stray "18", delete that token, and ensure surrounding markdown formatting/spacing is preserved (no other content should be changed).
🧹 Nitpick comments (1)
plugins/node-team/skills/node/references/development/worktrees.md (1)
7-11: ⚡ Quick winConsider adding a warning when fast-forward fails during sync.
Line 53 mentions "If a submodule has diverged, warn and skip," but the sync command on line 11 silently skips via
|| truewithout warning. While the command achieves the intended behavior, adding feedback would help users understand when submodules can't be synced.📢 Optional enhancement to show warnings
-git submodule foreach --quiet 'git fetch --quiet origin; git checkout main --quiet 2>/dev/null; git merge --ff-only origin/main --quiet 2>/dev/null || true' +git submodule foreach --quiet 'git fetch --quiet origin; git checkout main --quiet 2>/dev/null; git merge --ff-only origin/main --quiet 2>/dev/null || echo "⚠️ Could not fast-forward $sm_path, skipping" >&2'🤖 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/node-team/skills/node/references/development/worktrees.md` around lines 7 - 11, The git submodule sync command uses git submodule foreach with 'git merge --ff-only origin/main --quiet 2>/dev/null || true' which silently swallows non-fast-forward failures; modify the foreach command handling (the git submodule foreach line) to detect a non-fast-forward exit status and emit a warning (e.g., print a message to stderr or stdout identifying the submodule path) instead of simply swallowing the error with '|| true' so users are informed when a submodule has diverged and was skipped.
🤖 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/node-team/.claude-plugin/plugin.json`:
- Line 4: The plugin.json declares "version": "0.12.0" while marketplace.json
still lists "0.11.0"; update the "version" field in marketplace.json to "0.12.0"
so both files match. Locate the "version" key in marketplace.json (the
counterpart to plugin.json's version) and change its value to "0.12.0", then run
any validation or lint steps you have to ensure the manifest stays valid.
In `@plugins/node-team/skills/node/references/deployment/debug-binary.md`:
- Around line 13-15: The two unlabeled fenced code blocks in debug-binary.md
should be annotated with languages to satisfy MD040: add ```bash before the
bind-mount command line containing "mount --bind /home/core/crio /usr/bin/crio"
and add ```text before the ASCII diagram block that begins with "Local Machine
RHCOS Worker Node" (the multi-line diagram spanning the steps) so the first
block is marked as bash and the diagram as text.
In `@plugins/node-team/skills/node/references/deployment/debug-binary/crio.md`:
- Around line 34-40: Update the two unlabeled fenced code blocks by adding a
language identifier (e.g., text); specifically replace the opening ``` for the
block containing "libseccomp.so.2" (and its sibling lines) with ```text and do
the same for the block starting with "containers_image_ostree_stub" so both
blocks are labeled (this will satisfy markdownlint MD040 and improve rendering).
In `@plugins/node-team/skills/node/references/deployment/debug-binary/deploy.md`:
- Around line 10-13: The example uses StrictHostKeyChecking=no in the
scp/ProxyCommand which disables host-key verification; update the example to use
StrictHostKeyChecking=accept-new (or instruct using an explicitly managed
known_hosts) instead, and move StrictHostKeyChecking=no into a clearly labeled
"emergency workaround" note; adjust both the scp invocation and the ProxyCommand
string where StrictHostKeyChecking is referenced so they use accept-new by
default and document the no option only as a temporary fallback.
- Around line 59-61: The fenced code block containing the error message "Failed
to locate executable <path>: Permission denied" is unlabeled; update that fenced
block in deploy.md to use a language label of text (i.e., change ``` to ```text)
so markdownlint MD040 is satisfied and the docs remain lintable. Reference the
specific fenced snippet that contains the error message and only add the `text`
label—do not alter the message content.
In `@plugins/node-team/skills/node/references/development/mco-dev.md`:
- Around line 25-31: Update the doc to add explicit safety guardrails and a
deterministic rollback: mark the command as "DEV-ONLY" / for disposable clusters
only, capture the current replica count into PREV_REPLICAS using oc get on the
cluster-version-operator deployment before scaling, then run oc scale ...
--replicas=0, and include a restore command that scales back to
"${PREV_REPLICAS:-1}" to ensure exact restoration of the previous replica count;
reference PREV_REPLICAS, the cluster-version-operator deployment name, and the
oc scale/get commands in the updated instructions.
In `@plugins/node-team/skills/node/references/jira.md`:
- Around line 171-176: The code fence for the Carryover Detection pseudocode
lacks a language specifier; update the fenced block containing "sprints_carried
= count of items in customfield_10020 where state == \"closed\"" to use a
plaintext/text language tag (e.g., change ``` to ```text) so the pseudocode
(reference symbols: sprints_carried, customfield_10020) renders correctly.
- Around line 66-67: Replace the component label "Node / Numa aware Scheduling"
with the hyphenated and capitalized technical term "Node / NUMA-aware
Scheduling"; locate the string "Node / Numa aware Scheduling" in the file (e.g.,
within the list containing "Node, Node / CRI-O, Node / Kubelet, ...") and update
it to "Node / NUMA-aware Scheduling" to ensure correct terminology.
In `@plugins/node-team/skills/node/references/platform-docs.md`:
- Line 8: The doc currently instructs to list branches matching the pattern
`release-X.Y` and "sort, take last"/"take highest" without specifying
version-aware sorting, which can break for versions like `4.9` vs `4.10`; update
the text to explicitly require parsing the `X.Y` semantic version from branch
names (pattern `release-X.Y`) and performing a numeric/semver-aware sort (or
using a semver library) before selecting the latest, and replace ambiguous
phrases "sort, take last" / "take highest" with a clear statement like "perform
semver-aware sort on parsed X.Y and select the highest version."
---
Outside diff comments:
In `@plugins/node-team/skills/node/references/development/kueue-operator-dev.md`:
- Line 18: Remove the stray literal "18" from the markdown document (the
accidental artifact currently present as a standalone token) so the reference
doc renders correctly; edit the file containing the stray "18", delete that
token, and ensure surrounding markdown formatting/spacing is preserved (no other
content should be changed).
---
Nitpick comments:
In `@plugins/node-team/skills/node/references/development/worktrees.md`:
- Around line 7-11: The git submodule sync command uses git submodule foreach
with 'git merge --ff-only origin/main --quiet 2>/dev/null || true' which
silently swallows non-fast-forward failures; modify the foreach command handling
(the git submodule foreach line) to detect a non-fast-forward exit status and
emit a warning (e.g., print a message to stderr or stdout identifying the
submodule path) instead of simply swallowing the error with '|| true' so users
are informed when a submodule has diverged and was skipped.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21ed819d-e9a5-4c2f-94ad-113b7820cc1c
📒 Files selected for processing (23)
.claude-plugin/marketplace.jsonplugins/node-team/.claude-plugin/plugin.jsonplugins/node-team/config/team-roster-core.example.jsonplugins/node-team/config/team-roster-dra.example.jsonplugins/node-team/skills/node/SKILL.mdplugins/node-team/skills/node/references/INDEX.mdplugins/node-team/skills/node/references/SETUP.mdplugins/node-team/skills/node/references/deployment/debug-binary.mdplugins/node-team/skills/node/references/deployment/debug-binary/crio.mdplugins/node-team/skills/node/references/deployment/debug-binary/cross-compile.mdplugins/node-team/skills/node/references/deployment/debug-binary/deploy.mdplugins/node-team/skills/node/references/deployment/debug-binary/rollback.mdplugins/node-team/skills/node/references/deployment/debug-binary/ssh-bastion.mdplugins/node-team/skills/node/references/development/crio-dev.mdplugins/node-team/skills/node/references/development/crun-conmon.mdplugins/node-team/skills/node/references/development/kubelet-dev.mdplugins/node-team/skills/node/references/development/kueue-operator-dev.mdplugins/node-team/skills/node/references/development/mco-dev.mdplugins/node-team/skills/node/references/development/worktrees.mdplugins/node-team/skills/node/references/jira.mdplugins/node-team/skills/node/references/platform-docs.mdplugins/node-team/skills/node/references/prometheus.mdplugins/node-team/skills/node/references/support.md
✅ Files skipped from review due to trivial changes (11)
- plugins/node-team/config/team-roster-core.example.json
- plugins/node-team/config/team-roster-dra.example.json
- plugins/node-team/skills/node/references/INDEX.md
- plugins/node-team/skills/node/references/development/crio-dev.md
- plugins/node-team/skills/node/references/prometheus.md
- plugins/node-team/skills/node/references/deployment/debug-binary/cross-compile.md
- plugins/node-team/skills/node/references/deployment/debug-binary/rollback.md
- plugins/node-team/skills/node/references/support.md
- plugins/node-team/skills/node/references/development/kubelet-dev.md
- plugins/node-team/skills/node/references/deployment/debug-binary/ssh-bastion.md
- plugins/node-team/skills/node/references/SETUP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude-plugin/marketplace.json
- plugins/node-team/skills/node/SKILL.md
| { | ||
| "name": "node-team", | ||
| "description": "OpenShift Node team assistant for development, deployment, debugging, and workflow tasks across kubelet, MCO, CRI-O, crun, conmonrs, Kueue operator, Jira, Red Hat KB/support cases, Prometheus, and platform docs.", | ||
| "version": "0.12.0", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check marketplace.json for node-team plugin version
# Search for node-team entry in marketplace and extract version
rg -A 5 -B 2 '"node-team"' .claude-plugin/marketplace.jsonRepository: openshift-eng/ai-helpers
Length of output: 296
Synchronize version with marketplace.json.
The plugin declares version 0.12.0, but marketplace.json lists 0.11.0. This mismatch will cause inconsistent version display and registration issues. Update marketplace.json to version 0.12.0.
🤖 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/node-team/.claude-plugin/plugin.json` at line 4, The plugin.json
declares "version": "0.12.0" while marketplace.json still lists "0.11.0"; update
the "version" field in marketplace.json to "0.12.0" so both files match. Locate
the "version" key in marketplace.json (the counterpart to plugin.json's version)
and change its value to "0.12.0", then run any validation or lint steps you have
to ensure the manifest stays valid.
Comment on lines
+13
to
+15
| ``` | ||
| mount --bind /home/core/crio /usr/bin/crio | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify languages for fenced blocks in the overview doc.
Line 13 and Line 97 are unlabeled fenced blocks. Add bash for the bind-mount command and text for the ASCII diagram to resolve MD040.
Suggested patch
-```
+```bash
mount --bind /home/core/crio /usr/bin/crio- +text
Local Machine RHCOS Worker Node
───────────── ─────────────────
- Cross-compile in Docker
(linux/amd64, matching libs)
@@
oc adm uncordon
7. Verify: --version, node Ready
Also applies to: 97-113
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/node-team/skills/node/references/deployment/debug-binary.md` around
lines 13 - 15, The two unlabeled fenced code blocks in debug-binary.md should be
annotated with languages to satisfy MD040: add ```bash before the bind-mount
command line containing "mount --bind /home/core/crio /usr/bin/crio" and add
```text before the ASCII diagram block that begins with "Local Machine
RHCOS Worker Node" (the multi-line diagram spanning the steps) so the first
block is marked as bash and the diagram as text.
Comment on lines
+34
to
+40
| ``` | ||
| libseccomp.so.2 | ||
| libgpgme.so.11 | ||
| libassuan.so.0 | ||
| libgpg-error.so.0 | ||
| libc.so.6 | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced blocks (markdownlint MD040).
Line 34 and Line 50 use unlabeled fenced code blocks. Add a language tag (for example, text) to keep lint clean and improve rendering.
Suggested patch
-```
+```text
libseccomp.so.2
libgpgme.so.11
libassuan.so.0
libgpg-error.so.0
libc.so.6- +text
containers_image_ostree_stub
exclude_graphdriver_btrfs
btrfs_noversion
seccomp
selinux
Also applies to: 50-56
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/node-team/skills/node/references/deployment/debug-binary/crio.md`
around lines 34 - 40, Update the two unlabeled fenced code blocks by adding a
language identifier (e.g., text); specifically replace the opening ``` for the
block containing "libseccomp.so.2" (and its sibling lines) with ```text and do
the same for the block starting with "containers_image_ostree_stub" so both
blocks are labeled (this will satisfy markdownlint MD040 and improve rendering).
Comment on lines
+10
to
+13
| scp -i $SSH_KEY \ | ||
| -o StrictHostKeyChecking=no \ | ||
| -o ProxyCommand="ssh -i $SSH_KEY -A -o StrictHostKeyChecking=no -o ServerAliveInterval=30 -W %h:%p core@${BASTION_HOST}" \ | ||
| ./bin/<binary> core@${WORKER}:/home/core/<binary> |
Contributor
There was a problem hiding this comment.
Avoid documenting StrictHostKeyChecking=no as the default path.
Line 11 and Line 12 disable host-key verification, which normalizes MITM-prone SSH usage. Prefer accept-new (or explicitly managed known_hosts) in the main example, and keep no only as a clearly-labeled emergency workaround.
🤖 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/node-team/skills/node/references/deployment/debug-binary/deploy.md`
around lines 10 - 13, The example uses StrictHostKeyChecking=no in the
scp/ProxyCommand which disables host-key verification; update the example to use
StrictHostKeyChecking=accept-new (or instruct using an explicitly managed
known_hosts) instead, and move StrictHostKeyChecking=no into a clearly labeled
"emergency workaround" note; adjust both the scp invocation and the ProxyCommand
string where StrictHostKeyChecking is referenced so they use accept-new by
default and document the no option only as a temporary fallback.
Comment on lines
+59
to
+61
| ``` | ||
| Failed to locate executable <path>: Permission denied | ||
| ``` |
Contributor
There was a problem hiding this comment.
Label the fenced error snippet with a language.
Line 59 has an unlabeled fenced block; add text to satisfy markdownlint MD040 and keep docs lintable.
Suggested patch
-```
+```text
Failed to locate executable <path>: Permission denied</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/node-team/skills/node/references/deployment/debug-binary/deploy.md
around lines 59 - 61, The fenced code block containing the error message "Failed
to locate executable : Permission denied" is unlabeled; update that fenced
block in deploy.md to use a language label of text (i.e., change totext)
so markdownlint MD040 is satisfied and the docs remain lintable. Reference the
specific fenced snippet that contains the error message and only add the text
label—do not alter the message content.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+25
to
+31
| The Cluster Version Operator (CVO) will **revert manual image overrides** on MCO deployments. During development, scale down CVO: | ||
|
|
||
| ```bash | ||
| oc scale deployment cluster-version-operator -n openshift-cluster-version --replicas=0 | ||
| ``` | ||
|
|
||
| Remember to scale it back when done. |
Contributor
There was a problem hiding this comment.
Add explicit safety guardrails and a deterministic rollback for CVO scale-down.
The command on Line 28 is high-risk operationally; the section should explicitly restrict usage to disposable dev clusters and provide a concrete restore path (including restoring previous replica count), not just “scale it back.”
Suggested doc patch
## CVO Override Warning
The Cluster Version Operator (CVO) will **revert manual image overrides** on MCO deployments. During development, scale down CVO:
```bash
+# DEV-ONLY: use only on disposable development clusters, never on shared/prod.
+# Save current replica count first so it can be restored exactly.
+PREV_REPLICAS="$(oc get deployment cluster-version-operator -n openshift-cluster-version -o jsonpath='{.spec.replicas}')"
oc scale deployment cluster-version-operator -n openshift-cluster-version --replicas=0-Remember to scale it back when done.
+Restore CVO immediately after testing:
+
+bash +oc scale deployment cluster-version-operator -n openshift-cluster-version --replicas="${PREV_REPLICAS:-1}" +
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/node-team/skills/node/references/development/mco-dev.md around lines
25 - 31, Update the doc to add explicit safety guardrails and a deterministic
rollback: mark the command as "DEV-ONLY" / for disposable clusters only, capture
the current replica count into PREV_REPLICAS using oc get on the
cluster-version-operator deployment before scaling, then run oc scale ...
--replicas=0, and include a restore command that scales back to
"${PREV_REPLICAS:-1}" to ensure exact restoration of the previous replica count;
reference PREV_REPLICAS, the cluster-version-operator deployment name, and the
oc scale/get commands in the updated instructions.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+66
to
+67
|
|
||
| Node, Node / CRI-O, Node / Kubelet, Node / CPU manager, Node / Memory manager, Node / Topology manager, Node / Numa aware Scheduling, Node / Device Manager, Node / Pod resource API, Node / Node Problem Detector, Node / Kueue, Node / Instaslice-operator |
Contributor
There was a problem hiding this comment.
Hyphenate "NUMA-aware" for technical correctness.
The component name should read "NUMA-aware Scheduling" with a hyphen to follow standard technical terminology.
📝 Suggested fix
-Node, Node / CRI-O, Node / Kubelet, Node / CPU manager, Node / Memory manager, Node / Topology manager, Node / Numa aware Scheduling, Node / Device Manager, Node / Pod resource API, Node / Node Problem Detector, Node / Kueue, Node / Instaslice-operator
+Node, Node / CRI-O, Node / Kubelet, Node / CPU manager, Node / Memory manager, Node / Topology manager, Node / NUMA-aware Scheduling, Node / Device Manager, Node / Pod resource API, Node / Node Problem Detector, Node / Kueue, Node / Instaslice-operator📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Node, Node / CRI-O, Node / Kubelet, Node / CPU manager, Node / Memory manager, Node / Topology manager, Node / Numa aware Scheduling, Node / Device Manager, Node / Pod resource API, Node / Node Problem Detector, Node / Kueue, Node / Instaslice-operator | |
| Node, Node / CRI-O, Node / Kubelet, Node / CPU manager, Node / Memory manager, Node / Topology manager, Node / NUMA-aware Scheduling, Node / Device Manager, Node / Pod resource API, Node / Node Problem Detector, Node / Kueue, Node / Instaslice-operator |
🧰 Tools
🪛 LanguageTool
[grammar] ~67-~67: Use a hyphen to join words.
Context: ...er, Node / Topology manager, Node / Numa aware Scheduling, Node / Device Manager,...
(QB_NEW_EN_HYPHEN)
🤖 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/node-team/skills/node/references/jira.md` around lines 66 - 67,
Replace the component label "Node / Numa aware Scheduling" with the hyphenated
and capitalized technical term "Node / NUMA-aware Scheduling"; locate the string
"Node / Numa aware Scheduling" in the file (e.g., within the list containing
"Node, Node / CRI-O, Node / Kubelet, ...") and update it to "Node / NUMA-aware
Scheduling" to ensure correct terminology.
Comment on lines
+171
to
+176
| ## Carryover Detection | ||
|
|
||
| Count closed sprints in `customfield_10020` array to detect carryovers: | ||
| ``` | ||
| sprints_carried = count of items in customfield_10020 where state == "closed" | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifier to code fence.
The code block should include a language identifier for proper rendering and clarity. Since this is pseudocode/pattern documentation rather than executable code, use text or plaintext.
📝 Suggested fix
## Carryover Detection
Count closed sprints in `customfield_10020` array to detect carryovers:
-```
+```text
sprints_carried = count of items in customfield_10020 where state == "closed"</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
## Carryover Detection
Count closed sprints in `customfield_10020` array to detect carryovers:
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/node-team/skills/node/references/jira.md` around lines 171 - 176, The
code fence for the Carryover Detection pseudocode lacks a language specifier;
update the fenced block containing "sprints_carried = count of items in
customfield_10020 where state == \"closed\"" to use a plaintext/text language
tag (e.g., change ``` to ```text) so the pseudocode (reference symbols:
sprints_carried, customfield_10020) renders correctly.
| ## Kubernetes | ||
|
|
||
| - Repo: `kubernetes/website`, path: `content/en/docs/` | ||
| - Versioning: git branches named `release-X.Y` — discover latest by listing branches, grep `^release-`, sort, take last |
Contributor
There was a problem hiding this comment.
Use version-aware sorting when selecting the “latest” docs version.
Line 8 and Line 16 currently describe “sort/take last” and “take highest” without specifying version-aware sort semantics. That can select the wrong release if implemented lexicographically (for example, 4.9 vs 4.10).
Suggested doc tweak
-- Versioning: git branches named `release-X.Y` — discover latest by listing branches, grep `^release-`, sort, take last
+- Versioning: git branches named `release-X.Y` — discover latest by listing branches, grep `^release-`, then use **version-aware sorting** (e.g., `sort -V`) and take last
...
-- Versions are directories (e.g. `4.22`) — discover latest by listing `docs/`, filter numeric names, take highest
+- Versions are directories (e.g. `4.22`) — discover latest by listing `docs/`, filter numeric names, then use **version-aware sorting** (e.g., `sort -V`) and take highestAlso applies to: 16-16
🤖 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/node-team/skills/node/references/platform-docs.md` at line 8, The doc
currently instructs to list branches matching the pattern `release-X.Y` and
"sort, take last"/"take highest" without specifying version-aware sorting, which
can break for versions like `4.9` vs `4.10`; update the text to explicitly
require parsing the `X.Y` semantic version from branch names (pattern
`release-X.Y`) and performing a numeric/semver-aware sort (or using a semver
library) before selecting the latest, and replace ambiguous phrases "sort, take
last" / "take highest" with a clear statement like "perform semver-aware sort on
parsed X.Y and select the highest version."
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
node-teamplugin — an OpenShift Node team assistant for development, deployment, debugging, and workflow tasksStatus
WIP — initial drop of the plugin. Feedback welcome on structure and conventions.
Test plan
claude plugin listnodeplugin🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation