Update /project commands from upstream tnf-dev-env#110
Open
fonta-rh wants to merge 5 commits intoopenshift-eng:mainfrom
Open
Update /project commands from upstream tnf-dev-env#110fonta-rh wants to merge 5 commits intoopenshift-eng:mainfrom
fonta-rh wants to merge 5 commits intoopenshift-eng:mainfrom
Conversation
Sync the /project:new, /project:resume, and /project:close commands with the latest versions from fonta-rh/tnf-dev-env. Key changes: - Extract project resolution logic into shared resume-project.py script - Introduce lean index pattern for project CLAUDE.md (~50-80 lines) - Add detail file templates (investigation.md, ci-runs.md, etc.) - Implement lazy repo context loading in /project:resume - Add Reference Files table as manifest for detail file discovery Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Tolerate trailing whitespace in markdown table separator regex - Accept uppercase [X] in checklist parsing - Rename ambiguous variable `l` to `line` (ruff E741) Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert bold step labels to ### subheadings (MD036), add blank lines around lists (MD032), add language specifiers to fenced code blocks (MD040), indent table under ordered list item to preserve numbering (MD029), and escape angle brackets in inline examples (MD033). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
WalkthroughProject selection and resume/close flows were refactored to delegate resolution to a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 1
🧹 Nitpick comments (3)
multi-repo-development/scripts/resume-project.py (2)
225-230: ⚖️ Poor tradeoffFallback loses done-filtering and mtime-sorting from
recent-projects.py.When
recent-projects.pyis unavailable,_fallback_project_namesreturns all project directories sorted alphabetically. This differs from the primary behavior which filtersstatus: doneprojects and sorts by modification time (per context snippet 3).This may surface closed projects or show an unexpected order. Consider documenting this as a known limitation, or adding minimal done-filtering:
Optional: Add done-filtering to fallback
def _fallback_project_names(root: Path) -> list[str]: """Fallback: list project directories sorted alphabetically.""" projects_dir = root / "projects" if not projects_dir.is_dir(): return [] - return sorted(d.name for d in projects_dir.iterdir() if d.is_dir()) + names = [] + for d in projects_dir.iterdir(): + if not d.is_dir(): + continue + fm = parse_frontmatter(d / "CLAUDE.md") + if fm.get("status") == "done": + continue + names.append(d.name) + return sorted(names)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@multi-repo-development/scripts/resume-project.py` around lines 225 - 230, The fallback _fallback_project_names currently returns all directories sorted alphabetically which loses the primary behavior from recent-projects.py (filtering out projects with status: done and sorting by modification time); update _fallback_project_names to read each project's status (e.g., check a status file or metadata used by recent-projects.py), skip projects marked "done", and return names sorted by each project's modification time (mtime) descending to match recent-projects.py behavior, or if you choose not to implement filtering add a brief comment in the function docstring noting this limitation and why it differs from recent-projects.py.
247-251: 💤 Low valueMissing
error_messagefield forno_argumentstatus.When status is
no_argument, the return dict includes onlystatusandalternatives, but noterror_message. Whileclose.mdandresume.mddon't explicitly useerror_messagefor this status, the other error statuses (not_found,out_of_range,no_projects) consistently include it.For consistency and to allow callers to display a message if needed:
Suggested fix
if arg is None: names = get_recent_names(root) if not names: return {"status": "no_projects", "error_message": "No active projects found.", "alternatives": []} - return {"status": "no_argument", "alternatives": names} + return {"status": "no_argument", "error_message": "No project specified.", "alternatives": names}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@multi-repo-development/scripts/resume-project.py` around lines 247 - 251, The "no_argument" branch in the arg handling (inside the if arg is None block that calls get_recent_names) returns {"status": "no_argument", "alternatives": names} but omits an "error_message" key; update that return to include an appropriate error_message (e.g., "No argument provided. Please specify a project or choose from alternatives.") so its shape matches other error statuses like "no_projects" and "not_found" and callers can rely on error_message being present.multi-repo-development/.claude/commands/project/new.md (1)
200-205: 💤 Low valueReference Files table template column header mismatch with parser.
The template specifies
| File | Content |butparse_reference_files()inresume-project.py:109looks for"File" in lineto detect the header. While this works, the parser comment and docstring suggest it expects| File | Description |columns based on the output structure{"filename": ..., "description": ...}.Consider aligning the column name for clarity:
Suggested change
-3. **`## Reference Files`** — table with columns `| File | Content |`. +3. **`## Reference Files`** — table with columns `| File | Description |`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@multi-repo-development/.claude/commands/project/new.md` around lines 200 - 205, The Reference Files column header in the markdown template currently reads "| File | Content |" but parse_reference_files() expects a "Description" column in its output structure; update either the template or the parser to align: preferable fix is to change the template header to "| File | Description |" so it matches parse_reference_files() output keys, or modify parse_reference_files() to accept both "Content" and "Description" (e.g., normalize the header name before building {"filename": ..., "description": ..."}) so the function name parse_reference_files() continues to return the expected "description" field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@multi-repo-development/scripts/resume-project.py`:
- Around line 115-118: The separator-detection regex used in the loop (the line
testing re.match(r"^\|[-\s|]+\|\s*$", line)) rejects valid Markdown table
separators that include alignment colons (e.g., |:---|:---:|), causing the block
that sets skipped_separator to never run; update the match to allow optional
leading/trailing colons around the dashes (for example, accept ':' characters in
the character class or use a pattern like r"^\|\s*[:\-|\s]+\|\s*$") so the code
that sets skipped_separator when found_header is True will correctly detect
separator lines; modify the regex in the block that references found_header and
skipped_separator accordingly.
---
Nitpick comments:
In `@multi-repo-development/.claude/commands/project/new.md`:
- Around line 200-205: The Reference Files column header in the markdown
template currently reads "| File | Content |" but parse_reference_files()
expects a "Description" column in its output structure; update either the
template or the parser to align: preferable fix is to change the template header
to "| File | Description |" so it matches parse_reference_files() output keys,
or modify parse_reference_files() to accept both "Content" and "Description"
(e.g., normalize the header name before building {"filename": ...,
"description": ..."}) so the function name parse_reference_files() continues to
return the expected "description" field.
In `@multi-repo-development/scripts/resume-project.py`:
- Around line 225-230: The fallback _fallback_project_names currently returns
all directories sorted alphabetically which loses the primary behavior from
recent-projects.py (filtering out projects with status: done and sorting by
modification time); update _fallback_project_names to read each project's status
(e.g., check a status file or metadata used by recent-projects.py), skip
projects marked "done", and return names sorted by each project's modification
time (mtime) descending to match recent-projects.py behavior, or if you choose
not to implement filtering add a brief comment in the function docstring noting
this limitation and why it differs from recent-projects.py.
- Around line 247-251: The "no_argument" branch in the arg handling (inside the
if arg is None block that calls get_recent_names) returns {"status":
"no_argument", "alternatives": names} but omits an "error_message" key; update
that return to include an appropriate error_message (e.g., "No argument
provided. Please specify a project or choose from alternatives.") so its shape
matches other error statuses like "no_projects" and "not_found" and callers can
rely on error_message being present.
🪄 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: ea7dd846-5505-4f3f-85ff-4c83608c68cd
📒 Files selected for processing (4)
multi-repo-development/.claude/commands/project/close.mdmulti-repo-development/.claude/commands/project/new.mdmulti-repo-development/.claude/commands/project/resume.mdmulti-repo-development/scripts/resume-project.py
- scripts/resume-project.py:116: Accept alignment colons in table separator regex Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit review-body findings:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@multi-repo-development/scripts/resume-project.py`:
- Around line 253-261: The current digit-index branch in resume-project.py uses
get_recent_names() and then returns "out_of_range" even when names == [], which
causes retryable behavior; change the branch inside the if arg.isdigit() block
to first check if names is empty and return {"status": "no_projects",
"error_message": "No recent projects found.", "alternatives": []} when
get_recent_names() returns []; otherwise keep the existing index bounds check
(using names, idx) and return the existing "out_of_range" response only when
there are names but the index is invalid.
- Around line 47-180: Add unit tests that cover both valid and malformed inputs
for the new parsers: parse_frontmatter, parse_reference_files, and
extract_checklist. For parse_frontmatter, include tests for normal scalar
values, list values, empty list values ("[]"), missing closing fence (no
trailing "---"), and malformed indented list lines to ensure it returns {} or
the expected partial result; reference parse_frontmatter in your tests. For
parse_reference_files, add positive tests with a proper "## Reference Files"
header, header row and separator, multiple table rows, and negative tests with
malformed separator rows, missing header, extra pipes, and early section
termination (another "## " after header) to ensure correct results or empty
list; reference parse_reference_files. For extract_checklist, add tests for
checked/unchecked items under different headings, malformed checkbox tokens,
varying heading levels, and lists that cross section boundaries to ensure
section attribution and counts are accurate; reference extract_checklist. Ensure
each test asserts both expected parsed output and that invalid inputs fail
safely (empty dict/list or appropriate counts) to satisfy positive and negative
coverage requirements.
- Around line 207-230: The fallback _fallback_project_names currently returns
every directory under projects/ sorted alphabetically which diverges from
recent-projects.py; update _fallback_project_names(root: Path) to match
recent-projects.py semantics used by get_recent_names by excluding projects
marked done (e.g., presence of a .done marker or whatever recent-projects.py
uses), only including projects with readable content (e.g., contain files like
README or other non-hidden files), and order them by recency
(most-recently-updated first) instead of alphabetical so the selection behavior
remains consistent when the subprocess call in get_recent_names fails.
🪄 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: 00128df2-bcd2-4fc6-8945-877eab69592b
📒 Files selected for processing (1)
multi-repo-development/scripts/resume-project.py
Comment on lines
+47
to
+180
| def parse_frontmatter(path: Path) -> dict[str, str | list[str]]: | ||
| """Extract YAML frontmatter, handling both scalar values and lists.""" | ||
| try: | ||
| lines = path.read_text().splitlines() | ||
| except OSError: | ||
| return {} | ||
|
|
||
| if not lines or lines[0].strip() != "---": | ||
| return {} | ||
|
|
||
| result: dict[str, str | list[str]] = {} | ||
| current_list_key: str | None = None | ||
|
|
||
| for line in lines[1:]: | ||
| if line.strip() == "---": | ||
| break | ||
|
|
||
| if line.startswith(" - ") and current_list_key: | ||
| val = line.strip().removeprefix("- ") | ||
| lst = result[current_list_key] | ||
| if isinstance(lst, list): | ||
| lst.append(val) | ||
| continue | ||
|
|
||
| current_list_key = None | ||
|
|
||
| if ":" not in line: | ||
| continue | ||
|
|
||
| key, _, value = line.partition(":") | ||
| key = key.strip() | ||
| value = value.strip() | ||
|
|
||
| if value == "[]": | ||
| result[key] = [] | ||
| continue | ||
|
|
||
| if not value: | ||
| result[key] = [] | ||
| current_list_key = key | ||
| continue | ||
|
|
||
| result[key] = value | ||
| else: | ||
| return {} | ||
|
|
||
| return result | ||
|
|
||
|
|
||
| def parse_reference_files(text: str) -> list[dict[str, str]]: | ||
| """Parse the Reference Files markdown table into [{filename, description}].""" | ||
| in_section = False | ||
| found_header = False | ||
| skipped_separator = False | ||
| results = [] | ||
|
|
||
| for line in text.splitlines(): | ||
| if re.match(r"^##\s+Reference Files", line, re.IGNORECASE): | ||
| in_section = True | ||
| continue | ||
|
|
||
| if in_section and not found_header: | ||
| if "|" in line and "File" in line: | ||
| found_header = True | ||
| elif line.startswith("## "): | ||
| break | ||
| continue | ||
|
|
||
| if found_header and not skipped_separator: | ||
| if re.match(r"^\|[-:\s|]+\|\s*$", line): | ||
| skipped_separator = True | ||
| continue | ||
|
|
||
| if skipped_separator: | ||
| if not line.strip() or line.startswith("## "): | ||
| break | ||
| cells = [c.strip() for c in line.split("|")] | ||
| cells = [c for c in cells if c] | ||
| if len(cells) >= 2: | ||
| filename = cells[0].strip("`") | ||
| description = cells[1] | ||
| results.append({"filename": filename, "description": description}) | ||
|
|
||
| return results | ||
|
|
||
|
|
||
| def list_project_files(project_dir: Path) -> list[str]: | ||
| """Recursively list files relative to project_dir, sorted.""" | ||
| files = [] | ||
| for root, dirs, filenames in os.walk(project_dir): | ||
| dirs[:] = [d for d in dirs if d != ".git"] | ||
| for f in filenames: | ||
| rel = os.path.relpath(os.path.join(root, f), project_dir) | ||
| files.append(rel) | ||
| files.sort() | ||
| return files | ||
|
|
||
|
|
||
| def find_unregistered_files( | ||
| all_files: list[str], manifest_files: list[dict[str, str]] | ||
| ) -> list[str]: | ||
| """Files on disk not in the Reference Files manifest or ALWAYS_PRESENT.""" | ||
| known = ALWAYS_PRESENT | {m["filename"] for m in manifest_files} | ||
| return [f for f in all_files if f not in known and not f.startswith(".")] | ||
|
|
||
|
|
||
| def extract_checklist(text: str) -> dict: | ||
| """Extract checked/unchecked items with their section headings.""" | ||
| current_section = "" | ||
| checked_items = [] | ||
| unchecked_items = [] | ||
|
|
||
| for line in text.splitlines(): | ||
| heading_match = re.match(r"^#{2,3}\s+(.+)", line) | ||
| if heading_match: | ||
| current_section = heading_match.group(1).strip() | ||
| continue | ||
|
|
||
| item_match = re.match(r"^\s*- \[([ xX])\] (.+)$", line) | ||
| if item_match: | ||
| done = item_match.group(1).lower() == "x" | ||
| entry = {"text": item_match.group(2).strip(), "section": current_section} | ||
| if done: | ||
| checked_items.append(entry) | ||
| else: | ||
| unchecked_items.append(entry) | ||
|
|
||
| return { | ||
| "checked": len(checked_items), | ||
| "unchecked": len(unchecked_items), | ||
| "total": len(checked_items) + len(unchecked_items), | ||
| "unchecked_items": unchecked_items, | ||
| "checked_items": checked_items, | ||
| } |
Contributor
There was a problem hiding this comment.
Add positive and negative tests for the new parsers.
This introduces custom frontmatter, Reference Files, and checklist parsing, but there is still no automated coverage here for malformed separators, missing closing fences, empty list values, or section termination. A silent parse miss changes has_reference_files, checklist, and skill_suggestions downstream.
As per coding guidelines, "Validation/parsing changes: regex patterns and parsing rules (like project/Reference Files/frontmatter parsing) require positive + negative test coverage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-repo-development/scripts/resume-project.py` around lines 47 - 180, Add
unit tests that cover both valid and malformed inputs for the new parsers:
parse_frontmatter, parse_reference_files, and extract_checklist. For
parse_frontmatter, include tests for normal scalar values, list values, empty
list values ("[]"), missing closing fence (no trailing "---"), and malformed
indented list lines to ensure it returns {} or the expected partial result;
reference parse_frontmatter in your tests. For parse_reference_files, add
positive tests with a proper "## Reference Files" header, header row and
separator, multiple table rows, and negative tests with malformed separator
rows, missing header, extra pipes, and early section termination (another "## "
after header) to ensure correct results or empty list; reference
parse_reference_files. For extract_checklist, add tests for checked/unchecked
items under different headings, malformed checkbox tokens, varying heading
levels, and lists that cross section boundaries to ensure section attribution
and counts are accurate; reference extract_checklist. Ensure each test asserts
both expected parsed output and that invalid inputs fail safely (empty dict/list
or appropriate counts) to satisfy positive and negative coverage requirements.
Contributor
Author
There was a problem hiding this comment.
Won't fix — generic "add tests" advice without a specific untested edge case. Scope creep for an upstream-import PR.
Contributor
There was a problem hiding this comment.
@fonta-rh, understood — acknowledged and moving on. If edge cases surface during manual testing of /project:resume or /project:close, opening a follow-up issue to add targeted tests would be a natural next step.
Comment on lines
+207
to
+230
| def get_recent_names(root: Path) -> list[str]: | ||
| """Get recent non-done project names via recent-projects.py --names.""" | ||
| script = root / "scripts" / "recent-projects.py" | ||
| if not script.is_file(): | ||
| return _fallback_project_names(root) | ||
| try: | ||
| result = subprocess.run( | ||
| [sys.executable, str(script), "--names"], | ||
| capture_output=True, text=True, | ||
| env={**os.environ, "CLAUDE_PROJECT_DIR": str(root)}, | ||
| ) | ||
| if result.returncode != 0: | ||
| return _fallback_project_names(root) | ||
| return [line.strip() for line in result.stdout.splitlines() if line.strip()] | ||
| except OSError: | ||
| return _fallback_project_names(root) | ||
|
|
||
|
|
||
| def _fallback_project_names(root: Path) -> list[str]: | ||
| """Fallback: list project directories sorted alphabetically.""" | ||
| projects_dir = root / "projects" | ||
| if not projects_dir.is_dir(): | ||
| return [] | ||
| return sorted(d.name for d in projects_dir.iterdir() if d.is_dir()) |
Contributor
There was a problem hiding this comment.
Keep fallback resolution semantics aligned with recent-projects.py.
_fallback_project_names() widens the candidate set to every directory under projects/, sorted alphabetically, while multi-repo-development/scripts/recent-projects.py:55-79 only returns non-done projects with readable content in recency order. If the subprocess fails, numeric selection and the alternatives list can suddenly resolve to a different project than the normal path.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 213-213: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-repo-development/scripts/resume-project.py` around lines 207 - 230, The
fallback _fallback_project_names currently returns every directory under
projects/ sorted alphabetically which diverges from recent-projects.py; update
_fallback_project_names(root: Path) to match recent-projects.py semantics used
by get_recent_names by excluding projects marked done (e.g., presence of a .done
marker or whatever recent-projects.py uses), only including projects with
readable content (e.g., contain files like README or other non-hidden files),
and order them by recency (most-recently-updated first) instead of alphabetical
so the selection behavior remains consistent when the subprocess call in
get_recent_names fails.
Contributor
Author
There was a problem hiding this comment.
Won't fix — fallback is intentionally simple for degraded operation. Duplicating recent-projects.py logic creates a maintenance burden.
Contributor
There was a problem hiding this comment.
@fonta-rh, understood — keeping the fallback intentionally simple to avoid coupling it to recent-projects.py internals is a reasonable tradeoff for a degraded-operation path. Thanks for the context!
(◕‿◕✿)
📝 Learnings were identified and not saved (knowledge base disabled). Enable
When get_recent_names() returns no projects, the digit-index branch now returns no_projects instead of out_of_range, preventing callers from entering an empty retry flow. Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit review-body nitpicks:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@multi-repo-development/scripts/resume-project.py`:
- Around line 315-318: The code currently truncates list-valued project_type to
the first element; instead, when project_type is a list (from
parse_frontmatter()), compute suggestions by merging SKILL_SUGGESTIONS for each
type in the list (falling back to DEFAULT_SKILLS for unknown types) and
deduplicate (e.g., via set union) so multi-type projects get all relevant
skills; retain the existing behavior for a single string type and for
empty/missing types.
- Around line 64-89: The parser is incorrectly promoting every bare "key:" to an
empty list; change the logic so when value is empty you set result[key] = ""
(not []) and set current_list_key = key as a candidate for list promotion, and
then when you encounter a list item (the block handling line.startswith(" - ")
and current_list_key) check if result[current_list_key] == "" and if so replace
it with [] before appending the first val; update the branch that handles list
items (and the variable names result and current_list_key) accordingly, and add
a regression test that parses a document containing a bare empty scalar like
"jira:" and another real list-valued key in the same input to assert the empty
scalar stays "" while the list key becomes an array.
- Around line 213-222: The subprocess.run call that invokes recent-projects.py
(the call using [sys.executable, str(script), "--names"] with capture_output and
env) can block indefinitely; add a small timeout argument to that subprocess.run
invocation and catch subprocess.TimeoutExpired in the same try/except block so
it returns _fallback_project_names(root) just like the existing OSError path.
Update the code that currently catches OSError to also handle
subprocess.TimeoutExpired (or add a separate except subprocess.TimeoutExpired:
return _fallback_project_names(root)) and keep using the same fallback function
and variables (script, root) as in the original call.
🪄 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: d74a7242-6a18-40fe-a0d1-d74e9edbefdf
📒 Files selected for processing (1)
multi-repo-development/scripts/resume-project.py
Comment on lines
+64
to
+89
| if line.startswith(" - ") and current_list_key: | ||
| val = line.strip().removeprefix("- ") | ||
| lst = result[current_list_key] | ||
| if isinstance(lst, list): | ||
| lst.append(val) | ||
| continue | ||
|
|
||
| current_list_key = None | ||
|
|
||
| if ":" not in line: | ||
| continue | ||
|
|
||
| key, _, value = line.partition(":") | ||
| key = key.strip() | ||
| value = value.strip() | ||
|
|
||
| if value == "[]": | ||
| result[key] = [] | ||
| continue | ||
|
|
||
| if not value: | ||
| result[key] = [] | ||
| current_list_key = key | ||
| continue | ||
|
|
||
| result[key] = value |
Contributor
There was a problem hiding this comment.
Don’t coerce every blank frontmatter field into a list.
A bare key: currently becomes [] immediately, so empty scalars like jira: or status: change type in the JSON payload even when no - item lines follow. Keep the empty value as "" first, and only promote it to a list once an indented list item is actually parsed.
Suggested fix
- if line.startswith(" - ") and current_list_key:
- val = line.strip().removeprefix("- ")
- lst = result[current_list_key]
- if isinstance(lst, list):
- lst.append(val)
+ if line.startswith(" - ") and current_list_key:
+ val = line.strip().removeprefix("- ")
+ if not isinstance(result.get(current_list_key), list):
+ result[current_list_key] = []
+ lst = result[current_list_key]
+ if isinstance(lst, list):
+ lst.append(val)
continue
…
- if not value:
- result[key] = []
- current_list_key = key
+ if not value:
+ result[key] = ""
+ current_list_key = key
continuePlease add a regression test for an empty scalar (jira:) and a real list-valued key in the same pass. As per coding guidelines, "ensure any new/changed parsing rules have positive and negative test coverage per the “Test validation logic” guidance".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-repo-development/scripts/resume-project.py` around lines 64 - 89, The
parser is incorrectly promoting every bare "key:" to an empty list; change the
logic so when value is empty you set result[key] = "" (not []) and set
current_list_key = key as a candidate for list promotion, and then when you
encounter a list item (the block handling line.startswith(" - ") and
current_list_key) check if result[current_list_key] == "" and if so replace it
with [] before appending the first val; update the branch that handles list
items (and the variable names result and current_list_key) accordingly, and add
a regression test that parses a document containing a bare empty scalar like
"jira:" and another real list-valued key in the same input to assert the empty
scalar stays "" while the list key becomes an array.
Comment on lines
+213
to
+222
| result = subprocess.run( | ||
| [sys.executable, str(script), "--names"], | ||
| capture_output=True, text=True, | ||
| env={**os.environ, "CLAUDE_PROJECT_DIR": str(root)}, | ||
| ) | ||
| if result.returncode != 0: | ||
| return _fallback_project_names(root) | ||
| return [line.strip() for line in result.stdout.splitlines() if line.strip()] | ||
| except OSError: | ||
| return _fallback_project_names(root) |
Contributor
There was a problem hiding this comment.
Add a timeout to the recent-projects.py subprocess.
If that helper blocks, both command flows hang indefinitely instead of degrading to _fallback_project_names(). Set a small timeout and treat subprocess.TimeoutExpired the same way as the current OSError path.
Suggested fix
result = subprocess.run(
[sys.executable, str(script), "--names"],
- capture_output=True, text=True,
+ capture_output=True, text=True,
env={**os.environ, "CLAUDE_PROJECT_DIR": str(root)},
+ timeout=5,
)
…
- except OSError:
+ except (OSError, subprocess.TimeoutExpired):
return _fallback_project_names(root)🧰 Tools
🪛 Ruff (0.15.12)
[error] 213-213: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-repo-development/scripts/resume-project.py` around lines 213 - 222, The
subprocess.run call that invokes recent-projects.py (the call using
[sys.executable, str(script), "--names"] with capture_output and env) can block
indefinitely; add a small timeout argument to that subprocess.run invocation and
catch subprocess.TimeoutExpired in the same try/except block so it returns
_fallback_project_names(root) just like the existing OSError path. Update the
code that currently catches OSError to also handle subprocess.TimeoutExpired (or
add a separate except subprocess.TimeoutExpired: return
_fallback_project_names(root)) and keep using the same fallback function and
variables (script, root) as in the original call.
Comment on lines
+315
to
+318
| project_type = fm.get("type", "") | ||
| if isinstance(project_type, list): | ||
| project_type = project_type[0] if project_type else "" | ||
| suggestions = SKILL_SUGGESTIONS.get(project_type, DEFAULT_SKILLS) |
Contributor
There was a problem hiding this comment.
Merge skills for list-valued type instead of truncating to the first entry.
parse_frontmatter() can return type as a list, but this branch only keeps the first value. Multi-type projects will silently miss relevant commands even though the structured payload still exposes the full list.
Suggested fix
- project_type = fm.get("type", "")
- if isinstance(project_type, list):
- project_type = project_type[0] if project_type else ""
- suggestions = SKILL_SUGGESTIONS.get(project_type, DEFAULT_SKILLS)
+ project_types = fm.get("type", "")
+ if isinstance(project_types, str):
+ project_types = [project_types] if project_types else []
+ suggestions = list(
+ dict.fromkeys(
+ skill
+ for project_type in project_types
+ for skill in SKILL_SUGGESTIONS.get(project_type, [])
+ )
+ ) or DEFAULT_SKILLS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-repo-development/scripts/resume-project.py` around lines 315 - 318, The
code currently truncates list-valued project_type to the first element; instead,
when project_type is a list (from parse_frontmatter()), compute suggestions by
merging SKILL_SUGGESTIONS for each type in the list (falling back to
DEFAULT_SKILLS for unknown types) and deduplicate (e.g., via set union) so
multi-type projects get all relevant skills; retain the existing behavior for a
single string type and for empty/missing types.
Contributor
|
Adding a hold here while Coderabbit comments get addressed. Consider making this a Draft PR until the rounds of review are complete. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
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
2 participants
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
/project:new,/project:resume, and/project:closecommands with the latest versions from fonta-rh/tnf-dev-envscripts/resume-project.py— shared project resolution logic used by both/project:resumeand/project:close, replacing duplicated inline resolution in each commandTest plan
/project:new,/project:resume,/project:closein a multi-repo workspace🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
New Features
Documentation