Repository navigation
chore(harness): /commit-sweep + nightly automation (#461 A2) - #515
thomasluizon wants to merge 1 commit into
Conversation
Add a report-only cross-commit, cross-repo regression sweep over a window of recent `main` commits in BOTH repos, plus the nightly automation that runs it and surfaces findings. - .claude/skills/commit-sweep/SKILL.md: reads the last N commits (default 10) or `--since <when>` across both repo checkouts, applies a six-check incident rubric holistically (migration DDL dup -> 42P07, dropped RN pins, analyzer-silent-locally ORBIT0001/0002, cross-repo contract drift, parity/i18n drift, stale-artifact QA), and writes one ranked report to .claude/audits/commit-sweep.md plus a `.status` sidecar. Never remediates. Roots resolve from ORBIT_UI_ROOT/ORBIT_API_ROOT with local defaults. - .github/workflows/commit-sweep.yml: nightly cron + workflow_dispatch; checks out both public repos (main at root for skill discovery, orbit-api into ./orbit-api); runs claude-code-action@v1 with the subscription OAuth token; event-branched prompt (schedule -> --since 24h, dispatch -> -n N); plain run: steps upsert a single commit-sweep-labeled tracking issue on findings and always upload the report artifact + a job-summary line. - CLAUDE.md: one pointer line for /commit-sweep. Harness-only: no product code, no parity/i18n mirror. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: PR #515 — chore(harness): /commit-sweep + nightly automation (#461 A2)
Scope: PR #515 in thomasluizon/orbit-ui-mobile (base feature/461-b1-docs-tools, stacked on #514)
Recommendation: NEEDS WORK
Summary
This PR adds the report-only /commit-sweep skill (.claude/skills/commit-sweep/SKILL.md), its
nightly + on-demand GitHub Actions workflow (.github/workflows/commit-sweep.yml), and a one-line
CLAUDE.md pointer. It is harness-only — no product code, no apps/*/packages/shared files, no
DTOs — so parity, i18n, contract-drift, DESIGN.md, backend-hard-rules, and FEATURES.md are all
genuinely N/A (confirmed by inspecting the diff, not just taking the PR body's word for it). The
one real defect is a self-inflicted mismatch between the two new files: the workflow's manual
workflow_dispatch path builds a command the skill's own documented argument grammar cannot parse.
Findings
Critical
None.
High
[HIGH] Manual workflow_dispatch invocation sends the skill an -n flag its own argument grammar never defines
· dimension: Correctness (rubric #1)
· location: .github/workflows/commit-sweep.yml:56
· issue: The prompt's command line is:
${{ github.event_name == 'schedule' && '/commit-sweep --since "24 hours ago"' || format('/commit-sweep -n {0} {1}', inputs.commits, inputs.scope) }}
— so a manual run (the workflow_dispatch branch, e.g. the default inputs) sends the skill the literal command /commit-sweep -n 10 both. But .claude/skills/commit-sweep/SKILL.md, added in this same commit, defines the argument grammar as:
- frontmatter
argument-hint: <N=10 | --since <when>> <repo-scope: both|ui|api>— no-nanywhere. - Phase 1: "Count mode (default, manual). A numeric first token is the commit count
N… Scope (optional trailing token)… A non-numeric, non---sincefirst token is treated as the scope withNdefaulting to 10."
Applied literally to-n 10 both, the first token-nis non-numeric and not--since, so per the skill's own rule it gets swallowed as if it were the scope (an invalid value), leaving the real count (10) and scope (both) unaccounted for — not "N=10, scope=both" as the author intended. The commit message for0ec984ebitself documents the same wrong belief: "event-branched prompt (schedule -> --since 24h, dispatch -> -n N)", confirming this is an authored inconsistency between the two files added together, not a misreading on my part.
· risk: Every manual/commit-sweeprun viaworkflow_dispatch— the on-demand entry point this PR advertises ("Runs nightly … and on demand as/commit-sweep") — drives the skill with out-of-grammar arguments. Best case, the executing model infers intent anyway despite the malformed flag; worst case it follows Phase 1 literally and mis-resolves N/scope, silently sweeping the wrong window or wrong repo scope with no error surfaced (the skill has "no adversarial skeptic" per its own Golden Rule, so a wrong window ships as a wrong report with no self-check).
· fix: Drop the-nflag to match the documented grammar:format('/commit-sweep {0} {1}', inputs.commits, inputs.scope).
· reference:.claude/skills/commit-sweep/SKILL.mdPhase 1 ("Resolve roots + parse args") +argument-hint, both added in this same PR.
Medium
None.
Low / Info
None posted (signal gate — Low/Info are not surfaced on a PR review).
Subagents
All five gated subagents are N/A — this diff touches only .claude/skills/, .github/workflows/,
and root CLAUDE.md; no apps/web/**, apps/mobile/**, i18n JSON, packages/shared/src/types/*,
endpoints.ts, or orbit-api file changed.
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/**/apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no shared types/endpoints/DTOs changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI files changed |
Validation
Not run as a separate step — this review executed against the PR diff; the repo's own required CI
checks (lint/build/test) cover this PR. The changed surface (one skill markdown file, one workflow
YAML, one CLAUDE.md line) has no lint/type-check/test target of its own beyond YAML structural
validity, which was checked manually.
| Check | Result |
|---|---|
| Lint | N/A — no lintable source changed |
| Type check | N/A — no TS/C# changed |
| Tests | N/A — no test-bearing code changed |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- Dimensions 2, 3, 4, 6, 7 (dead code, SOLID, comment policy, type safety, console.log) — N/A:
no TypeScript/C# source changed; comment-policy lint and type-safety rules don't apply to
Markdown/YAML. - Dimensions 8, 9, 10 (DESIGN.md/AI-slop, parity, i18n) — N/A: no
apps/*UI files changed. - Dimension 11 (contract drift/backward-compat) — N/A: no
packages/shared/src/types/*,
endpoints.ts, or orbit-api DTOs changed. - Dimension 13 (backend hard rules) — N/A: orbit-api untouched.
- Dimension 14 (FEATURES.md parity) — N/A:
/commit-sweepis an internal harness dev-tool, not
a user-facing feature/Astra/MCP tool/plan-gating/platform surface;FEATURES.mdwas checked
(grep) and correctly has no entry for it. - Dimension 12 (Security) — reviewed, not deferred: the workflow interpolates
inputs.commits
andinputs.scope(controllable only by someone who already hasworkflow_dispatchtrigger
rights, i.e. existing write access) into theprompt:field of an actionwith:block, not into
arun:shell script — no shell-injection vector, at most a prompt-injection surface bounded by
the workflow's own--allowedToolsallowlist. Judged Low/not privilege-escalating; not posted
per the signal gate. - Files: all three changed files (
SKILL.md,commit-sweep.yml,CLAUDE.md) were read in
full and given a verdict. Nothing changed was left unexamined. - Adversarial verification note: an independent skeptic subagent was launched against the one
High finding per the verification protocol. Its background completion could not be reliably
retrieved in this run due to sandbox notification-delivery limits, so in its place the finding was
independently re-verified against primary sources — readingSKILL.md's frontmatter and Phase 1
text directly from the commit object twice, and cross-checking the commit message for0ec984eb,
which itself states the same-n Nintent, corroborating the mismatch is real and authored, not a
misreading. Flagging this deviation from the standard adversarial-subagent flow for transparency.
What's good
- Harness-only scope is honest and accurate — no product code touched, and the PR body's parity/i18n
N/A claim checks out on inspection rather than just being asserted. - The workflow's checkout/env-var wiring (
ORBIT_UI_ROOT= workspace root,ORBIT_API_ROOT=
<workspace>/orbit-api) matches the skill's Phase 1 root-resolution contract exactly. - Report-only guardrails are enforced structurally, not just by convention: the
--allowedTools
allowlist has nogh issue/gh prwrite access for the sweep step itself, so even a compromised
prompt can't post to GitHub directly — only the workflow's own trustedrun:steps do that. - The
.statussidecar branching (empty/clean/findings) is a machine-readable outcome the
run:steps consume directly rather than re-parsing the markdown report, which is a clean way to
avoid brittle string-matching. - The scheduled (
--since "24 hours ago") path is unaffected by the flagged bug — nightly runs will
work as intended; only the manualworkflow_dispatchpath is broken.
Recommendation
Fix the one High finding before merge: drop the -n flag from the format() call in
.github/workflows/commit-sweep.yml:56 so the manual-dispatch command matches the grammar
SKILL.md actually documents. This is a one-line change with no ripple effect on the rest of the
PR. Everything else in the diff is clean.
There was a problem hiding this comment.
Code Review: PR #515 — chore(harness): /commit-sweep + nightly automation (#461 A2)
Scope: PR #515 in thomasluizon/orbit-ui-mobile (base feature/461-b1-docs-tools, stacked on #514)
Recommendation: APPROVE
Note on this review
An earlier automated review on this PR (submitted 2026-07-14T23:12:43Z) requested changes based on
classifying the finding below as High. That classification was posted before this session's
mandatory adversarial verification pass finished running. Verification has since completed — two
independent passes (a dedicated skeptic subagent and a separate full independent re-review) both
confirmed the finding is real but concluded the severity should be Medium, not High, and both
recommended approval. This review reflects that completed verification and supersedes the earlier
one.
Summary
This PR adds the report-only /commit-sweep skill (.claude/skills/commit-sweep/SKILL.md), its
nightly + on-demand GitHub Actions workflow (.github/workflows/commit-sweep.yml), and a one-line
CLAUDE.md pointer. It is harness-only — no product code, no apps/*/packages/shared files, no
DTOs — so parity, i18n, contract-drift, DESIGN.md, backend-hard-rules, and FEATURES.md are all
genuinely N/A (confirmed by inspecting the diff, not just taking the PR body's word for it). All
five gated /pr-review subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer,
design-reviewer) are correctly N/A for the same reason.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Manual workflow_dispatch invocation sends the skill an -n flag its own argument grammar never defines
· dimension: Correctness
· location: .github/workflows/commit-sweep.yml:56
· issue: The prompt's command line is:
${{ github.event_name == 'schedule' && '/commit-sweep --since "24 hours ago"' || format('/commit-sweep -n {0} {1}', inputs.commits, inputs.scope) }}
— so a manual run (the workflow_dispatch branch) sends the skill the literal command /commit-sweep -n 10 both. But .claude/skills/commit-sweep/SKILL.md, added in the same commit, defines the argument grammar as:
- frontmatter
argument-hint: <N=10 | --since <when>> <repo-scope: both|ui|api>— no-nanywhere. - Phase 1: "Count mode (default, manual). A numeric first token is the commit count
N… Scope (optional trailing token)… A non-numeric, non---sincefirst token is treated as the scope withNdefaulting to 10."
Applied literally,-nis a non-numeric, non---sincefirst token, so per the skill's own written rule it would be swallowed as an (invalid) scope value rather than parsed as a count flag. The commit message for0ec984ebdocuments the same wrong belief ("dispatch -> -n N"), confirming this is an authored inconsistency between the two files added together in this PR.
· risk (as verified): downgraded from an initial High to Medium after adversarial review.$ARGUMENTSis consumed by an LLM executor reading the fullSKILL.mdin context, not a rigid CLI parser — "-n 10 both" is a conventional-enough flag idiom that the executor will very likely still inferN=10, scope=bothcorrectly despite the grammar mismatch. Blast radius is also low: the skill is strictly report-only (writes an audit doc, opens/updates one label + issue; no remediation, no destructive action, no product-code writes), so even a misparse can only affect the count/scope of a single manual sweep, not cause a destructive or silent-failure outcome. The scheduled nightly path (--since "24 hours ago") is unaffected and correctly grammared.
· fix: drop the flag —format('/commit-sweep {0} {1}', inputs.commits, inputs.scope).
· reference:.claude/skills/commit-sweep/SKILL.mdPhase 1 arg grammar (same PR).
Low / Info
None (signal gate — no nits, style, or "consider…" items).
What's good
- Clean report-only contract: the skill never remediates or posts destructive changes; the workflow's own
run:steps own all GitHub side effects (label, issue upsert, artifact, job summary) — consistent with the existing/audit-*convention. - The six-check incident rubric is well-evidenced, each check citing a concrete prior-incident memory key, and the migration-duplication grep robustly covers both structured EF builder calls and raw-SQL DDL forms.
ORBIT_UI_ROOT/ORBIT_API_ROOTenv-var resolution gives identical behavior locally and in CI; the workflow correctly wires both roots (workspace root +./orbit-apisibling checkout).--allowedToolsis tightly scoped (nogh, no write-capable GitHub tool for the sweep step itself) — consistent with the report-only contract even under a compromised prompt.workflow_dispatchinputs only ever flow into theprompt:field forclaude-code-action(never into arun:shell script), so there is no shell-injection vector; issue upsert uses--body-file, no untrusted interpolation.
Deferred / N/A (confirmed by gate, not skipped)
Dead/stale code, SOLID/clean-arch, type safety, console.log, comment policy — no TS/C# product code in this diff. DESIGN.md/AI-slop, cross-platform parity, i18n — no apps/* or i18n files touched. Contract drift / backward-compat guard — no packages/shared/types or DTOs touched. Backend hard rules / security-reviewer / contract-aligner — orbit-api is not checked out in this CI job and is untouched by this diff; not verifiable here, flagged rather than guessed. Validation (lint/typecheck/tests/build) — Build / Unit Tests / SonarCloud run as separate required CI checks per this workflow's CI adaptations; Phase 6 skipped accordingly.
Stage 8 composes the clock grid on the screen, so neither calendar-time-grid.tsx carries a direct status fill-token reference any more. The hand-written inventory still counted one each, so it asserted 56 references against 54 observed. Both files still exist; only their references are gone. packages/shared: 88 passed. Ticket #515 owns deriving this inventory instead of listing it. This is the fourth pull request it has blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lvh6rf1XMFyVJ5hXZoMWhA
* feat(calendar): rebuild week time grid (ORB-50) * fix(calendar): preserve all-day event copy * fix(calendar): improve future grid accessibility * test(shared): prune the time-grid rows stage 8 orphaned Stage 8 composes the clock grid on the screen, so neither calendar-time-grid.tsx carries a direct status fill-token reference any more. The hand-written inventory still counted one each, so it asserted 56 references against 54 observed. Both files still exist; only their references are gone. packages/shared: 88 passed. Ticket #515 owns deriving this inventory instead of listing it. This is the fourth pull request it has blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lvh6rf1XMFyVJ5hXZoMWhA * fix(calendar): align time-grid dates and geometry (ORB-50) * chore(shared): regenerate the theme inventory snapshot (ORB-50) Stage 8 rebuilt the week time grid on both platforms, which changes the derived status fill-token inventory. Regenerated with npm run update:theme-inventory -w @orbit/shared. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AfLBsbGNe7BefVxGNBdCY9 * fix(calendar): use account timezone consistently (ORB-50) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Part of #461, bundle 2 (A2).
Stacked on #514 (
feature/461-b1-docs-tools). The base auto-retargets tomainwhen #514 merges. Review B1 first.What landed
.claude/skills/commit-sweep/SKILL.md— report-only cross-commit, cross-repo regression sweep. Reads the last N commits (default 10) or--since <when>acrossmainof BOTH repos and diff-reviews them together, applying a six-check incident rubric (migration DDL dup → 42P07, dropped RN transitive pins / Expo-pin generic bump, analyzer-silent-locally ORBIT0001/0002, cross-repo contract drift, cross-platform parity + i18n drift, stale-artifact QA hazard). Emits one severity-ranked report to.claude/audits/commit-sweep.md+ acommit-sweep.statussidecar (empty|clean|findings). Never remediates. Roots resolve fromORBIT_UI_ROOT/ORBIT_API_ROOTwith local-absolute defaults so it runs identically locally and in CI..github/workflows/commit-sweep.yml— nightlyschedule(cron0 5 * * *) +workflow_dispatch(commits,scopeinputs). Two public checkouts (main at workspace root for skill discovery;thomasluizon/orbit-apiinto./orbit-api, bothfetch-depth: 0, defaultGITHUB_TOKEN). Runsanthropics/claude-code-action@v1withCLAUDE_CODE_OAUTH_TOKEN(the same subscription tokenclaude-review.ymluses — no new secret, no metered key); event-branched prompt (schedule→--since "24 hours ago",dispatch→-n <commits> <scope>). Plainrun:steps idempotently create thecommit-sweeplabel, branch on the.statussidecar, upsert a singlecommit-sweep-labeled tracking issue onfindings, and always upload the report artifact + a job-summary line.CLAUDE.md— one pointer line for/commit-sweep.Harness-only
No product code, no cross-platform mirror, no i18n keys — parity/i18n are N/A. The report + status stay under the already-gitignored
.claude/audits/, so the committed surface is exactly the skill, the workflow, and the pointer.Validation
claude-code-action@v1+CLAUDE_CODE_OAUTH_TOKEN, wires both window modes, checks out the public sibling, surfaces viagh issue; skill declares--since+ the.statussidecar + the report-only guardrail; no em/en dash in authored copy).actionlinthas no npm package on this box, so YAML parse is the pre-merge gate; aworkflow_dispatchrun is the post-merge live proof.git log --since="24 hours ago"and the count-mode window run clean (exit 0) on both repos; the check-1 42P07 grep (both DDL forms, full history) finds real migration DDL. The live/commit-sweepacceptance smoke (which spawns model analysis) was deliberately NOT run — arg-parsing and both window modes were confirmed statically from the SKILL.🤖 Generated with Claude Code