feat: Add --repos flag to scope weekly report - #32
Conversation
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Link the report header back to the github-weekly-report skill so a reader can find and improve the generator, matching the breadcrumb pattern requested in rossoctl/automation#41. Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
clawgenti
left a comment
There was a problem hiding this comment.
Adds --repos OWNER/REPO ... to report.py and epic-tracker.py so callers can scope the weekly report to an explicit repo set instead of org-wide discovery, and threads the same set into the epic tracker for consistency — plus SKILL.md docs and a report-header breadcrumb linking back to the skill.
All checks pass. Ready for human review.
Reviewed by clawgenti using the github-pr-review skill
esnible
left a comment
There was a problem hiding this comment.
Scoping works for the intended single-org case and the backward-compatible default (--repos omitted → org-wide discovery) is preserved cleanly. Commits are all signed off with conventional prefixes, and CI is green.
One blocking issue: --repos advertises OWNER/REPO but discards the owner, so passing a repo from a different org silently reports on the wrong repo. Details inline.
Areas reviewed: Python, Docs
Agent/IDE config (.claude/.vscode): none
Commits: 4, all signed-off: yes
CI status: passing (DCO, PR title, project automation)
| item = item.strip() | ||
| if not item: | ||
| continue | ||
| _, _, name = item.rpartition('/') |
There was a problem hiding this comment.
must-fix: the owner half of OWNER/REPO is parsed and then thrown away, so a repo outside --org is silently reported against the wrong owner.
rpartition keeps only the tail, and every downstream fetch rebuilds the slug from --org: report.py at get_merged_prs/get_open_prs/get_new_issues/get_open_issue_count/get_ci_runs (all -R f'{org}/{repo}'), and epic-tracker.py:47 in get_epics. Nothing validates that the parsed owner matches --org.
Reproduced with the function as written, --org rossoctl:
--repos value bare name actual query target reattributed?
rossoctl/operator operator rossoctl/operator no
otherorg/operator operator rossoctl/operator YES <-- bug
a/b/c c rossoctl/c YES <-- bug
The failure mode is the bad kind: no error, just a report whose header says one thing while the data came from another org's repo of the same name. a/b/c is also accepted rather than rejected.
Two ways out, depending on the intended contract:
-
Validate and reject (smaller fix, matches the flag's current single-org usage) — if the value contains a
/and the owner differs from--org, exit with an error, or at minimum warn on stderr:owner, sep, name = item.rpartition('/') if sep and owner != org: sys.exit(f"--repos entry '{item}' is not in --org '{org}'")
This needs
orgthreaded intonormalize_repo_args. -
Honor the owner end-to-end — keep the full slug and pass
-R <owner>/<name>through both scripts, so cross-org sets genuinely work as theOWNER/REPOmetavar implies.
If cross-org is out of scope for now, option 1 plus a metavar change to REPO (or documenting that entries must be within --org) would make the contract honest.
There was a problem hiding this comment.
Fixed in f4ae90e (option 1 — validate and reject within --org).
normalize_repo_args now takes org and accepts only a bare name or an owner-qualified name whose owner matches --org; a differing owner or a malformed entry (a/b/c) exits with a clear error instead of silently reattributing. Verified against your repro table:
operator -> {name: operator}
rossoctl/operator -> {name: operator}
otherorg/operator -> REJECTED: not in --org rossoctl
a/b/c -> REJECTED: malformed
Also changed the metavar OWNER/REPO -> REPO and documented the within-org contract, per your suggestion. Kept cross-org out of scope for this flag — broadening to arbitrary owners is a separate design question I am taking to a discussion.
esnible
left a comment
There was a problem hiding this comment.
Claude considers the org vs org/name confusion a "must-fix", but I will lower my response to "Comment".
Owner-stripping is unreachable via the weekly-report.sh wrapper; downgrading to non-blocking.
|
@esnible Thanks for the review and downgrade to comment. I did consider this design decision, and I opted for a simple |
|
Cross-linking the companion wrapper PR: rossoctl/automation#59 ( Ordering: automation#59 should merge after this PR — its Note for the owner-attribution must-fix on Assisted-By: Claude Code |
normalize_repo_args parsed OWNER/REPO and discarded the owner, while
every downstream fetch rebuilds the slug as f'{org}/{name}'. An entry
like 'otherorg/operator' was therefore queried as '{--org}/operator'
and silently reported against the wrong owner; a malformed 'a/b/c' was
also accepted.
Scope the flag to a single owner: accept a bare name or an
owner-qualified name whose owner matches --org, and exit with a clear
error otherwise (or on a malformed entry). Update the metavar to REPO
and document the within-org contract.
The automation wrapper (rossoctl/automation#59) emits '$ORG/name'
(owner always == $ORG), so it stays compatible with this contract.
Cross-owner repo sets are out of scope for this flag; broadening to
arbitrary owners is a separate design discussion.
Addresses esnible's must-fix review on rossoctl#32.
Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
Summary
Adds a
--reposflag to the github-weekly-report generator so callers canscope a report to an explicit set of repos instead of discovering every repo
in the org. This is the generator half of scoping the weekly report to the
curated core repos; the automation wrapper that supplies the allowlist lives
in rossoctl/automation.
Backward compatible: when
--reposis omitted, the report discovers all reposin the org exactly as before.
Changes
scripts/report.py— add--repos OWNER/REPO ...; when present, skiporg-wide discovery and report on exactly those repos. Thread the same repo
set into the epic tracker so epics are scoped consistently.
scripts/epic-tracker.py— add--repos; when present, scan only thoserepos instead of enumerating the org.
scripts/report.py— add a breadcrumb to the report header linking back tothe github-weekly-report skill, so a reader of a generated issue can find and
improve the generator (matches the pattern requested in
Feat: Add a link back to the skill when opening a link issue (breadcrumb) automation#41).
SKILL.md— document--reposand note that automation deployments passthe core-repo allowlist via the
weekly-report.shwrapper.Testing
Ran the generator scoped to explicit repos (correct section count, no org
discovery) and with
--reposomitted (identical org-wide output). Headerrenders the breadcrumb link.
Fixes #29
Assisted-By: Claude Code