Skip to content

chore(autoreview): use the shared canonical skill - #1599

Merged
vincentkoc merged 2 commits into
mainfrom
refactor/shared-autoreview-20261002
Oct 6, 2026
Merged

vincentkoc merged 2 commits into
mainfrom
refactor/shared-autoreview-20261002

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

What Problem This Solves

Repository-local autoreview copies drift and can retain retired prerequisites such as TruffleHog.

User Impact

Install autoreview once from openclaw/agent-skills using the setup linked in .agents/skills/autoreview/SKILL.md. Reviews run from this repository through that shared installation. Updating its source checkout updates all symlinked consumers; copy-mode users reinstall once. Product runtime behavior is unchanged.

Why This Change Was Made

Keep only the canonical Markdown entrypoint and remove the copied helper, fixtures, and implementation tests.

Canonical owner and preserved downstream fixes: openclaw/agent-skills#305. Shared changes belong upstream; review runs do not download or update code automatically.

Evidence

  • All 17 consumer entrypoints match the canonical template; each contains no local helper or implementation tests.
  • git diff --check passes; independent integration review checked callers and removed gates.
  • The shared installer and helper dry runs pass from consumer repositories without TruffleHog. Canonical regression tests and review are recorded in the upstream PR.
  • Product runtime suites were not run for this shared-tooling cutover.

Required Proof Pools

none: this changes developer review instructions and retires copied tooling; no Windows product behavior changes.

Validation

Entrypoint/template equality and git diff --check pass. Windows build, Shared tests, and Tray tests were not run on the Linux development host.

Real Behavior Proof

The installed canonical helper passes preparation and reviewer-isolation dry runs from consumer repositories without a repository-local helper or TruffleHog. Native Windows launcher execution remains covered by the canonical skill's Windows CI lane; it was not executed locally.

Security Impact

No product permissions, credentials, data access, or runtime network calls change. Reviewer isolation and result validation remain owned by the canonical skill.

Landing follow-up

Repository instructions explicitly retain the original P0–P3 review gate with --max-priority P3; direct invocation examples use that override. The shared entrypoint remains identical to the canonical template. The branch also incorporates the already-merged sidebar proof fix from #1597, which addresses the historical failed UI test; current-head CI is required.

Hosted UI startup follow-up

The new UI job aborted before application startup: it logged UIThreadProc: before bootstrap, never reached bootstrap complete, and hit the five-minute inactivity watchdog (job). This matches the existing bootstrap investigation across 16 unrelated branches. A single retry of that job is requested after this attribution; passing it would not establish a bootstrap fix. No application/test source, timeout, assertion, or required CI gate is weakened.

The single UI/accessibility retry passed, followed by the required CI Gate. The PR is merged; its shared entrypoint and retained P3 review policy were verified in the merge commit. The bootstrap failure remains an unresolved hosted-test issue, not a fix supplied by this PR.

@clawsweeper

clawsweeper Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper

clawsweeper Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed October 6, 2026, 3:07 PM ET / 19:07 UTC (Revision 3).

ClawSweeper review

What this changes

The branch replaces copied autoreview code and tests with instructions for a shared installation while preserving this repository’s P0–P3 review threshold.

Merge readiness

⛔ Needs changes before merge - 1 item remains

The consolidation remains useful and no blocking introduced defect was found. The earlier Windows closeout request remains unresolved: current-head CI passes its core and tray jobs, but the body still explicitly reports that the required local validation was not run.

Priority: P3
Reviewed head: ac7ba8204062b058e9617ffb3eec72839fb33e7d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The focused shared-tooling transition has normal good patch quality, with required Windows closeout evidence still outstanding.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER author is exempt from ordinary contributor proof. The changed owner is developer review tooling; the body reports consumer preparation and isolation dry runs, and the merged upstream PR reports Windows launcher coverage. No material authority transfer or stored-data contract changes require additional proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER author is exempt from ordinary contributor proof. The changed owner is developer review tooling; the body reports consumer preparation and isolation dry runs, and the merged upstream PR reports Windows launcher coverage. No material authority transfer or stored-data contract changes require additional proof.
Evidence reviewed 7 items Pinned migration: The introduced delta retires the local helper and its harness/tests, replaces the skill instructions with a shared-installation entrypoint, and adds the repository priority override. Searches found no surviving executable caller of the removed path; remaining direct invocations in setup documentation describe historical validation attempts.
Canonical dependency and merged rollout: The target entrypoint explicitly invokes this dependency. GitHub verifies that openclaw/agent-skills#305 merged as 0595f725087f0208c69edbaf74cb968ca876fd04. Its body documents shared installation, fresh/upgrade checks in symlink and copy modes, and native Windows launcher validation. The pinned canonical repository-entrypoint text matches the target entrypoint.
Working directory and isolation remain upstream: The pinned shared helper discovers the reviewed repository from Path.cwd(), retains Codex, Claude, and Pi isolation flags, and exposes the explicit P3 severity override. The shared instructions prohibit automatic review-time downloads or updates and identify secret scanning as a separate, intentional policy boundary.
Findings None None.
Security None None.

How this fits together

Autoreview supports developer code reviews rather than the Windows application runtime. The repository supplies Git changes and local review requirements to the installed shared helper, which returns structured findings.

flowchart TD
  A[Repository changes] --> D[Shared review helper]
  B[Local review requirements] --> D
  C[Repository skill entrypoint] --> E[Shared installation]
  E --> D
  D --> F[Structured review findings]
Loading

Before merge

  • Complete next step (P2) - Record current-head Windows results for ./build.ps1, dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore, and dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore, confirming non-zero test counts.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Tooling and test retirement helper +0/-8,982 lines; tests and harnesses +0/-5,662 lines The large deletion retires copied developer tooling in favor of the canonical installation rather than removing Windows product code.

Technical review

Best possible solution:

Keep one upstream review implementation with explicit installation/update guidance and local severity requirements, supported by the repository’s Windows closeout evidence.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this is a tooling consolidation rather than a reported runtime bug; source inspection confirms the copied implementation still exists on current main.

Is this the best way to solve the issue?

Yes. The existing shared installer and working-directory-aware helper provide a focused replacement, and the explicit P3 override preserves local review requirements.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 11523b485b08.

Labels

Label changes:

No label changes.

Label justifications:

  • P3: This is developer-tooling consolidation with no introduced Windows runtime behavior change or urgent user-facing regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER author is exempt from ordinary contributor proof. The changed owner is developer review tooling; the body reports consumer preparation and isolation dry runs, and the merged upstream PR reports Windows launcher coverage. No material authority transfer or stored-data contract changes require additional proof.

Evidence

What I checked:

  • Pinned migration: The introduced delta retires the local helper and its harness/tests, replaces the skill instructions with a shared-installation entrypoint, and adds the repository priority override. Searches found no surviving executable caller of the removed path; remaining direct invocations in setup documentation describe historical validation attempts. (.agents/skills/autoreview/SKILL.md:8, ac7ba8204062)
  • Canonical dependency and merged rollout: The target entrypoint explicitly invokes this dependency. GitHub verifies that fix(autoreview): share one installation across repositories agent-skills#305 merged as 0595f725087f0208c69edbaf74cb968ca876fd04. Its body documents shared installation, fresh/upgrade checks in symlink and copy modes, and native Windows launcher validation. The pinned canonical repository-entrypoint text matches the target entrypoint. (skills/autoreview/references/repository-entrypoint.md:8, 0595f725087f)
  • Working directory and isolation remain upstream: The pinned shared helper discovers the reviewed repository from Path.cwd(), retains Codex, Claude, and Pi isolation flags, and exposes the explicit P3 severity override. The shared instructions prohibit automatic review-time downloads or updates and identify secret scanning as a separate, intentional policy boundary. (skills/autoreview/scripts/autoreview:1995, 0595f725087f)
  • Repository-specific severity preserved: The added instructions require --max-priority P3, overriding the canonical helper’s P0 default without creating another implementation variant. (AGENTS.md:140, ac7ba8204062)
  • Current-main and release necessity: Current main still contains the copied implementation. The supplied latest release, v2026.9.4, also contains the local helper and implementation tests, so the shared-installation cutover is not already implemented in the target repository or its latest release. (.agents/skills/autoreview/scripts/autoreview, 3c43751b2bac)
  • Validation continuity: The previous completed review has no findings and requests current-head Windows build, Shared, and Tray closeout results. The captured body explicitly says those local commands were not run. Live job metadata confirms successful current-head core and tray build/test steps, but ci.yml uses split builds and Invoke-CiTest rather than the full local closeout. Downloading job logs was blocked by the reviewer network allowlist, so actual test counts were not independently inspected; this is a reviewer limitation, not missing contributor runtime proof. (.github/workflows/ci.yml:364, ac7ba8204062)

Likely related people:

  • vincentkoc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • paulcam206: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Record current-head results for ./build.ps1 and the required Shared and Tray test commands, including non-zero test counts.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-10-02T08:49:51.460Z sha 567eec5 :: needs changes before merge. :: none
  • reviewed 2026-10-06T18:50:49.915Z sha ac7ba82 :: needs changes before merge. :: none

@vincentkoc
vincentkoc marked this pull request as ready for review October 6, 2026 18:45
@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 6, 2026
@vincentkoc
vincentkoc merged commit 0c6e586 into main Oct 6, 2026
43 of 46 checks passed
@vincentkoc
vincentkoc deleted the refactor/shared-autoreview-20261002 branch October 6, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant