feat: Add weekly-report.sh scoped to core repos - #59
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>
clawgenti
left a comment
There was a problem hiding this comment.
Adds scripts/weekly-report.sh, a thin wrapper that resolves the org identity and core-repo allowlist via the shared library, then invokes report.py --repos scoped to exactly those repos — plus a hermetic test covering arg-building, date-range pass-through, and fail-loud on a missing generator.
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.
Clean, well-scoped wrapper. The hermetic test is the right shape — fixture allowlist via CORE_REPOS_FILE, stub generator via REPORT_PY, no network or real config — and it covers arg-building, window pass-through, and fail-loud on a missing generator. Wiring it into the suite runner in the same PR is good practice.
Two non-blocking notes inline. The first is a real path bug worth fixing before this gets used without an explicit REPORT_PY; approving on the understanding that automation callers set it explicitly.
Things I checked that are not problems, so they don't get re-raised:
PROFILE_FLAG/ORG_FLAGwithset -u— safe.load_org_profiledereferences both with:-defaults (org.sh:110,127), so leaving them unset is fine. Shellcheck reports SC2034 for both, but CI only shellchecks the five library modules (tests.yml:32), notscripts/*.sh, and the existingpr-review-impact.shonmaintriggers three of the same warning. Established convention here.- Unquoted
$repos— intentional word-splitting, correctly commented with anSC2086disable. - Interaction with the companion
--reposbug — agent-skills#32 strips the owner fromOWNER/REPOvalues, butget_core_reposemits$ORG/$repo, so the owner always equals--organd that bug cannot affect this caller.
Areas reviewed: Shell, CI/GitHub Actions, Tests
Agent/IDE config (.claude/.vscode): none
Commits: 2, all signed-off: yes
CI status: passing (DCO, PR title, tests on ubuntu-latest + macos-latest, project automation)
|
|
||
| # Locate the generator. Default points at the deployed report generator; override | ||
| # with REPORT_PY in dev. | ||
| REPORT_PY="${REPORT_PY:-$HOME/workspaces/shared/skills/github-report-generator/scripts/report.py}" |
There was a problem hiding this comment.
suggestion: the default path names a skill directory that does not exist, so the documented default can never resolve.
The directory in agent-skills is github-weekly-report, not github-report-generator:
skills/
automation-health-dashboard
dep-bump-fixer
dep-bump-scanner
github-pr-review
github-weekly-report <-- here
link-health-fixer
link-health-scanner
skills/github-report-generator returns 404, and an org-wide code search for github-report-generator finds it nowhere outside this PR. The companion PR (agent-skills#32) edits skills/github-weekly-report/scripts/report.py.
So anyone invoking the wrapper without presetting REPORT_PY gets:
Error: report generator not found at: $HOME/workspaces/shared/skills/github-report-generator/scripts/report.py
The fail-loud handling right below is good — it turns this into a clear error rather than something silent — but the default should point at the real directory:
REPORT_PY="${REPORT_PY:-$HOME/workspaces/shared/skills/github-weekly-report/scripts/report.py}"Worth double-checking the $HOME/workspaces/shared/skills/... prefix against how the deployment actually lays out skills, since I could only verify the trailing directory name from the repo.
There was a problem hiding this comment.
Fixed in 10a1f58 — default now points at github-weekly-report (the upstream skill name in agent-skills), and the doc comment is aligned.
Good catch on the prefix caveat too: I verified the $HOME/workspaces/shared/skills/... layout on the deployment host. The trailing dir there is currently github-report-generator (a legacy name), and this skill is its only consumer — so renaming it to the canonical github-weekly-report is safe. That host rename is a merge-time step gated on agent-skills#32 (flagged on the PR + blocked label); the running cron is unaffected meanwhile since its standing order sets REPORT_PY explicitly.
|
|
||
| got=$(ORG=rossoctl \ | ||
| CORE_REPOS_FILE="$TEST_TMPDIR/repos.txt" \ | ||
| REPORT_PY="$TEST_TMPDIR/report.py" \ |
There was a problem hiding this comment.
suggestion: all three invocations set REPORT_PY explicitly (here, line 39, and line 46), so the built-in default is never exercised — which is why the wrong path in the other comment ships with CI fully green.
That is the correct call for the two happy-path assertions: they must be hermetic, and pointing at a real deployed generator would break that.
If you want a cheap regression guard for the default without giving up hermeticity, assert on its shape rather than its resolvability — e.g. run the wrapper with REPORT_PY unset and HOME pointed at a temp dir, then check that the error message names the expected skill directory:
out=$(ORG=rossoctl CORE_REPOS_FILE="$TEST_TMPDIR/repos.txt" \
HOME="$TEST_TMPDIR/fakehome" bash "$WRAPPER" 2>&1 || true)
case "$out" in
*github-weekly-report/scripts/report.py*) ;;
*) echo "FAIL default REPORT_PY path: $out"; fail=1 ;;
esacThat pins the default against future renames while still touching no network and no real config. Entirely optional.
There was a problem hiding this comment.
Deferring this one to a follow-up. The shape-only guard is the right call, but the exact error-message shape depends on the finalized canonical path (which settles once agent-skills#32 lands and the host dir is renamed). Tracking it so it is not lost.
|
Thanks for the review and suggestions @esnible. I will address these with fresh eyes soon. |
The wrapper's REPORT_PY default named the skill directory `github-report-generator`, which does not exist upstream. The skill is named `github-weekly-report` in agent-skills (and it is the sole consumer of the deployed generator dir), so the documented default could never resolve without an explicit REPORT_PY override. Point the default at the canonical `github-weekly-report` name and align the doc comment. Addresses esnible's review on rossoctl#59. The built-in default is still not exercised by the hermetic tests (they set REPORT_PY explicitly); a shape-only regression guard for it is deferred to a follow-up. Assisted-By: Claude Code (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Gloire Rubambiza <gloire@ibm.com>
|
Addressed the Merge ordering — this PR must not merge before rossoctl/agent-skills#32. The wrapper is the caller of #32's The host redeploy (renaming the deployed dir to On the consistency point across the two PRs: Deferring your second suggestion (the hermetic shape-only regression guard for the default) to a follow-up, since it depends on the finalized canonical path. 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
scripts/weekly-report.sh, a thin wrapper that scopes the weekly orgreport to the curated core-repo allowlist instead of discovering every repo
in the org.
The wrapper resolves the org identity and the core-repo list through the
shared library (
load_org_profile+get_core_repos), then invokes thePython report generator with
--reposset to exactly those repos. When thegenerator is called without
--repos(the previous default), org-widediscovery is unchanged, so this is additive.
Changes
scripts/weekly-report.sh— wrapper: resolves org + core allowlist, buildsreport.py --org <org> --repos <core repos> [--since/--until/--output/--json-output],fails loud when the allowlist is empty or the generator is missing.
Supports
--profileand--orgoverrides.tests/test-weekly-report.sh— hermetic test (fixture allowlist viaCORE_REPOS_FILE, stub generator viaREPORT_PY): asserts arg-building,--since/--untilpass-through, and fail-loud on a missing generator. Nonetwork or real config touched.
.github/workflows/tests.yml— wires the new test into the suite runner.The companion
--repossupport in the generator lives inrossoctl/agent-skills (separate PR).
Fixes #57
Merge ordering (do not merge before agent-skills#32)
This PR carries the
blockedlabel. It must merge afterrossoctl/agent-skills#32, which adds the
--repossupport (and theowner-attribution fix) to the
report.pygenerator this wrapper calls. Thewrapper's
REPORT_PYdefault resolves to the deployedgithub-weekly-reportgenerator, so the host needs the merged generator in place first.
Merge-time deployment runbook (host:
kagenti-bot)Executed at merge time, after agent-skills#32 has merged and the nightly sync
has pulled it into
~/kagenti/agent-skills. Until then the running weeklycron is unaffected: its standing order sets
REPORT_PYto the legacygithub-report-generatorpath explicitly.Confirm the merged generator is present in the nightly clone:
ls ~/kagenti/agent-skills/skills/github-weekly-report/scripts/report.pyand that it contains the owner-validation fix (
normalize_repo_args(... org)).Deploy the generator under the canonical name into the shared skills dir:
~/kagenti/agent-skills/skills/github-weekly-report/scripts/{report.py,epic-tracker.py}to
~/workspaces/shared/skills/github-weekly-report/scripts/Repoint the weekly-report standing order's
REPORT_PYfrom the legacy.../github-report-generator/...to.../github-weekly-report/..., usingthe openclaw cron CLI (do NOT hand-edit
~/.openclaw/cron/jobs.json— ahand-edited job lacks id/createdAtMs and never fires).
Retire the legacy dir once nothing references it:
~/workspaces/shared/skills/github-report-generator/and itsSKILL.md.bak-*.Verify end to end:
REPORT_PYunset and confirmit finds
.../github-weekly-report/scripts/report.py.--since/--untilwindow and confirm thereport renders and scopes to the core repos.
Deploy the updated wrapper (
scripts/weekly-report.sh) to~/workspaces/clawgenti/scripts/(local repo copy is authoritative and wasidentical to the deployed copy before this fix).
Follow-ups (out of scope here)
REPORT_PYdefault(esnible's second review suggestion) — deferred until the canonical path is final.
canonical clones + an explicit hotfix-overlay / drift-detector.
Assisted-By: Claude Code