Skip to content

refactor(skills): run the weekly collection review as a normal cron agent turn - #136110

Merged
obviyus merged 7 commits into
mainfrom
feat/workshop-review-agent-turn
Sep 5, 2026
Merged

refactor(skills): run the weekly collection review as a normal cron agent turn#136110
obviyus merged 7 commits into
mainfrom
feat/workshop-review-agent-turn

Conversation

@obviyus

@obviyus obviyus commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What Problem This Solves

Weekly Workshop cleanup duplicated automation execution and used collection rollback that could overwrite concurrent edits.

Why This Change Was Made

Run it as a normal isolated scheduled job with the agent’s Workshop root and no audited skills injected as active instructions. Ordinary directory tools discover files in pages. Remove collection-specific scanning, rollback, backup creation, and history writing.

User Impact

Auto schedules review; Propose and Off disable it. Completed edits survive later failure or cancellation. Existing backups/history remain readable, with guarded restore. Unsupported runtime or sandbox choices fail visibly; no new schema or shell permission is added.

Evidence

Real Gateway/CLI and writable-container checks verified paged discovery, edits, readback, history, cancellation, and restart convergence. Read-only/no-access sandboxes failed without edits or provider calls. Cross-agent access was denied and unrelated files survived. Focused dispatch, migration, restore, containment, and policy suites passed. Controlled responses establish runtime behavior, not review quality; separate board-test and automation-alias issues remained.

Consumers: weekly review uses normal automation history and direct file-edit semantics.

Related: #135528.

@clawsweeper

clawsweeper Bot commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
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.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime cli CLI command changes agents Agent runtime and tooling size: XL maintainer Maintainer-authored PR labels Sep 2, 2026
@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 2, 2026
@clawsweeper

clawsweeper Bot commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 5, 2026, 1:50 PM ET / 17:50 UTC.

ClawSweeper review

What this changes

Moves weekly Skill Workshop cleanup into ordinary scheduled agent turns, adds bounded directory discovery across sandbox backends, and retains recovery from older backups.

Merge readiness

⛔ Blocked before merge - 3 items remain

This remains useful work distinct from the merged Workshop ownership change. The previous oversized-inventory finding is resolved, and no new blocking defect was found.

Priority: P2
Reviewed head: 210ebc0bcf3465bc30b750becebcffd92c380dd7

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A coherent, net-negative refactor with strong production-path evidence and the prior blocker resolved; documented compatibility tradeoffs remain visible.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): Captured Gateway/CLI and real Docker observations exercise the changed cron-to-filesystem path: complete discovery, successful edit/readback, cross-agent denial, stale-tool revocation, cancellation, and sandbox refusal. The tested production tree matches this head; retained earlier observations keep their original identities. The omitted body tail and unavailable raw local artifacts remain review limitations.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): Captured Gateway/CLI and real Docker observations exercise the changed cron-to-filesystem path: complete discovery, successful edit/readback, cross-agent denial, stale-tool revocation, cancellation, and sandbox refusal. The tested production tree matches this head; retained earlier observations keep their original identities. The omitted body tail and unavailable raw local artifacts remain review limitations.
Evidence reviewed 13 items Current main still uses the dedicated reviewer: Fetched main still declares skillCollectionReview payloads targeting the main session. The supplied merged metadata for #135528 explicitly excludes this follow-up. The supplied latest-release object was unavailable locally, so no release inclusion is claimed.
Previous inventory blocker resolved: The job now supplies fixed review instructions instead of a filesystem inventory. The shared directory tool budgets complete entries and its continuation footer together; ls.test.ts covers complete traversal with long filenames under byte and model budgets.
Gateway-to-runner boundary: The Gateway resolves the agent-owned Workshop root, forwards cancellation and execution identity, supplies an empty active-skill snapshot, and refreshes future skill snapshots in finally. Cron preparation and execution carry the root into ordinary embedded execution.
Findings None None.
Security None None.

How this fits together

Skill Workshop stores each agent’s learned procedures. The Gateway schedules their weekly review, and the embedded agent uses filesystem tools to update them while cron records execution outcomes.

flowchart TD
  A[Workshop configuration] --> B[Weekly cron job]
  B --> C[Runtime and sandbox checks]
  C --> D[Recorded refusal]
  C --> E[Rooted agent turn]
  E --> F[Paged directory and file tools]
  F --> G[Updated Workshop files]
  E --> H[Automation run history]
Loading

Before merge

  • Resolve merge risk (P1) - Existing non-embedded or read-only/no-access sandbox setups will refuse weekly reviews; third-party backends without directory-reading support cannot provide shell-free discovery.
  • Resolve merge risk (P1) - New reviews intentionally retain partial edits after failure and produce no collection backup; operators lose the previous automatic collection-wide recovery behavior.
  • Review config compatibility - Confirm compatibility and upgrade impact for the changed config or default surface before merge.
Agent review details

Security

None.

PR surface

PR surface statistics unavailable: the file list is incomplete.

Review metrics

Metric Value Why it matters
Production versus test LOC Production −1,014 net; tests/test support −1,552 net The refactor removes the bespoke review pipeline; retained restore code movement does not inflate the net reduction.
Contract changes 1 optional sandbox method added; 1 directory cursor added; 1 reported cron payload kind removed; 0 config options added The compatibility surface spans backend discovery and generated-job replacement without expanding operator configuration.

Stored data model

Persistent data-model change detected: serialized state: extensions/mxc/src/fs-bridge.ts, serialized state: extensions/openshell/src/backend.ts, serialized state: extensions/openshell/src/fs-bridge.ts, serialized state: src/infra/directory-entries.ts, serialized state: src/skills/workshop/collection-byte-limits.ts, unknown-data-model-change: src/skills/workshop/collection-byte-limits.ts, and 1 more. Migration or upgrade compatibility proof is recorded; maintainers should verify it before merge.

Merge-risk options

Maintainer options:

  1. Retain the documented cron semantics (recommended)
    Accept the stated runtime restrictions and ordinary file-edit recovery behavior while preserving optional backend support and retained legacy backups.

Technical review

Best possible solution:

Use the shared cron runner and existing directory tool, preserving mandatory Workshop roots and the documented upgrade and legacy-recovery behavior.

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

Not applicable to the refactor itself; the previous inventory-overflow mechanism is removed, with bounded pagination coverage and reported production-path verification.

Is this the best way to solve the issue?

Yes. Reusing cron and the existing directory tool removes competing execution and recovery machinery while keeping containment in the filesystem and runtime owners.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3e4becea5e2c.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Captured Gateway/CLI and real Docker observations exercise the changed cron-to-filesystem path: complete discovery, successful edit/readback, cross-agent denial, stale-tool revocation, cancellation, and sandbox refusal. The tested production tree matches this head; retained earlier observations keep their original identities. The omitted body tail and unavailable raw local artifacts remain review limitations.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a bounded improvement to scheduled Workshop maintenance with substantial existing implementation and validation.
  • merge-risk: 🚨 compatibility: Runtime eligibility, backend directory discovery, and collection recovery semantics change for existing installations.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Captured Gateway/CLI and real Docker observations exercise the changed cron-to-filesystem path: complete discovery, successful edit/readback, cross-agent denial, stale-tool revocation, cancellation, and sandbox refusal. The tested production tree matches this head; retained earlier observations keep their original identities. The omitted body tail and unavailable raw local artifacts remain review limitations.
  • proof: sufficient: Contributor real behavior proof is sufficient. Captured Gateway/CLI and real Docker observations exercise the changed cron-to-filesystem path: complete discovery, successful edit/readback, cross-agent denial, stale-tool revocation, cancellation, and sandbox refusal. The tested production tree matches this head; retained earlier observations keep their original identities. The omitted body tail and unavailable raw local artifacts remain review limitations.

Evidence

What I checked:

  • Current main still uses the dedicated reviewer: Fetched main still declares skillCollectionReview payloads targeting the main session. The supplied merged metadata for feat(skills)!: give each agent its own Skill Workshop #135528 explicitly excludes this follow-up. The supplied latest-release object was unavailable locally, so no release inclusion is claimed. (src/cron/skill-collection-review-monitor.ts:48, 3e4becea5e2c)
  • Previous inventory blocker resolved: The job now supplies fixed review instructions instead of a filesystem inventory. The shared directory tool budgets complete entries and its continuation footer together; ls.test.ts covers complete traversal with long filenames under byte and model budgets. (src/agents/sessions/tools/ls.ts:126, 210ebc0bcf34)
  • Gateway-to-runner boundary: The Gateway resolves the agent-owned Workshop root, forwards cancellation and execution identity, supplies an empty active-skill snapshot, and refreshes future skill snapshots in finally. Cron preparation and execution carry the root into ordinary embedded execution. (src/gateway/server-cron.ts:904, 210ebc0bcf34)
  • Permission changes preserve containment: Tool reconstruction retains requireWorkspaceOnly. Permission refresh aborts the old tool generation before resolving approvals, then rebuilds against the same permission root; mandatory containment also dominates apply_patch policy. Compaction forwards the same requirements. (src/agents/embedded-agent-runner/run/attempt-tool-prepare.ts:418, 210ebc0bcf34)
  • Runtime implementation boundary: Rooted reviews accept only OpenClaw’s embedded runtime and reject other runtime selections before execution. The changed filesystem behavior belongs to OpenClaw; the mocked alternate-runtime selection does not exercise or establish a dependency on the external Codex protocol. (src/cron/isolated-agent/run-prepare-runtime.ts:61, 210ebc0bcf34)
  • Sandbox API and dependency signal: The optional readDirectory bridge supplies sandbox listings; absent support hides ls without host-filesystem fallback. Host-contained listing directly calls fs-safe root.list, establishing the relevant dependency contract. Docker, remote, OpenShell, and MXC implementations reuse their existing path owners. (src/agents/core-coding-tools.ts:118, 210ebc0bcf34)

Likely related people:

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

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 (26 earlier review cycles; latest 8 shown)
  • reviewed 2026-09-03T09:45:03.899Z sha 671df0d :: blocked before merge. :: [P1] Ignore unchanged non-skill paths during review validation
  • reviewed 2026-09-03T10:17:20.201Z sha b08a58acfe429920f3b37cd49d4b7ff66678949e :: blocked before merge. :: none
  • reviewed 2026-09-03T10:37:20.542Z sha 618abf4 :: blocked before merge. :: [P1] Preserve the prior restore point on an unchanged started-turn error | [P2] Inventory and reject symbolic links created by the review turn | [P2] Describe the review's post-turn scan timing accurately
  • reviewed 2026-09-03T11:05:43.929Z sha 3e633aa :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-03T12:13:23.502Z sha 3e633aa :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-05T13:46:47.474Z sha 3e633aa :: needs real behavior proof before merge. :: [P1] [P1] Resolve the default autonomous mode before skipping review | [P1] [P1] Complete protected cleanup after a cancelled editing turn | [P2] [P2] Provide directory discovery without requiring shell approval
  • reviewed 2026-09-05T16:02:12.944Z sha 80afab4 :: needs real behavior proof before merge. :: [P1] [P1] Make mandatory containment override Full Access for apply_patch | [P2] [P2] Bound the serialized inventory before constructing the turn
  • reviewed 2026-09-05T16:54:48.271Z sha 65ccb4b :: blocked before merge. :: [P2] [P2] Bound the complete inventory before constructing the review turn

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Sep 2, 2026
@obviyus
obviyus force-pushed the feat/workshop-global-skill-ownership branch from b2af24b to 6ae549e Compare September 2, 2026 08:15
@obviyus
obviyus force-pushed the feat/workshop-review-agent-turn branch from 6ccaf48 to dae08d5 Compare September 2, 2026 08:53
@obviyus obviyus mentioned this pull request Sep 2, 2026
Merged
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Sep 2, 2026
@obviyus
obviyus force-pushed the feat/workshop-review-agent-turn branch from dae08d5 to a97089b Compare September 2, 2026 09:33
@obviyus
obviyus force-pushed the feat/workshop-global-skill-ownership branch from 14f14b0 to 8c8a561 Compare September 2, 2026 09:38
@obviyus
obviyus force-pushed the feat/workshop-review-agent-turn branch from a97089b to 73cd525 Compare September 2, 2026 09:39
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Sep 2, 2026
@obviyus
obviyus force-pushed the feat/workshop-global-skill-ownership branch from e5d4361 to acf82cb Compare September 3, 2026 08:27
@obviyus
obviyus force-pushed the feat/workshop-review-agent-turn branch from f6454bf to 4b64a74 Compare September 3, 2026 08:28
@obviyus

obviyus commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Landed as efa4b5df from reviewed head 210ebc0b.

  • Problem: weekly Workshop cleanup duplicated cron execution, history, and recovery; whole-tree inventories could overflow the model input.
  • Fix: one ordinary isolated cron turn, bounded directory pages, rooted file tools, and normal cancellation/history. The bespoke worker and collection rollback/backup pipeline are removed; historical restore remains available.
  • Proof: compiled CLI/Gateway and real Docker flows; seven independent acceptance clauses passed, including complete 250-file discovery, edit/readback, cross-agent denial, unchanged neighboring files, and read-only/no-access refusal. Scripted provider proof does not claim semantic learning quality.
  • Gates: exact-head CI passed; local ClawSweeper passed at Platinum with no findings or security concerns. Guarded dry run, merge receipt, and Ayaan attribution verified. No CI bypass or unrelated CI repair.
Before After
Separate collection execution and recovery machinery Cron owns execution, cancellation, and history
Full file tree injected into the initial prompt Complete names delivered through bounded directory pages
Competing whole-collection rollback Normal file-edit semantics with explicit run outcomes

The documented runtime/backend and recovery tradeoffs are retained under the maintainer's normal-cron direction. No configuration option or SQLite schema/version change. Net deletion: 2,555 lines overall, including 1,014 production lines. Hosted proof resources are released; evidence is retained.

@ceckert ceckert mentioned this pull request Sep 5, 2026
Closed
@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Open
This was referenced Sep 6, 2026
Closed
Closed
This was referenced Sep 7, 2026
Closed
Open
This was referenced Sep 7, 2026
Closed
Merged
This was referenced Sep 9, 2026
Closed
Merged
This was referenced Sep 12, 2026
Closed
Merged
This was referenced Sep 14, 2026
Closed
Closed
This was referenced Sep 23, 2026
Open
Closed
Closed
This was referenced Oct 7, 2026
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling app: web-ui App: web-ui cli CLI command changes commands Command implementations docker Docker and sandbox tooling docs Improvements or additions to documentation extensions: openshell gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. plugin: mxc proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XL status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@obviyus