[rubambiza]

Summary

weekly-report.sh defaulted REPORT_PY to $HOME/workspaces/shared/skills/github-weekly-report/scripts/report.py — an OpenClaw-host path. On a stock RepoMan plugin install (e.g. a DAM pod), repoman run weekly-report hard-failed before any gh call because that path does not exist.

The generator ships in the same plugin, beside the skill. This changes the default to resolve relative to the wrapper:

REPORT_PY="${REPORT_PY:-$SCRIPT_DIR/../../github-weekly-report/scripts/report.py}"

In the bundled plugin layout (skills/repoman/scripts/weekly-report.sh) this resolves to skills/github-weekly-report/scripts/report.py. REPORT_PY stays an escape hatch for other layouts (the OpenClaw deployment can export it explicitly). No change to the weekly-report run-profile row in repoman-run.sh.

Testing

• Extended tests/test-weekly-report.sh with a regression case: with REPORT_PY unset, the "not found" error must name the wrapper-relative path and must not name a $HOME/workspaces path. Watched it fail (RED) against the old default, pass (GREEN) after the fix.
• Verified in a simulated bundle layout: with REPORT_PY unset, the wrapper finds the bundled report.py and runs (exit 0).
• shellcheck clean.

Acceptance criteria

• repoman run weekly-report succeeds on a clean plugin install with REPORT_PY unset.
• Exporting REPORT_PY still overrides the default.
• No change to the weekly-report run-profile row.
• Re-vendored into rossoctl/repoman with a _vendor.json bump (follow-on, after this merges).

Fixes #126

Assisted-By: Claude Code

Scope (clarified per review)

This makes weekly-report portable on a plugin install (the self-contained
repoman bundle) and keeps the OpenClaw host working via a $HOME/workspaces
fallback candidate. It does not make bash scripts/weekly-report.sh runnable
from an automation checkout — no report.py ships in automation, so there the
default simply names a different nonexistent path. REPORT_PY remains the
explicit override for any other layout.

[clawgenti]

The wrapper-relative default is the right fix: SCRIPT_DIR is defined and quoted, and the bundled-layout claim checks out against the plugin/repo structure. CI is red on both platforms, though — shellcheck SC2034 flags the new expected_default variable as unused, so this can't merge as-is.

Findings:

• must-fix — tests/test-weekly-report.sh:87 — expected_default is assigned but never used; shellcheck at severity=warning fails the shellcheck (repoman tests) job on both ubuntu-latest and macos-latest, so the PR's shellcheck-clean claim does not hold in CI.
• suggestion — tests/test-weekly-report.sh:93 — the pass branch matches any path containing github-weekly-report/scripts/report.py; matching "$expected_default" exactly would both use the variable and make the regression assertion precise.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

The follow-up test commit resolves both findings from the last round: expected_default is now consumed by the tightened assertion (SC2034 gone, shellcheck green in CI) and the pass branch requires the exact wrapper-relative path while still rejecting any $HOME/workspaces default. CI is green on all four checks.

All checks pass. Ready for human review.

---

Reviewed by clawgenti using the github-pr-review skill

[jimcadden]

The wrapper-relative default is the right direction for the plugin layout, and I verified it end to end at 9870817. Two things I'd want settled before this merges: it regresses the deployed OpenClaw layout, and the new test never proves the positive path.

Verified (not taken from the description)

• bash tests/test-weekly-report.sh passes; RED when I restored origin/main's wrapper (fails on workspaces/shared), green after restoring the branch — the guard does what it claims.
• Simulated bundle layout (skills/repoman/scripts/ beside skills/github-weekly-report/scripts/report.py), REPORT_PY unset: the default resolves, the generator runs, the footer is appended, exit 0.
• shellcheck --severity=warning scripts/weekly-report.sh tests/test-weekly-report.sh clean. Note the wrapper is in no CI shellcheck group (.github/workflows/tests.yml:28-42 lints library modules and test files only), so that claim stays author-side — I reproduced it locally.

P1 — behavioural regression on the OpenClaw host

Scripts are deployed to ~/workspaces/clawgenti/scripts/ (README.md:95, docs/program-template.md:483), and the old default's own path says where the generator lives there: ~/workspaces/shared/skills/github-weekly-report/scripts/report.py. From that layout the new default is ~/workspaces/github-weekly-report/scripts/report.py, which cannot exist — so after the re-vendor, weekly-report hard-fails on that host unless the invocation exports REPORT_PY.

The description treats that as acceptable ("the OpenClaw deployment can export it explicitly"), but nothing in the deploy does it: config/org.env carries only PROFILE_* keys (README.md:74), and the cron command in docs/program-template.md:367 exports nothing. That is the "identical invocation everywhere" contract the issue was filed against.

Two ways out — either is fine, but it should be a decision, not a follow-on:

1. Candidate list (my preference, ~6 lines, bash 3.2 safe): when REPORT_PY is unset, take the bundled path if it exists, else the $HOME/workspaces/... path, else fail with the bundled path named. Portable on both layouts; removing the personal path stays with #116.

   if [ -z "${REPORT_PY:-}" ]; then
     bundled="$SCRIPT_DIR/../../github-weekly-report/scripts/report.py"
     deployed="$HOME/workspaces/shared/skills/github-weekly-report/scripts/report.py"
     for candidate in "$bundled" "$deployed"; do
       if [ -f "$candidate" ]; then REPORT_PY="$candidate"; break; fi
     done
     REPORT_PY="${REPORT_PY:-$bundled}"   # for the error message
   fi

   Cost: the personal path survives, as a fallback only.

2. Ship as-is, move the break into this PR: export REPORT_PY in the OpenClaw cron command and document it next to README.md:95, so the deployment change lands with the vendor bump instead of after it. Cost: a host-side change has to ship in lockstep or weekly-report breaks silently there.

P2 — the test only asserts the error text

The new case matches the string the wrapper prints, not that a bundled generator is found: it would still pass if the default were <wrapper>/../../nonsense/report.py. The acceptance criterion "repoman run weekly-report succeeds on a clean plugin install with REPORT_PY unset" is manual-only today.

~10 lines reusing the existing $TEST_TMPDIR fixtures would cover it — this is the shape I ran against the branch, green:

bundle="$TEST_TMPDIR/bundle"
mkdir -p "$bundle/skills/repoman/scripts" "$bundle/skills/github-weekly-report/scripts"
cp "$SCRIPT_DIR/../scripts/"*.sh "$bundle/skills/repoman/scripts/"   # wrapper + lib modules
printf 'import sys\nprint("RAN", " ".join(sys.argv[1:]))\n' \
  > "$bundle/skills/github-weekly-report/scripts/report.py"
got=$(REPOMAN_REPOS_FILE="$TEST_TMPDIR/repos.json" REPOMAN_SKILL_DIR="$TEST_TMPDIR/meta" \
      bash "$bundle/skills/repoman/scripts/weekly-report.sh" 2>/dev/null | head -n 1)
[ "$got" = "RAN --repos rossoctl/alpha alice/beta" ] || { echo "FAIL bundled default: [$got]"; fail=1; }

P3 — the exact match pins the implementation

expected_default recomputes <repo>/scripts/../../…, so option 1 above fails this test while the behaviour gets strictly better. I'd assert the property instead (tail github-weekly-report/scripts/report.py, and no workspaces/), and let the P2 case cover the real behaviour. The SC2034 catch from the previous round was right; it's the tightness of the assertion I'd relax.

Scope, for the description

This does not make bash scripts/weekly-report.sh work from an automation checkout — no generator ships in this repo, so the error simply names a different nonexistent path. "Portable on a plugin install", not "portable everywhere".

The re-vendor follow-on is bigger than a bump

skills/repoman/scripts/_vendor.json pins source_sha 0311752, which predates 63f78ef (the pinned-SHA footer), so re-vendoring also brings the footer and the exec python3 restructure. rossoctl/repoman has no weekly-report test (tests/test-vendor-engine.sh only), so the vendored wrapper would ship untested there. Not a blocker for this PR, just not a one-file bump.

Docs

#126's own notes ask for a spec line on how a program resolves its own assets; the run-path spec's §Deferred and #116 both still list the personal REPORT_PY default as outstanding. Whichever option you take above, one of those needs updating in this PR.

---

Mergeable once P1 is decided and P2 is in. Reviewed at 9870817c51997d7392406c7a5af1f6587d9cc55.

[rubambiza]

Thanks for the thorough pass — the P1 regression was a real miss. Addressed in 3a4c1ad (code) and 232a2b9 (docs):

P1 (OpenClaw regression) — candidate list, your option 1. REPORT_PY unset now tries the bundled plugin path, then the $HOME/workspaces/shared/... path, using the first that exists; the error names the bundled path if neither does. Both layouts work with no per-host config. The personal-path fallback is marked transitional and stays tracked in #116.

P2 (positive path) — added. A real bundle-layout fixture with the generator present and REPORT_PY unset now asserts the wrapper finds it, runs it, and emits the report (RAN --repos ..., exit 0), not just the error text. (Your | head -n 1 snippet tripped SIGPIPE → exit 141 under set -o pipefail; I capture full output first, then take line 1.)

P3 (exact match pinned the implementation) — relaxed. The negative case is now a property check (names the bundled generator, never a workspaces path), so it stays green under the candidate-list order while P2 carries the precise behaviour.

Scope wording — added a Scope section to the PR description: portable on a plugin install, not everywhere (no generator ships in automation itself).

Docs — done in 232a2b9. The run-path spec §Deferred / #116 note now says #116's remaining task for this file is dropping the personal fallback candidate, not the whole default (the bundled path is now primary).

Re-vendor follow-on — agreed it is more than a one-file bump: re-vendoring moves _vendor.json past 63f78ef (pinned-SHA footer + exec python3 restructure), and rossoctl/repoman has no weekly-report test yet. It is not separately tracked; it overlaps the re-vendor step already listed in rossoctl/repoman#10, and automation#126's acceptance criterion 4 is the other hook. I will land it as a repoman PR after this merges rather than fold it here.

On the /standing-orders + OpenClaw deploy-layout question I raised over Slack — whether that layout belongs in the /repoman plugin, moves to a dedicated branch, or is retired — let us settle it there, since it bounds how long the $HOME/workspaces fallback needs to live.

Reviewed-at bump: now at 232a2b9.

Assisted-By: Claude Code

[clawgenti]

All three of Jim's asks are addressed and verified against the head SHA: the candidate-list default covers both deployment layouts with no per-host config, the positive-path test proves the bundled generator is found and run with REPORT_PY unset, and the negative assertion is now a property check. shellcheck (severity=warning) is clean on both changed files and the full weekly-report test suite passes locally against 232a2b9.

Findings:

• suggestion — tests/test-weekly-report.sh:119 — the deployed-fallback branch of the candidate list (bundled copy absent, $HOME/workspaces copy present) has no test.
• nit — scripts/weekly-report.sh:55 — ENVIRONMENT help says "default: the bundled generator beside this skill"; the actual default is now the candidate list.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

suggestion — This case proves the bundled candidate end to end, but the deployed-fallback branch — bundled copy absent, $HOME/workspaces/shared/... present, so the wrapper resolves and runs the deployed generator — is the OpenClaw-host scenario that motivated the candidate list (Jim's P1) and is now the only resolution path with no test. A sibling case mirroring this one (fake $HOME containing only the deployed layout, REPORT_PY unset, assert the generator runs) would pin it.

[rubambiza]

Fixed in 2ddab94 — added a candidate-2 positive test (bundled absent, $HOME/workspaces present); verified it goes red when the fallback is dropped or reordered.

[clawgenti]

nit — The help reads "default: the bundled generator beside this skill", but the actual default is a candidate list — bundled path first, then the deployed $HOME/workspaces path. On the OpenClaw host the resolved default is the deployed path, so a short clause mentioning the fallback would keep the help accurate.

[rubambiza]

Fixed in 2ddab94 — the ENVIRONMENT help now describes the candidate list (bundled path first, then the $HOME/workspaces deploy path).

[jimcadden]

Re-review at 232a2b95

Fourth pass. All three points from the previous review are addressed, and I re-verified each one against the code rather than reading the description.

Verified locally

• bash tests/test-weekly-report.sh PASS; RED against origin/main's wrapper (the new negative case trips on the old $HOME/workspaces default) — the guard still bites.
• Candidate 2 actually works. Bundle layout with its generator removed, plus a fake HOME holding workspaces/shared/skills/github-weekly-report/scripts/report.py: the wrapper finds it, runs it, footer appended, exit 0. That is the OpenClaw fix from P1, behaving as documented.
• Explicit REPORT_PY is not silently rescued. With a valid candidate present, REPORT_PY=/nope.py still errors on /nope.py — override semantics preserved.
• Both candidates absent → error names the bundled path, matching the comment.
• shellcheck --severity=warning clean on both changed shell files; test-repoman-run.sh, test-attribution-consumers.sh, test-skill-meta.sh, test-lib-modules.sh PASS.
• DCO 4/4 signed off, all subjects conventional and ≤66 chars, CI 4/4 green.

Author: rubambiza (MEMBER — maintainer)
Areas reviewed: Shell, Tests, Docs (markdown)
Agent/IDE config (.claude/.vscode): none
Commits: 4, all signed-off: yes
CI status: passing

Findings (2 suggestions, 1 nit — none blocking)

1. tests/test-weekly-report.sh:103 (suggestion) — the positive case covers candidate 1 only; candidate 2, the fallback that keeps the OpenClaw layout working, is unexercised. A regression that drops or reorders it would still pass this file. Inline has an ~8-line addition reusing the new fixtures.
2. docs/specs/2026-10-18-repoman-run-path-design.md:464 (nit) — the doc now matches the code, but #116's body still cites scripts/weekly-report.sh:79 as "defaults to a personal path"; after this the task is only "drop the fallback". A comment on #116 keeps them from diverging.
3. scripts/weekly-report.sh:87 (nit) — deployed expands $HOME under set -u; an environment without HOME dies with HOME: unbound variable before the friendly error. Pre-existing shape, not introduced here.

One thing not in the inline set: the ## Summary block still shows the pre-candidate-list one-liner and says the OpenClaw deployment "can export it explicitly" (it no longer has to), and ## Testing doesn't mention the candidate list or the new positive case. Worth refreshing so the body matches the diff.

Verdict: APPROVE — no must-fix; suggestions and nits ride along inside the approval, per the skill's §4.3.

Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).

[jimcadden]

suggestion — the positive case covers candidate 1 (bundled) only. Candidate 2, the $HOME/workspaces fallback that keeps the OpenClaw layout working, is never exercised here, so a regression that drops or reorders it still passes this file. ~8 lines reusing the fixtures you just added, verified green at 232a2b9 and red when the fallback is dropped:

rm "$bundle/skills/github-weekly-report/scripts/report.py"
mkdir -p "$TEST_TMPDIR/home/workspaces/shared/skills/github-weekly-report/scripts"
printf 'import sys\nprint("RAN", " ".join(sys.argv[1:]))\n' \
  > "$TEST_TMPDIR/home/workspaces/shared/skills/github-weekly-report/scripts/report.py"
full=$(HOME="$TEST_TMPDIR/home" REPOMAN_REPOS_FILE="$TEST_TMPDIR/repos.json" \
       REPOMAN_SKILL_DIR="$TEST_TMPDIR/meta" \
       bash "$bundle/skills/repoman/scripts/weekly-report.sh" 2>/dev/null) || true
got=$(printf '%s\n' "$full" | head -n 1)
[ "$got" = "RAN --repos rossoctl/alpha alice/beta" ] \
  || { echo "FAIL deployed fallback not used: [$got]"; fail=1; }

(The || true and the capture-then-head are load-bearing: piping the wrapper straight into head trips SIGPIPE under set -o pipefail, the same trap your comment on the block above documents.)

Nit in the same block: the comment credits reviewers by name (clawgenti/Jim #127). The file otherwise cites issues (rossoctl/automation#126); (#127) alone ages better.

[rubambiza]

Both done in 2ddab94: added a candidate-2 positive test (bundled absent, $HOME/workspaces present) — verified it goes red when the fallback is dropped or reordered, green otherwise — using the || true + capture-then-head shape to avoid the SIGPIPE trap. Also dropped the reviewer names from the comment; it now cites #127 only.