-
Notifications
You must be signed in to change notification settings - Fork 0
chore/security #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
flyingrobots
commented
Nov 6, 2025
- Create SECURITY.md
- opus(PR#2): seed rehearsal score (410ce34)
- Update docs/code-reviews/PR2/410ce3439856a4cdf6ae7df8fd3936e7bbb67bba.md
- opus(PR#2): seed rehearsal score (016d60d)
- review: paginate seeding; add comments dumper; update security policy
seed_feedback_from_github.py: follow GitHub Link headers via gh_get_all; switch both review and issue comments to pagination so large PRs are fully captured; include URL in error messages. SECURITY.md: add security.txt reference and privacy/retention note; clarify supported versions policy (latest stable major only; only latest minor within that major; announce EOL in RELEASES.md). tools/review/get_comments_markdown.py: new helper to dump PR review + issue comments to Markdown; infers owner/repo from remote or slug; requires GITHUB_TOKEN; optional --out to write file.
Summary by CodeRabbit
WalkthroughAdds archived code-review documents and new/updated review tooling: a script to export PR comments to Markdown, pagination-aware GitHub API helpers, and updates to an existing seeding script to use paginated fetches. No public API or code behavior changes outside tooling and docs. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as get_comments_markdown.py
participant GH as GitHub API
participant FS as File System
User->>CLI: run CLI (pr=N, --out=path?, token)
CLI->>CLI: parse args, resolve repo slug
CLI->>GH: GET /pulls/{N}/comments?per_page=100
GH-->>CLI: page 1 + Link header
alt Link header -> next
CLI->>GH: GET next page URL
GH-->>CLI: page 2..N
end
CLI->>GH: GET /issues/{N}/comments?per_page=100
GH-->>CLI: issue comment pages...
CLI->>CLI: merge, normalize, sort comments
CLI->>CLI: render Markdown
alt --out provided
CLI->>FS: write file
FS-->>User: file saved
else
CLI-->>User: print Markdown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
SECURITY.mdis excluded by!*.md
📒 Files selected for processing (3)
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md(1 hunks)tools/review/get_comments_markdown.py(1 hunks)tools/review/seed_feedback_from_github.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: flyingrobots
Repo: flyingrobots/draft-punks PR: 2
File: SECURITY.md:5-15
Timestamp: 2025-09-29T09:02:31.228Z
Learning: In the draft-punks repository SECURITY.md file, maintain the whimsical German-accented English writing style with musical metaphors when making suggestions. The document uses phrases like "ze", "vill", "und" and references to BunBun, Kapellmeister, and musical terms.
📚 Learning: 2025-09-29T08:57:34.199Z
Learnt from: flyingrobots
Repo: flyingrobots/draft-punks PR: 2
File: docs/code-reviews/PR2/410ce3439856a4cdf6ae7df8fd3936e7bbb67bba.md:81-85
Timestamp: 2025-09-29T08:57:34.199Z
Learning: When reviewing auto-generated CodeRabbit content or review artifacts, do not criticize marketing links or promotional content that are part of the standard CodeRabbit template output.
Applied to files:
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md
🧬 Code graph analysis (2)
tools/review/get_comments_markdown.py (1)
tools/review/seed_feedback_from_github.py (3)
gh_get_all(72-109)normalize(112-113)main(116-200)
tools/review/seed_feedback_from_github.py (1)
tools/review/get_comments_markdown.py (1)
gh_get_all(73-107)
🪛 markdownlint-cli2 (0.18.1)
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md
113-113: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.3)
tools/review/get_comments_markdown.py
31-31: typing.Dict is deprecated, use dict instead
(UP035)
31-31: typing.List is deprecated, use list instead
(UP035)
31-31: typing.Tuple is deprecated, use tuple instead
(UP035)
42-42: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
44-44: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
99-99: Avoid specifying long messages outside the exception class
(TRY003)
112-116: Starting a process with a partial executable path
(S607)
117-117: Consider moving this statement to an else block
(TRY300)
118-118: Do not catch blind exception: Exception
(BLE001)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Starting a process with a partial executable path
(S607)
150-150: Do not catch blind exception: Exception
(BLE001)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Unpacked variable known is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
257-257: Local variable repo_name_arg is assigned to but never used
Remove assignment to unused variable repo_name_arg
(F841)
tools/review/seed_feedback_from_github.py
35-35: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
40-40: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
101-101: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md (1)
115-115: Verify that the{response}placeholder is populated by the workflow.This template variable appears to be intentionally left for later substitution. Ensure your documentation generation workflow correctly replaces this placeholder with actual content, otherwise readers will see a confusing
{response}string in the published artifact.tools/review/seed_feedback_from_github.py (2)
49-52: LGTM!Clean refactoring that separates concerns while maintaining backward compatibility.
135-138: LGTM!Pagination correctly implemented for both review and issue comments. This ensures large PRs with 100+ comments are fully captured.
tools/review/get_comments_markdown.py (4)
38-53: LGTM - Superior implementation.This HTTP client implementation is more robust than the one in
seed_feedback_from_github.pybecause it:
- Includes a 30-second timeout to prevent hanging
- Properly chains exceptions with
from exc- Handles both HTTP and network errors
When consolidating the duplicated code (per my earlier comment), use this implementation as the canonical version.
110-119: LGTM!The broad exception handling is appropriate here for a defensive check that should gracefully degrade if git is unavailable or the command fails.
122-154: LGTM!Robust handling of various Git remote URL formats (git@, https://, etc.) with appropriate fallback behavior when git commands fail.
170-214: LGTM!Clean implementation that aggregates both comment types, sorts chronologically, and generates well-structured Markdown output. The tie-breaker on URL ensures deterministic ordering.
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md
Outdated
Show resolved
Hide resolved
| import subprocess | ||
| import sys | ||
| import textwrap | ||
| from typing import Dict, List, Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Modernize type hints to use built-in generics.
With from __future__ import annotations already imported, you can use lowercase dict, list, and tuple directly instead of the deprecated typing versions per PEP 585 (Python 3.9+).
Apply this diff:
-from typing import Dict, List, Tuple
+from typing import TYPE_CHECKINGThen update all type hints throughout the file:
Dict[str, str]→dict[str, str]List[dict]→list[dict]Tuple[str, str]→tuple[str, str]
Based on static analysis.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
31-31: typing.Dict is deprecated, use dict instead
(UP035)
31-31: typing.List is deprecated, use list instead
(UP035)
31-31: typing.Tuple is deprecated, use tuple instead
(UP035)
🤖 Prompt for AI Agents
In tools/review/get_comments_markdown.py around line 31, the file imports typing
generics (Dict, List, Tuple) but the project uses from __future__ import
annotations and should use built-in generics per PEP 585; replace the import of
Dict, List, Tuple with built-in types (or remove those names from the typing
import) and update all annotations in the file: change Dict[str, str] to
dict[str, str], List[dict] to list[dict], and Tuple[str, str] to tuple[str,
str], ensuring any remaining typing usage is either removed or replaced with the
appropriate lowercase built-in generic types.
| remote_arg = args.remote or kv.get("remote") | ||
| slug_arg = args.slug or kv.get("slug") | ||
| repo_kv = kv.get("repo") | ||
| owner_arg = args.owner or kv.get("owner") | ||
| repo_name_arg = args.repo or (kv.get("repo_name") if repo_kv and "/" not in repo_kv else None) | ||
|
|
||
| if remote_arg: | ||
| owner, repo = parse_remote_url(remote_arg) | ||
| elif slug_arg: | ||
| owner, repo = parse_slug(slug_arg) | ||
| elif repo_kv and "/" in repo_kv: | ||
| owner, repo = parse_slug(repo_kv) | ||
| elif owner_arg and (args.repo or kv.get("repo")) and "/" not in (args.repo or kv.get("repo")): # owner/repo split | ||
| owner, repo = owner_arg, (args.repo or kv.get("repo")) | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify the owner/repo precedence logic and remove unused variable.
The precedence determination is overly complex with nested conditionals. Line 257 assigns repo_name_arg but never uses it (F841). The conditional logic on lines 263-266 is convoluted.
Apply this diff to remove the unused variable and simplify:
remote_arg = args.remote or kv.get("remote")
slug_arg = args.slug or kv.get("slug")
repo_kv = kv.get("repo")
owner_arg = args.owner or kv.get("owner")
- repo_name_arg = args.repo or (kv.get("repo_name") if repo_kv and "/" not in repo_kv else None)
if remote_arg:
owner, repo = parse_remote_url(remote_arg)
elif slug_arg:
owner, repo = parse_slug(slug_arg)
elif repo_kv and "/" in repo_kv:
owner, repo = parse_slug(repo_kv)
- elif owner_arg and (args.repo or kv.get("repo")) and "/" not in (args.repo or kv.get("repo")): # owner/repo split
- owner, repo = owner_arg, (args.repo or kv.get("repo"))
+ elif owner_arg:
+ repo_arg = args.repo or kv.get("repo")
+ if repo_arg and "/" not in repo_arg:
+ owner, repo = owner_arg, repo_arg
else:Based on static analysis.
🧰 Tools
🪛 Ruff (0.14.3)
257-257: Local variable repo_name_arg is assigned to but never used
Remove assignment to unused variable repo_name_arg
(F841)
🤖 Prompt for AI Agents
In tools/review/get_comments_markdown.py around lines 253 to 267, remove the
unused repo_name_arg assignment and simplify the owner/repo precedence: check
remote_arg first (parse_remote_url), then slug_arg (parse_slug), then if repo_kv
contains "/" parse_slug(repo_kv), then if owner_arg is provided and the repo
value (args.repo or kv.get("repo")) exists and does not contain a "/" use
owner_arg and that repo value; otherwise fall through to the existing else
branch. Drop the unused variable and collapse the nested conditionals into this
clear linear precedence.
| except urllib.error.HTTPError as e: | ||
| msg = e.read().decode("utf-8", errors="ignore") | ||
| raise SystemExit(f"GitHub API error {e.code} on {path}: {msg}") | ||
| raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Improve exception chaining for better debugging.
The error handling discards the original exception context. Use raise ... from e to preserve the traceback chain per PEP 3134, which aids debugging.
Apply this diff:
except urllib.error.HTTPError as e:
msg = e.read().decode("utf-8", errors="ignore")
- raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}")
+ raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}") from eBased on static analysis.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except urllib.error.HTTPError as e: | |
| msg = e.read().decode("utf-8", errors="ignore") | |
| raise SystemExit(f"GitHub API error {e.code} on {path}: {msg}") | |
| raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}") | |
| except urllib.error.HTTPError as e: | |
| msg = e.read().decode("utf-8", errors="ignore") | |
| raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}") from e |
🧰 Tools
🪛 Ruff (0.14.3)
46-46: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In tools/review/seed_feedback_from_github.py around lines 44 to 46, the except
block for urllib.error.HTTPError discards the original exception context; modify
the raise to preserve exception chaining by re-raising the SystemExit from the
caught exception (use "raise SystemExit(...) from e") so the original traceback
is retained for debugging.
| def parse_link_header(link_header: str) -> dict: | ||
| """Parse GitHub Link header into a dict of {rel: url}.""" | ||
| rels = {} | ||
| if not link_header: | ||
| return rels | ||
| parts = [p.strip() for p in link_header.split(",")] | ||
| for p in parts: | ||
| if ";" not in p: | ||
| continue | ||
| url_part, rel_part = p.split(";", 1) | ||
| url = url_part.strip().strip("<>") | ||
| if "rel=" in rel_part: | ||
| rel = rel_part.split("rel=", 1)[1].strip().strip('"') | ||
| rels[rel] = url | ||
| return rels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRITICAL: Eliminate code duplication across modules.
This parse_link_header function is duplicated in tools/review/get_comments_markdown.py (lines 56-70 as _parse_link_header). Both gh_get_all functions (this file lines 72-109 and get_comments_markdown.py lines 73-107) are also nearly identical. This violates DRY principles and creates a maintenance nightmare.
Solution: Extract shared utilities into a common module.
Create tools/review/github_api.py:
#!/usr/bin/env python3
"""Shared GitHub API utilities for review tooling."""
import json
import urllib.error
import urllib.parse as parse
import urllib.request
from typing import Dict, List, Tuple
API = "https://api.github.com"
def http_get(url: str, token: str) -> Tuple[object, Dict[str, str]]:
"""Execute GET request and return (json_data, headers)."""
req = urllib.request.Request(url)
req.add_header("Accept", "application/vnd.github+json")
if token:
req.add_header("Authorization", f"Bearer {token}")
try:
with urllib.request.urlopen(req, timeout=30) as r:
data = json.loads(r.read().decode("utf-8"))
headers = {k.lower(): v for k, v in r.headers.items()}
return data, headers
except urllib.error.HTTPError as e:
msg = e.read().decode("utf-8", errors="ignore")
raise SystemExit(f"GitHub API error {e.code} on {url}: {msg}") from e
except urllib.error.URLError as e:
raise SystemExit(f"Network error contacting GitHub: {e}") from e
def parse_link_header(link_header: str) -> Dict[str, str]:
"""Parse GitHub Link header into {rel: url} dict."""
rels = {}
if not link_header:
return rels
for part in link_header.split(","):
part = part.strip()
if not part or ";" not in part:
continue
url_part, rel_part = part.split(";", 1)
url = url_part.strip().strip("<>")
if "rel=" in rel_part:
rel = rel_part.split("rel=", 1)[1].strip().strip('"')
rels[rel] = url
return rels
def gh_get_all(path: str, token: str) -> List[dict]:
"""GET and paginate GitHub API, returning all items.
Adds per_page=100 and follows Link rel="next".
"""
url = API + path
parsed = parse.urlparse(url)
qs = parse.parse_qsl(parsed.query, keep_blank_values=True)
qs = [(k, v) for (k, v) in qs if k not in ("per_page", "page")]
qs.append(("per_page", "100"))
url = parse.urlunparse(parsed._replace(query=parse.urlencode(qs)))
items: List[dict] = []
while True:
data, headers = http_get(url, token)
if isinstance(data, list):
items.extend(data)
else:
for key in ("items", "nodes"):
if key in data and isinstance(data[key], list):
items.extend(data[key])
break
else:
raise SystemExit("Unexpected GitHub response shape during pagination")
links = parse_link_header(headers.get("link", ""))
if "next" in links:
url = links["next"]
continue
break
return items
def normalize_text(s: str) -> str:
"""Normalize line endings and whitespace."""
return (s or "").replace("\r\n", "\n").strip()Then update both scripts to import from this shared module. Would you like me to generate those refactored imports?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: James Ross <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/review/get_comments_markdown.py (1)
253-269: Kill the deadrepo_name_argand tighten precedence.
repo_name_argis still assigned then discarded, tripping Ruff F841 and leaving the owner/repo precedence brittle. Collapse the logic so you only compute the repo token when you actually need it—this is the exact fix already suggested earlier.- repo_name_arg = args.repo or (kv.get("repo_name") if repo_kv and "/" not in repo_kv else None) - if remote_arg: owner, repo = parse_remote_url(remote_arg) elif slug_arg: owner, repo = parse_slug(slug_arg) elif repo_kv and "/" in repo_kv: owner, repo = parse_slug(repo_kv) - elif owner_arg and (args.repo or kv.get("repo")) and "/" not in (args.repo or kv.get("repo")): # owner/repo split - owner, repo = owner_arg, (args.repo or kv.get("repo")) + elif owner_arg: + repo_arg = args.repo or kv.get("repo") + if repo_arg and "/" not in repo_arg: + owner, repo = owner_arg, repo_arg else:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md(1 hunks)docs/code-reviews/PR3/f775e24c1d3578c42f823710cd23295d4d90a652.md(1 hunks)tools/review/get_comments_markdown.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: flyingrobots
Repo: flyingrobots/draft-punks PR: 2
File: SECURITY.md:5-15
Timestamp: 2025-09-29T09:02:31.228Z
Learning: In the draft-punks repository SECURITY.md file, maintain the whimsical German-accented English writing style with musical metaphors when making suggestions. The document uses phrases like "ze", "vill", "und" and references to BunBun, Kapellmeister, and musical terms.
📚 Learning: 2025-09-29T08:57:34.199Z
Learnt from: flyingrobots
Repo: flyingrobots/draft-punks PR: 2
File: docs/code-reviews/PR2/410ce3439856a4cdf6ae7df8fd3936e7bbb67bba.md:81-85
Timestamp: 2025-09-29T08:57:34.199Z
Learning: When reviewing auto-generated CodeRabbit content or review artifacts, do not criticize marketing links or promotional content that are part of the standard CodeRabbit template output.
Applied to files:
docs/code-reviews/PR3/25cb2bb4b000cad26fd5fcc02cb5186775827c40.md
🧬 Code graph analysis (1)
tools/review/get_comments_markdown.py (1)
tools/review/seed_feedback_from_github.py (3)
gh_get_all(72-109)normalize(112-113)main(116-200)
🪛 LanguageTool
docs/code-reviews/PR3/f775e24c1d3578c42f823710cd23295d4d90a652.md
[style] ~610-~610: This phrase is redundant. Consider writing “scrutiny”.
Context: ...*get_comments_markdown.py**: Requires careful scrutiny of argument parsing logic, GitHub API i...
(CAREFUL_SCRUTINY)
🪛 markdownlint-cli2 (0.18.1)
docs/code-reviews/PR3/f775e24c1d3578c42f823710cd23295d4d90a652.md
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
82-82: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
220-220: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
308-308: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
388-388: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
500-500: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
626-626: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
673-673: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
tools/review/get_comments_markdown.py
31-31: typing.Dict is deprecated, use dict instead
(UP035)
31-31: typing.List is deprecated, use list instead
(UP035)
31-31: typing.Tuple is deprecated, use tuple instead
(UP035)
42-42: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
44-44: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
99-99: Avoid specifying long messages outside the exception class
(TRY003)
112-116: Starting a process with a partial executable path
(S607)
117-117: Consider moving this statement to an else block
(TRY300)
118-118: Do not catch blind exception: Exception
(BLE001)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Starting a process with a partial executable path
(S607)
150-150: Do not catch blind exception: Exception
(BLE001)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
162-162: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Local variable repo_name_arg is assigned to but never used
Remove assignment to unused variable repo_name_arg
(F841)
| kv = parse_kv_args(argv) | ||
| # Re-parse with provided argv | ||
| args = ap.parse_args([a for a in argv if "=" not in a]) | ||
|
|
||
| pr_number = args.pr or int(kv.get("pr", "0") or 0) | ||
| if not pr_number: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stop stripping --owner=foo flags.
By filtering out every token containing “=”, you silently drop perfectly valid GNU-style flags like --owner=foo, --repo=bar, and --remote=https://…. That leaves argparse blind to those inputs, so the script fails to detect repo coordinates and bombs out. Filter only the bare key=value shorthands, not canonical --flag=value arguments.
- args = ap.parse_args([a for a in argv if "=" not in a])
+ cli_args = [
+ a for a in argv
+ if not ("=" in a and not a.startswith("--"))
+ ]
+ args = ap.parse_args(cli_args)🤖 Prompt for AI Agents
In tools/review/get_comments_markdown.py around lines 239 to 244, the code
currently strips every token containing "=" before passing args to argparse,
which removes valid GNU-style flags like "--owner=foo". Change the filtering so
only bare key=value shorthands (tokens that do NOT start with "-" or "--") are
removed; preserve any token that contains "=" but begins with "-" (canonical
flags). Update the list comprehension used for ap.parse_args to exclude only
tokens matching the bare "key=value" pattern (no leading dash) and leave all
"--flag=value" or "-f=value" tokens intact.