Add MCO QE tooling: Polarion TC creation from GitHub PRs and Jira#6149
Open
ptalgulk01 wants to merge 1 commit into
Open
Add MCO QE tooling: Polarion TC creation from GitHub PRs and Jira#6149ptalgulk01 wants to merge 1 commit into
ptalgulk01 wants to merge 1 commit into
Conversation
Adds Claude Code commands, skills, and Python scripts to automate MCO QE workflows for creating and fetching Polarion test cases. Commands: - /mco-fetch: fetch Polarion TCs, GitHub PR comments, Jira issues - /mco-create-tc-from-pr: create TC from GitHub PR QE comment/review - /mco-create-tc-from-jira: create TC from Jira issue/comment - /automate-test: generate Go e2e test from Polarion spec (updated) Scripts (.claude/scripts/): - fetch_polarion.py, fetch_github_pr.py, fetch_jira.py - create_polarion_tc.py, create_tc_hybrid.py - update_polarion_tc.py (called internally; /mco-update-tc is future scope) - validate_tc.py, polarion_client.py, setup.sh Key rules enforced: - Validate → confirm → create (never create without user confirmation) - Always patch fields + steps via update_polarion_tc.py after creation (creation PATCH fails silently; fields may need manual Polarion fix) - Verbatim code blocks; no heredoc in expected results - STOP if parser returns 0 steps; never invent steps Auth: .env at repo root (gitignored); .env.example committed. Jira: MCO-2220 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThis PR introduces a complete MCO QE tooling system for managing Polarion test cases. It adds comprehensive documentation, Python scripts for fetching and creating test cases from GitHub PRs and Jira, a Polarion HTTP client supporting REST and SOAP, validation logic, configuration templates, and a setup script to bootstrap the environment. ChangesMCO QE Tooling
🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
.claude/commands/mco-create-tc-from-pr.md-10-12 (1)
10-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpecify languages for unlabeled fenced code blocks.
Add
text/bashwhere appropriate to satisfy MD040 consistently.Also applies to: 78-84
🤖 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 @.claude/commands/mco-create-tc-from-pr.md around lines 10 - 12, The markdown fenced code blocks containing the command examples (e.g., the snippet showing `/mco-create-tc-from-pr <PR-URL>`) need explicit language identifiers to satisfy MD040; update those fences to use an appropriate language tag such as "bash" or "text" (for example change ``` to ```bash or ```text) for the snippet in this file and the other similar fenced blocks referenced around the second occurrence (the block covering lines 78-84) so all unlabeled code fences are consistently labeled..claude/scripts/create_tc_hybrid.py-163-165 (1)
163-165:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidation failure output is mostly dropped.
validate_tc.pyprints its report to stdout, but this branch prints onlyvalidate_result.stderr, so users lose actionable failure details.🤖 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 @.claude/scripts/create_tc_hybrid.py around lines 163 - 165, The validation branch currently prints only validate_result.stderr so the useful report emitted by validate_tc.py (on stdout) is lost; update the failing branch that calls validate_result to print validate_result.stdout (or both validate_result.stdout and validate_result.stderr) to stderr before exiting so users see the full validation report—locate the print statements around validate_result in create_tc_hybrid.py and replace or augment printing validate_result.stderr with printing validate_result.stdout (and preserve stderr output as well)..claude/commands/mco-fetch.md-13-17 (1)
13-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language to the Usage code fence.
This resolves MD040 and keeps docs lint-clean.
🤖 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 @.claude/commands/mco-fetch.md around lines 13 - 17, The fenced code block showing the example commands (the lines starting with "/mco-fetch tc OCP-88122", "/mco-fetch pr 5691 [comment-id]", "/mco-fetch jira OCPBUGS-74223") is missing a language tag; update the opening triple-backtick fence to include a language (e.g., ```bash or ```sh) so the block is lint-clean and resolves MD040..claude/commands/mco-create-tc-from-jira.md-10-12 (1)
10-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tags to unlabeled code fences.
Use
textfor slash-command snippets andtext/bashfor heredoc replacement blocks to clear MD040.Also applies to: 80-86
🤖 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 @.claude/commands/mco-create-tc-from-jira.md around lines 10 - 12, The unlabeled code fence containing the slash-command snippet "/mco-create-tc-from-jira <JIRA-URL>" and the heredoc replacement blocks should have language tags added to satisfy MD040: add "text" to the single-line slash-command fence and add "text" or "bash" (as appropriate) to the heredoc replacement code fences referenced around the heredoc blocks (also update the similar fences at lines indicated for the other heredoc snippets, e.g., the blocks around lines 80-86) so each triple-backtick fence includes an explicit language tag..claude/scripts/validate_tc.py-242-244 (1)
242-244:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSample draft creation can fail when
/tmp/test-specsdoesn’t exist.Line 243 writes directly to a nested tmp path without ensuring the parent directory exists.
Suggested fix
+import os @@ filepath = '/tmp/test-specs/sample_draft.json' + os.makedirs(os.path.dirname(filepath), exist_ok=True) with open(filepath, 'w') as f: json.dump(sample, f, indent=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 @.claude/scripts/validate_tc.py around lines 242 - 244, The write to filepath = '/tmp/test-specs/sample_draft.json' can fail if the parent directory doesn't exist; before opening the file, ensure the parent directory exists by creating os.path.dirname(filepath) with os.makedirs(..., exist_ok=True) (and add an import for os if missing), then proceed to open and json.dump(sample, ...)..claude/README.md-165-170 (1)
165-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix broken documentation references in .claude/README.md (lines 165–170)
The QUICKSTART references these files, but they don’t exist in the repo:
.claude/FETCH_USAGE.md.claude/CREATE_TC_USAGE.md.claude/UPDATE_TC_USAGE.md.claude/COMPLETE_WORKFLOW.md.claude/FINAL_SUMMARY.mdAdd the missing docs or update the references to point to the correct existing content.
🤖 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 @.claude/README.md around lines 165 - 170, README references to .claude/FETCH_USAGE.md, .claude/CREATE_TC_USAGE.md, .claude/UPDATE_TC_USAGE.md, .claude/COMPLETE_WORKFLOW.md, and .claude/FINAL_SUMMARY.md are broken; either add those missing markdown files with the appropriate quickstart content or update the links in .claude/README.md to point to the actual existing docs. Locate the QUICKSTART block in .claude/README.md and for each referenced filename either (A) create a corresponding file with the expected usage/test-step content and a brief header, or (B) replace the link target with the correct existing file names (or inline the content into the README) so the references resolve; ensure the link text and file names match exactly..claude/README.md-248-248 (1)
248-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix broken PDF example references in
.claude/README.md.
.claude/README.mdreferenceswrong.pdfandOCP-88122.pdf“in repo root” (e.g., around line 248), but neither file exists anywhere in the repository, so the examples will be unusable. Update the doc to point to the actual location for these artifacts or include the PDFs in the repo/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 @.claude/README.md at line 248, The README references non-existent artifacts named "wrong.pdf" and "OCP-88122.pdf" in .claude/README.md; either update those references to point to the actual locations (e.g., correct relative paths, external URLs, or an artifacts directory), or add the two PDF files to the repo/PR and update the README to reference their repo paths; ensure the references to "wrong.pdf" and "OCP-88122.pdf" in .claude/README.md are accurate and accessible..claude/scripts/setup.sh-102-105 (1)
102-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrint the actual setup script path.
Lines 105 and 197 tell users to run
./scripts/setup.sh --verify, but this PR adds.claude/scripts/setup.sh. Following the printed command from repo root will fail.Also applies to: 195-198
🤖 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 @.claude/scripts/setup.sh around lines 102 - 105, Update the printed next steps so the path points to the new script location: replace the echo that prints "./scripts/setup.sh --verify" with ".claude/scripts/setup.sh --verify" (and update the matching echo on the other occurrence) so the "⚠️ NEXT STEPS:" message reflects the actual script path; locate the echo statements that output the run command string and change the path text accordingly.
🧹 Nitpick comments (5)
.claude/skills/mco-create-tc-workflow.md (2)
171-216: 💤 Low valueConsider documenting loop limit rationale.
The step deletion loop has a hardcoded limit of 10 attempts (line 205). For test cases with more than 10 steps, this could leave orphaned steps. Consider either:
- Increasing the limit (e.g., 50 or 100)
- Making it dynamic based on the number of steps to upload
- Adding a check after the loop to verify all steps were deleted
💡 Suggested improvement
-# Loop-delete: Polarion re-indexes IDs after each delete, so fetch fresh each round -for attempt in range(10): +# Loop-delete: Polarion re-indexes IDs after each delete, so fetch fresh each round +# Use generous limit to handle TCs with many steps +for attempt in range(50): result = client._make_request("GET", f"projects/OSE/workitems/{TC_ID}/teststeps") remaining = result.get("data", []) if not remaining: break for s in remaining: sid = s.get('id', '').split('/')[-1] client._make_request("DELETE", f"projects/OSE/workitems/{TC_ID}/teststeps/{sid}") time.sleep(0.5) + +# Verify all steps deleted +if remaining: + print(f"⚠️ Warning: {len(remaining)} steps remain after 50 attempts", file=sys.stderr)🤖 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 @.claude/skills/mco-create-tc-workflow.md around lines 171 - 216, The hardcoded retry limit in the deletion loop (for attempt in range(10)) can leave orphaned steps; update the loop around client._make_request DELETE calls (and the surrounding loop that reads result = client._make_request("GET", f"projects/OSE/workitems/{TC_ID}/teststeps")) to use a dynamic limit or higher cap (e.g., compute from initial remaining count or use 100) and add an explicit post-loop verification that remaining is empty—if not, log/raise an error so callers know deletion failed; keep references to load_env, TC_ID, and client._make_request when making changes.
27-41: ⚡ Quick winGitHub API authentication missing in example.
The Python snippet for fetching PR reviews uses unauthenticated requests, which are subject to GitHub's 60 requests/hour rate limit. For production use, consider adding authentication via
GITHUB_TOKENenvironment variable.🔐 Suggested enhancement
+import os import urllib.request, json def fetch(url): + headers = { + "Accept": "application/vnd.github.v3+json", + "User-Agent": "mco-tc-script" + } + # Use token if available to avoid rate limits + token = os.environ.get('GITHUB_TOKEN') + if token: + headers["Authorization"] = f"token {token}" + req = urllib.request.Request(url, headers={ - "Accept": "application/vnd.github.v3+json", - "User-Agent": "mco-tc-script" + **headers }) return json.loads(urllib.request.urlopen(req).read())🤖 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 @.claude/skills/mco-create-tc-workflow.md around lines 27 - 41, The example uses unauthenticated GitHub API calls in the fetch function which hit low rate limits; update fetch to read a GITHUB_TOKEN from environment and add an Authorization: token <GITHUB_TOKEN> header (and keep Accept/User-Agent) so requests are authenticated, and handle the missing token case by either raising a clear error or falling back with a warning; modify the fetch function and its header construction (refer to function name fetch and the existing header dict) so PR and reviews calls use the token..claude/skills/mco-automate-test-workflow.md (1)
33-36: 💤 Low valueConsider explicit validation script invocation.
The validation step states "Ensure test steps have commands (not just narrative)" but doesn't specify whether this is a manual check or should run
validate_tc.py. For consistency with the create-tc workflow (which explicitly runsvalidate_tc.py), consider clarifying whether automated validation is expected here.📝 Suggested clarification
3. **Validate**: + ```bash + python3 .claude/scripts/validate_tc.py /tmp/test-specs/<id>.txt + ``` - - Ensure test steps have commands (not just narrative) + - Verify test steps have commands (not just narrative) - Verify required fields present (Component, Sub Team, Version)Or alternatively, if manual validation is intended:
3. **Validate** (manual check): - Ensure test steps have commands (not just narrative) - Verify required fields present (Component, Sub Team, Version)🤖 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 @.claude/skills/mco-automate-test-workflow.md around lines 33 - 36, Clarify the "Validate" step by explicitly stating whether automated validation should run the existing script or remain manual: if automated, add a line invoking python3 .claude/scripts/validate_tc.py against the generated spec (e.g., `python3 .claude/scripts/validate_tc.py /tmp/test-specs/<id>.txt`) and change "Ensure test steps have commands (not just narrative)" to "Verify test steps have commands (not just narrative)"; if manual, add a note that reviewers must perform this check and reference the create-tc workflow as an example. Include the script name validate_tc.py and the create-tc workflow reference to make the expected action unambiguous..env.example (1)
29-29: ⚡ Quick winAdd quotes around multi-word values.
The value
Machine Config Operatorcontains spaces but is not quoted. While some.envparsers handle this, it's safer to quote multi-word values for consistency and portability.📝 Suggested fix
# MCO Test Defaults -MCO_COMPONENT=Machine Config Operator +MCO_COMPONENT="Machine Config Operator" MCO_SUB_TEAM=MCO MCO_TEST_TYPE=Functional MCO_PRODUCTS=OCP🤖 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 @.env.example at line 29, The MCO_COMPONENT environment value is a multi-word string and should be quoted for portability; update the .env example by changing the MCO_COMPONENT assignment to include quotes around "Machine Config Operator" so the variable MCO_COMPONENT is defined as MCO_COMPONENT="Machine Config Operator"..claude/TESTING.md (1)
126-130: ⚡ Quick winSimplify or remove the unsafe Python one-liner.
The test command attempts to manipulate
sys.pathand execute a file directly, which is complex and could be fragile.Consider a simpler approach:
🔄 Suggested alternative
### Missing Dependencies ```bash -# Simulate missing requests library -python3 -c "import sys; sys.path = [p for p in sys.path if 'site-packages' not in p]; exec(open('.claude/scripts/fetch_polarion.py').read())" +# Test graceful handling when requests library is missing +# (Remove requests temporarily or test in clean venv) +python3 -c "import requests" || echo "Expected: requests not installed" +python3 .claude/scripts/fetch_polarion.py OCP-88122-Expected: Clear error about missing
requestslibrary
+Expected: Script detects missing dependency and provides helpful error messageThe current one-liner is difficult to maintain and doesn't reliably simulate the missing dependency scenario.
🤖 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 @.claude/TESTING.md around lines 126 - 130, Replace the fragile sys.path hack in the TESTING.md example with a simpler, safer test for the missing requests dependency: remove the python -c exec one-liner and instead show a reproducible approach such as verifying import requests fails (or instructing to use a clean virtualenv) and then running .claude/scripts/fetch_polarion.py with a sample ticket ID (e.g., OCP-88122) to confirm the script emits a clear dependency error; update the "Expected" text to say the script detects the missing dependency and prints a helpful error message.
🤖 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 @.claude/commands/mco-create-tc-from-jira.md:
- Around line 130-141: The Step 7 example is inconsistent: it says "Always patch
fields + steps separately" but the shown command to run update_polarion_tc.py
only patches fields; add the steps argument to the example and clarify usage of
the --steps-file option. Update the Step 7 documentation to call the script
update_polarion_tc.py with both the field flags (e.g., --component, --sub-team,
--products, --test-type, --version, --trello-jira) and the --steps-file
"<PATH_TO_STEPS>" parameter so the command actually patches test steps as well.
In @.claude/scripts/create_polarion_tc.py:
- Around line 151-154: The code currently prints warnings when patch_result or
step upload fails but still returns a success status, so update the function
that builds and returns the result (the dict containing "status") to reflect
errors: check patch_result for "error" and the step upload result (e.g.,
step_upload_result or similar variable used in the 187-200 block) and if either
contains an error set the returned "status" to "failed" (or include a boolean
success flag) and include the error details in the return payload; ensure you
change both the custom-field patch branch (where patch_result and custom_fields
are used) and the step-upload branch to aggregate errors instead of only
printing warnings so downstream automation can reliably detect incomplete TC
creation.
- Around line 45-71: validate_draft currently only verifies that test_steps
exists and therefore allows drafts that the canonical validator (validate_tc.py)
would reject; update validate_draft to replicate the per-step validation from
validate_tc.py: iterate draft['test_steps'] and verify each step contains
required fields (e.g., 'action' and 'expected' or whatever the canonical
validator requires), enforce narrative-only constraints (disallow or flag steps
missing both action and expected unless marked as narrative), validate step
ordering/position if the canonical logic does, and append corresponding errors
to errors[] so the function returns False for invalid steps; reference
validate_draft, test_steps, and mirror the checks implemented in validate_tc.py
to ensure parity.
In @.claude/scripts/create_tc_hybrid.py:
- Around line 174-177: The dry-run subprocess invocation in create_tc_hybrid.py
currently ignores the child's return code; change the subprocess.run call that
executes ['python3', 'create_polarion_tc.py', draft_file, '--dry-run'] (in
create_tc_hybrid.py) to capture its CompletedProcess result and propagate
non-zero return codes (e.g., check result.returncode and
sys.exit(result.returncode) or raise SystemExit) so the script exits with the
child's error code when the dry-run fails.
- Around line 170-185: The script currently proceeds to call
subprocess.run(['python3', 'create_polarion_tc.py', draft_file]) when
args.dry_run is false, skipping the “confirm” step; modify the flow in the main
block where args.dry_run is checked to require an explicit confirmation before
invoking subprocess.run (either by adding support for an args.yes/--yes boolean
flag on the argparse parser or by prompting the user with input() and requiring
a "y/yes" response), and only call subprocess.run(...) and
sys.exit(result.returncode) if confirmation is received; reference the
args.dry_run conditional and the subprocess.run invocation for where to insert
the confirmation logic.
In @.claude/scripts/fetch_github_pr.py:
- Around line 310-323: The code currently always writes the parsed draft to
disk; add a fail-fast guard before the "Write draft" block that checks the
parsed draft's steps and aborts when none were parsed. After calling
parse_qe_comment (function parse_qe_comment) inspect the draft for the test
steps (e.g., draft.get("steps") or draft["steps"]) and if that list is empty,
log an error and exit non‑zero (or raise) instead of proceeding to open
output_file and json.dump; only proceed to create output_dir/open output_file
and write when there is at least one parsed step.
- Around line 98-105: The current Environment Setup capture regex (env_match)
stops at the next "Word:" field because it treats any inline field like
"Platform:" as a section boundary; change the lookahead so it only stops at a
real section header at the start of a line (optional '##' + capitalized header
name + ':'), e.g. replace the lookahead (?=\n(?:##\s*)?\w+:|\Z) with a
start-of-line anchored pattern using re.MULTILINE such as
(?=\n(?:##\s*)?[A-Z][A-Za-z0-9 _-]*:\s*\n|\Z) and apply re.MULTILINE in the
flags when compiling the env_match regex (update both regex attempts), so fields
like "Version:" and "Platform:" inside the Environment Setup block are captured
instead of terminating the match.
- Around line 62-82: fetch_pr_comments currently only requests a single page
from /issues/{pr_number}/comments and therefore misses paginated issue comments
and PR review comments; update fetch_pr_comments to iterate with per_page (e.g.,
100) and page params in a loop to collect all pages from the issues comments
endpoint, and additionally make one or two extra requests to collect review data
from /pulls/{pr_number}/reviews (and/or /pulls/{pr_number}/comments if you need
inline review comments), then merge the returned lists into a single List[Dict]
before returning; preserve the existing auth flow (including the 401 fallback
that removes Authorization) but ensure pagination loops repeat the
unauthenticated retry if needed and that response.raise_for_status is still used
for errors.
In @.claude/scripts/fetch_polarion.py:
- Around line 255-259: The PolarionClient is constructed without SOAP
credentials so the SOAP fallback used by _soap_get_test_steps() is unreachable;
update the PolarionClient(...) call (the instance created with polarion_url and
polarion_token) to also accept and pass through the SOAP credentials read from
the environment (e.g., polarion_username and polarion_password) so
_soap_get_test_steps() can authenticate and return steps for SOAP-only test
cases.
In @.claude/scripts/polarion_client.py:
- Around line 186-188: Replace the unsafe
xml.etree.ElementTree.fromstring(response.text) call used when parsing the SOAP
“get test steps” response: import and use defusedxml.ElementTree.fromstring
instead of xml.etree.ElementTree to prevent XXE/DoS attacks (locate the parsing
site where `root = ET.fromstring(response.text)` and the `ET` import), and
update the module imports to `import defusedxml.ElementTree as ET` (or
equivalent) for the function that handles the SOAP response; also ensure the
defusedxml package is added to the project dependencies so this script can
import it at runtime.
In @.claude/scripts/setup.sh:
- Around line 125-128: The script currently executes source "$REPO_ROOT/.env"
(bracketed by set -a / set +a) which can run arbitrary code; replace that
sourcing with a safe key/value parser that reads the .env file and exports only
well-formed VAR=VALUE lines (ignore/skip blank lines, comments, and lines with
shell metacharacters) instead of executing them; update the block that currently
contains set -a / source "$REPO_ROOT/.env" / set +a to call or inline a parser
(e.g., a function like parse_env_file) that validates lines and exports
variables securely.
In @.claude/scripts/update_polarion_tc.py:
- Around line 203-233: fix_metadata_fields currently builds defaults and passes
them to update_test_case_fields unconditionally, which can overwrite existing
non-empty metadata; instead, fetch the test case's current metadata via the
PolarionClient (e.g., use client.get_test_case or equivalent to retrieve current
fields for tc_id/project), then only include a field in the updates dict if the
existing value is empty/blank and a default is provided (leave version and
trello_jira untouched unless explicitly passed), and finally call
update_test_case_fields(client, tc_id, project, **updates) with that filtered
updates dict so non-empty fields are not overwritten by defaults.
In @.claude/scripts/validate_tc.py:
- Around line 152-155: The strict flag isn't causing warnings to be treated as
errors because warnings are still appended to self.warnings and the exit code
only checks self.errors; update the logic so that when self.strict is true
warnings are escalated: either append to self.errors instead of self.warnings in
the places that currently do "if self.strict: self.errors.append(msg) else:
self.warnings.append(msg)" or, at program exit where return/exit code is
computed, compute effective_errors = len(self.errors) + (len(self.warnings) if
self.strict else 0) and use effective_errors to decide non-zero exit; refer to
self.strict, self.errors, self.warnings and the exit/return code block currently
around the lines that check errors for non-zero exit.
- Around line 44-45: The validator in .claude/scripts/validate_tc.py currently
allows heredoc-style snippets like "cat <<EOF" (seen in the diff r'cat <<EOF')
even though workflow rules forbid heredocs; update the validation logic (in the
function that classifies command-like content—e.g., the validator function
handling command patterns / is_command_like or validate_test_case content) to
explicitly detect and reject heredoc tokens such as '<<' or '<<EOF' anywhere in
command strings and raise a validation error; ensure the same check is applied
to all places where command-like blocks are validated (the code paths covering
the ranges noted: ~lines 123-156 and 221-230) so heredoc usage is blocked and
reported as invalid.
---
Minor comments:
In @.claude/commands/mco-create-tc-from-jira.md:
- Around line 10-12: The unlabeled code fence containing the slash-command
snippet "/mco-create-tc-from-jira <JIRA-URL>" and the heredoc replacement blocks
should have language tags added to satisfy MD040: add "text" to the single-line
slash-command fence and add "text" or "bash" (as appropriate) to the heredoc
replacement code fences referenced around the heredoc blocks (also update the
similar fences at lines indicated for the other heredoc snippets, e.g., the
blocks around lines 80-86) so each triple-backtick fence includes an explicit
language tag.
In @.claude/commands/mco-create-tc-from-pr.md:
- Around line 10-12: The markdown fenced code blocks containing the command
examples (e.g., the snippet showing `/mco-create-tc-from-pr <PR-URL>`) need
explicit language identifiers to satisfy MD040; update those fences to use an
appropriate language tag such as "bash" or "text" (for example change ``` to
```bash or ```text) for the snippet in this file and the other similar fenced
blocks referenced around the second occurrence (the block covering lines 78-84)
so all unlabeled code fences are consistently labeled.
In @.claude/commands/mco-fetch.md:
- Around line 13-17: The fenced code block showing the example commands (the
lines starting with "/mco-fetch tc OCP-88122", "/mco-fetch pr 5691
[comment-id]", "/mco-fetch jira OCPBUGS-74223") is missing a language tag;
update the opening triple-backtick fence to include a language (e.g., ```bash or
```sh) so the block is lint-clean and resolves MD040.
In @.claude/README.md:
- Around line 165-170: README references to .claude/FETCH_USAGE.md,
.claude/CREATE_TC_USAGE.md, .claude/UPDATE_TC_USAGE.md,
.claude/COMPLETE_WORKFLOW.md, and .claude/FINAL_SUMMARY.md are broken; either
add those missing markdown files with the appropriate quickstart content or
update the links in .claude/README.md to point to the actual existing docs.
Locate the QUICKSTART block in .claude/README.md and for each referenced
filename either (A) create a corresponding file with the expected
usage/test-step content and a brief header, or (B) replace the link target with
the correct existing file names (or inline the content into the README) so the
references resolve; ensure the link text and file names match exactly.
- Line 248: The README references non-existent artifacts named "wrong.pdf" and
"OCP-88122.pdf" in .claude/README.md; either update those references to point to
the actual locations (e.g., correct relative paths, external URLs, or an
artifacts directory), or add the two PDF files to the repo/PR and update the
README to reference their repo paths; ensure the references to "wrong.pdf" and
"OCP-88122.pdf" in .claude/README.md are accurate and accessible.
In @.claude/scripts/create_tc_hybrid.py:
- Around line 163-165: The validation branch currently prints only
validate_result.stderr so the useful report emitted by validate_tc.py (on
stdout) is lost; update the failing branch that calls validate_result to print
validate_result.stdout (or both validate_result.stdout and
validate_result.stderr) to stderr before exiting so users see the full
validation report—locate the print statements around validate_result in
create_tc_hybrid.py and replace or augment printing validate_result.stderr with
printing validate_result.stdout (and preserve stderr output as well).
In @.claude/scripts/setup.sh:
- Around line 102-105: Update the printed next steps so the path points to the
new script location: replace the echo that prints "./scripts/setup.sh --verify"
with ".claude/scripts/setup.sh --verify" (and update the matching echo on the
other occurrence) so the "⚠️ NEXT STEPS:" message reflects the actual script
path; locate the echo statements that output the run command string and change
the path text accordingly.
In @.claude/scripts/validate_tc.py:
- Around line 242-244: The write to filepath =
'/tmp/test-specs/sample_draft.json' can fail if the parent directory doesn't
exist; before opening the file, ensure the parent directory exists by creating
os.path.dirname(filepath) with os.makedirs(..., exist_ok=True) (and add an
import for os if missing), then proceed to open and json.dump(sample, ...).
---
Nitpick comments:
In @.claude/skills/mco-automate-test-workflow.md:
- Around line 33-36: Clarify the "Validate" step by explicitly stating whether
automated validation should run the existing script or remain manual: if
automated, add a line invoking python3 .claude/scripts/validate_tc.py against
the generated spec (e.g., `python3 .claude/scripts/validate_tc.py
/tmp/test-specs/<id>.txt`) and change "Ensure test steps have commands (not just
narrative)" to "Verify test steps have commands (not just narrative)"; if
manual, add a note that reviewers must perform this check and reference the
create-tc workflow as an example. Include the script name validate_tc.py and the
create-tc workflow reference to make the expected action unambiguous.
In @.claude/skills/mco-create-tc-workflow.md:
- Around line 171-216: The hardcoded retry limit in the deletion loop (for
attempt in range(10)) can leave orphaned steps; update the loop around
client._make_request DELETE calls (and the surrounding loop that reads result =
client._make_request("GET", f"projects/OSE/workitems/{TC_ID}/teststeps")) to use
a dynamic limit or higher cap (e.g., compute from initial remaining count or use
100) and add an explicit post-loop verification that remaining is empty—if not,
log/raise an error so callers know deletion failed; keep references to load_env,
TC_ID, and client._make_request when making changes.
- Around line 27-41: The example uses unauthenticated GitHub API calls in the
fetch function which hit low rate limits; update fetch to read a GITHUB_TOKEN
from environment and add an Authorization: token <GITHUB_TOKEN> header (and keep
Accept/User-Agent) so requests are authenticated, and handle the missing token
case by either raising a clear error or falling back with a warning; modify the
fetch function and its header construction (refer to function name fetch and the
existing header dict) so PR and reviews calls use the token.
In @.claude/TESTING.md:
- Around line 126-130: Replace the fragile sys.path hack in the TESTING.md
example with a simpler, safer test for the missing requests dependency: remove
the python -c exec one-liner and instead show a reproducible approach such as
verifying import requests fails (or instructing to use a clean virtualenv) and
then running .claude/scripts/fetch_polarion.py with a sample ticket ID (e.g.,
OCP-88122) to confirm the script emits a clear dependency error; update the
"Expected" text to say the script detects the missing dependency and prints a
helpful error message.
In @.env.example:
- Line 29: The MCO_COMPONENT environment value is a multi-word string and should
be quoted for portability; update the .env example by changing the MCO_COMPONENT
assignment to include quotes around "Machine Config Operator" so the variable
MCO_COMPONENT is defined as MCO_COMPONENT="Machine Config Operator".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: da26e24c-e329-4d1a-a34a-47eeb715b61c
📒 Files selected for processing (25)
.claude/00_START_HERE.md.claude/JIRA_INTEGRATION.md.claude/PILOT_PR5691.md.claude/QUICKSTART.md.claude/README.md.claude/TESTING.md.claude/TEST_FETCH.md.claude/commands/automate-test.md.claude/commands/mco-create-tc-from-jira.md.claude/commands/mco-create-tc-from-pr.md.claude/commands/mco-fetch.md.claude/scripts/create_polarion_tc.py.claude/scripts/create_tc_hybrid.py.claude/scripts/fetch_github_pr.py.claude/scripts/fetch_jira.py.claude/scripts/fetch_polarion.py.claude/scripts/polarion_client.py.claude/scripts/setup.sh.claude/scripts/update_polarion_tc.py.claude/scripts/validate_tc.py.claude/skills/mco-automate-test-workflow.md.claude/skills/mco-create-tc-workflow.md.claude/skills/mco-fetch-workflow.md.env.example.gitignore
Comment on lines
+130
to
+141
| ### Step 7: Always patch fields + steps separately | ||
|
|
||
| Even if creation reports success, always run: | ||
| ```bash | ||
| python3 .claude/scripts/update_polarion_tc.py OCP-XXXXX \ | ||
| --component "Machine Config Operator" \ | ||
| --sub-team "MCO" \ | ||
| --products "OCP" \ | ||
| --test-type "Functional" \ | ||
| --version "<VERSION>" \ | ||
| --trello-jira "<JIRA>" | ||
| ``` |
There was a problem hiding this comment.
Step 7 says “fields + steps” but command patches only fields.
The example omits --steps-file, so it does not actually perform the documented “+ steps” part. This creates an inconsistent workflow and can leave step updates unapplied.
🤖 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 @.claude/commands/mco-create-tc-from-jira.md around lines 130 - 141, The Step
7 example is inconsistent: it says "Always patch fields + steps separately" but
the shown command to run update_polarion_tc.py only patches fields; add the
steps argument to the example and clarify usage of the --steps-file option.
Update the Step 7 documentation to call the script update_polarion_tc.py with
both the field flags (e.g., --component, --sub-team, --products, --test-type,
--version, --trello-jira) and the --steps-file "<PATH_TO_STEPS>" parameter so
the command actually patches test steps as well.
Comment on lines
+45
to
+71
| def validate_draft(draft: Dict) -> bool: | ||
| """Run validation checks (same as validate_tc.py)""" | ||
| import re | ||
|
|
||
| errors = [] | ||
|
|
||
| # Check title format | ||
| if not re.match(r'^\[MCO\]\[.+\] .+', draft.get('title', '')): | ||
| errors.append(f"Title must match [MCO][TICKET-ID] Description: {draft.get('title', '')}") | ||
|
|
||
| # Check required fields | ||
| required = ['component', 'sub_team', 'products', 'test_type', 'version', 'trello_jira'] | ||
| for field in required: | ||
| if not draft.get(field): | ||
| errors.append(f"Required field '{field}' is empty") | ||
|
|
||
| # Check test steps | ||
| if not draft.get('test_steps'): | ||
| errors.append("No test steps defined") | ||
|
|
||
| if errors: | ||
| print("❌ VALIDATION FAILED\n", file=sys.stderr) | ||
| for err in errors: | ||
| print(f" - {err}", file=sys.stderr) | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
validate_draft() diverges from the canonical validator and misses step-level checks.
This implementation only checks presence of test_steps, not required per-step fields or narrative-only constraints. It can pass drafts that validate_tc.py would reject.
Suggested fix
-def validate_draft(draft: Dict) -> bool:
- """Run validation checks (same as validate_tc.py)"""
- import re
- ...
- return True
+def validate_draft(draft: Dict) -> bool:
+ """Run canonical validation from validate_tc.py"""
+ from validate_tc import TCValidator
+ validator = TCValidator(strict=True)
+ is_valid = validator.validate(draft)
+ validator.print_report()
+ return is_valid and not validator.warnings🤖 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 @.claude/scripts/create_polarion_tc.py around lines 45 - 71, validate_draft
currently only verifies that test_steps exists and therefore allows drafts that
the canonical validator (validate_tc.py) would reject; update validate_draft to
replicate the per-step validation from validate_tc.py: iterate
draft['test_steps'] and verify each step contains required fields (e.g.,
'action' and 'expected' or whatever the canonical validator requires), enforce
narrative-only constraints (disallow or flag steps missing both action and
expected unless marked as narrative), validate step ordering/position if the
canonical logic does, and append corresponding errors to errors[] so the
function returns False for invalid steps; reference validate_draft, test_steps,
and mirror the checks implemented in validate_tc.py to ensure parity.
Comment on lines
+151
to
+154
| if "error" in patch_result: | ||
| print(f"⚠️ Warning: Failed to patch custom fields: {patch_result['error']}", file=sys.stderr) | ||
| else: | ||
| print(f"✓ Patched {len(custom_fields)} custom fields", file=sys.stderr) |
There was a problem hiding this comment.
Patch/step failures are reported as success, which masks incomplete TC creation.
If custom-field patch or step upload fails, the function still returns status: success. Downstream automation cannot detect incomplete creation reliably.
Also applies to: 187-200
🤖 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 @.claude/scripts/create_polarion_tc.py around lines 151 - 154, The code
currently prints warnings when patch_result or step upload fails but still
returns a success status, so update the function that builds and returns the
result (the dict containing "status") to reflect errors: check patch_result for
"error" and the step upload result (e.g., step_upload_result or similar variable
used in the 187-200 block) and if either contains an error set the returned
"status" to "failed" (or include a boolean success flag) and include the error
details in the return payload; ensure you change both the custom-field patch
branch (where patch_result and custom_fields are used) and the step-upload
branch to aggregate errors instead of only printing warnings so downstream
automation can reliably detect incomplete TC creation.
Comment on lines
+170
to
+185
| # Step 5: Create (or dry-run) | ||
| if args.dry_run: | ||
| print("[5/5] DRY RUN - Showing summary...", file=sys.stderr) | ||
| print() | ||
| subprocess.run( | ||
| ['python3', 'create_polarion_tc.py', draft_file, '--dry-run'], | ||
| cwd=os.path.dirname(os.path.abspath(__file__)) | ||
| ) | ||
| else: | ||
| print("[5/5] Creating Polarion TC...", file=sys.stderr) | ||
| print() | ||
| result = subprocess.run( | ||
| ['python3', 'create_polarion_tc.py', draft_file], | ||
| cwd=os.path.dirname(os.path.abspath(__file__)) | ||
| ) | ||
| sys.exit(result.returncode) |
There was a problem hiding this comment.
Creation runs without an explicit confirmation step.
The workflow safeguard says “validate → confirm → create,” but this path creates immediately when --dry-run is not set. Add an explicit confirmation prompt (or --yes flag) before Line 181.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 174-174: subprocess call: check for execution of untrusted input
(S603)
[error] 175-175: Starting a process with a partial executable path
(S607)
[error] 181-181: subprocess call: check for execution of untrusted input
(S603)
[error] 182-182: Starting a process with a partial executable path
(S607)
🤖 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 @.claude/scripts/create_tc_hybrid.py around lines 170 - 185, The script
currently proceeds to call subprocess.run(['python3', 'create_polarion_tc.py',
draft_file]) when args.dry_run is false, skipping the “confirm” step; modify the
flow in the main block where args.dry_run is checked to require an explicit
confirmation before invoking subprocess.run (either by adding support for an
args.yes/--yes boolean flag on the argparse parser or by prompting the user with
input() and requiring a "y/yes" response), and only call subprocess.run(...) and
sys.exit(result.returncode) if confirmation is received; reference the
args.dry_run conditional and the subprocess.run invocation for where to insert
the confirmation logic.
Comment on lines
+174
to
+177
| subprocess.run( | ||
| ['python3', 'create_polarion_tc.py', draft_file, '--dry-run'], | ||
| cwd=os.path.dirname(os.path.abspath(__file__)) | ||
| ) |
There was a problem hiding this comment.
Dry-run path ignores child return code.
If create_polarion_tc.py --dry-run fails, this script still exits 0. Capture and propagate the subprocess return code to avoid false-positive success.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 174-174: subprocess call: check for execution of untrusted input
(S603)
[error] 175-175: Starting a process with a partial executable path
(S607)
🤖 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 @.claude/scripts/create_tc_hybrid.py around lines 174 - 177, The dry-run
subprocess invocation in create_tc_hybrid.py currently ignores the child's
return code; change the subprocess.run call that executes ['python3',
'create_polarion_tc.py', draft_file, '--dry-run'] (in create_tc_hybrid.py) to
capture its CompletedProcess result and propagate non-zero return codes (e.g.,
check result.returncode and sys.exit(result.returncode) or raise SystemExit) so
the script exits with the child's error code when the dry-run fails.
Comment on lines
+186
to
+188
| import xml.etree.ElementTree as ET | ||
|
|
||
| root = ET.fromstring(response.text) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current XML parser usage and whether a dependency manifest already exists
# before swapping this call to defusedxml.
fd -a 'pyproject.toml|requirements*.txt|setup.py|Pipfile' .
rg -n 'xml\.etree\.ElementTree|defusedxml' .claude/scriptsRepository: openshift/machine-config-operator
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect polarion_client.py around XML parsing =="
sed -n '150,230p' .claude/scripts/polarion_client.py
echo "== Search for defusedxml and other hardened XML usage across repo =="
rg -n "defusedxml|defusedxml\.|safe|XMLParser|forbid|DANGEROUS|entit|DOCTYPE|resolve_entities" .
echo "== Check for python dependency manifests =="
fd -t f "pyproject.toml|requirements*.txt|setup.py|Pipfile|Pipfile.lock|poetry.lock|requirements*.in" . | head -n 200Repository: openshift/machine-config-operator
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python files using defusedxml =="
rg -n "defusedxml" . --glob "*.py" || true
echo "== Any xml.etree.ElementTree usage in .claude/scripts =="
rg -n "xml\.etree\.ElementTree|ET\.fromstring|ET\.parse" .claude/scripts --glob "*.py"
echo "== Other XML parsers in .claude/scripts =="
rg -n "XMLParser|fromstring\(|lxml|BeautifulSoup" .claude/scripts --glob "*.py" || true
echo "== Dependency manifests (excluding vendor) near .claude and repo root =="
fd -t f --exclude vendor \
"pyproject.toml|requirements*.txt|requirements*.in|Pipfile|Pipfile.lock|poetry.lock|setup.py" . \
| head -n 200
echo "== Dependency manifests inside .claude (if any) =="
fd -t f --exclude vendor .claude \
"pyproject.toml|requirements*.txt|Pipfile|setup.py|poetry.lock" \
| head -n 200
echo "== polarion_client.py SOAP set parsing block (if any) =="
rg -n "_soap_set_test_steps|ET\.fromstring|ET\.parse" .claude/scripts/polarion_client.pyRepository: openshift/machine-config-operator
Length of output: 776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect polarion_client.py SOAP parsing blocks (get/set) =="
sed -n '160,240p' .claude/scripts/polarion_client.py
sed -n '240,380p' .claude/scripts/polarion_client.py
echo "== Search defusedxml across non-vendor files =="
rg -n "defusedxml" . --glob "!vendor/**" || true
echo "== Find dependency manifests (excluding vendor) =="
fd -t f --exclude vendor \
-g "pyproject.toml" \
-g "poetry.lock" \
-g "Pipfile*" \
-g "requirements*.txt" \
-g "requirements*.in" \
-g "setup.py" \
-g "setup.cfg" \
-g "requirements*.lock" \
. | head -n 200
echo "== Search for XML hardened libraries in manifests =="
rg -n "defusedxml|lxml|securexml|XMLParser" . --glob "!vendor/**" --glob "{pyproject.toml,requirements*.txt,requirements*.in,Pipfile,Pipfile.lock,setup.py,setup.cfg,poetry.lock}" || true
echo "== Search for xml.etree usage in .claude/scripts =="
rg -n "xml\.etree\.ElementTree|ET\.fromstring|ET\.parse" .claude/scripts --glob "*.py"Repository: openshift/machine-config-operator
Length of output: 9043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python manifest files (excluding vendor) =="
find . \
-not -path "./vendor/*" \
-type f \
\( -iname "pyproject.toml" -o -iname "poetry.lock" -o -iname "pipfile" -o -iname "pipfile.lock" \
-o -iname "setup.py" -o -iname "setup.cfg" \
-o -iname "requirements.txt" -o -iname "requirements.in" -o -iname "requirements-dev.txt" -o -iname "requirements*.txt" \
-o -iname "requirements*.in" -o -iname "requirements*.lock" \) \
| sed 's|^\./||' | sort | head -n 200Repository: openshift/machine-config-operator
Length of output: 124
Harden XML parsing for SOAP responses.
In .claude/scripts/polarion_client.py (around line 188), response.text from the Polarion SOAP endpoint is parsed with xml.etree.ElementTree.fromstring, which is unsafe for untrusted network XML (XXE/billion-laughs risk). Switch to defusedxml.ElementTree.fromstring(...) (or equivalent defusedxml hardened parsing) for the SOAP “get test steps” path, and ensure defusedxml is available to these scripts (no existing defusedxml usage/dependency manifest was found outside vendor/).
🧰 Tools
🪛 Ruff (0.15.15)
[error] 188-188: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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 @.claude/scripts/polarion_client.py around lines 186 - 188, Replace the
unsafe xml.etree.ElementTree.fromstring(response.text) call used when parsing
the SOAP “get test steps” response: import and use
defusedxml.ElementTree.fromstring instead of xml.etree.ElementTree to prevent
XXE/DoS attacks (locate the parsing site where `root =
ET.fromstring(response.text)` and the `ET` import), and update the module
imports to `import defusedxml.ElementTree as ET` (or equivalent) for the
function that handles the SOAP response; also ensure the defusedxml package is
added to the project dependencies so this script can import it at runtime.
Comment on lines
+125
to
+128
| # Load .env | ||
| set -a | ||
| source "$REPO_ROOT/.env" | ||
| set +a |
There was a problem hiding this comment.
Do not source the repo .env during verification.
Lines 125-128 execute shell from a local config file. This script only needs key/value parsing here, so source turns a malformed or malicious .env into code execution on the developer machine.
Safer pattern
- # Load .env
- set -a
- source "$REPO_ROOT/.env"
- set +a
-
- POLARION_URL=${POLARION_URL:-https://polarion.engineering.redhat.com}
- POLARION_PROJECT=${POLARION_PROJECT:-OSE}
+ POLARION_URL="$(grep -E '^POLARION_URL=' "$REPO_ROOT/.env" | cut -d= -f2-)"
+ POLARION_PROJECT="$(grep -E '^POLARION_PROJECT=' "$REPO_ROOT/.env" | cut -d= -f2-)"
+ POLARION_URL=${POLARION_URL:-https://polarion.engineering.redhat.com}
+ POLARION_PROJECT=${POLARION_PROJECT:-OSE}🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 127-127: Not following: ./.env was not specified as input (see shellcheck -x).
(SC1091)
🤖 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 @.claude/scripts/setup.sh around lines 125 - 128, The script currently
executes source "$REPO_ROOT/.env" (bracketed by set -a / set +a) which can run
arbitrary code; replace that sourcing with a safe key/value parser that reads
the .env file and exports only well-formed VAR=VALUE lines (ignore/skip blank
lines, comments, and lines with shell metacharacters) instead of executing them;
update the block that currently contains set -a / source "$REPO_ROOT/.env" / set
+a to call or inline a parser (e.g., a function like parse_env_file) that
validates lines and exports variables securely.
Comment on lines
+203
to
+233
| def fix_metadata_fields( | ||
| client: PolarionClient, | ||
| tc_id: str, | ||
| project: str, | ||
| component: Optional[str] = None, | ||
| sub_team: Optional[str] = None, | ||
| products: Optional[str] = None, | ||
| test_type: Optional[str] = None, | ||
| version: Optional[str] = None, | ||
| trello_jira: Optional[str] = None | ||
| ) -> Dict[str, Any]: | ||
| """ | ||
| Fix empty metadata fields (like wrong.pdf - OCP-88941). | ||
|
|
||
| Use case: Batch update all required fields at once. | ||
| """ | ||
|
|
||
| # Default values for MCO TCs | ||
| defaults = { | ||
| 'component': component or 'Machine Config Operator', | ||
| 'sub_team': sub_team or 'MCO', | ||
| 'products': products or 'OCP', | ||
| 'test_type': test_type or 'Functional', | ||
| 'version': version, # No default - should be specified | ||
| 'trello_jira': trello_jira, # No default - should be specified | ||
| } | ||
|
|
||
| # Filter out None values | ||
| updates = {k: v for k, v in defaults.items() if v is not None} | ||
|
|
||
| return update_test_case_fields(client, tc_id, project, **updates) |
There was a problem hiding this comment.
--fix-metadata can overwrite existing non-empty fields instead of only fixing empties.
The function name/doc says “fix empty metadata fields,” but it unconditionally sends defaults for component/sub-team/products/test_type when args are omitted. This can silently replace valid existing data.
🤖 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 @.claude/scripts/update_polarion_tc.py around lines 203 - 233,
fix_metadata_fields currently builds defaults and passes them to
update_test_case_fields unconditionally, which can overwrite existing non-empty
metadata; instead, fetch the test case's current metadata via the PolarionClient
(e.g., use client.get_test_case or equivalent to retrieve current fields for
tc_id/project), then only include a field in the updates dict if the existing
value is empty/blank and a default is provided (leave version and trello_jira
untouched unless explicitly passed), and finally call
update_test_case_fields(client, tc_id, project, **updates) with that filtered
updates dict so non-empty fields are not overwritten by defaults.
Comment on lines
+44
to
+45
| r'cat <<EOF', | ||
| r'apiVersion:', # YAML content |
There was a problem hiding this comment.
Heredoc is not blocked even though workflow rules forbid it in expected results.
The validator should reject heredoc usage (<<EOF) to prevent Polarion rendering breakage, but current logic treats command-like content as acceptable, and the sample draft still demonstrates heredoc-style content.
Suggested fix
class TCValidator:
@@
def _validate_test_steps(self, steps: List[Dict[str, str]]):
@@
if not expected:
self.errors.append(f"Step {i}: 'expected_result' field is empty")
continue
+
+ if re.search(r"<<\s*EOF\b", expected, re.IGNORECASE):
+ self.errors.append(
+ f"Step {i}: heredoc syntax detected in expected_result. "
+ "Use file-based YAML + command output instead."
+ )
+ continueAlso applies to: 123-156, 221-230
🤖 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 @.claude/scripts/validate_tc.py around lines 44 - 45, The validator in
.claude/scripts/validate_tc.py currently allows heredoc-style snippets like "cat
<<EOF" (seen in the diff r'cat <<EOF') even though workflow rules forbid
heredocs; update the validation logic (in the function that classifies
command-like content—e.g., the validator function handling command patterns /
is_command_like or validate_test_case content) to explicitly detect and reject
heredoc tokens such as '<<' or '<<EOF' anywhere in command strings and raise a
validation error; ensure the same check is applied to all places where
command-like blocks are validated (the code paths covering the ranges noted:
~lines 123-156 and 221-230) so heredoc usage is blocked and reported as invalid.
Comment on lines
+152
to
+155
| if self.strict: | ||
| self.errors.append(msg) | ||
| else: | ||
| self.warnings.append(msg) |
There was a problem hiding this comment.
--strict does not actually treat all warnings as errors.
Line 254 promises strict mode escalates warnings, but Line 205 returns non-zero only for errors. Warnings like “No test steps defined” still exit 0 in strict mode.
Suggested fix
def get_exit_code(self) -> int:
"""Get exit code based on validation result"""
- return 1 if self.errors else 0
+ return 1 if self.errors or (self.strict and self.warnings) else 0Also applies to: 203-205, 254-255
🤖 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 @.claude/scripts/validate_tc.py around lines 152 - 155, The strict flag isn't
causing warnings to be treated as errors because warnings are still appended to
self.warnings and the exit code only checks self.errors; update the logic so
that when self.strict is true warnings are escalated: either append to
self.errors instead of self.warnings in the places that currently do "if
self.strict: self.errors.append(msg) else: self.warnings.append(msg)" or, at
program exit where return/exit code is computed, compute effective_errors =
len(self.errors) + (len(self.warnings) if self.strict else 0) and use
effective_errors to decide non-zero exit; refer to self.strict, self.errors,
self.warnings and the exit/return code block currently around the lines that
check errors for non-zero exit.
Contributor
|
@ptalgulk01: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
Adds Claude Code commands, skills, and Python scripts to automate MCO QE workflows for creating and fetching Polarion test cases.
Commands:
Scripts (.claude/scripts/):
Key rules enforced:
Summary by CodeRabbit
New Features
Documentation
Chores